[PATCH v3 0/2] arm64: BTI cleanups

[PATCH v3 0/2] arm64: BTI cleanups

[Mark Brown]

This patch series brings together a few cleanups for the assembly
portions of the in kernel BTI support.

v3:
 - Tweak the assembler macro for BTI as suggested by Ard, making it
   more compact.
v2:
 - Pull in the assembler macro for BTI proposed by Mark Rutland.
 - Unconditionally override the SYM_FUNC macros.

Mark Brown (2):
  arm64: Add macro version of the BTI instruction
  arm64: Unconditionally override SYM_FUNC macros

 arch/arm64/crypto/aes-modes.S      | 10 +++++-----
 arch/arm64/include/asm/assembler.h | 10 ++++++++++
 arch/arm64/include/asm/linkage.h   | 23 ++++++++++++-----------
 arch/arm64/kernel/entry-ftrace.S   |  4 ----
 arch/arm64/lib/kasan_sw_tags.S     |  2 --
 5 files changed, 27 insertions(+), 22 deletions(-)


base-commit: 0fcfb00b28c0b7884635dacf38e46d60bf3d4eb1
-- 
2.30.2


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

[PATCH v3 1/2] arm64: Add macro version of the BTI instruction

[Mark Brown]

BTI is only available from v8.5 so we need to encode it using HINT in
generic code and for older toolchains. Add an assembler macro written
by Mark Rutland which lets us use the mnemonic and update the existing
users.

Suggested-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/crypto/aes-modes.S      | 10 +++++-----
 arch/arm64/include/asm/assembler.h | 10 ++++++++++
 arch/arm64/include/asm/linkage.h   |  7 +------
 3 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/arch/arm64/crypto/aes-modes.S b/arch/arm64/crypto/aes-modes.S
index b495de22bb38..ff01f0167ba2 100644
--- a/arch/arm64/crypto/aes-modes.S
+++ b/arch/arm64/crypto/aes-modes.S
@@ -363,15 +363,15 @@ ST5(	mov		v4.16b, vctr.16b		)
 	adr		x16, 1f
 	sub		x16, x16, x12, lsl #3
 	br		x16
-	hint		34			// bti c
+	bti		c
 	mov		v0.d[0], vctr.d[0]
-	hint		34			// bti c
+	bti		c
 	mov		v1.d[0], vctr.d[0]
-	hint		34			// bti c
+	bti		c
 	mov		v2.d[0], vctr.d[0]
-	hint		34			// bti c
+	bti		c
 	mov		v3.d[0], vctr.d[0]
-ST5(	hint		34				)
+ST5(	bti		c				)
 ST5(	mov		v4.d[0], vctr.d[0]		)
 1:	b		2f
 	.previous
diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
index 136d13f3d6e9..e8bd0af0141c 100644
--- a/arch/arm64/include/asm/assembler.h
+++ b/arch/arm64/include/asm/assembler.h
@@ -790,6 +790,16 @@ alternative_endif
 .Lnoyield_\@:
 	.endm
 
+/*
+ * Branch Target Identifier (BTI)
+ */
+	.macro  bti, targets
+	.equ	.L__bti_targets_c, 34
+	.equ	.L__bti_targets_j, 36
+	.equ	.L__bti_targets_jc,38
+	hint	#.L__bti_targets_\targets
+	.endm
+
 /*
  * This macro emits a program property note section identifying
  * architecture features which require special handling, mainly for
diff --git a/arch/arm64/include/asm/linkage.h b/arch/arm64/include/asm/linkage.h
index 9906541a6861..c5d0c11d7709 100644
--- a/arch/arm64/include/asm/linkage.h
+++ b/arch/arm64/include/asm/linkage.h
@@ -6,12 +6,7 @@
 
 #if defined(CONFIG_ARM64_BTI_KERNEL) && defined(__aarch64__)
 
-/*
- * Since current versions of gas reject the BTI instruction unless we
- * set the architecture version to v8.5 we use the hint instruction
- * instead.
- */
-#define BTI_C hint 34 ;
+#define BTI_C bti c ;
 
 /*
  * When using in-kernel BTI we need to ensure that PCS-conformant assembly
-- 
2.30.2


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

[PATCH v3 2/2] arm64: Unconditionally override SYM_FUNC macros

[Mark Brown]

Currently we only override the SYM_FUNC macros when we need to insert
BTI C into them.  Since we now unconditionally provide the BTI_C macro
used to do that let's always override them, that way any issues with our
overrides it'll show up more consistently.

Since this means that we now have an unconditional definition of BTI_C
update the other users to remove the ifdefs around usage, making them
look a bit neater.

Suggested-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/include/asm/linkage.h | 16 +++++++++++-----
 arch/arm64/kernel/entry-ftrace.S |  4 ----
 arch/arm64/lib/kasan_sw_tags.S   |  2 --
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/arch/arm64/include/asm/linkage.h b/arch/arm64/include/asm/linkage.h
index c5d0c11d7709..1cfa8bb33edd 100644
--- a/arch/arm64/include/asm/linkage.h
+++ b/arch/arm64/include/asm/linkage.h
@@ -8,10 +8,18 @@
 
 #define BTI_C bti c ;
 
+#else
+
+#define BTI_C
+
+#endif
+
 /*
- * When using in-kernel BTI we need to ensure that PCS-conformant assembly
- * functions have suitable annotations.  Override SYM_FUNC_START to insert
- * a BTI landing pad at the start of everything.
+ * When using in-kernel BTI we need to ensure that PCS-conformant
+ * assembly functions have suitable annotations.  Override
+ * SYM_FUNC_START to insert a BTI landing pad at the start of
+ * everything, the override is done unconditionally so we're more
+ * likely to notice any drift from the overridden definitions.
  */
 #define SYM_FUNC_START(name)				\
 	SYM_START(name, SYM_L_GLOBAL, SYM_A_ALIGN)	\
@@ -37,8 +45,6 @@
 	SYM_START(name, SYM_L_WEAK, SYM_A_NONE)		\
 	BTI_C
 
-#endif
-
 /*
  * Annotate a function as position independent, i.e., safe to be called before
  * the kernel virtual mapping is activated.
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

Re: [PATCH v3 0/2] arm64: BTI cleanups

[Ard Biesheuvel]

On Wed, 8 Dec 2021 at 17:13, Mark Brown <broonie@kernel.org> wrote:
>
> This patch series brings together a few cleanups for the assembly
> portions of the in kernel BTI support.
>
> v3:
>  - Tweak the assembler macro for BTI as suggested by Ard, making it
>    more compact.
> v2:
>  - Pull in the assembler macro for BTI proposed by Mark Rutland.
>  - Unconditionally override the SYM_FUNC macros.
>
> Mark Brown (2):
>   arm64: Add macro version of the BTI instruction
>   arm64: Unconditionally override SYM_FUNC macros
>

Acked-by: Ard Biesheuvel <ardb@kernel.org>


>  arch/arm64/crypto/aes-modes.S      | 10 +++++-----
>  arch/arm64/include/asm/assembler.h | 10 ++++++++++
>  arch/arm64/include/asm/linkage.h   | 23 ++++++++++++-----------
>  arch/arm64/kernel/entry-ftrace.S   |  4 ----
>  arch/arm64/lib/kasan_sw_tags.S     |  2 --
>  5 files changed, 27 insertions(+), 22 deletions(-)
>
>
> base-commit: 0fcfb00b28c0b7884635dacf38e46d60bf3d4eb1
> --
> 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 v3 1/2] arm64: Add macro version of the BTI instruction

[Will Deacon]

On Wed, Dec 08, 2021 at 04:08:18PM +0000, Mark Brown wrote:
> BTI is only available from v8.5 so we need to encode it using HINT in
> generic code and for older toolchains. Add an assembler macro written
> by Mark Rutland which lets us use the mnemonic and update the existing
> users.
> 
> Suggested-by: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  arch/arm64/crypto/aes-modes.S      | 10 +++++-----
>  arch/arm64/include/asm/assembler.h | 10 ++++++++++
>  arch/arm64/include/asm/linkage.h   |  7 +------
>  3 files changed, 16 insertions(+), 11 deletions(-)

Acked-by: Will Deacon <will@kernel.org>

Will

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

Re: [PATCH v3 2/2] arm64: Unconditionally override SYM_FUNC macros

[Will Deacon]

On Wed, Dec 08, 2021 at 04:08:19PM +0000, Mark Brown wrote:
> Currently we only override the SYM_FUNC macros when we need to insert
> BTI C into them.  Since we now unconditionally provide the BTI_C macro
> used to do that let's always override them, that way any issues with our
> overrides it'll show up more consistently.
> 
> Since this means that we now have an unconditional definition of BTI_C
> update the other users to remove the ifdefs around usage, making them
> look a bit neater.
> 
> Suggested-by: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  arch/arm64/include/asm/linkage.h | 16 +++++++++++-----
>  arch/arm64/kernel/entry-ftrace.S |  4 ----
>  arch/arm64/lib/kasan_sw_tags.S   |  2 --
>  3 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/linkage.h b/arch/arm64/include/asm/linkage.h
> index c5d0c11d7709..1cfa8bb33edd 100644
> --- a/arch/arm64/include/asm/linkage.h
> +++ b/arch/arm64/include/asm/linkage.h
> @@ -8,10 +8,18 @@
>  
>  #define BTI_C bti c ;
>  
> +#else
> +
> +#define BTI_C
> +
> +#endif

Why do we need this hunk? Having the hint instruction should be fine, no?

Will

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

Re: [PATCH v3 2/2] arm64: Unconditionally override SYM_FUNC macros

[Mark Brown]

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

On Mon, Dec 13, 2021 at 06:46:54PM +0000, Will Deacon wrote:
> On Wed, Dec 08, 2021 at 04:08:19PM +0000, Mark Brown wrote:

> > --- a/arch/arm64/include/asm/linkage.h
> > +++ b/arch/arm64/include/asm/linkage.h
> > @@ -8,10 +8,18 @@
> >  
> >  #define BTI_C bti c ;
> >  
> > +#else
> > +
> > +#define BTI_C
> > +
> > +#endif

> Why do we need this hunk? Having the hint instruction should be fine, no?

We could unconditionally insert the hint but that would be a step back
from what we currently have and would mean that hand written assembly
would be that little bit worse than what the compiler outputs.  If this
had been causing us issues I could see simplifying but I'm not aware of
any - the issues I'm aware of have been due to not adding the landing
pads in SYM_CODE, not SYM_FUNC.

[-- 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 v3 2/2] arm64: Unconditionally override SYM_FUNC macros

[Will Deacon]

On Mon, Dec 13, 2021 at 07:21:25PM +0000, Mark Brown wrote:
> On Mon, Dec 13, 2021 at 06:46:54PM +0000, Will Deacon wrote:
> > On Wed, Dec 08, 2021 at 04:08:19PM +0000, Mark Brown wrote:
> 
> > > --- a/arch/arm64/include/asm/linkage.h
> > > +++ b/arch/arm64/include/asm/linkage.h
> > > @@ -8,10 +8,18 @@
> > >  
> > >  #define BTI_C bti c ;
> > >  
> > > +#else
> > > +
> > > +#define BTI_C
> > > +
> > > +#endif
> 
> > Why do we need this hunk? Having the hint instruction should be fine, no?
> 
> We could unconditionally insert the hint but that would be a step back
> from what we currently have and would mean that hand written assembly
> would be that little bit worse than what the compiler outputs.  If this
> had been causing us issues I could see simplifying but I'm not aware of
> any - the issues I'm aware of have been due to not adding the landing
> pads in SYM_CODE, not SYM_FUNC.

I don't have a strong opinion here, so whatever you like. I just tend to
think that most people will have BTI enabled and there's something to be
said for having an instruction always emitted when you have a token in
assembly code.

Will

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

Re: [PATCH v3 2/2] arm64: Unconditionally override SYM_FUNC macros

[Ard Biesheuvel]

On Tue, 14 Dec 2021 at 15:10, Will Deacon <will@kernel.org> wrote:
>
> On Mon, Dec 13, 2021 at 07:21:25PM +0000, Mark Brown wrote:
> > On Mon, Dec 13, 2021 at 06:46:54PM +0000, Will Deacon wrote:
> > > On Wed, Dec 08, 2021 at 04:08:19PM +0000, Mark Brown wrote:
> >
> > > > --- a/arch/arm64/include/asm/linkage.h
> > > > +++ b/arch/arm64/include/asm/linkage.h
> > > > @@ -8,10 +8,18 @@
> > > >
> > > >  #define BTI_C bti c ;
> > > >
> > > > +#else
> > > > +
> > > > +#define BTI_C
> > > > +
> > > > +#endif
> >
> > > Why do we need this hunk? Having the hint instruction should be fine, no?
> >
> > We could unconditionally insert the hint but that would be a step back
> > from what we currently have and would mean that hand written assembly
> > would be that little bit worse than what the compiler outputs.  If this
> > had been causing us issues I could see simplifying but I'm not aware of
> > any - the issues I'm aware of have been due to not adding the landing
> > pads in SYM_CODE, not SYM_FUNC.
>
> I don't have a strong opinion here, so whatever you like. I just tend to
> think that most people will have BTI enabled and there's something to be
> said for having an instruction always emitted when you have a token in
> assembly code.
>

+1

I already pointed out computed gotos, which are admittedly rare, but
there are other reasons why adhering to WYSIWYG is strongly preferred
for asm code IMO.

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

Re: [PATCH v3 2/2] arm64: Unconditionally override SYM_FUNC macros

[Mark Brown]

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

On Tue, Dec 14, 2021 at 03:12:01PM +0100, Ard Biesheuvel wrote:
> On Tue, 14 Dec 2021 at 15:10, Will Deacon <will@kernel.org> wrote:

> > I don't have a strong opinion here, so whatever you like. I just tend to
> > think that most people will have BTI enabled and there's something to be
> > said for having an instruction always emitted when you have a token in
> > assembly code.

> +1

> I already pointed out computed gotos, which are admittedly rare, but
> there are other reasons why adhering to WYSIWYG is strongly preferred
> for asm code IMO.

The use case for this macro is for SYM_FUNC_START() which is going to be
emitting the BTI without it really appearing directly in the code so I'm
not sure how much I buy that TBH.  I think you're pushing back on uses
of the macro outside of linkage.h more than on what linkage.h is doing,
that does make more sense to me.

[-- 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 v3 2/2] arm64: Unconditionally override SYM_FUNC macros

[Ard Biesheuvel]

On Tue, 14 Dec 2021 at 15:28, Mark Brown <broonie@kernel.org> wrote:
>
> On Tue, Dec 14, 2021 at 03:12:01PM +0100, Ard Biesheuvel wrote:
> > On Tue, 14 Dec 2021 at 15:10, Will Deacon <will@kernel.org> wrote:
>
> > > I don't have a strong opinion here, so whatever you like. I just tend to
> > > think that most people will have BTI enabled and there's something to be
> > > said for having an instruction always emitted when you have a token in
> > > assembly code.
>
> > +1
>
> > I already pointed out computed gotos, which are admittedly rare, but
> > there are other reasons why adhering to WYSIWYG is strongly preferred
> > for asm code IMO.
>
> The use case for this macro is for SYM_FUNC_START() which is going to be
> emitting the BTI without it really appearing directly in the code so I'm
> not sure how much I buy that TBH.  I think you're pushing back on uses
> of the macro outside of linkage.h more than on what linkage.h is doing,
> that does make more sense to me.

Fair point, but we don't really have any control over where else BTI_C
may end up being used, no?

In any case, I don't think it's worth it blowing up the validation
matrix over this, so if we can emit the same code for all
configurations, we should IMO

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

Re: [PATCH v3 2/2] arm64: Unconditionally override SYM_FUNC macros

[Mark Brown]

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

On Tue, Dec 14, 2021 at 03:31:07PM +0100, Ard Biesheuvel wrote:
> On Tue, 14 Dec 2021 at 15:28, Mark Brown <broonie@kernel.org> wrote:

> > > I already pointed out computed gotos, which are admittedly rare, but
> > > there are other reasons why adhering to WYSIWYG is strongly preferred
> > > for asm code IMO.

> > The use case for this macro is for SYM_FUNC_START() which is going to be
> > emitting the BTI without it really appearing directly in the code so I'm
> > not sure how much I buy that TBH.  I think you're pushing back on uses
> > of the macro outside of linkage.h more than on what linkage.h is doing,
> > that does make more sense to me.

> Fair point, but we don't really have any control over where else BTI_C
> may end up being used, no?

It's also a macro, which should put people off, and there's precious
little arm64 asm outside of arch/arm64 so I'm not sure how worried I am
about that.

> In any case, I don't think it's worth it blowing up the validation
> matrix over this, so if we can emit the same code for all
> configurations, we should IMO

We could have been doing that since the BTI kernel support was
originally merged...  TBH I'm just really surprised everyone's pushing
to generate worse code here, feels not the direction people usually go
in.

[-- 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