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

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

[Valeriy Vdovin]

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@virtuozzo.com>
---
 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)))
+
+		if (copy_to_user(cpuid_arg, &cpuid, sizeof(cpuid))) {
+			r = -EFAULT;
 			goto out;
-		r = 0;
+		}
 		break;
 	}
 	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);
 
-- 
2.17.1

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

[Vitaly Kuznetsov]

Valeriy Vdovin <valeriy.vdovin@virtuozzo.com> 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@virtuozzo.com>
> ---
>  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)

> +
> +		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). Also, if making such a change, API
documentation in virt/kvm/api.rst needs updating.

>  	}
>  	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

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@virtuozzo.com> 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@virtuozzo.com>
> > ---
> >  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
> 

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

[Sean Christopherson]

On Wed, Apr 28, 2021, Valeriy Vdovin wrote:
> On Wed, Apr 28, 2021 at 02:38:57PM +0200, Vitaly Kuznetsov wrote:
> > Valeriy Vdovin <valeriy.vdovin@virtuozzo.com> 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@virtuozzo.com>
> > > ---
> > >  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.

IMO, the current code is correct (well, least awful), albeit misleading.
Copying back the number of entries but not the entries themselves would be a bug.
That can obviously be remedied, but it adds even more complexity for no known
benefit, and training userspace to go spelunking on -E2BIG would likely yield
more bugs in the future.

I also think we should keep the -E2BIG behavior of KVM_GET_CPUID2 and
KVM_GET_{SUPPORTED,EMULATED,SUPPORTED_HV}_CPUID consistent.  Returning partial
information would make KVM_GET_CPUID2 an anomaly.


diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index c96f79c9fff2..c4dbc7c47b17 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -348,20 +348,15 @@ 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;
 }

 /* Mask kvm_cpu_caps for @leaf with the raw CPUID capabilities of this CPU. */