| Message ID | 20210406082642.20115-2-eesposit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| 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, 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. > > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Don't think it needs stable@, but I think it's worthwhile to add: Fixes: 433f4ba19041 ("KVM: x86: fix out-of-bounds write in KVM_GET_EMULATED_CPUID (CVE-2019-19332)") > --- > 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; Deep into nitpick territory... I think it makes sense to set entry.function only after the switch statement, that way it'll be a bit more obvious that the default case doesn't actually consume "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.eax = 0; > + entry.ecx = F(RDPID); > break; > + default: > + goto out; > } Maybe add a comment here to call out that the check is done if and only if there is a valid entry? > + if (array->nent >= array->maxnent) > + return -E2BIG; > + > + memcpy(&array->entries[array->nent++], &entry, sizeof(entry)); > + > +out: > return 0; > } > > -- > 2.30.2 >
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 >
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; + 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; }
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(-)