Messages in this thread | | | Date | Tue, 9 Nov 2021 19:02:21 +0000 | From | Quentin Perret <> | Subject | Re: [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
| |