Messages in this thread | | | Subject | Re: [PATCH v4 1/4] KVM: x86: Fix a spurious -E2BIG in KVM_GET_EMULATED_CPUID | From | Emanuele Giuseppe Esposito <> | Date | Fri, 9 Apr 2021 14:34:35 +0200 |
| |
On 08/04/2021 22:29, Sean Christopherson wrote: > On Thu, Apr 08, 2021, Emanuele Giuseppe Esposito wrote: >> 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. >> >> Fixes: 433f4ba19041 ("KVM: x86: fix out-of-bounds write in KVM_GET_EMULATED_CPUID (CVE-2019-19332)") >> 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..d30194081892 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)); >> >> 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.ecx = F(RDPID); >> break; >> + default: >> + goto out; >> } >> >> + /* This check is performed only when func is valid */ > > Sorry to keep nitpicking and bikeshedding.
No problem at all. Any comment is very welcome :)
Funcs aren't really "invalid", KVM > just doesn't have any features it emulates in other leafs. Maybe be more literal > in describing what triggers the check? > > /* Check the array capacity iff the entry is being copied over. */
What I mean here is that a func is "valid" if it matches one of the cases of the switch statement. If it is not valid, it ends up in the default case. But I agree, will change the comment your suggestion and resend.
Thank you, Emanuele
> > Not a sticking point, so either way: > > Reviewed-by: Sean Christopherson <seanjc@google.com> > >> + if (array->nent >= array->maxnent) >> + return -E2BIG; >> + >> + entry.function = func; >> + memcpy(&array->entries[array->nent++], &entry, sizeof(entry)); >> + >> +out: >> return 0; >> } >> >> -- >> 2.30.2 >> >
| |