Hi Marc,
On 25/01/2022 16:51, Marc Zyngier wrote:
> 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 :-(.
>> 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
>> @@ -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()...
Hmmm, I thought dead code elimination was a good thing!
Without the cpu_errata.c match, (which is also guarded by #ifdef), the cap will never be
true, even if an affected CPU showed up. This way the compiler knows it can remove all this.
>> + 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.
Yes - bother. Staring at it didn't let me spot that!
I can hit the conditions to test this, but due to lack of imagination the model doesn't
corrupt the SPSR.
> 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 &&
The vcpu's esr_el2 isn't yet set:
| ESR_ELx_EC(read_sysreg_el2(SYS_ESR)) == ESR_ELx_EC_PAC
(and I'll shuffle the order so this is last as its an extra sysreg read)
> + 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);
Even better, that saves the noise from moving esr_ec around!
> 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.
Will do. Shout if you strongly care about the IS_ENABLED().
Thanks,
James
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
