Re: [PATCH] KVM: nVMX: Re-arm the vmcs12 pages request if mapping the pages fails

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

Pre-existing issues:
- [High] The patch fixes the missing re-arming of `KVM_REQ_GET_NESTED_STATE_PAGES` in `nested_get_vmcs12_pages()` for VMX but overlooks identical vulnerabilities in `vmx_get_nested_state_pages()` (VMX eVMCS) and `svm_get_nested_state_pages()` (SVM).
--

--- Patch [1]: [PATCH] KVM: nVMX: Re-arm the vmcs12 pages request if mapping the pages fails ---
commit ce93ee9654c7f639f51372bb8a7e1b25a376436b
Author: Jinwoo Lee <[email protected]>

KVM: nVMX: Re-arm the vmcs12 pages request if mapping the pages fails

This commit ensures KVM_REQ_GET_NESTED_STATE_PAGES is re-armed if
nested_get_vmcs12_pages() fails to map the pages. This forces KVM to retry the
mapping on the next KVM_RUN rather than resuming L2 with a stale vmcs02, which
prevents hardware from accessing unpinned host physical pages.

> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index ddf6df7bee93b..9e9bd6c541baa 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
[ ... ]
> @@ -3525,6 +3525,20 @@ static bool nested_get_vmcs12_pages(struct kvm_vcpu *vcpu)
>  		exec_controls_clearbit(vmx, CPU_BASED_USE_MSR_BITMAPS);
>  
>  	return true;
> +
> +fail:
> +	/*
> +	 * Re-arm the request so that KVM retries the mapping instead of running
> +	 * L2 with a stale vmcs02.  Bailing here leaves the vCPU in guest mode
> +	 * with vmcs02 loaded and its APIC-access, virtual-APIC and posted
> +	 * interrupt descriptor addresses still pointing at the host pages that
> +	 * were mapped for the *previous* nested VM-Enter, which have since been
> +	 * unmapped and unpinned by nested_put_vmcs12_pages().  KVM returns to
> +	 * userspace without leaving guest mode, so if userspace resumes the
> +	 * vCPU, VM-Enter succeeds and hardware accesses those stale HPAs.
> +	 */
> +	kvm_make_request(KVM_REQ_GET_NESTED_STATE_PAGES, vcpu);
> +	return false;
>  }
>  
>  static bool vmx_get_nested_state_pages(struct kvm_vcpu *vcpu)

[Severity: High]
This is a pre-existing issue, but should the same re-arm be applied to
vmx_get_nested_state_pages() and svm_get_nested_state_pages()?

If nested_get_evmcs_page() fails in vmx_get_nested_state_pages(), it returns
false and exits to userspace with KVM_EXIT_INTERNAL_ERROR without re-arming
the request:

arch/x86/kvm/vmx/nested.c:vmx_get_nested_state_pages() {
	if (!nested_get_evmcs_page(vcpu)) {
		pr_debug_ratelimited("%s: enlightened vmptrld failed\n",
				     __func__);
		vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
		vcpu->run->internal.suberror =
			KVM_INTERNAL_ERROR_EMULATION;
		vcpu->run->internal.ndata = 0;

		return false;
	}
    ...
}

Similarly, in arch/x86/kvm/svm/nested.c:svm_get_nested_state_pages(), if
load_pdptrs(), kvm_hv_verify_vp_assist(), or nested_svm_merge_msrpm() fail,
they also exit without re-arming.

If a compromised QEMU sets KVM_REQ_GET_NESTED_STATE_PAGES and KVM clears the
request but fails in these setup steps, a subsequent KVM_RUN could skip state
setup and proceed directly to VM-Enter L2 with stale control structures.

Does this allow L2 to resume with unpinned HPAs and lead to a use-after-free?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1