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

Fri May 19 20:34:15 PDT 2023

MaskRay added inline comments.

================
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
----------------
peter.smith wrote:
> MaskRay wrote:
> > peter.smith wrote:
> > > CalleeTypeHash check?
> > `CalleeTypeHashPtr` seems clear. Do you mean to change `HashCmp` below to `CalleeTypeHashMatch`?
> > 
> > 
> Apologies; I meant the comment is stale, it still says RTTI pointer check
Sorry for missing the stale comment. Fixed!

================
Comment at: llvm/test/CodeGen/AArch64/patchable-function-entry-bti.ll:92
 ; CHECK-NEXT: .Ltmp{{.*}}:
 ; CHECK-NEXT:   nop
 ; CHECK-NEXT:   .word   3238382334  // 0xc105cafe
----------------
peter.smith wrote:
> MaskRay wrote:
> > MaskRay wrote:
> > > peter.smith wrote:
> > > > samitolvanen wrote:
> > > > > peter.smith wrote:
> > > > > > 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.
> > > > > No, this looks correct to me. Note that in AsmPrinter the type hash is emitted after the patchable-function-prefix nops, while the KCFI type hash is emitted before them.
> > > > My concern is more along the lines of how would this function be patched if the code doing the patching were unaware of the signature. I'm not familiar with how the kernel uses the nops so it could be that this is won't be a problem in practice.
> > > > 
> > > > With -fsanitize=kcfi it looks like there is no difference to the code doing the patching as the nops are in the same place with respect to the function entry point and there is no fall through into the signature.
> > > > 
> > > > With -fsanitize=function anything patching the first nop as an instruction would need to branch over the signature (unless the first entry of the signature was a branch instruction, but that would mean a target specific signature), obviously if the nop is patched with data and isn't the entry point then this doesn't matter. The code doing the patching would also need to know how to locate the nop ahead of the signature.
> > > The responsibility is on the user side to ensure that when M>0, they code patches one of the M NOPs to a branch over the signature (0xc105cafe). 
> > > 
> > > ```
> > > // -fsanitize=function -fpatchable-function-entry=3,3
> > > .Ltmp0:
> > >         nop
> > >         nop
> > >         nop    # ensure there is a branch over the signature
> > >         .long   3238382334                      # 0xc105cafe
> > >         .long   2772461324                      # 0xa540670c
> > > foo:
> > > ```
> > > 
> > > In addition, `-fpatchable-function-entry=N,M` where M>0 is uncommon. `-fpatchable-function-entry=` didn't work with `-fsanitize=function` before my changes.
> > > 
> > > My concern is more along the lines of how would this function be patched if the code doing the patching were unaware of the signature. I'm not familiar with how the kernel uses the nops so it could be that this is won't be a problem in practice.
> > 
> > I think the patching code must be aware if the user decides to use `-fpatchable-function-entry=N,M` where `M>0`, otherwise it's the pilot error to use the option with `-fsanitize=function`.
> > 
> > `-fsanitize=function` is designed to be compatible with uninstrumented functions (used as indirect call callees) and compatible with object files,  `-fpatchable-function-entry=N,M` functions, and regular functions. The call site must use the fixed offset unaffected by `-fpatchable-function-entry=N,M`.
> I don't have any objections here. Just wanted to make sure that we weren't breaking any expectations.
> 
> I found one use in the kernel that uses -fpatchable-function-entry=4,2  (https://lore.kernel.org/all/20230123134603.1064407-9-mark.rutland@arm.com/) 
> 
> Quoting from that:
> ```
> Currently, this approach is not compatible with CLANG_CFI, as the
> presence/absence of pre-function NOPs changes the offset of the
> pre-function type hash, and there's no existing mechanism to ensure a
> consistent offset for instrumented and uninstrumented functions. When
> CLANG_CFI is enabled, the existing scheme with a global ops->func
> pointer is used, and there should be no functional change. I am
> currently working with others to allow the two to work together in
> future (though this will liekly require updated compiler support).
> ```
> 
> I expect -fsanitize=kcfi to be used in this case over -fsanitize=functions so I don't think this will cause a problem in practice.
> I expect -fsanitize=kcfi to be used in this case over -fsanitize=functions so I don't think this will cause a problem in practice.

Yes, the kernel requires the addresses of the instrumented call sites which can only be provided by -fsanitize=kcfi. -fsanitize=function instrumented call sites are not recorded in a metadata section.
The kernel doesn't have a -fsanitize=function configuration.

For userspace programs that want to attempt both -fpatchable-function-entry=N,M where M>0 and -fsanitize=function, I expect that they need to check the -fsanitize=function signature...

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148785