lkml.org 
[lkml]   [2021]   [Nov]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v6 2/2] arm64: implement support for static call trampolines
On Tuesday 09 Nov 2021 at 19:09:21 (+0100), Ard Biesheuvel wrote:
> On Tue, 9 Nov 2021 at 18:55, Mark Rutland <mark.rutland@arm.com> wrote:
> >
> > Hi Ard,
> >
> > On Fri, Nov 05, 2021 at 03:59:17PM +0100, Ard Biesheuvel wrote:
> > > +static void *strip_cfi_jt(void *addr)
> > > +{
> > > + if (IS_ENABLED(CONFIG_CFI_CLANG)) {
> > > + void *p = addr;
> > > + u32 insn;
> > > +
> > > + /*
> > > + * Taking the address of a function produces the address of the
> > > + * jump table entry when Clang CFI is enabled. Such entries are
> > > + * ordinary jump instructions, preceded by a BTI C instruction
> > > + * if BTI is enabled for the kernel.
> > > + */
> > > + if (IS_ENABLED(CONFIG_ARM64_BTI_KERNEL))
> > > + p += 4;
> > > +
> > > + insn = le32_to_cpup(p);
> > > + if (aarch64_insn_is_b(insn))
> > > + return p + aarch64_get_branch_offset(insn);
> > > +
> > > + WARN_ON(1);
> > > + }
> > > + return addr;
> > > +}
> >
> > I'm somewhat uncomfortable with this, because it seems like the compiler could
> > easily violate our expectations in future, and then we're in for a massive
> > headache. I assume clang doesn't provide any guarnatee as to the format of the
> > jump table entries (and e.g. I can see scope for branch padding breaking this).
> >
> > In trying to sidestep that I ended up with:
> >
> > https://lore.kernel.org/linux-arm-kernel/20211109172408.49641-1-mark.rutland@arm.com/
> >
> > ... which I think is a good option for PREEMPT_DYNAMIC, but I don't know if
> > there were other places where we believe static calls would be critical for
> > performance rather than a nice-to-have, and whether we truly need static calls
> > on arm64. My mind is leaning towards "avoid if reasonable" at the moment (or at
> > least make that mutually exclusive with CFI so we can avoid that specific fun).
> >
> > I see you had at least one other user in:
> >
> > https://lore.kernel.org/r/20211109120336.3561463-1-ardb@kernel.org
> >
> > ... what were your thoughts on the criticality of that?
> >
>
> That particular use case does not rely on static calls being fast at
> all, so there it doesn't really matter which variety we implement. The
> reason I sent it out today is because it gives some test coverage for
> static calls used in a way that the API as designed should support,
> but which turned out to be slightly broken in practice.
>
> > FWIW other than the above this looks good to me. My major concern here is
> > fragility/maintenance, and secondly whether we're gaining much in practice. So
> > if you think we really need this, I'm not going to stand in the way.
> >
>
> Android relies heavily on tracepoints for vendor hooks, and given the
> performance impact of CFI on indirect calls, there has been interest
> in enabling static calls to replace them.
>
> Quentin, anything to add here?

Yes, Android should definitely benefit from static calls.

Modules attaching to tracepoints cause a measurable overhead w/ CFI as
the jump target is a bit harder to verify if it is not in-kernel. But
sadly that's a common pattern for GKI. The current 'workaround' in
Android has been to just plain disable CFI around all tracepoints in the
kernel, which is a bit sad from a security PoV. But there was really no
other option at the time, and we needed the performance back. Static
calls would be a far superior solution as they would avoid much of the
CFI overhead, and are not vulnerable in the CFI sense (that is, the
branch target can't be easily overridden with a random OOB write from a
dodgy driver). So yes, we'd really like to have those please :)

Thanks,
Quentin

\
 
 \ /
  Last update: 2021-11-09 20:03    [W:0.100 / U:0.544 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site