[PATCH] arm64: Don't insert a BTI instruction at inner labels

[PATCH] arm64: Don't insert a BTI instruction at inner labels

[Jean-Philippe Brucker]

Some ftrace features are broken since commit 714a8d02ca4d ("arm64: asm:
Override SYM_FUNC_START when building the kernel with BTI"). For example
the function_graph tracer:

$ echo function_graph > /sys/kernel/debug/tracing/current_tracer
[   36.107016] WARNING: CPU: 0 PID: 115 at kernel/trace/ftrace.c:2691 ftrace_modify_all_code+0xc8/0x14c

When ftrace_modify_graph_caller() attempts to write a branch at
ftrace_graph_call, it finds the "BTI J" instruction inserted by
SYM_INNER_LABEL() instead of a NOP, and aborts.

It turns out we don't currently need the BTI landing pads inserted by
SYM_INNER_LABEL:

* ftrace_call and ftrace_graph_call are only used for runtime patching
  of the active tracer. The patched code is not reached from a branch.
* install_el2_stub is reached from a CBZ instruction, which doesn't
  change PSTATE.BTYPE.
* __guest_exit is reached from B instructions in the hyp-entry vectors,
  which aren't subject to BTI checks either.

Remove the BTI annotation from SYM_INNER_LABEL.

Fixes: 714a8d02ca4d ("arm64: asm: Override SYM_FUNC_START when building the kernel with BTI")
Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
Tested on QEMU with and without BTI, but only ftrace not KVM.
---
 arch/arm64/include/asm/linkage.h | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/arch/arm64/include/asm/linkage.h b/arch/arm64/include/asm/linkage.h
index 81fefd2a1d023..ba89a9af820ab 100644
--- a/arch/arm64/include/asm/linkage.h
+++ b/arch/arm64/include/asm/linkage.h
@@ -12,7 +12,6 @@
  * instead.
  */
 #define BTI_C hint 34 ;
-#define BTI_J hint 36 ;
 
 /*
  * When using in-kernel BTI we need to ensure that PCS-conformant assembly
@@ -43,11 +42,6 @@
 	SYM_START(name, SYM_L_WEAK, SYM_A_NONE)		\
 	BTI_C
 
-#define SYM_INNER_LABEL(name, linkage)			\
-	.type name SYM_T_NONE ASM_NL			\
-	SYM_ENTRY(name, linkage, SYM_A_NONE)		\
-	BTI_J
-
 #endif
 
 /*
-- 
2.27.0


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

Re: [PATCH] arm64: Don't insert a BTI instruction at inner labels

[Mark Brown]

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

On Wed, Jun 24, 2020 at 01:22:54PM +0200, Jean-Philippe Brucker wrote:

> It turns out we don't currently need the BTI landing pads inserted by
> SYM_INNER_LABEL:

> * ftrace_call and ftrace_graph_call are only used for runtime patching
>   of the active tracer. The patched code is not reached from a branch.
> * install_el2_stub is reached from a CBZ instruction, which doesn't
>   change PSTATE.BTYPE.
> * __guest_exit is reached from B instructions in the hyp-entry vectors,
>   which aren't subject to BTI checks either.

> Remove the BTI annotation from SYM_INNER_LABEL.

This fixes things for now but it feels like it's going to be fragile in
the long run since inner labels are a bit of a wild west in terms of how
they're going to be referenced - I can't think of a better solution at
this level though :(

Reviewed-by: Mark Brown <broonie@kernel.org>

[-- 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: Don't insert a BTI instruction at inner labels

[Dave Martin]

On Wed, Jun 24, 2020 at 02:21:14PM +0100, Mark Brown wrote:
> On Wed, Jun 24, 2020 at 01:22:54PM +0200, Jean-Philippe Brucker wrote:
> 
> > It turns out we don't currently need the BTI landing pads inserted by
> > SYM_INNER_LABEL:
> 
> > * ftrace_call and ftrace_graph_call are only used for runtime patching
> >   of the active tracer. The patched code is not reached from a branch.
> > * install_el2_stub is reached from a CBZ instruction, which doesn't
> >   change PSTATE.BTYPE.
> > * __guest_exit is reached from B instructions in the hyp-entry vectors,
> >   which aren't subject to BTI checks either.
> 
> > Remove the BTI annotation from SYM_INNER_LABEL.
> 
> This fixes things for now but it feels like it's going to be fragile in
> the long run since inner labels are a bit of a wild west in terms of how
> they're going to be referenced - I can't think of a better solution at
> this level though :(
> 
> Reviewed-by: Mark Brown <broonie@kernel.org>

Since inner labels requiring landing pads are going to be the exception
rather than the rule, perhaps we can introduce a different macro for
this.  It feels arch-specific to me (indeed, inner labels are kind of
arch-specific, since they're inevitably in the middle of some asm that
is unlikely to be handled by core code).

Do we know of any code that requires landing pads on inner labels?

The uaccess fault stuff probably doesn't, because the error path is
reached via ERET.  I wondered about the suspend/resume code in sleep.S,
but I don't see any inner labels in there.

Cheers
---Dave

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

Re: [PATCH] arm64: Don't insert a BTI instruction at inner labels

[Mark Brown]

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

On Wed, Jun 24, 2020 at 02:44:25PM +0100, Dave Martin wrote:

> Since inner labels requiring landing pads are going to be the exception
> rather than the rule, perhaps we can introduce a different macro for
> this.  It feels arch-specific to me (indeed, inner labels are kind of
> arch-specific, since they're inevitably in the middle of some asm that
> is unlikely to be handled by core code).

Yes, we'd need a different macro (or argument to it or something).

> Do we know of any code that requires landing pads on inner labels?

There aren't any, that was what Jean-Philippe's analysis in the
changelog covered.

[-- 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: Don't insert a BTI instruction at inner labels

[Will Deacon]

On Wed, 24 Jun 2020 13:22:54 +0200, Jean-Philippe Brucker wrote:
> Some ftrace features are broken since commit 714a8d02ca4d ("arm64: asm:
> Override SYM_FUNC_START when building the kernel with BTI"). For example
> the function_graph tracer:
> 
> $ echo function_graph > /sys/kernel/debug/tracing/current_tracer
> [   36.107016] WARNING: CPU: 0 PID: 115 at kernel/trace/ftrace.c:2691 ftrace_modify_all_code+0xc8/0x14c
> 
> [...]

Applied to arm64 (for-next/fixes), thanks!

[1/1] arm64: Don't insert a BTI instruction at inner labels
      https://git.kernel.org/arm64/c/2d21889f8b5c

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