Re: [PATCH] KVM: x86: Fix KVM_GET_CPUID2 ioctl to return cpuid entries count

[Valeriy Vdovin]

On Wed, Apr 28, 2021 at 02:38:57PM +0200, Vitaly Kuznetsov wrote:
> Valeriy Vdovin <valeriy.vdovin@xxxxxxxxxxxxx> writes:
> 
> > KVM_GET_CPUID2 kvm ioctl is not very well documented, but the way it is
> > implemented in function kvm_vcpu_ioctl_get_cpuid2 suggests that even at
> > error path it will try to return number of entries to the caller. But
> > The dispatcher kvm vcpu ioctl dispatcher code in kvm_arch_vcpu_ioctl
> > ignores any output from this function if it sees the error return code.
> >
> > It's very explicit by the code that it was designed to receive some
> > small number of entries to return E2BIG along with the corrected number.
> >
> > This lost logic in the dispatcher code has been restored by removing the
> > lines that check for function return code and skip if error is found.
> > Without it, the ioctl caller will see both the number of entries and the
> > correct error.
> >
> > In selftests relevant function vcpu_get_cpuid has also been modified to
> > utilize the number of cpuid entries returned along with errno E2BIG.
> >
> > Signed-off-by: Valeriy Vdovin <valeriy.vdovin@xxxxxxxxxxxxx>
> > ---
> >  arch/x86/kvm/x86.c                            | 10 +++++-----
> >  .../selftests/kvm/lib/x86_64/processor.c      | 20 +++++++++++--------
> >  2 files changed, 17 insertions(+), 13 deletions(-)
> >
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index efc7a82ab140..df8a3e44e722 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -4773,14 +4773,14 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
> >  		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)))
> 
> It may make sense to check that 'r == -E2BIG' before trying to write
> anything back. I don't think it is correct/expected to modify nent in
> other cases (e.g. when kvm_vcpu_ioctl_get_cpuid2() returns -EFAULT)
> 
That's a good point. The caller could expect and rely on the fact that nent
is unmodified in any error case except E2BIG. I will add this in the next
version.
> > +
> > +		if (copy_to_user(cpuid_arg, &cpuid, sizeof(cpuid))) {
> > +			r = -EFAULT;
> >  			goto out;
> > -		r = 0;
> > +		}
> >  		break;
> 
> How is KVM userspace supposed to know if it can trust the 'nent' value
> (KVM is fixed case) or not (KVM is not fixed case)? This can probably be
> resolved with adding a new capability (but then I'm not sure the change
> is worth it to be honest).

As I see it KVM userspace should set nent to 0, and then expect any non-zero
value in return along with E2BIG. This is the same approach I've used in the
modified test code in the same patch.

> Also, if making such a change, API
> documentation in virt/kvm/api.rst needs updating.

Of course. I will add changes to the documentation and comments in case if this
change in general will have a go.

> 
> >  	}
> >  	case KVM_GET_MSRS: {
> > diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> > index a8906e60a108..a412b39ad791 100644
> > --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
> > +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> > @@ -727,17 +727,21 @@ struct kvm_cpuid2 *vcpu_get_cpuid(struct kvm_vm *vm, uint32_t vcpuid)
> >  
> >  	cpuid = allocate_kvm_cpuid2();
> >  	max_ent = cpuid->nent;
> > +	cpuid->nent = 0;
> >  
> > -	for (cpuid->nent = 1; cpuid->nent <= max_ent; cpuid->nent++) {
> > -		rc = ioctl(vcpu->fd, KVM_GET_CPUID2, cpuid);
> > -		if (!rc)
> > -			break;
> > +	rc = ioctl(vcpu->fd, KVM_GET_CPUID2, cpuid);
> > +	TEST_ASSERT(rc == -1 && errno == E2BIG,
> > +		    "KVM_GET_CPUID2 should return E2BIG: %d %d",
> > +		    rc, errno);
> >  
> > -		TEST_ASSERT(rc == -1 && errno == E2BIG,
> > -			    "KVM_GET_CPUID2 should either succeed or give E2BIG: %d %d",
> > -			    rc, errno);
> > -	}
> > +	TEST_ASSERT(cpuid->nent,
> > +		    "KVM_GET_CPUID2 failed to set cpuid->nent with E2BIG");
> > +
> > +	TEST_ASSERT(cpuid->nent < max_ent,
> > +		"KVM_GET_CPUID2 has %d entries, expected maximum: %d",
> > +		cpuid->nent, max_ent);
> >  
> > +	rc = ioctl(vcpu->fd, KVM_GET_CPUID2, cpuid);
> >  	TEST_ASSERT(rc == 0, "KVM_GET_CPUID2 failed, rc: %i errno: %i",
> >  		    rc, errno);
> 
> -- 
> Vitaly
>