Re: [PATCH] arm64: asmlinkage: Enable use of BTI_C macro in SYM_CODE

[Mark Rutland <mark.rutland@arm.com>]

On Fri, Dec 03, 2021 at 01:03:35PM +0000, Mark Brown wrote:
> A couple of SYM_CODE sections have added usage of BTI_C which is
> currently only defined when building for BTI.  This means that the
> users have ugly ifdefs for the case where BTI is disabled so let's
> provide an empty definition in that case and remove the ifdefs.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  arch/arm64/include/asm/linkage.h | 4 ++++
>  arch/arm64/kernel/entry-ftrace.S | 4 ----
>  arch/arm64/lib/kasan_sw_tags.S   | 2 --
>  3 files changed, 4 insertions(+), 6 deletions(-)

Looking around, there are other places that open-code `hint 34`, e.g.
arch/arm64/crypto/aes-modes.S. Those are unconditional, so we should probably
figure out whether we want those to be conditional (or if we're happy to make
the other cases similarly unconditional).

I'd argue we should probably place BTIs in assembly unconditionally, on the
assumption that they shouldn't have an measureable performance impact in
practice (as we're already assuming that when CONFIG_ARM64_BTI_KERNEL is
selected anyhow). Thoughts?

Regardless, I reckon we should probably define a `bti` macro centrally in
arch/arm64/include/asm/assembler.h, something like:

| /*
|  * Branch Target Identifier
|  */
|        .macro  bti, targets
|        .equ    .L__bti_targets_c, 1
|        .equ    .L__bti_targets_j, 2
|        .equ    .L__bti_targets_jc,3
|        .if     .L__bti_targets_\targets == .L__bti_targets_c
|        hint    #34
|        .elseif .L__bti_targets_\targets == .L__bti_targets_j
|        hint    #36
|        .elseif .L__bti_targets_\targets == .L__bti_targets_jc
|        hint    #38
|        .else
|        .error "Unsupported BTI targets '\targets\()'"
|        .endif
|        .endm
| 

The `.equ` gunk is because there's no `.elseifc`, and without that we have to
have a chain of `.endif` for each `.ifc`, and can't produce a warning reliably.

That seems to do the right thing:

| [mark@lakrids:~/src/linux]% git diff -- arch/arm64/kernel/head.S 
| diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
| index 6a98f1a38c29..f6968563bab8 100644
| --- a/arch/arm64/kernel/head.S
| +++ b/arch/arm64/kernel/head.S
| @@ -89,6 +89,10 @@
|          *  x24        __primary_switch() .. relocate_kernel()  current RELR displacement
|          */
|  SYM_CODE_START(primary_entry)
| +       bti     c
| +       bti     j
| +       bti     jc
| +       bti     no
|         bl      preserve_boot_args
|         bl      init_kernel_el                  // w0=cpu_boot_mode
|         adrp    x23, __PHYS_OFFSET
| [mark@lakrids:~/src/linux]% usekorg 11.1.0 make ARCH=arm64 CROSS_COMPILE=aarch64-linux- arch/arm64/kernel/head.o
| Makefile:1811: warning: overriding recipe for target 'arch/arm64/kernel/head.o'
| Makefile:1167: warning: ignoring old recipe for target 'arch/arm64/kernel/head.o'
|   CALL    scripts/checksyscalls.sh
|   CALL    scripts/atomic/check-atomics.sh
|   AS      arch/arm64/kernel/head.o
| arch/arm64/kernel/head.S: Assembler messages:
| arch/arm64/kernel/head.S:95: Error: Unsupported BTI targets 'no'
| make[2]: *** [scripts/Makefile.build:388: arch/arm64/kernel/head.o] Error 1
| make[1]: *** [scripts/Makefile.build:549: arch/arm64/kernel] Error 2
| make: *** [Makefile:1846: arch/arm64] Error 2

> diff --git a/arch/arm64/include/asm/linkage.h b/arch/arm64/include/asm/linkage.h
> index 9906541a6861..9c70136d7f94 100644
> --- a/arch/arm64/include/asm/linkage.h
> +++ b/arch/arm64/include/asm/linkage.h
> @@ -42,6 +42,10 @@
>  	SYM_START(name, SYM_L_WEAK, SYM_A_NONE)		\
>  	BTI_C
>  
> +#else
> +
> +#define BTI_C
> +
>  #endif

Can we simplify the ifdeffery so that we only conditionally define BTI_C, and
unconditionally define the sites using it? That way if there's a problem with
any of the users that will consistently show up in testing, regardless of
whether CONFIG_ARM64_BTI_KERNEL is selected.

e.g. we can make this:

| #ifdef CONFIG_ARM64_BTI_KERNEL
| #define BTI_C	hint #34 ;
| #else
| #define BTI_C	
| #endif
|
| #define SYM_FUNC_START(name)					\
| SYM_START(name, SYM_L_GLOBAL, SYM_A_ALIGN)      		\
| BTI_C
|
| ...

... or if we define an unconditional `bti` macro:

| #define SYM_FUNC_START(name)					\
| SYM_START(name, SYM_L_GLOBAL, SYM_A_ALIGN)      		\
| bti c
|
| ...

Thanks,
Mark.

>  /*
> diff --git a/arch/arm64/kernel/entry-ftrace.S b/arch/arm64/kernel/entry-ftrace.S
> index 8cf970d219f5..46a2de864794 100644
> --- a/arch/arm64/kernel/entry-ftrace.S
> +++ b/arch/arm64/kernel/entry-ftrace.S
> @@ -77,17 +77,13 @@
>  	.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)
> diff --git a/arch/arm64/lib/kasan_sw_tags.S b/arch/arm64/lib/kasan_sw_tags.S
> index 5b04464c045e..a6d6fa2f761e 100644
> --- a/arch/arm64/lib/kasan_sw_tags.S
> +++ b/arch/arm64/lib/kasan_sw_tags.S
> @@ -38,9 +38,7 @@
>   * incremented by 256 prior to return).
>   */
>  SYM_CODE_START(__hwasan_tag_mismatch)
> -#ifdef BTI_C
>  	BTI_C
> -#endif
>  	add	x29, sp, #232
>  	stp	x2, x3, [sp, #8 * 2]
>  	stp	x4, x5, [sp, #8 * 4]
> -- 
> 2.30.2
> 

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