Re: kCFI && patchable-function-entry=M,N

[Sami Tolvanen]

On Mon, Oct 24, 2022 at 4:18 AM Mark Rutland <mark.rutland@xxxxxxx> wrote:
>
> On Fri, Oct 21, 2022 at 09:14:41PM -0700, Fangrui Song wrote:
> > On Fri, Oct 21, 2022 at 10:39 AM Sami Tolvanen <samitolvanen@xxxxxxxxxx> wrote:
> > >
> > > On Fri, Oct 21, 2022 at 8:56 AM Mark Rutland <mark.rutland@xxxxxxx> wrote:
> > > >
> > > > Hi,
> > > >
> > > > For arm64, I'd like to use -fatchable-function-entry=M,N (where N > 0), for our
> > > > ftrace implementation, which instruments *some* but not all functions.
> > > > Unfortuntately, this doesn't play nicely with -fsanitize=kcfi, as instrumented
> > > > and non-instrumented functions don't agree on where the type hash should live
> > > > relative to the function entry point, making them incompatible with one another.
> > >
> > > Yes, the current implementation assumes that if prefix nops are used,
> > > all functions have the same number of them.
> > >
> > > > Is there any mechanism today that we could use to solve this, or could we
> > > > extend clang to have some options to control this behaviour?
> > >
> > > I don't think there's a mechanism to work around the issue right now,
> > > but we could just change where the hash is emitted on arm64.
> > >
> > > > It would also be helpful to have a symbol before both the hash and pre-function
> > > > NOPs so that we can filter those out of probes patching (I see that x86 does
> > > > this with the __cfi_function symbol).
> > >
> > > Adding a symbol before the hash isn't a problem, but if we move the
> > > hash and want the symbol to be placed before the prefix nops as well,
> > > we might need a flag to control this. Fangrui, what do you think?
> > >
> > > Sami
> >
> > Since the kcfi code expects the hash to appear in a specific location
> > so that an instrumented indirect jump can reliably obtain the hash.
> > For a translation unit `-fpatchable-function-entry=N,M` may be
> > specified or not, and we want both to work. Therefore, I agree that a
> > consistent hash location will help. This argument favors placing M
> > nops before the hash. The downside is a restriction on how the M nops
> > can be used. Previously if M>0, the runtime code needs to check
> > whether a BTI exists to locate the N-M after-function-entry NOPs. If
> > the hash appears after the M nops, the runtime code needs to
> > additionally knows whether the hash exists. My question is how to
> > reliably detect this.
>
> That's a fair point.
>
> For detecting BTI we can scan the binary for BTI/NOP at M instructions into the
> patch-site, but a similar approach won't be reliable for the type hash since
> the type hash itself could have the same bit pattern as an instruction.

We could always change the compiler to ensure the hash doesn't match
specific instructions. For example, if the hash can never be a NOP,
you could just skip any non-NOP instructions after the initial prefix
NOPs, right?

> > If there is motivation using M>0, I'd like to know the concrete code
> > sequence for `-fpatchable-function-entry=N,M` and how the runtime code
> > detects the NOPs with optional hash and optional BTI.
>
> For the BTI case alone, I have code at:
>
>   https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/commit/?h=arm64/ftrace/per-callsite-ops&id=272a580fd5b7acc31747505d71530cee7cc2837d
>
> ... the gist being that it checks the instruction M insns after the start of
> the patch site.
>
> For the type hash, I think there are a few options, e.g.
>
> * Restricting the type hash to a set of values that can be identified (e.g.
>   encoding those as a permanently-undefined UDF with a 16-bit immediate).

Which would be quite similar, except instead of restricting hash
values to a specific range, we'd just have a list of unallowed hash
values.

Sami