Messages in this thread | | | 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 | From | Emanuele Giuseppe Esposito <> | Date | Thu, 1 Apr 2021 11:28:33 +0200 |
| |
On 31/03/2021 20:31, Sean Christopherson wrote: > 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.
I will update it with v3, thanks.
> >> 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. You're right, I should have zeroed it. I agree that memsetting and memcopying is not elegant either, but unless I am missing something and it changes the intended behavior, IMHO this avoids duplicate checks and makes it simpler to add a new 'func'.
Emanuele > > 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 >> >
| |