* [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: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
* 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
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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).