Message-ID: <YfF1TQx/vsV5OepU@google.com>
Date:   Wed, 26 Jan 2022 16:22:37 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] KVM: nVMX: WARN on any attempt to allocate shadow VMCS
 for vmcs02

On Wed, Jan 26, 2022, Paolo Bonzini wrote:
> On 1/26/22 16:56, Vitaly Kuznetsov wrote:
> > > -	WARN_ON(loaded_vmcs == &vmx->vmcs01 && loaded_vmcs->shadow_vmcs);
> > > +	if (WARN_ON(loaded_vmcs != &vmx->vmcs01 || loaded_vmcs->shadow_vmcs))
> > > +		return loaded_vmcs->shadow_vmcs;
> > Stupid question: why do we want to care about 'loaded_vmcs' at all,
> > i.e. why can't we hardcode 'vmx->vmcs01' in alloc_shadow_vmcs()?

Not a stupid question, I strongly considered doing exactly that, but elected to
keep the WARN only because of the reason Paolo stated below.

> > The only caller is enter_vmx_operation() and AFAIU 'loaded_vmcs' will
> > always be pointing to 'vmx->vmcs01' (as enter_vmx_operation() allocates
> > &vmx->nested.vmcs02 so 'loaded_vmcs' can't point there!).
> > 
> 
> Well, that's why the WARN never happens.  The idea is that if shadow VMCS
> _virtualization_ (not emulation, i.e. running L2 VMREAD/VMWRITE without even
> a vmexit to L0) was supported, then you would need a non-NULL shadow_vmcs in
> vmx->vmcs02.
> 
> Regarding the patch, the old WARN was messy but it was also trying to avoid
> a NULL pointer dereference in the caller.

But the sole caller does:

	if (enable_shadow_vmcs && !alloc_shadow_vmcs(vcpu))
		goto out_shadow_vmcs;

> What about:
> 
> 	if (WARN_ON(loaded_vmcs->shadow_vmcs))
> 		return loaded_vmcs->shadow_vmcs;
> 
> 	/* Go ahead anyway.  */
> 	WARN_ON(loaded_vmcs != &vmx->vmcs01);
> 
> ?

I don't like preceeding, because that will likely lead to a crash and/or WARNs if
KVM call the helper at the right time but with the wrong VMCS loaded, i.e. if
vmcs01.shadow_vmcs is left NULL, as many paths assumes vmcs01 is allocated if they
are reached with VMCS shadowing enabled.  At the very least, it will leak memory
because vmcs02.shadow_vmcs is never freed.

Maybe this to try and clarify things?  Compile tested only...

From: Sean Christopherson <seanjc@...gle.com>
Date: Tue, 25 Jan 2022 12:14:42 -0800
Subject: [PATCH] KVM: nVMX: WARN on any attempt to allocate shadow VMCS for
 vmcs02

WARN if KVM attempts to allocate a shadow VMCS for vmcs02 and mark the VM
as dead.  KVM emulates VMCS shadowing but doesn't virtualize it, i.e. KVM
should never allocate a "real" shadow VMCS for L2.  Many downstream flows
assume vmcs01.shadow_vmcs is non-NULL when VMCS shadowing is enabled, and
vmcs02.shadow_vmcs is (rightly) never freed, so continuing on in this
case is dangerous.

Opportunistically return an error code instead of a pointer to make it
more obvious that the helper sets the correct pointer in vmcs01, and that
the return value needs to be checked/handled.

Signed-off-by: Sean Christopherson <seanjc@...gle.com>
---
 arch/x86/kvm/vmx/nested.c | 35 ++++++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 13 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index f235f77cbc03..ccc10b92a92a 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -4845,25 +4845,29 @@ static int nested_vmx_get_vmptr(struct kvm_vcpu *vcpu, gpa_t *vmpointer,
  * VMCS, unless such a shadow VMCS already exists. The newly allocated
  * VMCS is also VMCLEARed, so that it is ready for use.
  */
-static struct vmcs *alloc_shadow_vmcs(struct kvm_vcpu *vcpu)
+static int alloc_shadow_vmcs(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 	struct loaded_vmcs *loaded_vmcs = vmx->loaded_vmcs;

 	/*
-	 * We should allocate a shadow vmcs for vmcs01 only when L1
-	 * executes VMXON and free it when L1 executes VMXOFF.
-	 * As it is invalid to execute VMXON twice, we shouldn't reach
-	 * here when vmcs01 already have an allocated shadow vmcs.
+	 * KVM allocates a shadow VMCS only when L1 executes VMXON and frees it
+	 * when L1 executes VMXOFF or the vCPU is forced out of nested
+	 * operation.  VMXON faults if the CPU is already post-VMXON, so it
+	 * should be impossible to already have an allocated shadow VMCS.  KVM
+	 * doesn't support virtualization of VMCS shadowing, so vmcs01 should
+	 * always be the loaded VMCS.
 	 */
-	WARN_ON(loaded_vmcs == &vmx->vmcs01 && loaded_vmcs->shadow_vmcs);
+	if (KVM_BUG_ON(loaded_vmcs != &vmx->vmcs01, vcpu->kvm))
+		return -EIO;

-	if (!loaded_vmcs->shadow_vmcs) {
+	if (!WARN_ON_ONCE(!loaded_vmcs->shadow_vmcs)) {
 		loaded_vmcs->shadow_vmcs = alloc_vmcs(true);
 		if (loaded_vmcs->shadow_vmcs)
 			vmcs_clear(loaded_vmcs->shadow_vmcs);
 	}
-	return loaded_vmcs->shadow_vmcs;
+
+	return 0;
 }

 static int enter_vmx_operation(struct kvm_vcpu *vcpu)
@@ -4872,7 +4876,7 @@ static int enter_vmx_operation(struct kvm_vcpu *vcpu)
 	int r;

 	r = alloc_loaded_vmcs(&vmx->nested.vmcs02);
-	if (r < 0)
+	if (r)
 		goto out_vmcs02;

 	vmx->nested.cached_vmcs12 = kzalloc(VMCS12_SIZE, GFP_KERNEL_ACCOUNT);
@@ -4881,11 +4885,16 @@ static int enter_vmx_operation(struct kvm_vcpu *vcpu)

 	vmx->nested.shadow_vmcs12_cache.gpa = INVALID_GPA;
 	vmx->nested.cached_shadow_vmcs12 = kzalloc(VMCS12_SIZE, GFP_KERNEL_ACCOUNT);
-	if (!vmx->nested.cached_shadow_vmcs12)
+	if (!vmx->nested.cached_shadow_vmcs12) {
+		r = -ENOMEM;
 		goto out_cached_shadow_vmcs12;
+	}

-	if (enable_shadow_vmcs && !alloc_shadow_vmcs(vcpu))
-		goto out_shadow_vmcs;
+	if (enable_shadow_vmcs) {
+		r = alloc_shadow_vmcs(vcpu);
+		if (r)
+			goto out_shadow_vmcs;
+	}

 	hrtimer_init(&vmx->nested.preemption_timer, CLOCK_MONOTONIC,
 		     HRTIMER_MODE_ABS_PINNED);
@@ -4913,7 +4922,7 @@ static int enter_vmx_operation(struct kvm_vcpu *vcpu)
 	free_loaded_vmcs(&vmx->nested.vmcs02);

 out_vmcs02:
-	return -ENOMEM;
+	return r;
 }

 /* Emulate the VMXON instruction. */
--

Powered by blists - more mailing lists