| Message ID | 1582773688-4956-1-git-send-email-linmiaohe@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] KVM: X86: deprecate obsolete KVM_GET_CPUID2 ioctl | expand |
linmiaohe <linmiaohe@huawei.com> writes: > From: Miaohe Lin <linmiaohe@huawei.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@redhat.com> > Signed-off-by: Miaohe Lin <linmiaohe@huawei.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; > } Braces are not really needed not but all other cases in the switch have it so let's leave them here too. > 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. */ "should not be used" pre-patch, post-patch we can say "Can only be used as a reliable source of -EINVAL" :-) > #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) Surprisingly (or not), KVM_GET_CPUID2 is not even described in Documentation/virt/kvm/api.txt. Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
On 27/02/20 12:36, Vitaly Kuznetsov wrote: >> - if (copy_to_user(cpuid_arg, &cpuid, sizeof(cpuid))) >> - goto out; >> - r = 0; >> + r = -EINVAL; >> break; >> } > Braces are not really needed not but all other cases in the switch have > it so let's leave them here too. > We can remove the case altogether. Paolo
On 27/02/20 04:21, linmiaohe wrote: > From: Miaohe Lin <linmiaohe@huawei.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@redhat.com> > Signed-off-by: Miaohe Lin <linmiaohe@huawei.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
On Wed, Feb 26, 2020 at 7:20 PM linmiaohe <linmiaohe@huawei.com> wrote: > > From: Miaohe Lin <linmiaohe@huawei.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. I don't know how you can make the assertion that this ioctl is not used anywhere. For instance, I see a use of it in Google's code base.
On 02/03/20 18:01, Jim Mattson wrote: >> And in fact, it's not used anywhere. So it should be >> deprecated. > I don't know how you can make the assertion that this ioctl is not > used anywhere. For instance, I see a use of it in Google's code base. Right, it does not seem to be used anywhere according to e.g. Debian code search but of course it can have users. What are you using it for? It's true that cpuid->nent is never written back to userspace, so the ioctl is basically unusable unless you already know how many entries are written. Or unless you fill the CPUID entries with garbage before calling it, I guess; is that what you are doing? Paolo
On Mon, Mar 2, 2020 at 9:09 AM Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 02/03/20 18:01, Jim Mattson wrote: > >> And in fact, it's not used anywhere. So it should be > >> deprecated. > > I don't know how you can make the assertion that this ioctl is not > > used anywhere. For instance, I see a use of it in Google's code base. > > Right, it does not seem to be used anywhere according to e.g. Debian > code search but of course it can have users. > > What are you using it for? It's true that cpuid->nent is never written > back to userspace, so the ioctl is basically unusable unless you already > know how many entries are written. Or unless you fill the CPUID entries > with garbage before calling it, I guess; is that what you are doing? One could use GET_CPUID2 after SET_CPUID2, to see what changes kvm made to the requested guest CPUID information without telling you.
On 02/03/20 18:44, Jim Mattson wrote: > On Mon, Mar 2, 2020 at 9:09 AM Paolo Bonzini <pbonzini@redhat.com> wrote: >> >> On 02/03/20 18:01, Jim Mattson wrote: >>>> And in fact, it's not used anywhere. So it should be >>>> deprecated. >>> I don't know how you can make the assertion that this ioctl is not >>> used anywhere. For instance, I see a use of it in Google's code base. >> >> Right, it does not seem to be used anywhere according to e.g. Debian >> code search but of course it can have users. >> >> What are you using it for? It's true that cpuid->nent is never written >> back to userspace, so the ioctl is basically unusable unless you already >> know how many entries are written. Or unless you fill the CPUID entries >> with garbage before calling it, I guess; is that what you are doing? > > One could use GET_CPUID2 after SET_CPUID2, to see what changes kvm > made to the requested guest CPUID information without telling you. Yeah, I think GET_CPUID2 with the same number of leaves that you have passed to SET_CPUID2 should work. Paolo
On Mon, Mar 2, 2020 at 10:02 AM Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 02/03/20 18:44, Jim Mattson wrote: > > On Mon, Mar 2, 2020 at 9:09 AM Paolo Bonzini <pbonzini@redhat.com> wrote: > >> > >> On 02/03/20 18:01, Jim Mattson wrote: > >>>> And in fact, it's not used anywhere. So it should be > >>>> deprecated. > >>> I don't know how you can make the assertion that this ioctl is not > >>> used anywhere. For instance, I see a use of it in Google's code base. > >> > >> Right, it does not seem to be used anywhere according to e.g. Debian > >> code search but of course it can have users. > >> > >> What are you using it for? It's true that cpuid->nent is never written > >> back to userspace, so the ioctl is basically unusable unless you already > >> know how many entries are written. Or unless you fill the CPUID entries > >> with garbage before calling it, I guess; is that what you are doing? > > > > One could use GET_CPUID2 after SET_CPUID2, to see what changes kvm > > made to the requested guest CPUID information without telling you. > > Yeah, I think GET_CPUID2 with the same number of leaves that you have > passed to SET_CPUID2 should work. Having said that, it doesn't look like the method that invokes this ioctl (in Google's code base) gets called from anywhere.
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)