KVM: arm64: Fully zero the vcpu state on reset
Subsystems (4)
Marc Zyngier
maintainerPATCH v2 1/1 KVM: arm64: Fully zero the vcpu state on reset
On vcpu reset, we expect all the registers to be brought back to their initial state, which happens to be a bunch of zeroes.
However, some recent commit broke this, and is now leaving a bunch of registers (such as the FP state) with whatever was left by the guest. My bad.
Zero the reset of the state (32bit SPSRs and FPSIMD state).
---
Notes:
v2: Only reset the FPSIMD state and the AArch32 SPSRs to avoid corrupting CNTVOFF in creative ways.
arch/arm64/kvm/reset.c | 5 +++++ 1 file changed, 5 insertions(+)
arch/arm64/kvm/reset.c +5 -0
@@ -242,6 +242,11 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)/* Reset core registers */ memset (vcpu_gp_regs (vcpu),0 ,sizeof (*vcpu_gp_regs (vcpu))); +memset (&vcpu->arch.ctxt.fp_regs,0 ,sizeof (vcpu->arch.ctxt.fp_regs)); + vcpu->arch.ctxt.spsr_abt =0 ; + vcpu->arch.ctxt.spsr_und =0 ; + vcpu->arch.ctxt.spsr_irq =0 ; + vcpu->arch.ctxt.spsr_fiq =0 ;vcpu_gp_regs (vcpu)->pstate = pstate;/* Reset system registers */
Signature · 1 line
2.30.2
Hi Marc,
Quoted 9 lines · Marc Zyngier
On vcpu reset, we expect all the registers to be brought back to their initial state, which happens to be a bunch of zeroes.
However, some recent commit broke this, and is now leaving a bunch of registers (such as the FP state) with whatever was left by the guest. My bad.
Zero the reset of the state (32bit SPSRs and FPSIMD state).
Quoted 8 lines
Fixes e47c2055c68e ("KVM: arm64: Make struct kvm_regs userspace-only")Signed-off-by Marc Zyngier <[email protected]>---
Notes:
v2: Only reset the FPSIMD state and the AArch32 SPSRs to avoid corrupting CNTVOFF in creative ways.
I missed that last time, sorry.
Quoted 9 lines
arch/arm64/kvm/reset.c | 5 +++++ 1 file changed, 5 insertions(+)arch/arm64/kvm/reset.c +0
-0
arch/arm64/kvm/reset.c int kvm_reset_vcpu(struct kvm_vcpu *vcpu) · line 249
Quoted 8 lines
arch/arm64/kvm/reset.c +5
-0@@ -242,6 +242,11 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)/* Reset core registers */ memset (vcpu_gp_regs (vcpu),0 ,sizeof (*vcpu_gp_regs (vcpu))); +memset (&vcpu->arch.ctxt.fp_regs,0 ,sizeof (vcpu->arch.ctxt.fp_regs)); + vcpu->arch.ctxt.spsr_abt =0 ; + vcpu->arch.ctxt.spsr_und =0 ; + vcpu->arch.ctxt.spsr_irq =0 ; + vcpu->arch.ctxt.spsr_fiq =0 ;
Checked, and the only fields not touched by the change from struct kvm_cpu_context are sys_regs and __hyp_running_cpu. __hyp_running_cpu is assumed to be NULL for a guest and it doesn't change during the lifetime of a VCPU; it is set to NULL when struct kvm_vcpu is allocated.
CNTVOFF_EL2 is not accessible to userspace (it's not in the sys_reg_descs and in the invariant_sys_regs arrays), so it's not necessary to reset it in case userspace changed it. Same for the other registers that are part of the VCPU context, but are not in sys_reg_descs.
I think it's a good choice to skip zeroing the system registers. I compared vcpu->arch.ctxt.sys_regs with sys_regs_descs, there were quite a few registers that were part of the vcpu context (pointer authentication registers, virtual timer registers, and others) and not part of sys_regs_descs. If someone adds a register to the VCPU context, but not to sys_regs_descs, I think it would have been easy to miss the fact that KVM_ARM_VCPU_INIT zeroes it.
Hopefully I haven't missed anything:
Thanks,
Alex
arch/arm64/kvm/reset.c int kvm_reset_vcpu(struct kvm_vcpu *vcpu) · line 252
Quoted 3 lines
arch/arm64/kvm/reset.c +0
-0vcpu_gp_regs (vcpu)->pstate = pstate;/* Reset system registers */
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm