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

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

[Sean Christopherson]

Force the vCPU into L1 before synthesizing SHUTDOWN if RSM to L2 fails due
to vendor specific checks, e.g. because SMRAM was corrupted with illegal
state, and the bad state was detected *after* starting the transition back
to L2.  Architecturally, any fatal error when loading CPU state during RSM
is supposed to send the CPU into SHUTDOWN *before* switching to guest mode,
and leaving the vCPU in L2 is all but guaranteed to effectively corrupt
vCPU state (which doesn't matter for the guest since it's already hosed,
but matters if userspace misbehaves and ignores the SHUTDOWN, i.e. runs the
vCPU even though it's in a bad state).

Fixes: 3f6821aa147b ("KVM: x86: Forcibly leave nested if RSM to L2 hits shutdown")
Cc: stable@vger.kernel.org
Reported-by: Stefan Teodorescu <fane@google.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/smm.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/smm.c b/arch/x86/kvm/smm.c
index 656a38dad7e7..23cb8211ec74 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;

base-commit: d21eef2084a9af87ac2a783bc7c2b175ba114e4e
-- 
2.56.1.703.gf848ed807c-goog

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

[sashiko-bot]

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