[PATCH] D148785: -fsanitize=function: use type hashes instead of RTTI objects

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

Should `HANDLER(__ubsan_handle_function_type_mismatch,"function")` be added to ubsan_minimal_runtime if this is supported in the minimal runtime?



================
Comment at: clang/lib/CodeGen/CGExpr.cpp:5382
+          getPointerAlign());
       llvm::Value *CalleeRTTIMatch =
+          Builder.CreateICmpEQ(CalleeTypeHash, TypeHash);
----------------
Would CalleeTypeHashMatch be a better name?


================
Comment at: clang/lib/CodeGen/CodeGenFunction.h:120
   SANITIZER_CHECK(FloatCastOverflow, float_cast_overflow, 0)                   \
-  SANITIZER_CHECK(FunctionTypeMismatch, function_type_mismatch, 1)             \
+  SANITIZER_CHECK(FunctionTypeMismatch, function_type_mismatch, 0)             \
   SANITIZER_CHECK(ImplicitConversion, implicit_conversion, 0)                  \
----------------
Presumably the signature is different to the original v0 shouldn't it be 2; or is it effectively so long since the last one that we can reuse the original without fear?


================
Comment at: clang/test/CodeGenCXX/catch-undef-behavior.cpp:408
 
   // RTTI pointer check
+  // CHECK: [[CalleeTypeHashPtr:%.+]] = getelementptr <{ i32, i32 }>, <{ i32, i32 }>* [[PTR]], i32 -1, i32 1
----------------
CalleeTypeHash check?


================
Comment at: llvm/test/CodeGen/AArch64/patchable-function-entry-bti.ll:92
 ; CHECK-NEXT: .Ltmp{{.*}}:
 ; CHECK-NEXT:   nop
 ; CHECK-NEXT:   .word   3238382334  // 0xc105cafe
----------------
Assuming the test is the equivalent of `-fpatchable-function-entry=1,1` I think this is the wrong place for the nop, I think it needs to be after the signature and the loads adjusted. For example with -fsanitize=kcfi -fpatchable-function-entries=1,1
```
typedef int Fptr(void);

int function(void) {
  return 0;
}

int call(Fptr* fp) {
  return fp();
}
```
Results in code like:
```
        .word   1670944012                      // @call
                                        // 0x6398950c
.Ltmp1:
        nop
call:
.Lfunc_begin1:
        .cfi_startproc
// %bb.0:                               // %entry
        ldur    w16, [x0, #-8]
        movk    w17, #50598
        movk    w17, #14001, lsl #16
        cmp     w16, w17
        b.eq    .Ltmp2
        brk     #0x8220
.Ltmp2:
        br      x0
.Lfunc_end1:
```
Note the NOP is after the signature, with the `ldur` having an offset of -8 and not the usual -4. I think you would need to make sure the signature is a branch instruction for each target for this scheme to work.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148785/new/

https://reviews.llvm.org/D148785