[PATCH] KVM: nVMX: Clear nested.smm.guest_mode on a failed RSM-to-L2

[PATCH] KVM: nVMX: Clear nested.smm.guest_mode on a failed RSM-to-L2

[Amirmohammad Eftekhar]

vmx_leave_smm() consumes nested.smm.vmxon into nested.vmxon up front, but
clears nested.smm.guest_mode only on the success path.  On either failure
arm (nested_vmx_check_restored_vmcs12() < 0, or
nested_vmx_enter_non_root_mode() != NVMX_VMENTRY_SUCCESS) it returns early
with nested.smm.guest_mode still true while nested.smm.vmxon is already
false.

em_rsm() turns that non-zero return into a triple fault, but the vCPU is
left with the internal pair (smm.guest_mode=true, smm.vmxon=false).  That
pair is not representable in migration state: vmx_get_nested_state()
serializes KVM_STATE_NESTED_SMM_GUEST_MODE without
KVM_STATE_NESTED_SMM_VMXON, and vmx_set_nested_state() rejects exactly
SMM_GUEST_MODE && !SMM_VMXON with -EINVAL.  KVM thus serializes a state it
refuses to restore -- a save/restore (live-migration) round-trip break.

The leak is sticky: free_nested() and vCPU reset do not clear
nested.smm.guest_mode (kvm_vcpu_reset() gates the nested unwind on
is_guest_mode(), false here), so a later VMXON re-exposes the bad
KVM_GET_NESTED_STATE with no SMI/RSM involved, for the life of the vCPU.

Clear nested.smm.guest_mode before the consistency check and re-entry, so
it is cleared on every exit from the block.  nested.vmxon is left true,
which is correct (L1 does have VMX on) and round-trips cleanly on its own.
Not a memory-safety issue; fidelity / migration only.

Cc: stable@vger.kernel.org
Signed-off-by: Amirmohammad Eftekhar <amirmohammad.eftekhar@cispa.de>
---
 arch/x86/kvm/vmx/vmx.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 1f802cdbe..56332a85c 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -8381,6 +8381,18 @@ int vmx_leave_smm(struct kvm_vcpu *vcpu, const union kvm_smram *smram)
 	}
 
 	if (vmx->nested.smm.guest_mode) {
+		/*
+		 * Clear smm.guest_mode up front so the failure arms below, which
+		 * triple-fault the vCPU, cannot leave the internal pair
+		 * (smm.guest_mode=true, smm.vmxon=false): smm.vmxon was already
+		 * consumed into nested.vmxon above, and KVM_GET_NESTED_STATE
+		 * would then serialize SMM_GUEST_MODE without SMM_VMXON, which
+		 * KVM_SET_NESTED_STATE rejects (-EINVAL), breaking the
+		 * save/restore round-trip.  The flag has served its only purpose
+		 * (bringing the vCPU into this re-entry) by now.
+		 */
+		vmx->nested.smm.guest_mode = false;
+
 		/* Triple fault if the state is invalid.  */
 		if (nested_vmx_check_restored_vmcs12(vcpu) < 0)
 			return 1;
@@ -8390,7 +8402,6 @@ int vmx_leave_smm(struct kvm_vcpu *vcpu, const union kvm_smram *smram)
 			return 1;
 
 		vcpu->arch.nested_run_pending = KVM_NESTED_RUN_PENDING;
-		vmx->nested.smm.guest_mode = false;
 	}
 	return 0;
 }
-- 
2.54.0 (Apple Git-157)

Re: [PATCH] KVM: nVMX: Clear nested.smm.guest_mode on a failed RSM-to-L2

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] KVM: nVMX: free_nested() fails to clear nested.smm.guest_mode
--

commit 9761932a71b71304448767969048672413614979
Author: Amirmohammad Eftekhar <amirmohammad.eftekhar@cispa.de>

KVM: nVMX: Clear nested.smm.guest_mode on a failed RSM-to-L2

This commit clears vmx->nested.smm.guest_mode before the vmcs12 consistency
check and L2 re-entry in vmx_leave_smm() so that a failed RSM-to-L2 does not
leave nested.smm.guest_mode set after nested.smm.vmxon has been cleared.

> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 612ab07d4100f..b704edab847be 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -8511,6 +8511,18 @@ int vmx_leave_smm(struct kvm_vcpu *vcpu, const union kvm_smram *smram)
>  	}
>  
>  	if (vmx->nested.smm.guest_mode) {
> +		/*
> +		 * Clear smm.guest_mode up front so the failure arms below, which
> +		 * triple-fault the vCPU, cannot leave the internal pair
> +		 * (smm.guest_mode=true, smm.vmxon=false): smm.vmxon was already
> +		 * consumed into nested.vmxon above, and KVM_GET_NESTED_STATE
> +		 * would then serialize SMM_GUEST_MODE without SMM_VMXON, which
> +		 * KVM_SET_NESTED_STATE rejects (-EINVAL), breaking the
> +		 * save/restore round-trip.  The flag has served its only purpose
> +		 * (bringing the vCPU into this re-entry) by now.
> +		 */
> +		vmx->nested.smm.guest_mode = false;
> +
>  		/* Triple fault if the state is invalid.  */
>  		if (nested_vmx_check_restored_vmcs12(vcpu) < 0)
>  			return 1;

[Severity: High]
Should free_nested() in arch/x86/kvm/vmx/nested.c also clear
vmx->nested.smm.guest_mode?

As noted in the commit message, free_nested() clears vmx->nested.smm.vmxon
and frees vmx->nested.cached_vmcs12, but leaves vmx->nested.smm.guest_mode
untouched:

arch/x86/kvm/vmx/nested.c:free_nested() {
	...
	vmx->nested.vmxon = false;
	vmx->nested.smm.vmxon = false;
	vmx->nested.vmxon_ptr = INVALID_GPA;
	...
	kfree(vmx->nested.cached_vmcs12);
	vmx->nested.cached_vmcs12 = NULL;
	...
}

If a vCPU enters SMM from L2 via vmx_enter_smm() (or has SMM_VMXON |
SMM_GUEST_MODE restored via vmx_set_nested_state()), both
vmx->nested.smm.vmxon and vmx->nested.smm.guest_mode are set to true while
is_guest_mode(vcpu) is false.

If vmx_leave_nested() -> free_nested() is then called while the vCPU is in
SMM -- for example, via vmx_set_nested_state() with vmxon_pa == INVALID_GPA,
or via a host-initiated write of 0 to MSR_IA32_FEAT_CTL in vmx_set_msr() --
free_nested() sets vmx->nested.smm.vmxon = false and frees
vmx->nested.cached_vmcs12 (setting it to NULL), while leaving
vmx->nested.smm.guest_mode = true.

When the guest subsequently executes RSM, emulator_leave_smm() ->
vmx_leave_smm() sees vmx->nested.smm.guest_mode == true and calls
nested_vmx_check_restored_vmcs12(vcpu), which dereferences a NULL vmcs12:

arch/x86/kvm/vmx/nested.c:nested_vmx_check_restored_vmcs12() {
	enum vm_entry_failure_code ignored;
	struct vmcs12 *vmcs12 = get_vmcs12(vcpu);

	if (nested_cpu_has_shadow_vmcs(vmcs12) &&
	...
}

Similarly, leaving SMM via kvm_vcpu_ioctl_x86_set_vcpu_events() calls
kvm_leave_nested() -> vmx_leave_nested() -> free_nested(), or calling
vmx_set_nested_state() to replace an SMM_GUEST_MODE state with an
SMM_VMXON-only state relies on vmx_leave_nested() -> free_nested() to clear
the prior SMM state:

arch/x86/kvm/vmx/nested.c:vmx_set_nested_state() {
	...
	vmx_leave_nested(vcpu);

	if (kvm_state->hdr.vmx.vmxon_pa == INVALID_GPA)
		return 0;
	...
	if (kvm_state->hdr.vmx.smm.flags & KVM_STATE_NESTED_SMM_VMXON) {
		vmx->nested.smm.vmxon = true;
		vmx->nested.vmxon = false;

		if (kvm_state->hdr.vmx.smm.flags & KVM_STATE_NESTED_SMM_GUEST_MODE)
			vmx->nested.smm.guest_mode = true;
	}
	...
}

Without clearing vmx->nested.smm.guest_mode in free_nested(), those paths
still leave (smm.guest_mode=true, smm.vmxon=false) or leak a stale
smm.guest_mode=true into the newly restored state, causing either a NULL
pointer dereference in nested_vmx_check_restored_vmcs12() on RSM or the same
KVM_GET_NESTED_STATE / KVM_SET_NESTED_STATE round-trip failure when the guest
later executes VMXON.

> @@ -8520,7 +8532,6 @@ int vmx_leave_smm(struct kvm_vcpu *vcpu, const union kvm_smram *smram)
>  			return 1;
>  
>  		vcpu->arch.nested_run_pending = KVM_NESTED_RUN_PENDING;
> -		vmx->nested.smm.guest_mode = false;
>  	}
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261017093016.32348-1-amirmohammad.eftekhar@cispa.de?part=1