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(-)
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
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
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.
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
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,
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-stable-mirror@lists.linaro.org
-
Mark Rutland -
Rob Herring -
Will Deacon