linux-kernel - Re: [PATCH v2] KVM: X86: deprecate obsolete KVM_GET

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <d94a44f0-7fed-b5e4-49e8-6dd1b89185db@redhat.com>
Date:   Mon, 2 Mar 2020 17:31:54 +0100
From:   Paolo Bonzini <pbonzini@...hat.com>
To:     linmiaohe <linmiaohe@...wei.com>, rkrcmar@...hat.com,
        sean.j.christopherson@...el.com, vkuznets@...hat.com,
        wanpengli@...cent.com, jmattson@...gle.com, joro@...tes.org,
        tglx@...utronix.de, mingo@...hat.com, bp@...en8.de, hpa@...or.com
Cc:     kvm@...r.kernel.org, linux-kernel@...r.kernel.org, x86@...nel.org
Subject: Re: [PATCH v2] KVM: X86: deprecate obsolete KVM_GET_CPUID2 ioctl

On 27/02/20 04:21, linmiaohe wrote:
> From: Miaohe Lin <linmiaohe@...wei.com>
> 
> When kvm_vcpu_ioctl_get_cpuid2() fails, we set cpuid->nent to the value of
> vcpu->arch.cpuid_nent. But this is in vain as cpuid->nent is not copied to
> userspace by copy_to_user() from call site. Also cpuid->nent is not updated
> to indicate how many entries were retrieved on success case. So this ioctl
> is straight up broken. And in fact, it's not used anywhere. So it should be
> deprecated.
> 
> Suggested-by: Paolo Bonzini <pbonzini@...hat.com>
> Signed-off-by: Miaohe Lin <linmiaohe@...wei.com>
> ---
>  arch/x86/kvm/cpuid.c           | 20 --------------------
>  arch/x86/kvm/cpuid.h           |  3 ---
>  arch/x86/kvm/x86.c             | 16 ++--------------
>  include/uapi/linux/kvm.h       |  1 +
>  tools/include/uapi/linux/kvm.h |  1 +
>  5 files changed, 4 insertions(+), 37 deletions(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index b1c469446b07..5e041a1282b8 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -261,26 +261,6 @@ int kvm_vcpu_ioctl_set_cpuid2(struct kvm_vcpu *vcpu,
>  	return r;
>  }
>  
> -int kvm_vcpu_ioctl_get_cpuid2(struct kvm_vcpu *vcpu,
> -			      struct kvm_cpuid2 *cpuid,
> -			      struct kvm_cpuid_entry2 __user *entries)
> -{
> -	int r;
> -
> -	r = -E2BIG;
> -	if (cpuid->nent < vcpu->arch.cpuid_nent)
> -		goto out;
> -	r = -EFAULT;
> -	if (copy_to_user(entries, &vcpu->arch.cpuid_entries,
> -			 vcpu->arch.cpuid_nent * sizeof(struct kvm_cpuid_entry2)))
> -		goto out;
> -	return 0;
> -
> -out:
> -	cpuid->nent = vcpu->arch.cpuid_nent;
> -	return r;
> -}
> -
>  static __always_inline void cpuid_mask(u32 *word, int wordnum)
>  {
>  	reverse_cpuid_check(wordnum);
> diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
> index 7366c618aa04..76555de38e1b 100644
> --- a/arch/x86/kvm/cpuid.h
> +++ b/arch/x86/kvm/cpuid.h
> @@ -19,9 +19,6 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
>  int kvm_vcpu_ioctl_set_cpuid2(struct kvm_vcpu *vcpu,
>  			      struct kvm_cpuid2 *cpuid,
>  			      struct kvm_cpuid_entry2 __user *entries);
> -int kvm_vcpu_ioctl_get_cpuid2(struct kvm_vcpu *vcpu,
> -			      struct kvm_cpuid2 *cpuid,
> -			      struct kvm_cpuid_entry2 __user *entries);
>  bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
>  	       u32 *ecx, u32 *edx, bool check_limit);
>  
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index ddd1d296bd20..a6d99abedb2c 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4295,21 +4295,9 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>  					      cpuid_arg->entries);
>  		break;
>  	}
> +	/* KVM_GET_CPUID2 is deprecated, should not be used. */
>  	case KVM_GET_CPUID2: {
> -		struct kvm_cpuid2 __user *cpuid_arg = argp;
> -		struct kvm_cpuid2 cpuid;
> -
> -		r = -EFAULT;
> -		if (copy_from_user(&cpuid, cpuid_arg, sizeof(cpuid)))
> -			goto out;
> -		r = kvm_vcpu_ioctl_get_cpuid2(vcpu, &cpuid,
> -					      cpuid_arg->entries);
> -		if (r)
> -			goto out;
> -		r = -EFAULT;
> -		if (copy_to_user(cpuid_arg, &cpuid, sizeof(cpuid)))
> -			goto out;
> -		r = 0;
> +		r = -EINVAL;
>  		break;
>  	}
>  	case KVM_GET_MSRS: {
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 4b95f9a31a2f..61524780603d 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1380,6 +1380,7 @@ struct kvm_s390_ucas_mapping {
>  #define KVM_GET_LAPIC             _IOR(KVMIO,  0x8e, struct kvm_lapic_state)
>  #define KVM_SET_LAPIC             _IOW(KVMIO,  0x8f, struct kvm_lapic_state)
>  #define KVM_SET_CPUID2            _IOW(KVMIO,  0x90, struct kvm_cpuid2)
> +/* KVM_GET_CPUID2 is deprecated, should not be used. */
>  #define KVM_GET_CPUID2            _IOWR(KVMIO, 0x91, struct kvm_cpuid2)
>  /* Available with KVM_CAP_VAPIC */
>  #define KVM_TPR_ACCESS_REPORTING  _IOWR(KVMIO, 0x92, struct kvm_tpr_access_ctl)
> diff --git a/tools/include/uapi/linux/kvm.h b/tools/include/uapi/linux/kvm.h
> index f0a16b4adbbd..2ef719af4c57 100644
> --- a/tools/include/uapi/linux/kvm.h
> +++ b/tools/include/uapi/linux/kvm.h
> @@ -1379,6 +1379,7 @@ struct kvm_s390_ucas_mapping {
>  #define KVM_GET_LAPIC             _IOR(KVMIO,  0x8e, struct kvm_lapic_state)
>  #define KVM_SET_LAPIC             _IOW(KVMIO,  0x8f, struct kvm_lapic_state)
>  #define KVM_SET_CPUID2            _IOW(KVMIO,  0x90, struct kvm_cpuid2)
> +/* KVM_GET_CPUID2 is deprecated, should not be used. */
>  #define KVM_GET_CPUID2            _IOWR(KVMIO, 0x91, struct kvm_cpuid2)
>  /* Available with KVM_CAP_VAPIC */
>  #define KVM_TPR_ACCESS_REPORTING  _IOWR(KVMIO, 0x92, struct kvm_tpr_access_ctl)
> 

Queued, thanks.

Paolo