| Message ID | 20200201185218.24473-2-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: Introduce KVM cpu caps | expand |
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>
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.
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
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;
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(-)