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