| Message ID | 20181117120506.89840-1-liran.alon@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: nVMX: Fix kernel info-leak when enabling KVM_CAP_HYPERV_ENLIGHTENED_VMCS more than once | expand |
Liran Alon <liran.alon@oracle.com> writes: > Consider the case that userspace enables KVM_CAP_HYPERV_ENLIGHTENED_VMCS twice: > 1) kvm_vcpu_ioctl_enable_cap() is called to enable > KVM_CAP_HYPERV_ENLIGHTENED_VMCS which calls nested_enable_evmcs(). > 2) nested_enable_evmcs() sets enlightened_vmcs_enabled to true and fills > vmcs_version which is then copied to userspace. > 3) kvm_vcpu_ioctl_enable_cap() is called again to enable > KVM_CAP_HYPERV_ENLIGHTENED_VMCS which calls nested_enable_evmcs(). > 4) This time nested_enable_evmcs() just returns 0 as > enlightened_vmcs_enabled is already true. *Without filling > vmcs_version*. > 5) kvm_vcpu_ioctl_enable_cap() continues as usual and copies > *uninitialized* vmcs_version to userspace which leads to kernel info-leak. > > Fix this issue by simply changing nested_enable_evmcs() to always fill > vmcs_version output argument. Even when enlightened_vmcs_enabled is > already set to true. > > Note that SVM's nested_enable_evmcs() should not be modified because it > always returns a non-zero value (-ENODEV) which results in > kvm_vcpu_ioctl_enable_cap() skipping the copy of vmcs_version to > userspace (as it should). > > Fixes: 57b119da3594 ("KVM: nVMX: add KVM_CAP_HYPERV_ENLIGHTENED_VMCS capability") > > Reported-by: syzbot+cfbc368e283d381f8cef@syzkaller.appspotmail.com > Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> > Signed-off-by: Liran Alon <liran.alon@oracle.com> > --- > arch/x86/kvm/vmx.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index 6e92624fe8d6..be1c6e37b57c 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -1610,12 +1610,6 @@ static int nested_enable_evmcs(struct kvm_vcpu *vcpu, > { > struct vcpu_vmx *vmx = to_vmx(vcpu); > > - /* We don't support disabling the feature for simplicity. */ > - if (vmx->nested.enlightened_vmcs_enabled) > - return 0; > - > - vmx->nested.enlightened_vmcs_enabled = true; > - > /* > * vmcs_version represents the range of supported Enlightened VMCS > * versions: lower 8 bits is the minimal version, higher 8 bits is the > @@ -1625,6 +1619,12 @@ static int nested_enable_evmcs(struct kvm_vcpu *vcpu, > if (vmcs_version) > *vmcs_version = (KVM_EVMCS_VERSION << 8) | 1; > > + /* We don't support disabling the feature for simplicity. */ > + if (vmx->nested.enlightened_vmcs_enabled) > + return 0; > + > + vmx->nested.enlightened_vmcs_enabled = true; > + > vmx->nested.msrs.pinbased_ctls_high &= ~EVMCS1_UNSUPPORTED_PINCTRL; > vmx->nested.msrs.entry_ctls_high &= ~EVMCS1_UNSUPPORTED_VMENTRY_CTRL; > vmx->nested.msrs.exit_ctls_high &= ~EVMCS1_UNSUPPORTED_VMEXIT_CTRL; Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com> Thanks!
On 17/11/18 13:05, Liran Alon wrote: > Consider the case that userspace enables KVM_CAP_HYPERV_ENLIGHTENED_VMCS twice: > 1) kvm_vcpu_ioctl_enable_cap() is called to enable > KVM_CAP_HYPERV_ENLIGHTENED_VMCS which calls nested_enable_evmcs(). > 2) nested_enable_evmcs() sets enlightened_vmcs_enabled to true and fills > vmcs_version which is then copied to userspace. > 3) kvm_vcpu_ioctl_enable_cap() is called again to enable > KVM_CAP_HYPERV_ENLIGHTENED_VMCS which calls nested_enable_evmcs(). > 4) This time nested_enable_evmcs() just returns 0 as > enlightened_vmcs_enabled is already true. *Without filling > vmcs_version*. > 5) kvm_vcpu_ioctl_enable_cap() continues as usual and copies > *uninitialized* vmcs_version to userspace which leads to kernel info-leak. > > Fix this issue by simply changing nested_enable_evmcs() to always fill > vmcs_version output argument. Even when enlightened_vmcs_enabled is > already set to true. > > Note that SVM's nested_enable_evmcs() should not be modified because it > always returns a non-zero value (-ENODEV) which results in > kvm_vcpu_ioctl_enable_cap() skipping the copy of vmcs_version to > userspace (as it should). > > Fixes: 57b119da3594 ("KVM: nVMX: add KVM_CAP_HYPERV_ENLIGHTENED_VMCS capability") > > Reported-by: syzbot+cfbc368e283d381f8cef@syzkaller.appspotmail.com > Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> > Signed-off-by: Liran Alon <liran.alon@oracle.com> > --- > arch/x86/kvm/vmx.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index 6e92624fe8d6..be1c6e37b57c 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -1610,12 +1610,6 @@ static int nested_enable_evmcs(struct kvm_vcpu *vcpu, > { > struct vcpu_vmx *vmx = to_vmx(vcpu); > > - /* We don't support disabling the feature for simplicity. */ > - if (vmx->nested.enlightened_vmcs_enabled) > - return 0; > - > - vmx->nested.enlightened_vmcs_enabled = true; > - > /* > * vmcs_version represents the range of supported Enlightened VMCS > * versions: lower 8 bits is the minimal version, higher 8 bits is the > @@ -1625,6 +1619,12 @@ static int nested_enable_evmcs(struct kvm_vcpu *vcpu, > if (vmcs_version) > *vmcs_version = (KVM_EVMCS_VERSION << 8) | 1; > > + /* We don't support disabling the feature for simplicity. */ > + if (vmx->nested.enlightened_vmcs_enabled) > + return 0; > + > + vmx->nested.enlightened_vmcs_enabled = true; > + > vmx->nested.msrs.pinbased_ctls_high &= ~EVMCS1_UNSUPPORTED_PINCTRL; > vmx->nested.msrs.entry_ctls_high &= ~EVMCS1_UNSUPPORTED_VMENTRY_CTRL; > vmx->nested.msrs.exit_ctls_high &= ~EVMCS1_UNSUPPORTED_VMEXIT_CTRL; > Queued, thanks. Paolo
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 6e92624fe8d6..be1c6e37b57c 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -1610,12 +1610,6 @@ static int nested_enable_evmcs(struct kvm_vcpu *vcpu, { struct vcpu_vmx *vmx = to_vmx(vcpu); - /* We don't support disabling the feature for simplicity. */ - if (vmx->nested.enlightened_vmcs_enabled) - return 0; - - vmx->nested.enlightened_vmcs_enabled = true; - /* * vmcs_version represents the range of supported Enlightened VMCS * versions: lower 8 bits is the minimal version, higher 8 bits is the @@ -1625,6 +1619,12 @@ static int nested_enable_evmcs(struct kvm_vcpu *vcpu, if (vmcs_version) *vmcs_version = (KVM_EVMCS_VERSION << 8) | 1; + /* We don't support disabling the feature for simplicity. */ + if (vmx->nested.enlightened_vmcs_enabled) + return 0; + + vmx->nested.enlightened_vmcs_enabled = true; + vmx->nested.msrs.pinbased_ctls_high &= ~EVMCS1_UNSUPPORTED_PINCTRL; vmx->nested.msrs.entry_ctls_high &= ~EVMCS1_UNSUPPORTED_VMENTRY_CTRL; vmx->nested.msrs.exit_ctls_high &= ~EVMCS1_UNSUPPORTED_VMEXIT_CTRL;
Consider the case that userspace enables KVM_CAP_HYPERV_ENLIGHTENED_VMCS twice: 1) kvm_vcpu_ioctl_enable_cap() is called to enable KVM_CAP_HYPERV_ENLIGHTENED_VMCS which calls nested_enable_evmcs(). 2) nested_enable_evmcs() sets enlightened_vmcs_enabled to true and fills vmcs_version which is then copied to userspace. 3) kvm_vcpu_ioctl_enable_cap() is called again to enable KVM_CAP_HYPERV_ENLIGHTENED_VMCS which calls nested_enable_evmcs(). 4) This time nested_enable_evmcs() just returns 0 as enlightened_vmcs_enabled is already true. *Without filling vmcs_version*. 5) kvm_vcpu_ioctl_enable_cap() continues as usual and copies *uninitialized* vmcs_version to userspace which leads to kernel info-leak. Fix this issue by simply changing nested_enable_evmcs() to always fill vmcs_version output argument. Even when enlightened_vmcs_enabled is already set to true. Note that SVM's nested_enable_evmcs() should not be modified because it always returns a non-zero value (-ENODEV) which results in kvm_vcpu_ioctl_enable_cap() skipping the copy of vmcs_version to userspace (as it should). Fixes: 57b119da3594 ("KVM: nVMX: add KVM_CAP_HYPERV_ENLIGHTENED_VMCS capability") Reported-by: syzbot+cfbc368e283d381f8cef@syzkaller.appspotmail.com Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> Signed-off-by: Liran Alon <liran.alon@oracle.com> --- arch/x86/kvm/vmx.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)