Re: [PATCH 1/7] KVM: nSVM: Check validity of vmcs01 save area copied from SMRAM on RSM

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] KVM: nSVM: missing EFER.LMA == EFER.LME check in nested_svm_check_vmcb01_save()
- [Medium] KVM: nSVM: svm_leave_smm() clobbers active vmcb01 save area before validation
--

commit c828fbc1de480891becf95d339311e3eed500e73
Author: Sean Christopherson <seanjc@google.com>

KVM: nSVM: Check validity of vmcs01 save area copied from SMRAM on RSM

This commit factors out a helper to validate the vmcb01 save area and calls
it in svm_leave_smm() after copying L1's state from SMRAM when returning from
SMM into L2.

> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index af0ef678433d7..07554089ec05b 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -1889,6 +1889,21 @@ static void nested_copy_vmcb_cache_to_control(struct vmcb_control_area *dst,
>  	/* 'clean' and 'hv_enlightenments' are not changed by KVM */
>  }
>  
> +bool nested_svm_check_vmcb01_save(struct kvm_vcpu *vcpu,
> +				  struct vmcb_save_area *save)
> +{
> +	struct vmcb_save_area_cached save_cached;
> +
> +	if (!(save->cr0 & X86_CR0_PG) ||
> +	    !(save->cr0 & X86_CR0_PE) ||
> +	    (save->rflags & X86_EFLAGS_VM) ||
> +	    ((save->efer & EFER_LMA) && !(save->efer & EFER_LME)))
> +		return false;

[Severity: High]
Since nested_svm_check_vmcb01_save() requires save->cr0 & X86_CR0_PG to be
set, should this check also reject !(save->efer & EFER_LMA) &&
(save->efer & EFER_LME) (and CS.L=1 when EFER_LMA=0)?

nested_vmcb_check_save() does not check EFER_LMA against CR0.PG && EFER_LME,
so a vmcb01 save area with CR0.PG=1, CR4.PAE=1, EFER_LME=1, and EFER_LMA=0
passes nested_svm_check_vmcb01_save() when reached from svm_leave_smm() or
svm_set_nested_state().

When entering L2 with nested NPT enabled, nested_svm_init_mmu_context()
passes svm->vmcb01.ptr->save.efer directly into kvm_init_shadow_npt_mmu():

arch/x86/kvm/svm/nested.c:nested_svm_init_mmu_context() {
	...
	kvm_init_shadow_npt_mmu(vcpu, svm->vmcb01.ptr->save.cr4,
				svm->vmcb01.ptr->save.efer,
				svm->nested.ctl.nested_cr3,
				svm->nested.ctl.misc_ctl);
	...
}

Because kvm_calc_cpu_role() checks EFER_LMA via ____is_efer_lma(regs) rather
than EFER_LME, the nested NPT shadow MMU gets initialized as 32-bit PAE
paging instead of 64-bit 4/5-level paging.

In addition, on a subsequent nested #VMEXIT from L2 to L1,
nested_svm_vmexit() restores vmcb01->save.efer and vmcb01->save.cr0:

arch/x86/kvm/svm/nested.c:nested_svm_vmexit() {
	...
	svm_set_efer(vcpu, vmcb01->save.efer);
	svm_set_cr0(vcpu, vmcb01->save.cr0 | X86_CRCR0_PE);
	svm_set_cr4(vcpu, vmcb01->save.cr4);
	...
}

Since svm_set_cr0() only sets EFER_LMA when transitioning from
!is_paging(vcpu) to CR0.PG=1, if L2 had CR0.PG=1, L1 is left running with
CR0.PG=1, EFER_LME=1, and EFER_LMA=0 in vcpu->arch.efer.

[ ... ]

> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 136069dfcd44f..e30ee80624b72 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -5223,6 +5223,8 @@ static int svm_leave_smm(struct kvm_vcpu *vcpu, const union kvm_smram *smram)
>  	 */
>  
>  	svm_copy_vmrun_state(vcpu, &svm->vmcb01.ptr->save, m_save.map.hva + 0x400);
> +	if (!nested_svm_check_vmcb01_save(vcpu, &svm->vmcb01.ptr->save))
> +		return 1;

[Severity: Medium]
Can copying the untrusted HSAVE state directly into svm->vmcb01.ptr->save
before validating it leave vcpu->arch and the active VMCB01 out of sync if
nested_svm_check_vmcb01_save() (or nested_svm_check_cached_vmcb12()) fails?

Before calling svm_leave_smm(), emulator_leave_smm() transitions vcpu->arch
to unpaged real mode while !is_guest_mode(vcpu) and svm->vmcb points to
svm->vmcb01.ptr.

If svm_leave_smm() returns 1 after svm_copy_vmrun_state() has already
overwritten svm->vmcb01.ptr->save, emulator_leave_smm() aborts with
X86EMUL_UNHANDLEABLE and em_rsm() requests KVM_REQ_TRIPLE_FAULT, exiting to
userspace with KVM_EXIT_SHUTDOWN while still in L1.

At that point, vcpu->arch and vcpu->arch.root_mmu reflect unpaged real mode,
while the active svm->vmcb->save holds the rejected HSAVE state (including
segment registers, CR0, CR3, CR4, and EFER). Unlike svm_set_nested_state(),
which validates the temporary save buffer before calling
svm_copy_vmrun_state(), a subsequent KVM_GET_SREGS or KVM_RUN on the vCPU
will see the clobbered vmcb01 save area and copy svm->vmcb->save.cr0 and cr3
into vcpu->arch in svm_vcpu_run().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102231428.13523-1-pbonzini@redhat.com?part=1