Messages in this thread Patch in this message | | | Date | Wed, 31 Mar 2021 18:31:10 +0000 | From | Sean Christopherson <> | Subject | Re: [PATCH v2 1/4] kvm: cpuid: adjust the returned nent field of kvm_cpuid2 for KVM_GET_SUPPORTED_CPUID and KVM_GET_EMULATED_CPUID |
| |
On Wed, Mar 31, 2021, Emanuele Giuseppe Esposito wrote: > Calling the kvm KVM_GET_[SUPPORTED/EMULATED]_CPUID ioctl requires > a nent field inside the kvm_cpuid2 struct to be big enough to contain > all entries that will be set by kvm. > Therefore if the nent field is too high, kvm will adjust it to the > right value. If too low, -E2BIG is returned. > > However, when filling the entries do_cpuid_func() requires an > additional entry, so if the right nent is known in advance, > giving the exact number of entries won't work because it has to be > increased by one.
I'd strong prefer to reword the shortlog and changelog. It's not immediately obvious what this is changing without the context from the v1 thread. E.g.
KVM: x86: Fix a spurious -E2BIG in KVM_GET_EMULATED_CPUID
When retrieving emulated CPUID entries, check for an insufficient array size if and only if KVM is actually inserting an entry. If userspace has a priori knowledge of the exact array size, KVM_GET_EMULATED_CPUID will incorrectly fail due to effectively requiring an extra, unused entry.
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> > --- > arch/x86/kvm/cpuid.c | 35 ++++++++++++++++++----------------- > 1 file changed, 18 insertions(+), 17 deletions(-) > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > index 6bd2f8b830e4..02a51f921548 100644 > --- a/arch/x86/kvm/cpuid.c > +++ b/arch/x86/kvm/cpuid.c > @@ -567,34 +567,34 @@ static struct kvm_cpuid_entry2 *do_host_cpuid(struct kvm_cpuid_array *array, > > static int __do_cpuid_func_emulated(struct kvm_cpuid_array *array, u32 func) > { > - struct kvm_cpuid_entry2 *entry; > - > - if (array->nent >= array->maxnent) > - return -E2BIG; > + struct kvm_cpuid_entry2 entry; > > - entry = &array->entries[array->nent]; > - entry->function = func; > - entry->index = 0; > - entry->flags = 0; > + entry.function = func; > + entry.index = 0; > + entry.flags = 0;
Depending on the leaf, eax/ebx/ecx/edx will be left uninitialized. This wasn't a bug before since @array is zeroed on allocation.
What about pre-checking @func? I don't particular like the duplicate checks, but none of the solutions are particularly elegant. E.g.
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index 6bd2f8b830e4..9824947bd5ad 100644 --- a/arch/x86/kvm/cpuid.c +++ b/arch/x86/kvm/cpuid.c @@ -565,14 +565,18 @@ static struct kvm_cpuid_entry2 *do_host_cpuid(struct kvm_cpuid_array *array, return entry; }
-static int __do_cpuid_func_emulated(struct kvm_cpuid_array *array, u32 func) +static noinline int __do_cpuid_func_emulated(struct kvm_cpuid_array *array, u32 func) { struct kvm_cpuid_entry2 *entry;
+ if (func != 0 && func != 1 && func != 7) + return 0; + if (array->nent >= array->maxnent) return -E2BIG;
- entry = &array->entries[array->nent]; + entry = &array->entries[array->nent++]; + entry->function = func; entry->index = 0; entry->flags = 0; @@ -580,19 +584,17 @@ static int __do_cpuid_func_emulated(struct kvm_cpuid_array *array, u32 func) switch (func) { case 0: entry->eax = 7; - ++array->nent; break; case 1: entry->ecx = F(MOVBE); - ++array->nent; break; case 7: entry->flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX; entry->eax = 0; entry->ecx = F(RDPID); - ++array->nent; - default: break; + default: + BUG(); }
return 0
> > switch (func) { > case 0: > - entry->eax = 7; > - ++array->nent; > + entry.eax = 7; > break; > case 1: > - entry->ecx = F(MOVBE); > - ++array->nent; > + entry.ecx = F(MOVBE); > break; > case 7: > - entry->flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX; > - entry->eax = 0; > - entry->ecx = F(RDPID); > - ++array->nent; > - default: > + entry.flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX; > + entry.eax = 0; > + entry.ecx = F(RDPID); > break; > + default: > + goto out; > } > > + if (array->nent >= array->maxnent) > + return -E2BIG; > + > + memcpy(&array->entries[array->nent++], &entry, sizeof(entry)); > + > +out: > return 0; > } > > @@ -975,6 +975,7 @@ int kvm_dev_ioctl_get_cpuid(struct kvm_cpuid2 *cpuid, > > if (cpuid->nent < 1) > return -E2BIG; > + > if (cpuid->nent > KVM_MAX_CPUID_ENTRIES) > cpuid->nent = KVM_MAX_CPUID_ENTRIES; > > -- > 2.30.2 >
| |