[PATCH] KVM: nVMX: Put vmcs12 pages if nested VM-Enter fails due to invalid guest state

[PATCH] KVM: nVMX: Put vmcs12 pages if nested VM-Enter fails due to invalid guest state

[Paolo Bonzini]

From: Sean Christopherson <seanjc@google.com>

Put all vmcs12 pages if KVM synthesizes a nested VM-Exit due to invalid
guest while emulating VMLAUNCH or VMRESUME.  The invalid guest state path
doesn't use nested_vmx_vmexit() as that API is intended to be used if and
only if L2 is active, and the open coded equivalent neglects to put the
vmcs12 pages.  Failure to put the vmcs12 pages leaks any pinned pages
(and/or mappings) if L1 retries VMLAUNCH/VMRESUME.

Note, the !from_vmenter scenario doesn't suffer the same problem, as
vmx_get_nested_state_pages() only gets/pins/maps the vmcs12 pages if L2 is
active, i.e. if a "full" VM-Exit is guaranteed before KVM will retry
getting vmcs12 pages.

Fixes: 96c66e87deee ("KVM/nVMX: Use kvm_vcpu_map when mapping the virtual APIC page")
Fixes: 3278e0492554 ("KVM/nVMX: Use kvm_vcpu_map when mapping the posted interrupt descriptor table")
Fixes: fe1911aa443e ("KVM: nVMX: Use kvm_vcpu_map() to get/pin vmcs12's APIC-access page")
Reported-by: Minh Nguyen <minhnguyen.080505@gmail.com>
Cc: stable@vger.kernel.org
Signed-off-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/vmx/nested.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index bb0eb40b4448..220d42ebc82e 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -3761,6 +3761,8 @@ enum nvmx_vmentry_status nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu,
 	if (!from_vmentry)
 		return NVMX_VMENTRY_VMEXIT;
 
+	nested_put_vmcs12_pages(vcpu);
+
 	load_vmcs12_host_state(vcpu, vmcs12);
 	vmcs12->vm_exit_reason = exit_reason.full;
 	if (enable_shadow_vmcs || nested_vmx_is_evmptr12_valid(vmx))
-- 
2.54.0

Re: [PATCH] KVM: nVMX: Put vmcs12 pages if nested VM-Enter fails due to invalid guest state

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The patch resolves a memory leak for the nested VM-Exit path but completely misses an identical leak on the adjacent nested VM-Fail path when `nested_vmx_check_controls_late` fails.
--

--- Patch [1]: [PATCH] KVM: nVMX: Put vmcs12 pages if nested VM-Enter fails due to invalid guest state ---
commit 6b002f51033edcc9544a018d075246a30742f7ef
Author: Sean Christopherson <seanjc@google.com>

KVM: nVMX: Put vmcs12 pages if nested VM-Enter fails due to invalid guest state

This commit adds a missing call to nested_put_vmcs12_pages() when KVM
synthesizes a nested VM-Exit due to invalid guest state. This prevents
a page leak if L1 retries VMLAUNCH/VMRESUME.

> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -3733,6 +3733,8 @@ enum nvmx_vmentry_status nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu,
>  	if (!from_vmentry)
>  		return NVMX_VMENTRY_VMEXIT;
>  
> +	nested_put_vmcs12_pages(vcpu);
> +
>  	load_vmcs12_host_state(vcpu, vmcs12);

[Severity: High]
This isn't a bug introduced by this patch, but does the fix miss an identical
leak on the adjacent nested VM-Fail path?

Earlier in nested_vmx_enter_non_root_mode(), pages are mapped by
nested_get_vmcs12_pages(). Immediately following that, if
nested_vmx_check_controls_late() fails, it returns NVMX_VMENTRY_VMFAIL:

arch/x86/kvm/vmx/nested.c:nested_vmx_enter_non_root_mode() {
	...
	if (from_vmentry) {
		if (unlikely(!nested_get_vmcs12_pages(vcpu))) {
			vmx_switch_vmcs(vcpu, &vmx->vmcs01);
			return NVMX_VMENTRY_KVM_INTERNAL_ERROR;
		}

		if (nested_vmx_check_controls_late(vcpu, vmcs12)) {
			vmx_switch_vmcs(vcpu, &vmx->vmcs01);
			return NVMX_VMENTRY_VMFAIL;
		}
	...
}

This early return bypasses the newly added nested_put_vmcs12_pages() cleanup
since it doesn't jump to the vmentry_fail_vmexit label.

If an L1 guest triggers VMLAUNCH with an invalid state, such as an invalid
tpr_threshold vs vtpr combination, and then retries the operation, does
__kvm_vcpu_map() unconditionally overwrite the map pointers without
releasing the old ones?

virt/kvm/kvm_main.c:__kvm_vcpu_map() {
	...
	map->pinned_page = NULL;
	map->page = NULL;
	map->hva = NULL;
	map->gfn = gfn;
	...
}

Could this sequence allow an untrusted L1 guest to exhaust host memory by
repeatedly leaking page references?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716074636.756689-1-pbonzini@redhat.com?part=1