[PATCH] arm64: ftrace: add missing BTIs

[PATCH] arm64: ftrace: add missing BTIs

[Mark Rutland]

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(+)

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)
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: ftrace: add missing BTIs

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 580 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: ftrace: add missing BTIs

[Mark Rutland]

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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: ftrace: add missing BTIs

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1262 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: ftrace: add missing BTIs

[Mark Rutland]

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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: ftrace: add missing BTIs

[Will Deacon]

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,
-- 
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: ftrace: add missing BTIs

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 570 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: ftrace: add missing BTIs

[Mark Rutland]

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.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: ftrace: add missing BTIs

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 469 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel