[PATCH] arm64: asmlinkage: Enable use of BTI_C macro in SYM_CODE
Ard Biesheuvel
ardb at kernel.org
Mon Dec 6 08:52:16 PST 2021
On Mon, 6 Dec 2021 at 17:30, Mark Rutland <mark.rutland at arm.com> wrote:
>
> 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 at 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?
>
>From the top of my head, I can't say for sure but there are some
computed gotos in the crypto code where removing an instruction may
throw off the calculation. So keeping the hint unconditionally makes
sense to me, and by the same reasoning, it would be better not to
introduce macros that shadow existing instructions if they may resolve
to a sequence of a different size.
> 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 at 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 at 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
> >
More information about the linux-arm-kernel
mailing list