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 <[email protected]>
---
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(-)
--
2.30.2
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 <[email protected]>
---
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;
+ 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;
}
--
2.30.2
As the similar kvm_get_supported_cpuid(),
kvm_get_emulated_cpuid allocates and gets
a struct kvm_cpuid2 filled with emulated features.
Signed-off-by: Emanuele Giuseppe Esposito <[email protected]>
---
.../selftests/kvm/include/x86_64/processor.h | 1 +
.../selftests/kvm/lib/x86_64/processor.c | 33 +++++++++++++++++++
2 files changed, 34 insertions(+)
diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
index 0b30b4e15c38..ae1b9530e187 100644
--- a/tools/testing/selftests/kvm/include/x86_64/processor.h
+++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
@@ -353,6 +353,7 @@ void vcpu_load_state(struct kvm_vm *vm, uint32_t vcpuid,
struct kvm_msr_list *kvm_get_msr_index_list(void);
uint64_t kvm_get_feature_msr(uint64_t msr_index);
struct kvm_cpuid2 *kvm_get_supported_cpuid(void);
+struct kvm_cpuid2 *kvm_get_emulated_cpuid(void);
struct kvm_cpuid2 *vcpu_get_cpuid(struct kvm_vm *vm, uint32_t vcpuid);
void vcpu_set_cpuid(struct kvm_vm *vm, uint32_t vcpuid,
diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
index e676fe40bfe6..2ea14421bdfe 100644
--- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
+++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
@@ -669,6 +669,39 @@ struct kvm_cpuid2 *kvm_get_supported_cpuid(void)
return cpuid;
}
+/*
+ * KVM Emulated CPUID Get
+ *
+ * Input Args: None
+ *
+ * Output Args:
+ *
+ * Return: The emulated KVM CPUID
+ *
+ * Get the guest CPUID emulated by KVM.
+ */
+struct kvm_cpuid2 *kvm_get_emulated_cpuid(void)
+{
+ static struct kvm_cpuid2 *cpuid;
+ int ret;
+ int kvm_fd;
+
+ if (cpuid)
+ return cpuid;
+
+ cpuid = allocate_kvm_cpuid2();
+ kvm_fd = open(KVM_DEV_PATH, O_RDONLY);
+ if (kvm_fd < 0)
+ exit(KSFT_SKIP);
+
+ ret = ioctl(kvm_fd, KVM_GET_EMULATED_CPUID, cpuid);
+ TEST_ASSERT(ret == 0, "KVM_GET_EMULATED_CPUID failed %d %d\n",
+ ret, errno);
+
+ close(kvm_fd);
+ return cpuid;
+}
+
/*
* KVM Get MSR
*
--
2.30.2
Extend the get_cpuid_test.c selftest to include the KVM_GET_EMULATED_CPUID
ioctl. Since the behavior and functionality is similar to
KVM_GET_SUPPORTED_CPUID, we only check additionally:
1) checks for corner case in the nent field of the struct kvm_cpuid2.
2) sets and gets it as cpuid from the guest VM
Signed-off-by: Emanuele Giuseppe Esposito <[email protected]>
---
.../selftests/kvm/x86_64/get_cpuid_test.c | 90 ++++++++++++++++++-
1 file changed, 88 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/kvm/x86_64/get_cpuid_test.c b/tools/testing/selftests/kvm/x86_64/get_cpuid_test.c
index 9b78e8889638..b9f0fba1b0ea 100644
--- a/tools/testing/selftests/kvm/x86_64/get_cpuid_test.c
+++ b/tools/testing/selftests/kvm/x86_64/get_cpuid_test.c
@@ -13,6 +13,7 @@
#include "processor.h"
#define VCPU_ID 0
+#define MAX_NENT 1000
/* CPUIDs known to differ */
struct {
@@ -137,7 +138,8 @@ static void run_vcpu(struct kvm_vm *vm, uint32_t vcpuid, int stage)
}
}
-struct kvm_cpuid2 *vcpu_alloc_cpuid(struct kvm_vm *vm, vm_vaddr_t *p_gva, struct kvm_cpuid2 *cpuid)
+static struct kvm_cpuid2 *vcpu_alloc_cpuid(struct kvm_vm *vm, vm_vaddr_t *p_gva,
+ struct kvm_cpuid2 *cpuid)
{
int size = sizeof(*cpuid) + cpuid->nent * sizeof(cpuid->entries[0]);
vm_vaddr_t gva = vm_vaddr_alloc(vm, size,
@@ -150,9 +152,84 @@ struct kvm_cpuid2 *vcpu_alloc_cpuid(struct kvm_vm *vm, vm_vaddr_t *p_gva, struct
return guest_cpuids;
}
+static struct kvm_cpuid2 *alloc_custom_kvm_cpuid2(int nent)
+{
+ struct kvm_cpuid2 *cpuid;
+ size_t size;
+
+ size = sizeof(*cpuid);
+ size += nent * sizeof(struct kvm_cpuid_entry2);
+ cpuid = calloc(1, size);
+ if (!cpuid) {
+ perror("malloc");
+ abort();
+ }
+
+ cpuid->nent = nent;
+
+ return cpuid;
+}
+
+static void clean_entries_kvm_cpuid2(struct kvm_cpuid2 *cpuid)
+{
+ size_t size;
+ int old_nent = cpuid->nent;
+
+ size = sizeof(*cpuid);
+ size += MAX_NENT * sizeof(struct kvm_cpuid_entry2);
+ memset(cpuid, 0, size);
+ cpuid->nent = old_nent;
+}
+
+static void test_emulated_entries(struct kvm_vm *vm)
+{
+ int res, right_nent;
+ struct kvm_cpuid2 *cpuid;
+
+ cpuid = alloc_custom_kvm_cpuid2(MAX_NENT);
+
+ /* 0 nent, return E2BIG */
+ cpuid->nent = 0;
+ res = _kvm_ioctl(vm, KVM_GET_EMULATED_CPUID, cpuid);
+ TEST_ASSERT(res == -1 && errno == E2BIG, "nent=0 should fail as E2BIG");
+ clean_entries_kvm_cpuid2(cpuid);
+
+ /* high nent, set the entries and adjust */
+ cpuid->nent = MAX_NENT;
+ res = _kvm_ioctl(vm, KVM_GET_EMULATED_CPUID, cpuid);
+ TEST_ASSERT(res == 0, "nent > actual nent should not fail");
+ right_nent = cpuid->nent;
+ clean_entries_kvm_cpuid2(cpuid);
+
+ /* high nent, set the entries and adjust */
+ cpuid->nent++;
+ res = _kvm_ioctl(vm, KVM_GET_EMULATED_CPUID, cpuid);
+ TEST_ASSERT(res == 0, "nent > actual nent should not fail");
+ TEST_ASSERT(right_nent == cpuid->nent, "nent should be always the same");
+ clean_entries_kvm_cpuid2(cpuid);
+
+ /* low nent, return E2BIG */
+ if (right_nent > 1) {
+ cpuid->nent = 1;
+ res = _kvm_ioctl(vm, KVM_GET_EMULATED_CPUID, cpuid);
+ TEST_ASSERT(res == -1 && errno == E2BIG, "nent=1 should fail");
+ clean_entries_kvm_cpuid2(cpuid);
+ }
+
+ /* exact nent */
+ cpuid->nent = right_nent;
+ res = _kvm_ioctl(vm, KVM_GET_EMULATED_CPUID, cpuid);
+ TEST_ASSERT(res == 0, "nent == actual nent should not fail");
+ TEST_ASSERT(cpuid->nent == right_nent,
+ "KVM_GET_EMULATED_CPUID should be invaried when nent is exact");
+ clean_entries_kvm_cpuid2(cpuid);
+
+ free(cpuid);
+}
+
int main(void)
{
- struct kvm_cpuid2 *supp_cpuid, *cpuid2;
+ struct kvm_cpuid2 *supp_cpuid, *emul_cpuid, *cpuid2;
vm_vaddr_t cpuid_gva;
struct kvm_vm *vm;
int stage;
@@ -171,5 +248,14 @@ int main(void)
for (stage = 0; stage < 3; stage++)
run_vcpu(vm, VCPU_ID, stage);
+ if (kvm_check_cap(KVM_CAP_EXT_EMUL_CPUID)) {
+ emul_cpuid = kvm_get_emulated_cpuid();
+ vcpu_set_cpuid(vm, VCPU_ID, emul_cpuid);
+ cpuid2 = vcpu_get_cpuid(vm, VCPU_ID);
+
+ test_emulated_entries(vm);
+ compare_cpuids(emul_cpuid, cpuid2);
+ }
+
kvm_vm_free(vm);
}
--
2.30.2
Emanuele Giuseppe Esposito <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
--
Vitaly
On Tue, Apr 06, 2021, Vitaly Kuznetsov wrote:
> Emanuele Giuseppe Esposito <[email protected]> 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 <[email protected]>
> > ---
> > 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 <[email protected]>
>
> --
> Vitaly
>
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 <[email protected]>
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
>