Subject: Re: [PATCH 4/4] KVM: arm64: Workaround Cortex-A510's single-step and PAC trap errata

From: Marc Zyngier <[email-masked]>
Date: Tue, 25 Jan 2022 16:51:45 +0000

Hi James,

Thanks for this. I guess.

On Tue, 25 Jan 2022 15:38:03 +0000,
James Morse <[email-masked]> 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: [email-masked] # ${GITHASHHERE}: arm64: Add Cortex-A510 CPU part definition
> Cc: [email-masked]
> Signed-off-by: James Morse <[email-masked]>
> ---
>  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.

_______________________________________________
linux-arm-kernel mailing list
[email-masked]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel