Re: [PATCH] KVM: nVMX: Fix kernel info-leak when enabling KVM_CAP_HYPERV_ENLIGHTENED_VMCS more than once

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Liran Alon <liran.alon@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxxxx
> Reviewed-by: Krish Sadhukhan <krish.sadhukhan@xxxxxxxxxx>
> Signed-off-by: Liran Alon <liran.alon@xxxxxxxxxx>
> ---
>  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@xxxxxxxxxx>

Thanks!

-- 
Vitaly



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux