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

[sashiko-bot@kernel.org]

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