| Message ID | 20210406082642.20115-1-eesposit@redhat.com |
|---|---|
| Headers | show |
| Series | KVM: cpuid: fix KVM_GET_EMULATED_CPUID implementation | expand |
Emanuele Giuseppe Esposito <eesposit@redhat.com> writes: > 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 | 33 ++++++++++++++++----------------- > 1 file changed, 16 insertions(+), 17 deletions(-) > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > index 6bd2f8b830e4..27059ddf9f0a 100644 > --- a/arch/x86/kvm/cpuid.c > +++ b/arch/x86/kvm/cpuid.c > @@ -567,34 +567,33 @@ 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; > + memset(&entry, 0, sizeof(entry)); > + entry.function = func; > > 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; Nitpick: there's no need to set entry.eax = 0 as the whole structure was zeroed. Also, '|=' for flags could be just '='. > + 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; > } Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
On Tue, Apr 06, 2021, Vitaly Kuznetsov wrote: > Emanuele Giuseppe Esposito <eesposit@redhat.com> writes: > > > 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 | 33 ++++++++++++++++----------------- > > 1 file changed, 16 insertions(+), 17 deletions(-) > > > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > > index 6bd2f8b830e4..27059ddf9f0a 100644 > > --- a/arch/x86/kvm/cpuid.c > > +++ b/arch/x86/kvm/cpuid.c > > @@ -567,34 +567,33 @@ 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; > > + memset(&entry, 0, sizeof(entry)); > > + entry.function = func; > > > > 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; > > Nitpick: there's no need to set entry.eax = 0 as the whole structure was > zeroed. Also, '|=' for flags could be just '='. Agreed on dropping "entry.eax = 0". I could go either way on flags; I do like that the "|=" is consistent with do_host_cpuid(). > > + 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; > > } > > Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com> > > -- > Vitaly >
This series aims to clarify the behavior of the KVM_GET_EMULATED_CPUID ioctl, and fix a corner case where -E2BIG is returned when the nent field of struct kvm_cpuid2 is matching the amount of emulated entries that kvm returns. Patch 1 proposes the nent field fix to cpuid.c, patch 2 updates the ioctl documentation accordingly and patches 3 and 4 extend the x86_64/get_cpuid_test.c selftest to check the intended behavior of KVM_GET_EMULATED_CPUID. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> --- v3: - clearer commit message and problem explanation - pre-initialize the stack variable 'entry' in __do_cpuid_func_emulated so that the various eax/ebx/ecx are initialized if not set by func. Emanuele Giuseppe Esposito (4): KVM: x86: Fix a spurious -E2BIG in KVM_GET_EMULATED_CPUID Documentation: KVM: update KVM_GET_EMULATED_CPUID ioctl description selftests: add kvm_get_emulated_cpuid to processor.h selftests: KVM: extend get_cpuid_test to include KVM_GET_EMULATED_CPUID Documentation/virt/kvm/api.rst | 10 +-- arch/x86/kvm/cpuid.c | 33 ++++--- .../selftests/kvm/include/x86_64/processor.h | 1 + .../selftests/kvm/lib/x86_64/processor.c | 33 +++++++ .../selftests/kvm/x86_64/get_cpuid_test.c | 90 ++++++++++++++++++- 5 files changed, 142 insertions(+), 25 deletions(-)