* [PATCH] KVM: nVMX: WARN on any attempt to allocate shadow VMCS for vmcs02
@ 2022-01-25 22:05 Sean Christopherson
2022-01-26 15:56 ` Vitaly Kuznetsov
0 siblings, 1 reply; 6+ messages in thread
From: Sean Christopherson @ 2022-01-25 22:05 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, kvm, linux-kernel
WARN if KVM attempts to allocate a shadow VMCS for vmcs02. KVM emulates
VMCS shadowing but doesn't virtualize it, i.e. KVM should never allocate
a "real" shadow VMCS for L2.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/x86/kvm/vmx/nested.c | 22 ++++++++++++----------
1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index f235f77cbc03..92ee0d821a06 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -4851,18 +4851,20 @@ static struct vmcs *alloc_shadow_vmcs(struct kvm_vcpu *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 (WARN_ON(loaded_vmcs != &vmx->vmcs01 || loaded_vmcs->shadow_vmcs))
+ return loaded_vmcs->shadow_vmcs;
+
+ loaded_vmcs->shadow_vmcs = alloc_vmcs(true);
+ if (loaded_vmcs->shadow_vmcs)
+ vmcs_clear(loaded_vmcs->shadow_vmcs);
- if (!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;
}
base-commit: edb9e50dbe18394d0fc9d0494f5b6046fc912d33
--
2.35.0.rc0.227.g00780c9af4-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: nVMX: WARN on any attempt to allocate shadow VMCS for vmcs02
2022-01-25 22:05 [PATCH] KVM: nVMX: WARN on any attempt to allocate shadow VMCS for vmcs02 Sean Christopherson
@ 2022-01-26 15:56 ` Vitaly Kuznetsov
2022-01-26 16:05 ` Paolo Bonzini
0 siblings, 1 reply; 6+ messages in thread
From: Vitaly Kuznetsov @ 2022-01-26 15:56 UTC (permalink / raw)
To: Sean Christopherson
Cc: Paolo Bonzini, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel
Sean Christopherson <seanjc@google.com> writes:
> WARN if KVM attempts to allocate a shadow VMCS for vmcs02. KVM emulates
> VMCS shadowing but doesn't virtualize it, i.e. KVM should never allocate
> a "real" shadow VMCS for L2.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
> arch/x86/kvm/vmx/nested.c | 22 ++++++++++++----------
> 1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index f235f77cbc03..92ee0d821a06 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -4851,18 +4851,20 @@ static struct vmcs *alloc_shadow_vmcs(struct kvm_vcpu *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 (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()? 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!).
> +
> + loaded_vmcs->shadow_vmcs = alloc_vmcs(true);
> + if (loaded_vmcs->shadow_vmcs)
> + vmcs_clear(loaded_vmcs->shadow_vmcs);
>
> - if (!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;
> }
>
>
> base-commit: edb9e50dbe18394d0fc9d0494f5b6046fc912d33
--
Vitaly
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: nVMX: WARN on any attempt to allocate shadow VMCS for vmcs02
2022-01-26 15:56 ` Vitaly Kuznetsov
@ 2022-01-26 16:05 ` Paolo Bonzini
2022-01-26 16:22 ` Sean Christopherson
2022-01-26 16:27 ` Vitaly Kuznetsov
0 siblings, 2 replies; 6+ messages in thread
From: Paolo Bonzini @ 2022-01-26 16:05 UTC (permalink / raw)
To: Vitaly Kuznetsov, Sean Christopherson
Cc: Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel
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()? 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.
What about:
if (WARN_ON(loaded_vmcs->shadow_vmcs))
return loaded_vmcs->shadow_vmcs;
/* Go ahead anyway. */
WARN_ON(loaded_vmcs != &vmx->vmcs01);
?
Paolo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: nVMX: WARN on any attempt to allocate shadow VMCS for vmcs02
2022-01-26 16:05 ` Paolo Bonzini
@ 2022-01-26 16:22 ` Sean Christopherson
2022-01-26 17:05 ` Paolo Bonzini
2022-01-26 16:27 ` Vitaly Kuznetsov
1 sibling, 1 reply; 6+ messages in thread
From: Sean Christopherson @ 2022-01-26 16:22 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm,
linux-kernel
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@google.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@google.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. */
--
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: nVMX: WARN on any attempt to allocate shadow VMCS for vmcs02
2022-01-26 16:05 ` Paolo Bonzini
2022-01-26 16:22 ` Sean Christopherson
@ 2022-01-26 16:27 ` Vitaly Kuznetsov
1 sibling, 0 replies; 6+ messages in thread
From: Vitaly Kuznetsov @ 2022-01-26 16:27 UTC (permalink / raw)
To: Paolo Bonzini, Sean Christopherson
Cc: Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel
Paolo Bonzini <pbonzini@redhat.com> writes:
> 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()? 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.
>
> What about:
>
> if (WARN_ON(loaded_vmcs->shadow_vmcs))
> return loaded_vmcs->shadow_vmcs;
>
> /* Go ahead anyway. */
> WARN_ON(loaded_vmcs != &vmx->vmcs01);
>
> ?
>
FWIW, this looks better [to my personal taste].
--
Vitaly
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: nVMX: WARN on any attempt to allocate shadow VMCS for vmcs02
2022-01-26 16:22 ` Sean Christopherson
@ 2022-01-26 17:05 ` Paolo Bonzini
0 siblings, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2022-01-26 17:05 UTC (permalink / raw)
To: Sean Christopherson
Cc: Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm,
linux-kernel
On 1/26/22 17:22, Sean Christopherson wrote:
> 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...
Your patch is okay, just with an extra paragraph in the commit message:
The previous code WARNed but continued anyway with the allocation,
presumably in an attempt to avoid NULL pointer dereference.
However, alloc_vmcs (and hence alloc_shadow_vmcs) can fail, and
indeed the sole caller does:
if (enable_shadow_vmcs && !alloc_shadow_vmcs(vcpu))
goto out_shadow_vmcs;
which makes it not a useful attempt.
Paolo
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-01-26 17:05 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-25 22:05 [PATCH] KVM: nVMX: WARN on any attempt to allocate shadow VMCS for vmcs02 Sean Christopherson
2022-01-26 15:56 ` Vitaly Kuznetsov
2022-01-26 16:05 ` Paolo Bonzini
2022-01-26 16:22 ` Sean Christopherson
2022-01-26 17:05 ` Paolo Bonzini
2022-01-26 16:27 ` Vitaly Kuznetsov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.