[cfi] Fix handling of sanitize trap/recover flags in the cross-DSO CFI mode.

[eugenis]

Summary

In the cross-DSO CFI mode clang emits cfi_check_fail that handles
errors triggered from other modules with targets in the current
module. With this change, cfi_check_fail will handle errors for
CFI kinds that are not enabled in the current module as if they
have the trapping behaviour (-fsanitize-trap=...).

This fixes a bug where some combinations of -fsanitize* flags may
result in a link failure due to a missing sanitizer runtime library
for the diagnostic calls in __cfi_check_fail.

[pcc]

Why can't we make it so that a trap flag doesn't affect the behaviour if a sanitizer is disabled (this looks like what CodeGenModule::NeedAllVtablesBitSet is already doing?)

[eugenis]

This lets us support the following case:
module A checks vcalls and casts, with diagnostics
module B checks vcalls but not casts (but it still has bitsets for vtables), with diagnostics
then a cast check from module A with a target in module B should print diagnostics instead of trapping

It's definitely not critical functionality, but could be nice to have, especially as it does not cost us anything.

And yes, CodeGenModule::NeedAllVtablesBitSet needs to be fixed for this work.

WDYT?

[pcc]

What I meant was that it looks like a hack that this is being handled in the driver. The frontend shouldn't care what the value of a trap flag is if a sanitizer is disabled.

Why are we even emitting checks for disabled sanitizers in the target DSO anyway? Can we fail open if the target DSO does not have a particular CFI sanitizer enabled (same as we fail open for non-instrumented DSOs)? That would allow you to remove a little of the complication you added to EmitCheck in D15699.

[eugenis]

OK, done. Please take another look.
This is inferior to the original patch in terms of functionality, but the implementation is a lot simpler.

[eugenis]

ping

[pcc]

LGTM

lib/CodeGen/CGExpr.cpp

2483–2484

You can remove this comment now.

2675

I mentioned failing open, but yes, it's probably better to fail closed.

[eugenis]

r263180, thanks for the review!

[eugenis]

No, this is not committed.
I've run dcommit in the wrong checkout and landed D17900 instead.

[eugenis]

r263578, finally

lib/CodeGen/CGExpr.cpp

void CodeGenFunction::EmitCheck(
 
   llvm::Value *JointCond;
   if (FatalCond && RecoverableCond)
     JointCond = Builder.CreateAnd(FatalCond, RecoverableCond);
   else
     JointCond = FatalCond ? FatalCond : RecoverableCond;
   assert(JointCond);
 
   CheckRecoverableKind RecoverKind = getRecoverableKind(Checked[0].second);
-  // In cross-DSO CFI mode this code is used to generate __cfi_check_fail, which
-  // includes all checks, even those that are not in SanOpts.
-  assert(CGM.getCodeGenOpts().SanitizeCfiCrossDso ||
-         SanOpts.has(Checked[0].second));
+  assert(SanOpts.has(Checked[0].second));
 #ifndef NDEBUG
   for (int i = 1, n = Checked.size(); i < n; ++i) {
     assert(RecoverKind == getRecoverableKind(Checked[i].second) &&
            "All recoverable kinds in a single check must be same!");
-    assert(CGM.getCodeGenOpts().SanitizeCfiCrossDso ||
-           SanOpts.has(Checked[i].second));
+    assert(SanOpts.has(Checked[i].second));
   }
 #endif
 
   llvm::BasicBlock *Cont = createBasicBlock("cont");
   llvm::BasicBlock *Handlers = createBasicBlock("handler." + CheckName);
   llvm::Instruction *Branch = Builder.CreateCondBr(JointCond, Cont, Handlers);
   // Give hint that we very much don't expect to execute the handler
   // Value chosen to match UR_NONTAKEN_WEIGHT, see BranchProbabilityInfo.cpp
const std::pair<int, SanitizerMask> CheckKinds[] = {
       {CFITCK_ICall, SanitizerKind::CFIICall}};
 
   SmallVector<std::pair<llvm::Value *, SanitizerMask>, 5> Checks;
   for (auto CheckKindMaskPair : CheckKinds) {
     int Kind = CheckKindMaskPair.first;
     SanitizerMask Mask = CheckKindMaskPair.second;
     llvm::Value *Cond =
         Builder.CreateICmpNE(CheckKind, llvm::ConstantInt::get(Int8Ty, Kind));
+    if (CGM.getLangOpts().Sanitize.has(Mask))
       EmitCheck(std::make_pair(Cond, Mask), "cfi_check_fail", {},
                 {Data, Addr, ValidVtable});
+    else
+      EmitTrapCheck(Cond);
   }
 
   FinishFunction();
   // The only reference to this function will be created during LTO link.
   // Make sure it survives until then.
   CGM.addUsedGlobal(F);
 }
 

test/CodeGen/cfi-check-fail.c

-// RUN: %clang_cc1 -triple x86_64-unknown-linux -O0 -fsanitize=cfi-icall -fsanitize-cfi-cross-dso \
+// RUN: %clang_cc1 -triple x86_64-unknown-linux -O0 -fsanitize-cfi-cross-dso \
+// RUN:     -fsanitize=cfi-icall,cfi-nvcall,cfi-vcall,cfi-unrelated-cast,cfi-derived-cast \
 // RUN:     -fsanitize-trap=cfi-icall,cfi-nvcall -fsanitize-recover=cfi-vcall,cfi-unrelated-cast \
 // RUN:     -emit-llvm -o - %s | FileCheck %s
 
 void caller(void (*f)()) {
   f();
 }
 
 // CHECK: define weak_odr hidden void @__cfi_check_fail(i8*, i8*) {

test/CodeGen/cfi-check-fail2.c

+// __cfi_check_fail codegen when not all CFI checkers are enabled.
+// RUN: %clang_cc1 -triple x86_64-unknown-linux -O0 -fsanitize-cfi-cross-dso \
+// RUN:     -fsanitize=cfi-vcall \
+// RUN:     -emit-llvm -o - %s | FileCheck %s
+
+void caller(void (*f)()) {
+  f();
+}
+
+// CHECK: define weak_odr hidden void @__cfi_check_fail(i8*, i8*) {
+// CHECK: store i8* %0, i8** %[[ALLOCA0:.*]], align 8
+// CHECK: store i8* %1, i8** %[[ALLOCA1:.*]], align 8
+// CHECK: %[[DATA:.*]] = load i8*, i8** %[[ALLOCA0]], align 8
+// CHECK: %[[ADDR:.*]] = load i8*, i8** %[[ALLOCA1]], align 8
+// CHECK: %[[ICMP_NOT_NULL:.*]] = icmp ne i8* %[[DATA]], null
+// CHECK: br i1 %[[ICMP_NOT_NULL]], label %[[CONT0:.*]], label %[[TRAP:.*]],
+
+// CHECK: [[TRAP]]:
+// CHECK-NEXT:   call void @llvm.trap()
+// CHECK-NEXT:   unreachable
+
+// CHECK: [[CONT0]]:
+// CHECK:   %[[A:.*]] = bitcast i8* %[[DATA]] to { i8, { i8*, i32, i32 }, i8* }*
+// CHECK:   %[[KINDPTR:.*]] = getelementptr {{.*}} %[[A]], i32 0, i32 0
+// CHECK:   %[[KIND:.*]] = load i8, i8* %[[KINDPTR]], align 4
+// CHECK:   %[[VTVALID0:.*]] = call i1 @llvm.bitset.test(i8* %[[ADDR]], metadata !"all-vtables")
+// CHECK:   %[[VTVALID:.*]] = zext i1 %[[VTVALID0]] to i64
+// CHECK:   %[[NOT_0:.*]] = icmp ne i8 %[[KIND]], 0
+// CHECK:   br i1 %[[NOT_0]], label %[[CONT1:.*]], label %[[HANDLE0:.*]], !prof
+
+// CHECK: [[HANDLE0]]:
+// CHECK:   %[[DATA0:.*]] = ptrtoint i8* %[[DATA]] to i64,
+// CHECK:   %[[ADDR0:.*]] = ptrtoint i8* %[[ADDR]] to i64,
+// CHECK:   call void @__ubsan_handle_cfi_check_fail_abort(i64 %[[DATA0]], i64 %[[ADDR0]], i64 %[[VTVALID]])
+// CHECK:   unreachable
+
+// CHECK: [[CONT1]]:
+// CHECK:   %[[NOT_1:.*]] = icmp ne i8 %[[KIND]], 1
+// CHECK:   br i1 %[[NOT_1]], label %[[CONT2:.*]], label %[[HANDLE1:.*]], !nosanitize
+
+// CHECK: [[HANDLE1]]:
+// CHECK-NEXT:   call void @llvm.trap()
+// CHECK-NEXT:   unreachable
+
+// CHECK: [[CONT2]]:
+// CHECK:   %[[NOT_2:.*]] = icmp ne i8 %[[KIND]], 2
+// CHECK:   br i1 %[[NOT_2]], label %[[CONT3:.*]], label %[[HANDLE2:.*]], !nosanitize
+
+// CHECK: [[HANDLE2]]:
+// CHECK-NEXT:   call void @llvm.trap()
+// CHECK-NEXT:   unreachable
+
+// CHECK: [[CONT3]]:
+// CHECK:   %[[NOT_3:.*]] = icmp ne i8 %[[KIND]], 3
+// CHECK:   br i1 %[[NOT_3]], label %[[CONT4:.*]], label %[[HANDLE3:.*]], !nosanitize
+
+// CHECK: [[HANDLE3]]:
+// CHECK-NEXT:   call void @llvm.trap()
+// CHECK-NEXT:   unreachable
+
+// CHECK: [[CONT4]]:
+// CHECK:   %[[NOT_4:.*]] = icmp ne i8 %[[KIND]], 4
+// CHECK:   br i1 %[[NOT_4]], label %[[CONT5:.*]], label %[[HANDLE4:.*]], !nosanitize
+
+// CHECK: [[HANDLE4]]:
+// CHECK-NEXT:   call void @llvm.trap()
+// CHECK-NEXT:   unreachable
+
+// CHECK: [[CONT5]]:
+// CHECK:   ret void