Re: [PATCH] KVM: nSVM: Leave nested if loading CR3 fails at the end of KVM_SET_NESTED_STATE

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] KVM: nSVM: unrestored vmcb01 save state in
  svm_set_nested_state() on error
- [Medium] KVM: nSVM: svm_leave_nested() fails to reinitialize MMU
  context on L2 exit
--

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

KVM: nSVM: Leave nested if loading CR3 fails at the end of KVM_SET_NESTED_STATE

This commit calls svm_leave_nested() when nested_svm_load_cr3() fails at
the end of svm_set_nested_state().

> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index 07554089ec05b..4b1cdc7798bb7 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -2114,8 +2114,10 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
>  	 */
>  	ret = nested_svm_load_cr3(&svm->vcpu, vcpu->arch.cr3,
>  				  nested_npt_enabled(svm), false);
> -	if (ret)
> +	if (ret) {
> +		svm_leave_nested(vcpu);
>  		goto out_free;
> +	}

[Severity: Medium]
Does calling svm_leave_nested() here leave vmcb01.ptr->save and vCPU
controls corrupted when nested_svm_load_cr3() fails?

By this point in svm_set_nested_state(), svm_copy_vmrun_state() has
already overwritten vmcb01.ptr->save with the raw user-supplied save area
without updating vcpu->arch, and nested_vmcb02_prepare_control() has
already applied L2 TSC scaling, TSC offset, and virtual NMI state.
svm_leave_nested() switches svm->vmcb back to vmcb01 without restoring
vmcb01.ptr->save from vmcb02.ptr->save or undoing the control changes,
leaving vmcb01's CR0/CR3/CR4/EFER out of sync with vcpu->arch and lacking
required shadow-paging bits when NPT is disabled on the host.

Since nested_svm_load_cr3() is called with reload_pdptrs=false, its only
failure condition is !kvm_vcpu_is_legal_cr3(vcpu, vcpu->arch.cr3), which
could be checked during the validation phase before mutating vCPU state.

[Severity: Medium]
Can svm_leave_nested() leave the vCPU's MMU uninitialized when forcing a
vCPU out of L2?

arch/x86/kvm/svm/nested.c:svm_leave_nested() {
    ...
		leave_guest_mode(vcpu);
    ...
		svm_switch_vmcb(svm, &svm->vmcb01);

		nested_svm_uninit_mmu_context(vcpu);
}

nested_svm_uninit_mmu_context() only sets vcpu->arch.mmu back to
&vcpu->arch.root_mmu without calling kvm_init_mmu().  If the vCPU was
already in L2 with nested NPT enabled and userspace updates CPUID (such
as reducing MAXPHYADDR, which also causes nested_svm_load_cr3() to fail),
kvm_mmu_after_set_cpuid() sets root_mmu.root_role.invalid = 1, while
kvm_init_mmu() skips reinitializing root_mmu because mmu_is_nested(vcpu)
is true.

After svm_leave_nested() exits guest mode without calling kvm_init_mmu(),
the vCPU is left in L1 with L2's gva_walk and root_mmu.root_role.invalid
still set.  On the next KVM_RUN with TDP MMU enabled,
kvm_tdp_mmu_alloc_root() allocates a root with role.invalid = 1,
triggering KVM_MMU_WARN_ON(!root || root->role.invalid) in
kvm_tdp_mmu_map() and leaking the invalid root on VM teardown.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113170405.473841-1-seanjc@google.com?part=1