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.
eugenis updated this revision to Diff 50379. eugenis marked 2 inline comments as done. 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 @@ -2481,16 +2481,12 @@ 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 @@ -2672,8 +2668,11 @@ SanitizerMask Mask = CheckKindMaskPair.second; llvm::Value *Cond = Builder.CreateICmpNE(CheckKind, llvm::ConstantInt::get(Int8Ty, Kind)); -EmitCheck(std::make_pair(Cond, Mask), "cfi_check_fail", {}, - {Data, Addr, ValidVtable}); +if (CGM.getLangOpts().Sanitize.h
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, K
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