peter.smith 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
----------------
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
================
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:
> 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.rutl...@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.
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
