[PATCH 0/2] arm64: fix+cleanup for ARM64_WORKAROUND_SPECULATIVE_UNPRIV_LOAD

[PATCH 0/2] arm64: fix+cleanup for ARM64_WORKAROUND_SPECULATIVE_UNPRIV_LOAD

[Mark Rutland]

Hi,

While testing an unrelated patch on the arm64 for-next/core branch, I
spotted an issue in the ARM64_WORKAROUND_SPECULATIVE_UNPRIV_LOAD
workaround. The first patch fixes that issue, and the second patch
cleans up the remaining logic.

The issue has existed since the workaround was introduced in commit:

  471470bc7052d28c ("arm64: errata: Add Cortex-A520 speculative unprivileged load workaround")

As that logic has recently been reworked in the arm64 for-next/core
branch, these patches are based atop that rework, specifically atop
commit:

  546b7cde9b1dd360 ("arm64: Rename ARM64_WORKAROUND_2966298")

As the patches alter the KPTI exception return logic, I've given this
testing with KPTI forced on, forced off, and disabled at build time,
which all appear to be fine. I don't have any hardware requiring the
ARM64_WORKAROUND_SPECULATIVE_UNPRIV_LOAD workaround, but as the
resulting logic for this is very simple I do not expect any issues with
that part of the logic.

Mark.

Mark Rutland (2):
  arm64: entry: fix ARM64_WORKAROUND_SPECULATIVE_UNPRIV_LOAD
  arm64: entry: simplify kernel_exit logic

 arch/arm64/kernel/entry.S | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

-- 
2.30.2


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

[PATCH 1/2] arm64: entry: fix ARM64_WORKAROUND_SPECULATIVE_UNPRIV_LOAD

[Mark Rutland]

Currently the ARM64_WORKAROUND_SPECULATIVE_UNPRIV_LOAD workaround isn't
quite right, as it is supposed to be applied after the last explicit
memory access, but is immediately followed by an LDR.

The ARM64_WORKAROUND_SPECULATIVE_UNPRIV_LOAD workaround is used to
handle Cortex-A520 erratum 2966298 and Cortex-A510 erratum 3117295,
which are described in:

* https://developer.arm.com/documentation/SDEN2444153/0600/?lang=en
* https://developer.arm.com/documentation/SDEN1873361/1600/?lang=en

In both cases the workaround is described as:

| If pagetable isolation is disabled, the context switch logic in the
| kernel can be updated to execute the following sequence on affected
| cores before exiting to EL0, and after all explicit memory accesses:
|
| 1. A non-shareable TLBI to any context and/or address, including
|    unused contexts or addresses, such as a `TLBI VALE1 Xzr`.
|
| 2. A DSB NSH to guarantee completion of the TLBI.

The important part being that the TLBI+DSB must be placed "after all
explicit memory accesses".

Unfortunately, as-implemented, the TLBI+DSB is immediately followed by
an LDR, as we have:

| alternative_if ARM64_WORKAROUND_SPECULATIVE_UNPRIV_LOAD
| 	tlbi	vale1, xzr
| 	dsb	nsh
| alternative_else_nop_endif
| alternative_if_not ARM64_UNMAP_KERNEL_AT_EL0
| 	ldr	lr, [sp, #S_LR]
| 	add	sp, sp, #PT_REGS_SIZE		// restore sp
| 	eret
| alternative_else_nop_endif
|
| [ ... KPTI exception return path ... ]

This patch fixes this by reworking the logic to place the TLBI+DSB
immediately before the ERET, after all explicit memory accesses.

The ERET is currently in a separate alternative block, and alternatives
cannot be nested. To account for this, the alternative block for
ARM64_UNMAP_KERNEL_AT_EL0 is replaced with a single alternative branch
to skip the KPTI logic, with the new shape of the logic being:

| alternative_insn "b .L_skip_tramp_exit_\@", nop, ARM64_UNMAP_KERNEL_AT_EL0
| 	[ ... KPTI exception return path ... ]
| .L_skip_tramp_exit_\@:
|
| 	ldr	lr, [sp, #S_LR]
| 	add	sp, sp, #PT_REGS_SIZE		// restore sp
|
| alternative_if ARM64_WORKAROUND_SPECULATIVE_UNPRIV_LOAD
| 	tlbi	vale1, xzr
| 	dsb	nsh
| alternative_else_nop_endif
| 	eret

The new structure means that the workaround is only applied when KPTI is
not in use; this is fine as noted in the documented implications of the
erratum:

| Pagetable isolation between EL0 and higher level ELs prevents the
| issue from occurring.

... and as per the workaround description quoted above, the workaround
is only necessary "If pagetable isolation is disabled".

Fixes: 471470bc7052d28c ("arm64: errata: Add Cortex-A520 speculative unprivileged load workaround")
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: James Morse <james.morse@arm.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Will Deacon <will@kernel.org>
Cc: stable@vger.kernel.org
---
 arch/arm64/kernel/entry.S | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 544ab46649f3f..7fcbee0f6c0e4 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -428,16 +428,9 @@ alternative_else_nop_endif
 	ldp	x28, x29, [sp, #16 * 14]
 
 	.if	\el == 0
-alternative_if ARM64_WORKAROUND_SPECULATIVE_UNPRIV_LOAD
-	tlbi	vale1, xzr
-	dsb	nsh
-alternative_else_nop_endif
-alternative_if_not ARM64_UNMAP_KERNEL_AT_EL0
-	ldr	lr, [sp, #S_LR]
-	add	sp, sp, #PT_REGS_SIZE		// restore sp
-	eret
-alternative_else_nop_endif
 #ifdef CONFIG_UNMAP_KERNEL_AT_EL0
+	alternative_insn "b .L_skip_tramp_exit_\@", nop, ARM64_UNMAP_KERNEL_AT_EL0
+
 	msr	far_el1, x29
 
 	ldr_this_cpu	x30, this_cpu_vector, x29
@@ -446,7 +439,18 @@ alternative_else_nop_endif
 	ldr		lr, [sp, #S_LR]		// restore x30
 	add		sp, sp, #PT_REGS_SIZE	// restore sp
 	br		x29
+
+.L_skip_tramp_exit_\@:
 #endif
+	ldr	lr, [sp, #S_LR]
+	add	sp, sp, #PT_REGS_SIZE		// restore sp
+
+	/* This must be after the last explicit memory access */
+alternative_if ARM64_WORKAROUND_SPECULATIVE_UNPRIV_LOAD
+	tlbi	vale1, xzr
+	dsb	nsh
+alternative_else_nop_endif
+	eret
 	.else
 	ldr	lr, [sp, #S_LR]
 	add	sp, sp, #PT_REGS_SIZE		// restore sp
-- 
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 1/2] arm64: entry: fix ARM64_WORKAROUND_SPECULATIVE_UNPRIV_LOAD

[Rob Herring]

On Tue, Jan 16, 2024 at 5:02 AM Mark Rutland <mark.rutland@arm.com> wrote:
>
> Currently the ARM64_WORKAROUND_SPECULATIVE_UNPRIV_LOAD workaround isn't
> quite right, as it is supposed to be applied after the last explicit
> memory access, but is immediately followed by an LDR.

This isn't necessary. The LDR in question is an unprivileged load from
the EL0 stack. The erratum write-up is not really clear in that
regard.

It's the same as the KPTI case. After switching the page tables, there
are unprivileged loads from the EL0 stack.

Rob

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

Re: [PATCH 1/2] arm64: entry: fix ARM64_WORKAROUND_SPECULATIVE_UNPRIV_LOAD

[Mark Rutland]

Hi Rob,

Sorry for the confusion here; I should have synced up with you before sending
this out.

On Fri, Jan 19, 2024 at 09:11:33AM -0600, Rob Herring wrote:
> On Tue, Jan 16, 2024 at 5:02 AM Mark Rutland <mark.rutland@arm.com> wrote:
> >
> > Currently the ARM64_WORKAROUND_SPECULATIVE_UNPRIV_LOAD workaround isn't
> > quite right, as it is supposed to be applied after the last explicit
> > memory access, but is immediately followed by an LDR.
> 
> This isn't necessary. The LDR in question is an unprivileged load from
> the EL0 stack. The erratum write-up is not really clear in that
> regard.

I see from internal notes that the rationale is that the LDR in question only
loads data that EL0 already has in its registers, and hence it doesn't matter
if that data is leaked to EL0. That's reasonable, but we didn't note that
anywhere (i.e. neither in the commit message nor in any comments).

To avoid confusion, the LDR in question *is* a privileged load (whereas an LDTR
at EL1 would be an unprivileged load); for memory accesses the architecture
uses the terms privileged and unprivileged to distinguish the way those are
handled by the MMU.

I agree that given the rationale above this patch isn't strictly necessary, but
I would prefer result of these two patches as it's less likely that we'll add
loads of sensitive information in future as this code is changed.

> It's the same as the KPTI case. After switching the page tables, there
> are unprivileged loads from the EL0 stack.

I'm not sure what you mean here; maybe I'm missing something?

AFAICT we don't do any loads within the kernel after switching the translation
tables. In tramp_exit we follow tramp_unmap_kernel with:

	MRS; ERET; SB

... and in __sdei_asm_exit_trampoline we follow tramp_unmap_kernel with

	CMP; B.NE; {HVC,SMC}; B .

... so there are no explicit loads at EL1 before the ERET to EL0. In the SDEI
case any loads at a higher EL don't matter because they're in a different
translation regime.

Thanks,
Mark.

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

[PATCH 2/2] arm64: entry: simplify kernel_exit logic

[Mark Rutland]

For historical reasons, the non-KPTI exception return path is duplicated for
EL1 and EL0, with the structure:

	.if \el == 0
	[ KPTI handling ]
	ldr     lr, [sp, #S_LR]
 	add	sp, sp, #PT_REGS_SIZE		// restore sp
	[ EL0 exception return workaround ]
	eret
	.else
	ldr     lr, [sp, #S_LR]
 	add	sp, sp, #PT_REGS_SIZE		// restore sp
	[ EL1 exception return workaround ]
	eret
	.endif
	sb

This would be simpler and clearer with the common portions factored out,
e.g.

	.if \el == 0
	[ KPTI handling ]
	.endif

	ldr     lr, [sp, #S_LR]
 	add	sp, sp, #PT_REGS_SIZE		// restore sp

	.if \el == 0
	[ EL0 exception return workaround ]
	.else
	[ EL1 exception return workaround ]
	.endif

	eret
	sb

This expands to the same code, but is simpler for a human to follow as
it avoids duplicates the restore of LR+SP, and makes it clear that the
ERET is associated with the SB.

There should be no functional change as a result of this patch.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: James Morse <james.morse@arm.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/kernel/entry.S | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 7fcbee0f6c0e4..7ef0e127b149f 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -442,24 +442,23 @@ alternative_else_nop_endif
 
 .L_skip_tramp_exit_\@:
 #endif
+	.endif
+
 	ldr	lr, [sp, #S_LR]
 	add	sp, sp, #PT_REGS_SIZE		// restore sp
 
+	.if \el == 0
 	/* This must be after the last explicit memory access */
 alternative_if ARM64_WORKAROUND_SPECULATIVE_UNPRIV_LOAD
 	tlbi	vale1, xzr
 	dsb	nsh
 alternative_else_nop_endif
-	eret
 	.else
-	ldr	lr, [sp, #S_LR]
-	add	sp, sp, #PT_REGS_SIZE		// restore sp
-
 	/* Ensure any device/NC reads complete */
 	alternative_insn nop, "dmb sy", ARM64_WORKAROUND_1508412
+	.endif
 
 	eret
-	.endif
 	sb
 	.endm
 
-- 
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 0/2] arm64: fix+cleanup for ARM64_WORKAROUND_SPECULATIVE_UNPRIV_LOAD

[Will Deacon]

On Tue, 16 Jan 2024 11:02:19 +0000, Mark Rutland wrote:
> While testing an unrelated patch on the arm64 for-next/core branch, I
> spotted an issue in the ARM64_WORKAROUND_SPECULATIVE_UNPRIV_LOAD
> workaround. The first patch fixes that issue, and the second patch
> cleans up the remaining logic.
> 
> The issue has existed since the workaround was introduced in commit:
> 
> [...]

Cheers, I picked these up, but you might need to shepherd them
through -stable, so please keep an eye out for any "failed to apply"
mails.

Talking of which, the original workaround didn't make it to any kernels
before 6.1:

[5.15] https://lore.kernel.org/r/2023100743-evasion-figment-fbcc@gregkh
[5.10] https://lore.kernel.org/r/2023100745-statute-component-dd0f@gregkh

Please can you or Rob have a crack at that?

[1/2] arm64: entry: fix ARM64_WORKAROUND_SPECULATIVE_UNPRIV_LOAD
      https://git.kernel.org/arm64/c/832dd634bd1b
[2/2] arm64: entry: simplify kernel_exit logic
      https://git.kernel.org/arm64/c/da59f1d051d5

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 0/2] arm64: fix+cleanup for ARM64_WORKAROUND_SPECULATIVE_UNPRIV_LOAD

[Mark Rutland]

On Thu, Jan 18, 2024 at 12:02:26PM +0000, Will Deacon wrote:
> On Tue, 16 Jan 2024 11:02:19 +0000, Mark Rutland wrote:
> > While testing an unrelated patch on the arm64 for-next/core branch, I
> > spotted an issue in the ARM64_WORKAROUND_SPECULATIVE_UNPRIV_LOAD
> > workaround. The first patch fixes that issue, and the second patch
> > cleans up the remaining logic.
> > 
> > The issue has existed since the workaround was introduced in commit:
> > 
> > [...]
> 
> Cheers, I picked these up, but you might need to shepherd them
> through -stable, so please keep an eye out for any "failed to apply"
> mails.
> 
> Talking of which, the original workaround didn't make it to any kernels
> before 6.1:
> 
> [5.15] https://lore.kernel.org/r/2023100743-evasion-figment-fbcc@gregkh
> [5.10] https://lore.kernel.org/r/2023100745-statute-component-dd0f@gregkh

From a quick look, these failed because we forgot to backport some prior errata
workarounds (which are still missing from stable), and backported others
out-of-order relative to mainline, so every subsequent backport is likely to
hit a massive text conflict in the diff.

I'll have a go at backorting the missing pieces in-order to get this closer to
mainline. I suspect that'll take a short while...

Going forwards, we should check that errata patches are CC'd to stable
appropriately when we merge them in the arm64 tree, and we should make sure
those are successfully backported in-order.

Mark.

> 
> Please can you or Rob have a crack at that?
> 
> [1/2] arm64: entry: fix ARM64_WORKAROUND_SPECULATIVE_UNPRIV_LOAD
>       https://git.kernel.org/arm64/c/832dd634bd1b
> [2/2] arm64: entry: simplify kernel_exit logic
>       https://git.kernel.org/arm64/c/da59f1d051d5
> 
> 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