Re: [PATCH] D17360: [cfi] Fix handling of sanitize trap/recover flags in the cross-DSO CFI mode.
eugenis added a comment. r263578, finally Repository: rL LLVM http://reviews.llvm.org/D17360 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D17360: [cfi] Fix handling of sanitize trap/recover flags in the cross-DSO CFI mode.
eugenis added a comment. No, this is not committed. I've run dcommit in the wrong checkout and landed http://reviews.llvm.org/D17900 instead. Repository: rL LLVM http://reviews.llvm.org/D17360 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D17360: [cfi] Fix handling of sanitize trap/recover flags in the cross-DSO CFI mode.
eugenis closed this revision. eugenis added a comment. r263180, thanks for the review! Repository: rL LLVM http://reviews.llvm.org/D17360 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D17360: [cfi] Fix handling of sanitize trap/recover flags in the cross-DSO CFI mode.
pcc accepted this revision. pcc added a comment. This revision is now accepted and ready to land. LGTM Comment at: lib/CodeGen/CGExpr.cpp:2484-2485 @@ -2483,3 +2483,4 @@ 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(SanOpts.has(Checked[0].second)); You can remove this comment now. Comment at: lib/CodeGen/CGExpr.cpp:2677 @@ -2677,1 +2676,3 @@ +else + EmitTrapCheck(Cond); } I mentioned failing open, but yes, it's probably better to fail closed. Repository: rL LLVM http://reviews.llvm.org/D17360 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D17360: [cfi] Fix handling of sanitize trap/recover flags in the cross-DSO CFI mode.
eugenis added a comment. ping Repository: rL LLVM http://reviews.llvm.org/D17360 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D17360: [cfi] Fix handling of sanitize trap/recover flags in the cross-DSO CFI mode.
eugenis updated the summary for this revision.
eugenis updated this revision to Diff 49115.
eugenis added a comment.
OK, done. Please take another look.
This is inferior to the original patch in terms of functionality, but the
implementation is a lot simpler.
Repository:
rL LLVM
http://reviews.llvm.org/D17360
Files:
lib/CodeGen/CGExpr.cpp
test/CodeGen/cfi-check-fail.c
test/CodeGen/cfi-check-fail2.c
Index: test/CodeGen/cfi-check-fail2.c
===
--- /dev/null
+++ test/CodeGen/cfi-check-fail2.c
@@ -0,0 +1,70 @@
+// __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
Index: test/CodeGen/cfi-check-fail.c
===
--- test/CodeGen/cfi-check-fail.c
+++ test/CodeGen/cfi-check-fail.c
@@ -1,4 +1,5 @@
-// 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
Index: lib/CodeGen/CGExpr.cpp
===
--- lib/CodeGen/CGExpr.cpp
+++ lib/CodeGen/CGExpr.cpp
@@ -2483,14 +2483,12 @@
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
@@ -2672,8 +2670,11 @@
SanitizerMask Mask = CheckKindMaskPair.second;
llvm::Value *Cond =
Builder.CreateICmpNE(CheckKind, llvm::ConstantInt::get(Int8Ty,
Re: [PATCH] D17360: [cfi] Fix handling of sanitize trap/recover flags in the cross-DSO CFI mode.
pcc added a comment. 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 http://reviews.llvm.org/D15699. Repository: rL LLVM http://reviews.llvm.org/D17360 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D17360: [cfi] Fix handling of sanitize trap/recover flags in the cross-DSO CFI mode.
eugenis added a comment. 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? Repository: rL LLVM http://reviews.llvm.org/D17360 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D17360: [cfi] Fix handling of sanitize trap/recover flags in the cross-DSO CFI mode.
pcc added a comment. 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?) Repository: rL LLVM http://reviews.llvm.org/D17360 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D17360: [cfi] Fix handling of sanitize trap/recover flags in the cross-DSO CFI mode.
eugenis created this revision.
eugenis added reviewers: pcc, krasin.
eugenis added a subscriber: cfe-commits.
eugenis set the repository for this revision to rL LLVM.
In the cross-DSO CFI mode a module may be asked to handle any type of
CFI error, even if the module itself is not checked for that type of
error. Therefore, trap/recover flags should be preserved all CFI
checkers and not just for the ones that are enabled.
This fixes a linker error caused by the missing cfi_diag runtime
library with certain combinations of CFI flags (see the new test
case).
Repository:
rL LLVM
http://reviews.llvm.org/D17360
Files:
lib/Driver/SanitizerArgs.cpp
test/Driver/fsanitize.c
Index: test/Driver/fsanitize.c
===
--- test/Driver/fsanitize.c
+++ test/Driver/fsanitize.c
@@ -272,6 +272,21 @@
// CHECK-CFI-NO-CROSS-DSO: -emit-llvm-bc
// CHECK-CFI-NO-CROSS-DSO-NOT: -fsanitize-cfi-cross-dso
+// In the non-cross-dso CFI mode, -fsanitize-trap only appears for enabled CFI
checkers.
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=cfi-vcall -c %s -### 2>&1 |
FileCheck %s --check-prefix=CHECK-CFI-VCALL-TRAP
+// CHECK-CFI-VCALL-TRAP: "-fsanitize=cfi-vcall" "-fsanitize-trap=cfi-vcall"
+
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=cfi-vcall
-fno-sanitize-trap=cfi-vcall -c %s -### 2>&1 | FileCheck %s
--check-prefix=CHECK-CFI-VCALL-NO-TRAP
+// CHECK-CFI-VCALL-NO-TRAP: "-fsanitize=cfi-vcall"
+// CHECK-CFI-VCALL-NO-TRAP-NOT: -fsanitize-trap=
+
+// In the cross-dso CFI mode, -fsanitize-trap appears for all CFI checkers.
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=cfi-vcall
-fsanitize-cfi-cross-dso -flto -c %s -### 2>&1 | FileCheck %s
--check-prefix=CHECK-CFI-CROSS-DSO-VCALL-TRAP
+// CHECK-CFI-CROSS-DSO-VCALL-TRAP: "-fsanitize=cfi-vcall"
"-fsanitize-trap=cfi-derived-cast,cfi-icall,cfi-unrelated-cast,cfi-nvcall,cfi-vcall"
+
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=cfi-vcall
-fno-sanitize-trap=cfi-vcall -fsanitize-cfi-cross-dso -flto -c %s -### 2>&1 |
FileCheck %s --check-prefix=CHECK-CFI-CROSS-DSO-VCALL-NO-TRAP
+// CHECK-CFI-CROSS-DSO-VCALL-NO-TRAP: "-fsanitize=cfi-vcall"
"-fsanitize-trap=cfi-derived-cast,cfi-icall,cfi-unrelated-cast,cfi-nvcall"
+
// RUN: %clang -target x86_64-linux-gnu -fsanitize=cfi -fsanitize-stats -flto
-c %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-CFI-STATS
// CHECK-CFI-STATS: -fsanitize-stats
Index: lib/Driver/SanitizerArgs.cpp
===
--- lib/Driver/SanitizerArgs.cpp
+++ lib/Driver/SanitizerArgs.cpp
@@ -166,11 +166,11 @@
}
bool SanitizerArgs::needsCfiRt() const {
- return !(Sanitizers.Mask & CFI & ~TrapSanitizers.Mask) && CfiCrossDso;
+ return !(CFI & ~TrapSanitizers.Mask) && CfiCrossDso;
}
bool SanitizerArgs::needsCfiDiagRt() const {
- return (Sanitizers.Mask & CFI & ~TrapSanitizers.Mask) && CfiCrossDso;
+ return (CFI & ~TrapSanitizers.Mask) && CfiCrossDso;
}
bool SanitizerArgs::requiresPIE() const {
@@ -361,10 +361,27 @@
<< DeprecatedReplacement;
}
}
- RecoverableKinds &= Kinds;
- RecoverableKinds &= ~Unrecoverable;
- TrappingKinds &= Kinds;
+ if (AllAddedKinds & CFI) {
+CfiCrossDso = Args.hasFlag(options::OPT_fsanitize_cfi_cross_dso,
+ options::OPT_fno_sanitize_cfi_cross_dso, false);
+// Without PIE, external function address may resolve to a PLT record,
which
+// can not be verified by the target module.
+NeedPIE |= CfiCrossDso;
+ }
+
+ // In the cross-DSO CFI mode a module may be asked to handle any type of CFI
+ // error, even if the module itself is not checked for that type of error.
+ // Therefore, trap/recover flags should be preserved all CFI checkers.
+ if (CfiCrossDso) {
+TrappingKinds &= (Kinds | CFI);
+RecoverableKinds &= (Kinds | CFI);
+ } else {
+TrappingKinds &= Kinds;
+RecoverableKinds &= Kinds;
+ }
+
+ RecoverableKinds &= ~Unrecoverable;
// Setup blacklist files.
// Add default blacklist from resource directory.
@@ -424,14 +441,6 @@
TC.getTriple().getArch() == llvm::Triple::x86_64);
}
- if (AllAddedKinds & CFI) {
-CfiCrossDso = Args.hasFlag(options::OPT_fsanitize_cfi_cross_dso,
- options::OPT_fno_sanitize_cfi_cross_dso, false);
-// Without PIE, external function address may resolve to a PLT record,
which
-// can not be verified by the target module.
-NeedPIE |= CfiCrossDso;
- }
-
Stats = Args.hasFlag(options::OPT_fsanitize_stats,
options::OPT_fno_sanitize_stats, false);
Index: test/Driver/fsanitize.c
===
--- test/Driver/fsanitize.c
+++ test/Driver/fsanitize.c
@@ -272,6 +272,21 @@
// CHECK-CFI-NO-CROSS-DSO: -emit-llvm-bc
// CHECK-CFI-NO-CROSS-DSO-NOT: -fsanitize-cfi-cross-dso
+// In the
