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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.


================
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:
> 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.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148785