[01/61] KVM: x86: Return -E2BIG when KVM_GET_SUPPORTED_CPUID hits max entries
diff mbox series

Message ID 20200201185218.24473-2-sean.j.christopherson@intel.com
State New
Headers show
Series
  • KVM: x86: Introduce KVM cpu caps
Related show

Commit Message

Sean Christopherson Feb. 1, 2020, 6:51 p.m. UTC
Fix a long-standing bug that causes KVM to return 0 instead of -E2BIG
when userspace's array is insufficiently sized.

Note, while the Fixes: tag is accurate with respect to the immediate
bug, it's likely that similar bugs in KVM_GET_SUPPORTED_CPUID existed
prior to the refactoring, e.g. Qemu contains a workaround for the broken
KVM_GET_SUPPORTED_CPUID behavior that predates the buggy commit by over
two years.  The Qemu workaround is also likely the main reason the bug
has gone unreported for so long.

Qemu hack:
  commit 76ae317f7c16aec6b469604b1764094870a75470
  Author: Mark McLoughlin <markmc@redhat.com>
  Date:   Tue May 19 18:55:21 2009 +0100

    kvm: work around supported cpuid ioctl() brokenness

    KVM_GET_SUPPORTED_CPUID has been known to fail to return -E2BIG
    when it runs out of entries. Detect this by always trying again
    with a bigger table if the ioctl() fills the table.

Fixes: 831bf664e9c1f ("KVM: Refactor and simplify kvm_dev_ioctl_get_supported_cpuid")
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/cpuid.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Vitaly Kuznetsov Feb. 3, 2020, 12:55 p.m. UTC | #1
Sean Christopherson <sean.j.christopherson@intel.com> writes:

> Fix a long-standing bug that causes KVM to return 0 instead of -E2BIG
> when userspace's array is insufficiently sized.
>
> Note, while the Fixes: tag is accurate with respect to the immediate
> bug, it's likely that similar bugs in KVM_GET_SUPPORTED_CPUID existed
> prior to the refactoring, e.g. Qemu contains a workaround for the broken
> KVM_GET_SUPPORTED_CPUID behavior that predates the buggy commit by over
> two years.  The Qemu workaround is also likely the main reason the bug
> has gone unreported for so long.
>
> Qemu hack:
>   commit 76ae317f7c16aec6b469604b1764094870a75470
>   Author: Mark McLoughlin <markmc@redhat.com>
>   Date:   Tue May 19 18:55:21 2009 +0100
>
>     kvm: work around supported cpuid ioctl() brokenness
>
>     KVM_GET_SUPPORTED_CPUID has been known to fail to return -E2BIG
>     when it runs out of entries. Detect this by always trying again
>     with a bigger table if the ioctl() fills the table.
>
> Fixes: 831bf664e9c1f ("KVM: Refactor and simplify kvm_dev_ioctl_get_supported_cpuid")
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/kvm/cpuid.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index b1c469446b07..47ce04762c20 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -908,9 +908,14 @@ int kvm_dev_ioctl_get_cpuid(struct kvm_cpuid2 *cpuid,
>  			goto out_free;
>  
>  		limit = cpuid_entries[nent - 1].eax;
> -		for (func = ent->func + 1; func <= limit && nent < cpuid->nent && r == 0; ++func)
> +		for (func = ent->func + 1; func <= limit && r == 0; ++func) {
> +			if (nent >= cpuid->nent) {
> +				r = -E2BIG;
> +				goto out_free;
> +			}
>  			r = do_cpuid_func(&cpuid_entries[nent], func,
>  				          &nent, cpuid->nent, type);
> +		}
>  
>  		if (r)
>  			goto out_free;

Is fixing a bug a valid reason for breaking buggy userspace? :-)
Personally, I think so. In particular, here the change is both the
return value and the fact that we don't do copy_to_user() anymore so I
think it's possible to meet a userspace which is going to get broken by
the change.

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Sean Christopherson Feb. 3, 2020, 3:59 p.m. UTC | #2
On Mon, Feb 03, 2020 at 01:55:40PM +0100, Vitaly Kuznetsov wrote:
> Sean Christopherson <sean.j.christopherson@intel.com> writes:
> 
> > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > index b1c469446b07..47ce04762c20 100644
> > --- a/arch/x86/kvm/cpuid.c
> > +++ b/arch/x86/kvm/cpuid.c
> > @@ -908,9 +908,14 @@ int kvm_dev_ioctl_get_cpuid(struct kvm_cpuid2 *cpuid,
> >  			goto out_free;
> >  
> >  		limit = cpuid_entries[nent - 1].eax;
> > -		for (func = ent->func + 1; func <= limit && nent < cpuid->nent && r == 0; ++func)
> > +		for (func = ent->func + 1; func <= limit && r == 0; ++func) {
> > +			if (nent >= cpuid->nent) {
> > +				r = -E2BIG;
> > +				goto out_free;
> > +			}
> >  			r = do_cpuid_func(&cpuid_entries[nent], func,
> >  				          &nent, cpuid->nent, type);
> > +		}
> >  
> >  		if (r)
> >  			goto out_free;
> 
> Is fixing a bug a valid reason for breaking buggy userspace? :-)
> Personally, I think so.

Linus usually disagrees :-)

> In particular, here the change is both the
> return value and the fact that we don't do copy_to_user() anymore so I
> think it's possible to meet a userspace which is going to get broken by
> the change.

Ugh, yeah, it would be possible.  Qemu (retries), CrosVM (hardcoded to
256 entries) and Firecracker (doesn't use the ioctl()) are all ok,
hopefully all other VMMs used in production environments follow suit.
Paolo Bonzini Feb. 25, 2020, 2:36 p.m. UTC | #3
On 03/02/20 16:59, Sean Christopherson wrote:
> 
>> In particular, here the change is both the
>> return value and the fact that we don't do copy_to_user() anymore so I
>> think it's possible to meet a userspace which is going to get broken by
>> the change.
> Ugh, yeah, it would be possible.  Qemu (retries), CrosVM (hardcoded to
> 256 entries) and Firecracker (doesn't use the ioctl()) are all ok,
> hopefully all other VMMs used in production environments follow suit.
> 

Also: lkvm and selftests both hardcode the limit to 100.

Both would be broken by this change, but as long as the limit is < 100
now it is fine to change.

Paolo

Patch
diff mbox series

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index b1c469446b07..47ce04762c20 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -908,9 +908,14 @@  int kvm_dev_ioctl_get_cpuid(struct kvm_cpuid2 *cpuid,
 			goto out_free;
 
 		limit = cpuid_entries[nent - 1].eax;
-		for (func = ent->func + 1; func <= limit && nent < cpuid->nent && r == 0; ++func)
+		for (func = ent->func + 1; func <= limit && r == 0; ++func) {
+			if (nent >= cpuid->nent) {
+				r = -E2BIG;
+				goto out_free;
+			}
 			r = do_cpuid_func(&cpuid_entries[nent], func,
 				          &nent, cpuid->nent, type);
+		}
 
 		if (r)
 			goto out_free;