Re: [PATCH v3 1/4] KVM: x86: Fix a spurious -E2BIG in KVM_GET_EMULATED_CPUID

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