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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
