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
----------------
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`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D148785/new/
https://reviews.llvm.org/D148785
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
