| Message ID | 20211129135709.2274019-1-mark.rutland@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: ftrace: add missing BTIs | expand |
On Mon, Nov 29, 2021 at 01:57:09PM +0000, Mark Rutland wrote: > When branch target identifiers are in use, code reachable via an > indirect branch requires a BTI landing pad at the branch target site. Reviewed-by: Mark Brown <broonie@kernel.org> > In future we may wish to consider adding a new SYM_CODE_START_*() > variant which has an implicit BTI. > +#ifdef BTI_C > + BTI_C > +#endif The ifdefs here feel ugly enough that it might be worth doing that right now TBH. I'm trying to think of any cases where we might also need a BTI J but nothing springs to mind right now.
On Mon, Nov 29, 2021 at 04:50:39PM +0000, Mark Brown wrote: > On Mon, Nov 29, 2021 at 01:57:09PM +0000, Mark Rutland wrote: > > > When branch target identifiers are in use, code reachable via an > > indirect branch requires a BTI landing pad at the branch target site. > > Reviewed-by: Mark Brown <broonie@kernel.org> Cheers! > > In future we may wish to consider adding a new SYM_CODE_START_*() > > variant which has an implicit BTI. > > > +#ifdef BTI_C > > + BTI_C > > +#endif > > The ifdefs here feel ugly enough that it might be worth doing that right > now TBH. I'm trying to think of any cases where we might also need a > BTI J but nothing springs to mind right now. Agreed on the ugliness -- I'd like to revisit that with some related cleanup/improvement to our existing SYM_*() macros. I just didn't want to do that as a prerequisite for the fix as it'd make backports painful, e.g. by creating a dependency on commit: 1cbdf60bd1b74e39 ("kasan: arm64: support specialized outlined tag mismatch checks") ... which uses the ifdef pattern above. I'm also not sure what naming/structure we'd like, or whether it's simpler to unconditionally define BTI_C. Thanks, Mark.
On Mon, Nov 29, 2021 at 05:37:51PM +0000, Mark Rutland wrote: > On Mon, Nov 29, 2021 at 04:50:39PM +0000, Mark Brown wrote: > > > +#ifdef BTI_C > > > + BTI_C > > > +#endif > > The ifdefs here feel ugly enough that it might be worth doing that right > > now TBH. I'm trying to think of any cases where we might also need a > > BTI J but nothing springs to mind right now. > Agreed on the ugliness -- I'd like to revisit that with some related > cleanup/improvement to our existing SYM_*() macros. I just didn't want to do > that as a prerequisite for the fix as it'd make backports painful, e.g. by > creating a dependency on commit: > 1cbdf60bd1b74e39 ("kasan: arm64: support specialized outlined tag mismatch checks") > ... which uses the ifdef pattern above. Ugh, that's not great. But yes, backporting was why I did the Reviwed-by. If we knew the names we were going for it'd be easy enough to add a new SYM_CODE_ varaint without introducing dependencies but that might be getting ahead of things. > I'm also not sure what naming/structure we'd like, or whether it's simpler to > unconditionally define BTI_C. The unconditional definition seems like it might be neater as a minimally intrusive thing for backporting, we can always refactor later.
On Mon, Nov 29, 2021 at 05:48:46PM +0000, Mark Brown wrote: > On Mon, Nov 29, 2021 at 05:37:51PM +0000, Mark Rutland wrote: > > On Mon, Nov 29, 2021 at 04:50:39PM +0000, Mark Brown wrote: > > > > > +#ifdef BTI_C > > > > + BTI_C > > > > +#endif > > > > The ifdefs here feel ugly enough that it might be worth doing that right > > > now TBH. I'm trying to think of any cases where we might also need a > > > BTI J but nothing springs to mind right now. > > > Agreed on the ugliness -- I'd like to revisit that with some related > > cleanup/improvement to our existing SYM_*() macros. I just didn't want to do > > that as a prerequisite for the fix as it'd make backports painful, e.g. by > > creating a dependency on commit: > > > 1cbdf60bd1b74e39 ("kasan: arm64: support specialized outlined tag mismatch checks") > > > ... which uses the ifdef pattern above. > > Ugh, that's not great. But yes, backporting was why I did the Reviwed-by. Sure, and thanks for that! > If we knew the names we were going for it'd be easy enough to add a new > SYM_CODE_ varaint without introducing dependencies but that might be > getting ahead of things. > > > I'm also not sure what naming/structure we'd like, or whether it's simpler to > > unconditionally define BTI_C. > > The unconditional definition seems like it might be neater as a > minimally intrusive thing for backporting, we can always refactor later. The problem with that is that for consistency we should remove the ifdeferry from the kasan trampoline at the same time, which either means an unnecessary dependency as above, or splitting this into three patches, one of which should not be backported. Given that, I think that as-is this is the simplest form for backporting. I completely agree the ifdeferry is ugly, and I do intend to follow up on that regardless. If you feel strongly that we should get rid of that now, I can spin a v2 with a second patch to clean that up; otherwise I'll do that as part of a later series. Thanks, Mark.
On Mon, 29 Nov 2021 13:57:09 +0000, Mark Rutland wrote: > When branch target identifiers are in use, code reachable via an > indirect branch requires a BTI landing pad at the branch target site. > > When building FTRACE_WITH_REGS atop patchable-function-entry, we miss > BTIs at the start start of the `ftrace_caller` and `ftrace_regs_caller` > trampolines, and when these are called from a module via a PLT (which > will use a `BR X16`), we will encounter a BTI failure, e.g. > > [...] Applied to arm64 (for-next/fixes), thanks! [1/1] arm64: ftrace: add missing BTIs https://git.kernel.org/arm64/c/35b6b28e6998 Cheers,
On Thu, Dec 02, 2021 at 10:43:39AM +0000, Mark Rutland wrote: > On Mon, Nov 29, 2021 at 05:48:46PM +0000, Mark Brown wrote: > > The unconditional definition seems like it might be neater as a > > minimally intrusive thing for backporting, we can always refactor later. > The problem with that is that for consistency we should remove the ifdeferry > from the kasan trampoline at the same time, which either means an unnecessary > dependency as above, or splitting this into three patches, one of which should > not be backported. Or just two, this one and a cleanup.
On Thu, Dec 02, 2021 at 12:34:05PM +0000, Mark Brown wrote: > On Thu, Dec 02, 2021 at 10:43:39AM +0000, Mark Rutland wrote: > > On Mon, Nov 29, 2021 at 05:48:46PM +0000, Mark Brown wrote: > > > > The unconditional definition seems like it might be neater as a > > > minimally intrusive thing for backporting, we can always refactor later. > > > The problem with that is that for consistency we should remove the ifdeferry > > from the kasan trampoline at the same time, which either means an unnecessary > > dependency as above, or splitting this into three patches, one of which should > > not be backported. > > Or just two, this one and a cleanup. Sure, a follow-up cleanup is fine, but critically the cleanup need not be backported, which is why I said: | I completely agree the ifdeferry is ugly, and I do intend to follow up on | that regardless. If you feel strongly that we should get rid of that now, I | can spin a v2 with a second patch to clean that up; otherwise I'll do that as | part of a later series. Since Will has taken this patch as-is, and the fixes for v5.16-rc4 have already been tagged, I'll follow up with cleanup once that's merged to avoid being based on an ephemeral branch. Thanks, Mark.
On Fri, Dec 03, 2021 at 11:50:44AM +0000, Mark Rutland wrote: > Since Will has taken this patch as-is, and the fixes for v5.16-rc4 have already > been tagged, I'll follow up with cleanup once that's merged to avoid being > based on an ephemeral branch. Oh, I already sent the cleanup before I saw this message. Hopefully that's fine for getting things sorted anyway. I'd actually been sitting on a patch adding the empty definition since forever waiting for users.
diff --git a/arch/arm64/kernel/entry-ftrace.S b/arch/arm64/kernel/entry-ftrace.S index b3e4f9a088b1..8cf970d219f5 100644 --- a/arch/arm64/kernel/entry-ftrace.S +++ b/arch/arm64/kernel/entry-ftrace.S @@ -77,11 +77,17 @@ .endm SYM_CODE_START(ftrace_regs_caller) +#ifdef BTI_C + BTI_C +#endif ftrace_regs_entry 1 b ftrace_common SYM_CODE_END(ftrace_regs_caller) SYM_CODE_START(ftrace_caller) +#ifdef BTI_C + BTI_C +#endif ftrace_regs_entry 0 b ftrace_common SYM_CODE_END(ftrace_caller)
When branch target identifiers are in use, code reachable via an indirect branch requires a BTI landing pad at the branch target site. When building FTRACE_WITH_REGS atop patchable-function-entry, we miss BTIs at the start start of the `ftrace_caller` and `ftrace_regs_caller` trampolines, and when these are called from a module via a PLT (which will use a `BR X16`), we will encounter a BTI failure, e.g. | # insmod lkdtm.ko | lkdtm: No crash points registered, enable through debugfs | # echo function_graph > /sys/kernel/debug/tracing/current_tracer | # cat /sys/kernel/debug/provoke-crash/DIRECT | Unhandled 64-bit el1h sync exception on CPU0, ESR 0x34000001 -- BTI | CPU: 0 PID: 174 Comm: cat Not tainted 5.16.0-rc2-dirty #3 | Hardware name: linux,dummy-virt (DT) | pstate: 60400405 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=jc) | pc : ftrace_caller+0x0/0x3c | lr : lkdtm_debugfs_open+0xc/0x20 [lkdtm] | sp : ffff800012e43b00 | x29: ffff800012e43b00 x28: 0000000000000000 x27: ffff800012e43c88 | x26: 0000000000000000 x25: 0000000000000000 x24: ffff0000c171f200 | x23: ffff0000c27b1e00 x22: ffff0000c2265240 x21: ffff0000c23c8c30 | x20: ffff8000090ba380 x19: 0000000000000000 x18: 0000000000000000 | x17: 0000000000000000 x16: ffff80001002bb4c x15: 0000000000000000 | x14: 0000000000000000 x13: 0000000000000000 x12: 0000000000900ff0 | x11: ffff0000c4166310 x10: ffff800012e43b00 x9 : ffff8000104f2384 | x8 : 0000000000000001 x7 : 0000000000000000 x6 : 000000000000003f | x5 : 0000000000000040 x4 : ffff800012e43af0 x3 : 0000000000000001 | x2 : ffff8000090b0000 x1 : ffff0000c171f200 x0 : ffff0000c23c8c30 | Kernel panic - not syncing: Unhandled exception | CPU: 0 PID: 174 Comm: cat Not tainted 5.16.0-rc2-dirty #3 | Hardware name: linux,dummy-virt (DT) | Call trace: | dump_backtrace+0x0/0x1a4 | show_stack+0x24/0x30 | dump_stack_lvl+0x68/0x84 | dump_stack+0x1c/0x38 | panic+0x168/0x360 | arm64_exit_nmi.isra.0+0x0/0x80 | el1h_64_sync_handler+0x68/0xd4 | el1h_64_sync+0x78/0x7c | ftrace_caller+0x0/0x3c | do_dentry_open+0x134/0x3b0 | vfs_open+0x38/0x44 | path_openat+0x89c/0xe40 | do_filp_open+0x8c/0x13c | do_sys_openat2+0xbc/0x174 | __arm64_sys_openat+0x6c/0xbc | invoke_syscall+0x50/0x120 | el0_svc_common.constprop.0+0xdc/0x100 | do_el0_svc+0x84/0xa0 | el0_svc+0x28/0x80 | el0t_64_sync_handler+0xa8/0x130 | el0t_64_sync+0x1a0/0x1a4 | SMP: stopping secondary CPUs | Kernel Offset: disabled | CPU features: 0x0,00000f42,da660c5f | Memory Limit: none | ---[ end Kernel panic - not syncing: Unhandled exception ]--- Fix this by adding the required `BTI C`, as we only require these to be reachable via BL for direct calls or BR X16/X17 for PLTs. For now, these are open-coded in the function prologue, matching the style of the `__hwasan_tag_mismatch` trampoline. In future we may wish to consider adding a new SYM_CODE_START_*() variant which has an implicit BTI. When ftrace is built atop mcount, the trampolines are marked with SYM_FUNC_START(), and so get an implicit BTI. We may need to change these over to SYM_CODE_START() in future for RELIABLE_STACKTRACE, in case we need to apply special care aroud the return address being rewritten. Fixes: 97fed779f2a68937 ("arm64: bti: Provide Kconfig for kernel mode BTI") Signed-off-by: Mark Rutland <mark.rutland@arm.com> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Mark Brown <broonie@kernel.org> Cc: Will Deacon <will@kernel.org> --- arch/arm64/kernel/entry-ftrace.S | 6 ++++++ 1 file changed, 6 insertions(+)