Re: [PATCH] KVM: x86: Leave nested if vendor specific logic fails on RSM to L2

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] KVM: x86: stale L2 CR/EFER and MMU state on error in
  emulator_leave_smm()
--

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

KVM: x86: Leave nested if vendor specific logic fails on RSM to L2

This commit jumps to the exit path in emulator_leave_smm() when the vendor
leave_smm callback fails so that kvm_leave_nested() is called if the vCPU
already entered guest mode.

> diff --git a/arch/x86/kvm/smm.c b/arch/x86/kvm/smm.c
> index 656a38dad7e7a..23cb8211ec74e 100644
> --- a/arch/x86/kvm/smm.c
> +++ b/arch/x86/kvm/smm.c
> @@ -639,8 +639,10 @@ int emulator_leave_smm(struct x86_emulate_ctxt *ctxt)
>  	 * reasons), and so nSVM/nVMX would clobber state that is loaded from
>  	 * SMRAM and from the VMCS/VMCB.
>  	 */
> -	if (kvm_x86_call(leave_smm)(vcpu, &smram))
> -		return X86EMUL_UNHANDLEABLE;
> +	if (kvm_x86_call(leave_smm)(vcpu, &smram)) {
> +		ret = X86EMUL_UNHANDLEABLE;
> +		goto out;
> +	}
>  
>  #ifdef CONFIG_X86_64
>  	if (guest_cpu_cap_has(vcpu, X86_FEATURE_LM))
> @@ -661,6 +663,7 @@ int emulator_leave_smm(struct x86_emulate_ctxt *ctxt)
>  	 * the shutdown, so that L1 enters shutdown instead of seeing a VM-Exit
>  	 * that architecturally shouldn't be possible.
>  	 */
> +out:
>  	if (ret != X86EMUL_CONTINUE && is_guest_mode(vcpu))
>  		kvm_leave_nested(vcpu);
>  	return ret;

[Severity: Medium]
Does jumping directly to kvm_leave_nested() when leave_smm() fails on SVM
leave vcpu->arch.{efer,cr0,cr4} with partially loaded vmcb12 state while
switching back to L1?

Before calling leave_smm(), emulator_leave_smm() puts the vCPU into 32-bit
unpaged real mode (CR0.PG=0, CR0.PE=0, EFER=0).  In svm_leave_smm(),
nested_svm_check_cached_vmcb12() only checks kvm_vcpu_is_legal_cr3() when
both EFER_LME and X86_CR0_PG are set in vmcb12.  If vmcb12 has an illegal
CR3 with CR0.PG=0, enter_svm_guest_mode() enters guest mode and overwrites
vcpu->arch.{efer,cr0,cr4} before failing in nested_svm_load_cr3():

arch/x86/kvm/svm/nested.c:enter_svm_guest_mode() {
	...
	nested_vmcb02_prepare_control(svm);
	nested_vmcb02_prepare_save(svm);

	ret = nested_svm_load_cr3(&svm->vcpu, svm->nested.save.cr3,
				  nested_npt_enabled(svm), from_vmrun);
	if (ret)
		return ret;
	...
}

Because is_paging(vcpu) is false during RSM, nested_vmcb02_prepare_save()
and svm_set_cr0() leave EFER_LMA set in vcpu->arch.efer if vmcb12 has
EFER_LME=1, EFER_LMA=1, and CR0.PG=0.  Jumping to out: skips
rsm_load_state_*(), and svm_leave_nested() switches back to vmcb01 (CPL=0)
and clears HF_GUEST_MASK without restoring vcpu->arch.{efer,cr0,cr4} or
resetting the MMU.

When vmcb12 also has CR0.PE=0, em_rsm() then fails in
emulator_recalc_and_set_mode() because EFER_LMA=1 with CR0.PE=0, and
handle_emulation_failure() sees !is_guest_mode(vcpu) and CPL == 0 and
exits to userspace with KVM_EXIT_INTERNAL_ERROR instead of
KVM_EXIT_SHUTDOWN.  If CR0.PE=1 and userspace ignores KVM_EXIT_SHUTDOWN,
the vCPU runs in L1 with illegal or desynchronized CR/EFER and MMU state
(for example, EFER_LMA=1 with CR0.PG=0 and CR4.PAE=0, which can trigger
the KVM_BUG_ON() in kvm_calc_cpu_role()).

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