Hi James,
Thanks for this. I guess.
On Tue, 25 Jan 2022 15:38:03 +0000,
James Morse <james.morse@xxxxxxx> wrote:
>
> Cortex-A510's erratum #2077057 causes SPSR_EL2 to be corrupted when
> single-stepping authenticated ERET instructions. A single step is
> expected, but a pointer authentication trap is taken instead. The
> erratum causes SPSR_EL1 to be copied to SPSR_EL2, which could allow
> EL1 to cause a return to EL2 with a guest controlled ELR_EL2.
>
> Because the conditions require an ERET into active-not-pending state,
> this is only a problem for the EL2 when EL2 is stepping EL1. In this case
> the previous SPSR_EL2 value is preserved in struct kvm_vcpu, and can be
> restored.
Urgh. That's pretty nasty :-(.
>
> Cc: stable@xxxxxxxxxxxxxxx # ${GITHASHHERE}: arm64: Add Cortex-A510 CPU part definition
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: James Morse <james.morse@xxxxxxx>
> ---
> Documentation/arm64/silicon-errata.rst | 2 ++
> arch/arm64/Kconfig | 16 ++++++++++++++++
> arch/arm64/kernel/cpu_errata.c | 8 ++++++++
> arch/arm64/kvm/hyp/include/hyp/switch.h | 24 +++++++++++++++++++++---
> arch/arm64/tools/cpucaps | 1 +
> 5 files changed, 48 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/arm64/silicon-errata.rst b/Documentation/arm64/silicon-errata.rst
> index 5342e895fb60..ac1ae34564c9 100644
> --- a/Documentation/arm64/silicon-errata.rst
> +++ b/Documentation/arm64/silicon-errata.rst
> @@ -92,6 +92,8 @@ stable kernels.
> +----------------+-----------------+-----------------+-----------------------------+
> | ARM | Cortex-A77 | #1508412 | ARM64_ERRATUM_1508412 |
> +----------------+-----------------+-----------------+-----------------------------+
> +| ARM | Cortex-A510 | #2077057 | ARM64_ERRATUM_2077057 |
> ++----------------+-----------------+-----------------+-----------------------------+
> | ARM | Cortex-A710 | #2119858 | ARM64_ERRATUM_2119858 |
> +----------------+-----------------+-----------------+-----------------------------+
> | ARM | Cortex-A710 | #2054223 | ARM64_ERRATUM_2054223 |
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 6978140edfa4..02b542ec18c8 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -670,6 +670,22 @@ config ARM64_ERRATUM_1508412
> config ARM64_WORKAROUND_TRBE_OVERWRITE_FILL_MODE
> bool
>
> +config ARM64_ERRATUM_2077057
> + bool "Cortex-A510: 2077057: workaround software-step corrupting SPSR_EL2"
> + help
> + This option adds the workaround for ARM Cortex-A510 erratum 2077057.
> + Affected Cortex-A510 may corrupt SPSR_EL2 when the a step exception is
> + expected, but a Pointer Authentication trap is taken instead. The
> + erratum causes SPSR_EL1 to be copied to SPSR_EL2, which could allow
> + EL1 to cause a return to EL2 with a guest controlled ELR_EL2.
> +
> + This can only happen when EL2 is stepping EL1.
> +
> + When these conditions occur, the SPSR_EL2 value is unchanged from the
> + previous guest entry, and can be restored from the in-memory copy.
> +
> + If unsure, say Y.
> +
> config ARM64_ERRATUM_2119858
> bool "Cortex-A710: 2119858: workaround TRBE overwriting trace data in FILL mode"
> default y
> diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
> index 9e1c1aef9ebd..04a014c63251 100644
> --- a/arch/arm64/kernel/cpu_errata.c
> +++ b/arch/arm64/kernel/cpu_errata.c
> @@ -597,6 +597,14 @@ const struct arm64_cpu_capabilities arm64_errata[] = {
> .type = ARM64_CPUCAP_WEAK_LOCAL_CPU_FEATURE,
> CAP_MIDR_RANGE_LIST(trbe_write_out_of_range_cpus),
> },
> +#endif
> +#ifdef CONFIG_ARM64_ERRATUM_2077057
> + {
> + .desc = "ARM erratum 2077057",
> + .capability = ARM64_WORKAROUND_2077057,
> + .type = ARM64_CPUCAP_LOCAL_CPU_ERRATUM,
> + ERRATA_MIDR_REV_RANGE(MIDR_CORTEX_A510, 0, 0, 2),
> + },
> #endif
> {
> }
> diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h
> index 331dd10821df..93bf140cc972 100644
> --- a/arch/arm64/kvm/hyp/include/hyp/switch.h
> +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h
> @@ -409,6 +409,8 @@ static inline bool kvm_hyp_handle_exit(struct kvm_vcpu *vcpu, u64 *exit_code)
> */
> static inline bool fixup_guest_exit(struct kvm_vcpu *vcpu, u64 *exit_code)
> {
> + u8 esr_ec;
> +
> /*
> * Save PSTATE early so that we can evaluate the vcpu mode
> * early on.
> @@ -421,13 +423,13 @@ static inline bool fixup_guest_exit(struct kvm_vcpu *vcpu, u64 *exit_code)
> */
> early_exit_filter(vcpu, exit_code);
>
> - if (ARM_EXCEPTION_CODE(*exit_code) != ARM_EXCEPTION_IRQ)
> + if (ARM_EXCEPTION_CODE(*exit_code) != ARM_EXCEPTION_IRQ) {
> vcpu->arch.fault.esr_el2 = read_sysreg_el2(SYS_ESR);
> + esr_ec = kvm_vcpu_trap_get_class(vcpu);
> + }
>
> if (ARM_SERROR_PENDING(*exit_code) &&
> ARM_EXCEPTION_CODE(*exit_code) != ARM_EXCEPTION_IRQ) {
> - u8 esr_ec = kvm_vcpu_trap_get_class(vcpu);
> -
> /*
> * HVC already have an adjusted PC, which we need to
> * correct in order to return to after having injected
> @@ -440,6 +442,22 @@ static inline bool fixup_guest_exit(struct kvm_vcpu *vcpu, u64 *exit_code)
> write_sysreg_el2(read_sysreg_el2(SYS_ELR) - 4, SYS_ELR);
> }
>
> + /*
> + * Check for the conditions of Cortex-A510's #2077057. When these occur
> + * SPSR_EL2 can't be trusted, but isn't needed either as it is
> + * unchanged from the value in vcpu_gp_regs(vcpu)->pstate.
> + * Did we just take a PAC exception when a step exception was expected?
> + */
> + if (IS_ENABLED(CONFIG_ARM64_ERRATUM_2077057) &&
nit: we can drop this IS_ENABLED()...
> + cpus_have_const_cap(ARM64_WORKAROUND_2077057) &&
and make this a final cap. Being a ARM64_CPUCAP_LOCAL_CPU_ERRATUM, we
won't accept late CPUs on a system that wasn't affected until then.
> + ARM_EXCEPTION_CODE(*exit_code) != ARM_EXCEPTION_IRQ &&
> + esr_ec == ESR_ELx_EC_PAC &&
> + vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
> + /* Active-not-pending? */
> + if (*vcpu_cpsr(vcpu) & DBG_SPSR_SS)
> + write_sysreg_el2(*vcpu_cpsr(vcpu), SYS_SPSR);
Err... Isn't this way too late? The function starts with:
vcpu->arch.ctxt.regs.pstate = read_sysreg_el2(SYS_SPSR);
which is just another way to write:
*vcpu_cpsr(vcpu) = read_sysreg_el2(SYS_SPSR);
By that time, the vcpu's PSTATE is terminally corrupted.
I think you need to hoist this workaround way up, before we call into
early_exit_filter() as it will assume that the guest pstate is correct
(this is used by both pKVM and the NV code).
Something like this (untested):
diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h
index 93bf140cc972..a8a1502db237 100644
--- a/arch/arm64/kvm/hyp/include/hyp/switch.h
+++ b/arch/arm64/kvm/hyp/include/hyp/switch.h
@@ -402,6 +402,26 @@ static inline bool kvm_hyp_handle_exit(struct kvm_vcpu *vcpu, u64 *exit_code)
return false;
}
+static inline void synchronize_vcpu_pstate(struct kvm_vcpu *vcpu,
+ u64 *exit_code)
+{
+ /*
+ * Check for the conditions of Cortex-A510's #2077057. When these occur
+ * SPSR_EL2 can't be trusted, but isn't needed either as it is
+ * unchanged from the value in vcpu_gp_regs(vcpu)->pstate.
+ * Did we just take a PAC exception when a step exception (being in the
+ * Active-not-pending state) was expected?
+ */
+ if (cpus_have_final_cap(ARM64_WORKAROUND_2077057) &&
+ ARM_EXCEPTION_CODE(*exit_code) != ARM_EXCEPTION_IRQ &&
+ kvm_vcpu_trap_get_class(vcpu) == ESR_ELx_EC_PAC &&
+ vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP &&
+ *vcpu_cpsr(vcpu) & DBG_SPSR_SS)
+ write_sysreg_el2(*vcpu_cpsr(vcpu), SYS_SPSR);
+
+ *vcpu_cpsr(vcpu) = read_sysreg_el2(SYS_SPSR);
+}
+
/*
* Return true when we were able to fixup the guest exit and should return to
* the guest, false when we should restore the host state and return to the
@@ -415,7 +435,7 @@ static inline bool fixup_guest_exit(struct kvm_vcpu *vcpu, u64 *exit_code)
* Save PSTATE early so that we can evaluate the vcpu mode
* early on.
*/
- vcpu->arch.ctxt.regs.pstate = read_sysreg_el2(SYS_SPSR);
+ synchronize_vcpu_pstate(vcpu, exit_code);
/*
* Check whether we want to repaint the state one way or
@@ -442,22 +462,6 @@ static inline bool fixup_guest_exit(struct kvm_vcpu *vcpu, u64 *exit_code)
write_sysreg_el2(read_sysreg_el2(SYS_ELR) - 4, SYS_ELR);
}
- /*
- * Check for the conditions of Cortex-A510's #2077057. When these occur
- * SPSR_EL2 can't be trusted, but isn't needed either as it is
- * unchanged from the value in vcpu_gp_regs(vcpu)->pstate.
- * Did we just take a PAC exception when a step exception was expected?
- */
- if (IS_ENABLED(CONFIG_ARM64_ERRATUM_2077057) &&
- cpus_have_const_cap(ARM64_WORKAROUND_2077057) &&
- ARM_EXCEPTION_CODE(*exit_code) != ARM_EXCEPTION_IRQ &&
- esr_ec == ESR_ELx_EC_PAC &&
- vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
- /* Active-not-pending? */
- if (*vcpu_cpsr(vcpu) & DBG_SPSR_SS)
- write_sysreg_el2(*vcpu_cpsr(vcpu), SYS_SPSR);
- }
-
/*
* We're using the raw exception code in order to only process
* the trap if no SError is pending. We will come back to the
> + }
> +
> /*
> * We're using the raw exception code in order to only process
> * the trap if no SError is pending. We will come back to the
> diff --git a/arch/arm64/tools/cpucaps b/arch/arm64/tools/cpucaps
> index 870c39537dd0..2e7cd3fecca6 100644
> --- a/arch/arm64/tools/cpucaps
> +++ b/arch/arm64/tools/cpucaps
> @@ -55,6 +55,7 @@ WORKAROUND_1418040
> WORKAROUND_1463225
> WORKAROUND_1508412
> WORKAROUND_1542419
> +WORKAROUND_2077057
> WORKAROUND_TRBE_OVERWRITE_FILL_MODE
> WORKAROUND_TSB_FLUSH_FAILURE
> WORKAROUND_TRBE_WRITE_OUT_OF_RANGE
Other than that, I'm happy to take the series as a whole ASAP, if only
for the two pretty embarrassing bug fixes. If you can respin it
shortly and address the comments above, I'd like it into -rc2.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
