| Message ID | 20160708223606.GA17035@jmattson.sea.corp.google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09/07/2016 00:36, Jim Mattson wrote: > When freeing the nested resources of a vcpu, there is an assumption that > the vcpu's vmcs01 is the current VMCS on the CPU that executes > nested_release_vmcs12(). If this assumption is violated, the vcpu's > vmcs01 may be made active on multiple CPUs at the same time, in > violation of Intel's specification. Moreover, since the vcpu's vmcs01 is > not VMCLEARed on every CPU on which it is active, it can linger in a > CPU's VMCS cache after it has been freed and potentially > repurposed. Subsequent eviction from the CPU's VMCS cache on a capacity > miss can result in memory corruption. > > It is not sufficient for vmx_free_vcpu() to call vmx_load_vmcs01(). If > the vcpu in question was last loaded on a different CPU, it must be > migrated to the current CPU before calling vmx_load_vmcs01(). > > Signed-off-by: Jim Mattson <jmattson@google.com> > > --- > arch/x86/kvm/vmx.c | 19 +++++++++++++++++-- > virt/kvm/kvm_main.c | 2 ++ > 2 files changed, 19 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index 003618e..afb1ab3 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -8855,6 +8855,22 @@ static void vmx_load_vmcs01(struct kvm_vcpu *vcpu) > put_cpu(); > } > > +/* > + * Ensure that the current vmcs of the logical processor is the > + * vmcs01 of the vcpu before calling free_nested(). > + */ > +static void vmx_free_vcpu_nested(struct kvm_vcpu *vcpu) > +{ > + struct vcpu_vmx *vmx = to_vmx(vcpu); > + int r; > + > + r = vcpu_load(vcpu); > + BUG_ON(r); > + vmx_load_vmcs01(vcpu); > + free_nested(vmx); > + vcpu_put(vcpu); > +} > + > static void vmx_free_vcpu(struct kvm_vcpu *vcpu) > { > struct vcpu_vmx *vmx = to_vmx(vcpu); > @@ -8863,8 +8879,7 @@ static void vmx_free_vcpu(struct kvm_vcpu *vcpu) > vmx_destroy_pml_buffer(vmx); > free_vpid(vmx->vpid); > leave_guest_mode(vcpu); > - vmx_load_vmcs01(vcpu); > - free_nested(vmx); > + vmx_free_vcpu_nested(vcpu); > free_loaded_vmcs(vmx->loaded_vmcs); > kfree(vmx->guest_msrs); > kvm_vcpu_uninit(vcpu); > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 48bd520..dd25346 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -148,6 +148,7 @@ int vcpu_load(struct kvm_vcpu *vcpu) > put_cpu(); > return 0; > } > +EXPORT_SYMBOL_GPL(vcpu_load); > > void vcpu_put(struct kvm_vcpu *vcpu) > { > @@ -157,6 +158,7 @@ void vcpu_put(struct kvm_vcpu *vcpu) > preempt_enable(); > mutex_unlock(&vcpu->mutex); > } > +EXPORT_SYMBOL_GPL(vcpu_put); > > static void ack_flush(void *_completed) > { > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Looks good, thanks! I haven't yet pushed it out, but I plan to send it to Linus for 4.7. So also Cc: stable <stable@vger.kernel.org> Thanks, Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 003618e..afb1ab3 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -8855,6 +8855,22 @@ static void vmx_load_vmcs01(struct kvm_vcpu *vcpu) put_cpu(); } +/* + * Ensure that the current vmcs of the logical processor is the + * vmcs01 of the vcpu before calling free_nested(). + */ +static void vmx_free_vcpu_nested(struct kvm_vcpu *vcpu) +{ + struct vcpu_vmx *vmx = to_vmx(vcpu); + int r; + + r = vcpu_load(vcpu); + BUG_ON(r); + vmx_load_vmcs01(vcpu); + free_nested(vmx); + vcpu_put(vcpu); +} + static void vmx_free_vcpu(struct kvm_vcpu *vcpu) { struct vcpu_vmx *vmx = to_vmx(vcpu); @@ -8863,8 +8879,7 @@ static void vmx_free_vcpu(struct kvm_vcpu *vcpu) vmx_destroy_pml_buffer(vmx); free_vpid(vmx->vpid); leave_guest_mode(vcpu); - vmx_load_vmcs01(vcpu); - free_nested(vmx); + vmx_free_vcpu_nested(vcpu); free_loaded_vmcs(vmx->loaded_vmcs); kfree(vmx->guest_msrs); kvm_vcpu_uninit(vcpu); diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 48bd520..dd25346 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -148,6 +148,7 @@ int vcpu_load(struct kvm_vcpu *vcpu) put_cpu(); return 0; } +EXPORT_SYMBOL_GPL(vcpu_load); void vcpu_put(struct kvm_vcpu *vcpu) { @@ -157,6 +158,7 @@ void vcpu_put(struct kvm_vcpu *vcpu) preempt_enable(); mutex_unlock(&vcpu->mutex); } +EXPORT_SYMBOL_GPL(vcpu_put); static void ack_flush(void *_completed) {
When freeing the nested resources of a vcpu, there is an assumption that the vcpu's vmcs01 is the current VMCS on the CPU that executes nested_release_vmcs12(). If this assumption is violated, the vcpu's vmcs01 may be made active on multiple CPUs at the same time, in violation of Intel's specification. Moreover, since the vcpu's vmcs01 is not VMCLEARed on every CPU on which it is active, it can linger in a CPU's VMCS cache after it has been freed and potentially repurposed. Subsequent eviction from the CPU's VMCS cache on a capacity miss can result in memory corruption. It is not sufficient for vmx_free_vcpu() to call vmx_load_vmcs01(). If the vcpu in question was last loaded on a different CPU, it must be migrated to the current CPU before calling vmx_load_vmcs01(). Signed-off-by: Jim Mattson <jmattson@google.com> --- arch/x86/kvm/vmx.c | 19 +++++++++++++++++-- virt/kvm/kvm_main.c | 2 ++ 2 files changed, 19 insertions(+), 2 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html