| Message ID | 1563572390-28823-1-git-send-email-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] KVM: nVMX: do not use dangling shadow VMCS after guest reset | expand |
> On 20 Jul 2019, at 0:39, Paolo Bonzini <pbonzini@redhat.com> wrote: > > If a KVM guest is reset while running a nested guest, free_nested will > disable the shadow VMCS execution control in the vmcs01. However, > on the next KVM_RUN vmx_vcpu_run would nevertheless try to sync > the VMCS12 to the shadow VMCS which has since been freed. > > This causes a vmptrld of a NULL pointer on my machime, but Jan reports > the host to hang altogether. Let's see how much this trivial patch fixes. > > Reported-by: Jan Kiszka <jan.kiszka@siemens.com> > Cc: Liran Alon <liran.alon@oracle.com> > Cc: stable@vger.kernel.org > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> 1) Are we sure we prefer WARN_ON() instead of WARN_ON_ONCE()? 2) Should we also check for WARN_ON(!vmcs12)? As free_nested() also kfree(vmx->nested.cached_vmcs12). In fact, because free_nested() don’t put NULL in cached_vmcs12 after kfree() it, I wonder if we shouldn’t create a separate patch that does: (a) Modify free_nested() to put NULL in cached_vmcs12 after kfree(). (b) Put BUG_ON(!cached_vmcs12) in get_vmcs12() before returning value. -Liran > --- > arch/x86/kvm/vmx/nested.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c > index 4f23e34f628b..0f1378789bd0 100644 > --- a/arch/x86/kvm/vmx/nested.c > +++ b/arch/x86/kvm/vmx/nested.c > @@ -194,6 +194,7 @@ static void vmx_disable_shadow_vmcs(struct vcpu_vmx *vmx) > { > secondary_exec_controls_clearbit(vmx, SECONDARY_EXEC_SHADOW_VMCS); > vmcs_write64(VMCS_LINK_POINTER, -1ull); > + vmx->nested.need_vmcs12_to_shadow_sync = false; > } > > static inline void nested_release_evmcs(struct kvm_vcpu *vcpu) > @@ -1341,6 +1342,9 @@ static void copy_shadow_to_vmcs12(struct vcpu_vmx *vmx) > unsigned long val; > int i; > > + if (WARN_ON(!shadow_vmcs)) > + return; > + > preempt_disable(); > > vmcs_load(shadow_vmcs); > @@ -1373,6 +1377,9 @@ static void copy_vmcs12_to_shadow(struct vcpu_vmx *vmx) > unsigned long val; > int i, q; > > + if (WARN_ON(!shadow_vmcs)) > + return; > + > vmcs_load(shadow_vmcs); > > for (q = 0; q < ARRAY_SIZE(fields); q++) { > @@ -4436,7 +4443,6 @@ static inline void nested_release_vmcs12(struct kvm_vcpu *vcpu) > /* copy to memory all shadowed fields in case > they were modified */ > copy_shadow_to_vmcs12(vmx); > - vmx->nested.need_vmcs12_to_shadow_sync = false; > vmx_disable_shadow_vmcs(vmx); > } > vmx->nested.posted_intr_nv = -1; > -- > 1.8.3.1 >
On 20/07/19 00:06, Liran Alon wrote: > > >> On 20 Jul 2019, at 0:39, Paolo Bonzini <pbonzini@redhat.com> wrote: >> >> If a KVM guest is reset while running a nested guest, free_nested will >> disable the shadow VMCS execution control in the vmcs01. However, >> on the next KVM_RUN vmx_vcpu_run would nevertheless try to sync >> the VMCS12 to the shadow VMCS which has since been freed. >> >> This causes a vmptrld of a NULL pointer on my machime, but Jan reports >> the host to hang altogether. Let's see how much this trivial patch fixes. >> >> Reported-by: Jan Kiszka <jan.kiszka@siemens.com> >> Cc: Liran Alon <liran.alon@oracle.com> >> Cc: stable@vger.kernel.org >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > > 1) Are we sure we prefer WARN_ON() instead of WARN_ON_ONCE()? I don't think you can get it to be called in a loop, the calls are generally guarded by ifs. > 2) Should we also check for WARN_ON(!vmcs12)? As free_nested() also kfree(vmx->nested.cached_vmcs12). Well, it doesn't NULL it but it does NULL shadow_vmcs so the extra warning wouldn't add much. > In fact, because free_nested() don’t put NULL in cached_vmcs12 after kfree() it, I wonder if we shouldn’t create a separate patch that does: > (a) Modify free_nested() to put NULL in cached_vmcs12 after kfree(). > (b) Put BUG_ON(!cached_vmcs12) in get_vmcs12() before returning value. This is useful but a separate improvement (and not a bugfix, I want this patch to be small so it applies to older trees). Paolo > -Liran > >> --- >> arch/x86/kvm/vmx/nested.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c >> index 4f23e34f628b..0f1378789bd0 100644 >> --- a/arch/x86/kvm/vmx/nested.c >> +++ b/arch/x86/kvm/vmx/nested.c >> @@ -194,6 +194,7 @@ static void vmx_disable_shadow_vmcs(struct vcpu_vmx *vmx) >> { >> secondary_exec_controls_clearbit(vmx, SECONDARY_EXEC_SHADOW_VMCS); >> vmcs_write64(VMCS_LINK_POINTER, -1ull); >> + vmx->nested.need_vmcs12_to_shadow_sync = false; >> } >> >> static inline void nested_release_evmcs(struct kvm_vcpu *vcpu) >> @@ -1341,6 +1342,9 @@ static void copy_shadow_to_vmcs12(struct vcpu_vmx *vmx) >> unsigned long val; >> int i; >> >> + if (WARN_ON(!shadow_vmcs)) >> + return; >> + >> preempt_disable(); >> >> vmcs_load(shadow_vmcs); >> @@ -1373,6 +1377,9 @@ static void copy_vmcs12_to_shadow(struct vcpu_vmx *vmx) >> unsigned long val; >> int i, q; >> >> + if (WARN_ON(!shadow_vmcs)) >> + return; >> + >> vmcs_load(shadow_vmcs); >> >> for (q = 0; q < ARRAY_SIZE(fields); q++) { >> @@ -4436,7 +4443,6 @@ static inline void nested_release_vmcs12(struct kvm_vcpu *vcpu) >> /* copy to memory all shadowed fields in case >> they were modified */ >> copy_shadow_to_vmcs12(vmx); >> - vmx->nested.need_vmcs12_to_shadow_sync = false; >> vmx_disable_shadow_vmcs(vmx); >> } >> vmx->nested.posted_intr_nv = -1; >> -- >> 1.8.3.1 >> >
> On 20 Jul 2019, at 1:21, Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 20/07/19 00:06, Liran Alon wrote: >> >> >>> On 20 Jul 2019, at 0:39, Paolo Bonzini <pbonzini@redhat.com> wrote: >>> >>> If a KVM guest is reset while running a nested guest, free_nested will >>> disable the shadow VMCS execution control in the vmcs01. However, >>> on the next KVM_RUN vmx_vcpu_run would nevertheless try to sync >>> the VMCS12 to the shadow VMCS which has since been freed. >>> >>> This causes a vmptrld of a NULL pointer on my machime, but Jan reports >>> the host to hang altogether. Let's see how much this trivial patch fixes. >>> >>> Reported-by: Jan Kiszka <jan.kiszka@siemens.com> >>> Cc: Liran Alon <liran.alon@oracle.com> >>> Cc: stable@vger.kernel.org >>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> >> 1) Are we sure we prefer WARN_ON() instead of WARN_ON_ONCE()? > > I don't think you can get it to be called in a loop, the calls are > generally guarded by ifs. > >> 2) Should we also check for WARN_ON(!vmcs12)? As free_nested() also kfree(vmx->nested.cached_vmcs12). > > Well, it doesn't NULL it but it does NULL shadow_vmcs so the extra > warning wouldn't add much. > >> In fact, because free_nested() don’t put NULL in cached_vmcs12 after kfree() it, I wonder if we shouldn’t create a separate patch that does: >> (a) Modify free_nested() to put NULL in cached_vmcs12 after kfree(). >> (b) Put BUG_ON(!cached_vmcs12) in get_vmcs12() before returning value. > > This is useful but a separate improvement (and not a bugfix, I want this > patch to be small so it applies to older trees). > > Paolo ACK on all the above. :) Reviewed-by: Liran Alon <liran.alon@oracle.com> -Liran > >> -Liran >> >>> --- >>> arch/x86/kvm/vmx/nested.c | 8 +++++++- >>> 1 file changed, 7 insertions(+), 1 deletion(-) >>> >>> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c >>> index 4f23e34f628b..0f1378789bd0 100644 >>> --- a/arch/x86/kvm/vmx/nested.c >>> +++ b/arch/x86/kvm/vmx/nested.c >>> @@ -194,6 +194,7 @@ static void vmx_disable_shadow_vmcs(struct vcpu_vmx *vmx) >>> { >>> secondary_exec_controls_clearbit(vmx, SECONDARY_EXEC_SHADOW_VMCS); >>> vmcs_write64(VMCS_LINK_POINTER, -1ull); >>> + vmx->nested.need_vmcs12_to_shadow_sync = false; >>> } >>> >>> static inline void nested_release_evmcs(struct kvm_vcpu *vcpu) >>> @@ -1341,6 +1342,9 @@ static void copy_shadow_to_vmcs12(struct vcpu_vmx *vmx) >>> unsigned long val; >>> int i; >>> >>> + if (WARN_ON(!shadow_vmcs)) >>> + return; >>> + >>> preempt_disable(); >>> >>> vmcs_load(shadow_vmcs); >>> @@ -1373,6 +1377,9 @@ static void copy_vmcs12_to_shadow(struct vcpu_vmx *vmx) >>> unsigned long val; >>> int i, q; >>> >>> + if (WARN_ON(!shadow_vmcs)) >>> + return; >>> + >>> vmcs_load(shadow_vmcs); >>> >>> for (q = 0; q < ARRAY_SIZE(fields); q++) { >>> @@ -4436,7 +4443,6 @@ static inline void nested_release_vmcs12(struct kvm_vcpu *vcpu) >>> /* copy to memory all shadowed fields in case >>> they were modified */ >>> copy_shadow_to_vmcs12(vmx); >>> - vmx->nested.need_vmcs12_to_shadow_sync = false; >>> vmx_disable_shadow_vmcs(vmx); >>> } >>> vmx->nested.posted_intr_nv = -1; >>> -- >>> 1.8.3.1 >>> >> >
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c index 4f23e34f628b..0f1378789bd0 100644 --- a/arch/x86/kvm/vmx/nested.c +++ b/arch/x86/kvm/vmx/nested.c @@ -194,6 +194,7 @@ static void vmx_disable_shadow_vmcs(struct vcpu_vmx *vmx) { secondary_exec_controls_clearbit(vmx, SECONDARY_EXEC_SHADOW_VMCS); vmcs_write64(VMCS_LINK_POINTER, -1ull); + vmx->nested.need_vmcs12_to_shadow_sync = false; } static inline void nested_release_evmcs(struct kvm_vcpu *vcpu) @@ -1341,6 +1342,9 @@ static void copy_shadow_to_vmcs12(struct vcpu_vmx *vmx) unsigned long val; int i; + if (WARN_ON(!shadow_vmcs)) + return; + preempt_disable(); vmcs_load(shadow_vmcs); @@ -1373,6 +1377,9 @@ static void copy_vmcs12_to_shadow(struct vcpu_vmx *vmx) unsigned long val; int i, q; + if (WARN_ON(!shadow_vmcs)) + return; + vmcs_load(shadow_vmcs); for (q = 0; q < ARRAY_SIZE(fields); q++) { @@ -4436,7 +4443,6 @@ static inline void nested_release_vmcs12(struct kvm_vcpu *vcpu) /* copy to memory all shadowed fields in case they were modified */ copy_shadow_to_vmcs12(vmx); - vmx->nested.need_vmcs12_to_shadow_sync = false; vmx_disable_shadow_vmcs(vmx); } vmx->nested.posted_intr_nv = -1;
If a KVM guest is reset while running a nested guest, free_nested will disable the shadow VMCS execution control in the vmcs01. However, on the next KVM_RUN vmx_vcpu_run would nevertheless try to sync the VMCS12 to the shadow VMCS which has since been freed. This causes a vmptrld of a NULL pointer on my machime, but Jan reports the host to hang altogether. Let's see how much this trivial patch fixes. Reported-by: Jan Kiszka <jan.kiszka@siemens.com> Cc: Liran Alon <liran.alon@oracle.com> Cc: stable@vger.kernel.org Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- arch/x86/kvm/vmx/nested.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)