| Message ID | 1582293926-23388-1-git-send-email-linmiaohe@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: X86: eliminate some meaningless code | expand |
On Fri, Feb 21, 2020 at 10:05:26PM +0800, 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. Get rid of this meaningless > assignment and further cleanup the var r and out jump label. Ha, took me a while to see that. > On the other hand, when kvm_vcpu_ioctl_get_cpuid2() succeeds, we do not > change the content of struct cpuid. We can avoid copy_to_user() from call > site as struct cpuid remain unchanged. > > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> > --- > arch/x86/kvm/cpuid.c | 14 ++++---------- > arch/x86/kvm/x86.c | 6 ------ > 2 files changed, 4 insertions(+), 16 deletions(-) > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > index b1c469446b07..b83cedc63328 100644 > --- a/arch/x86/kvm/cpuid.c > +++ b/arch/x86/kvm/cpuid.c > @@ -265,20 +265,14 @@ 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; > + return -E2BIG; > + > if (copy_to_user(entries, &vcpu->arch.cpuid_entries, > vcpu->arch.cpuid_nent * sizeof(struct kvm_cpuid_entry2))) > - goto out; > - return 0; Hmm, so this ioctl() is straight up broken. cpuid->nent should be updated on success so that userspace knows how many entries were retrieved, i.e. the code should look something like below, with kvm_arch_vcpu_ioctl() unchanged. I'm guessing no VMM actually uses this ioctl(), e.g. neither Qemu or CrosVM use it, which is why the broken behavior has gone unnoticed. Don't suppose you'd want to write a selftest to hammer KVM_{SET,GET}_CPUID2? int kvm_vcpu_ioctl_get_cpuid2(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid, struct kvm_cpuid_entry2 __user *entries) { if (cpuid->nent < vcpu->arch.cpuid_nent) return -E2BIG; if (copy_to_user(entries, &vcpu->arch.cpuid_entries, vcpu->arch.cpuid_nent * sizeof(struct kvm_cpuid_entry2))) return -EFAULT; cpuid->nent = vcpu->arch.cpuid_nent; return 0; } > + return -EFAULT; > > -out: > - cpuid->nent = vcpu->arch.cpuid_nent; > - return r; > + return 0; > } > > static __always_inline void cpuid_mask(u32 *word, int wordnum) > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index fbabb2f06273..683c54e7be36 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -4295,12 +4295,6 @@ long kvm_arch_vcpu_ioctl(struct file *filp, > 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; > break; > } > case KVM_GET_MSRS: { > -- > 2.19.1 >
On 21/02/20 16:23, Sean Christopherson wrote: > > I'm guessing no VMM actually uses this ioctl(), e.g. neither Qemu or CrosVM > use it, which is why the broken behavior has gone unnoticed. Don't suppose > you'd want to write a selftest to hammer KVM_{SET,GET}_CPUID2? > > int kvm_vcpu_ioctl_get_cpuid2(struct kvm_vcpu *vcpu, > struct kvm_cpuid2 *cpuid, > struct kvm_cpuid_entry2 __user *entries) > { > if (cpuid->nent < vcpu->arch.cpuid_nent) > return -E2BIG; > > if (copy_to_user(entries, &vcpu->arch.cpuid_entries, > vcpu->arch.cpuid_nent * sizeof(struct kvm_cpuid_entry2))) > return -EFAULT; > > cpuid->nent = vcpu->arch.cpuid_nent; > > return 0; > } I would just drop KVM_GET_CPUID2 altogether and see if someone complains. Paolo
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index b1c469446b07..b83cedc63328 100644 --- a/arch/x86/kvm/cpuid.c +++ b/arch/x86/kvm/cpuid.c @@ -265,20 +265,14 @@ 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; + return -E2BIG; + if (copy_to_user(entries, &vcpu->arch.cpuid_entries, vcpu->arch.cpuid_nent * sizeof(struct kvm_cpuid_entry2))) - goto out; - return 0; + return -EFAULT; -out: - cpuid->nent = vcpu->arch.cpuid_nent; - return r; + return 0; } static __always_inline void cpuid_mask(u32 *word, int wordnum) diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index fbabb2f06273..683c54e7be36 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -4295,12 +4295,6 @@ long kvm_arch_vcpu_ioctl(struct file *filp, 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; break; } case KVM_GET_MSRS: {