[PATCH v3 0/4] KVM: x86: Partially allow KVM_SET_CPUID{,2} after KVM_RUN for CPU hotplug

[PATCH v3 0/4] KVM: x86: Partially allow KVM_SET_CPUID{,2} after KVM_RUN for CPU hotplug

[Vitaly Kuznetsov]

Changes since v2:
- Tighten the check for CPUID data equality, require CPUID entries for the
update to be supplied in the exact same order as the original data [Paolo]

Recently, KVM made it illegal to change CPUID after KVM_RUN but
unfortunately this change is not fully compatible with existing VMMs.
In particular, QEMU reuses vCPU fds for CPU hotplug after unplug and it
calls KVM_SET_CPUID2. Relax the requirement by implementing an allowing
KVM_SET_CPUID{,2} with the exact same data.

Vitaly Kuznetsov (4):
  KVM: x86: Do runtime CPUID update before updating
    vcpu->arch.cpuid_entries
  KVM: x86: Partially allow KVM_SET_CPUID{,2} after KVM_RUN
  KVM: selftests: Rename 'get_cpuid_test' to 'cpuid_test'
  KVM: selftests: Test KVM_SET_CPUID2 after KVM_RUN

 arch/x86/kvm/cpuid.c                          | 70 ++++++++++++++++---
 arch/x86/kvm/x86.c                            | 19 -----
 tools/testing/selftests/kvm/.gitignore        |  2 +-
 tools/testing/selftests/kvm/Makefile          |  4 +-
 .../selftests/kvm/include/x86_64/processor.h  |  7 ++
 .../selftests/kvm/lib/x86_64/processor.c      | 33 +++++++--
 .../x86_64/{get_cpuid_test.c => cpuid_test.c} | 30 ++++++++
 7 files changed, 129 insertions(+), 36 deletions(-)
 rename tools/testing/selftests/kvm/x86_64/{get_cpuid_test.c => cpuid_test.c} (83%)

-- 
2.34.1

[PATCH v3 1/4] KVM: x86: Do runtime CPUID update before updating vcpu->arch.cpuid_entries

[Vitaly Kuznetsov]

kvm_update_cpuid_runtime() mangles CPUID data coming from userspace
VMM after updating 'vcpu->arch.cpuid_entries', this makes it
impossible to compare an update with what was previously
supplied. Introduce __kvm_update_cpuid_runtime() version which can be
used to tweak the input before it goes to 'vcpu->arch.cpuid_entries'
so the upcoming update check can compare tweaked data.

No functional change intended.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 arch/x86/kvm/cpuid.c | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index c55e57b30e81..812190a707f6 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -145,14 +145,21 @@ static void kvm_update_kvm_cpuid_base(struct kvm_vcpu *vcpu)
 	}
 }
 
-static struct kvm_cpuid_entry2 *kvm_find_kvm_cpuid_features(struct kvm_vcpu *vcpu)
+static struct kvm_cpuid_entry2 *__kvm_find_kvm_cpuid_features(struct kvm_vcpu *vcpu,
+					      struct kvm_cpuid_entry2 *entries, int nent)
 {
 	u32 base = vcpu->arch.kvm_cpuid_base;
 
 	if (!base)
 		return NULL;
 
-	return kvm_find_cpuid_entry(vcpu, base | KVM_CPUID_FEATURES, 0);
+	return cpuid_entry2_find(entries, nent, base | KVM_CPUID_FEATURES, 0);
+}
+
+static struct kvm_cpuid_entry2 *kvm_find_kvm_cpuid_features(struct kvm_vcpu *vcpu)
+{
+	return __kvm_find_kvm_cpuid_features(vcpu, vcpu->arch.cpuid_entries,
+					     vcpu->arch.cpuid_nent);
 }
 
 void kvm_update_pv_runtime(struct kvm_vcpu *vcpu)
@@ -167,11 +174,12 @@ void kvm_update_pv_runtime(struct kvm_vcpu *vcpu)
 		vcpu->arch.pv_cpuid.features = best->eax;
 }
 
-void kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu)
+static void __kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *entries,
+				       int nent)
 {
 	struct kvm_cpuid_entry2 *best;
 
-	best = kvm_find_cpuid_entry(vcpu, 1, 0);
+	best = cpuid_entry2_find(entries, nent, 1, 0);
 	if (best) {
 		/* Update OSXSAVE bit */
 		if (boot_cpu_has(X86_FEATURE_XSAVE))
@@ -182,33 +190,38 @@ void kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu)
 			   vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE);
 	}
 
-	best = kvm_find_cpuid_entry(vcpu, 7, 0);
+	best = cpuid_entry2_find(entries, nent, 7, 0);
 	if (best && boot_cpu_has(X86_FEATURE_PKU) && best->function == 0x7)
 		cpuid_entry_change(best, X86_FEATURE_OSPKE,
 				   kvm_read_cr4_bits(vcpu, X86_CR4_PKE));
 
-	best = kvm_find_cpuid_entry(vcpu, 0xD, 0);
+	best = cpuid_entry2_find(entries, nent, 0xD, 0);
 	if (best)
 		best->ebx = xstate_required_size(vcpu->arch.xcr0, false);
 
-	best = kvm_find_cpuid_entry(vcpu, 0xD, 1);
+	best = cpuid_entry2_find(entries, nent, 0xD, 1);
 	if (best && (cpuid_entry_has(best, X86_FEATURE_XSAVES) ||
 		     cpuid_entry_has(best, X86_FEATURE_XSAVEC)))
 		best->ebx = xstate_required_size(vcpu->arch.xcr0, true);
 
-	best = kvm_find_kvm_cpuid_features(vcpu);
+	best = __kvm_find_kvm_cpuid_features(vcpu, entries, nent);
 	if (kvm_hlt_in_guest(vcpu->kvm) && best &&
 		(best->eax & (1 << KVM_FEATURE_PV_UNHALT)))
 		best->eax &= ~(1 << KVM_FEATURE_PV_UNHALT);
 
 	if (!kvm_check_has_quirk(vcpu->kvm, KVM_X86_QUIRK_MISC_ENABLE_NO_MWAIT)) {
-		best = kvm_find_cpuid_entry(vcpu, 0x1, 0);
+		best = cpuid_entry2_find(entries, nent, 0x1, 0);
 		if (best)
 			cpuid_entry_change(best, X86_FEATURE_MWAIT,
 					   vcpu->arch.ia32_misc_enable_msr &
 					   MSR_IA32_MISC_ENABLE_MWAIT);
 	}
 }
+
+void kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu)
+{
+	__kvm_update_cpuid_runtime(vcpu, vcpu->arch.cpuid_entries, vcpu->arch.cpuid_nent);
+}
 EXPORT_SYMBOL_GPL(kvm_update_cpuid_runtime);
 
 static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
@@ -298,6 +311,8 @@ static int kvm_set_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *e2,
 {
 	int r;
 
+	__kvm_update_cpuid_runtime(vcpu, e2, nent);
+
 	r = kvm_check_cpuid(vcpu, e2, nent);
 	if (r)
 		return r;
@@ -307,7 +322,6 @@ static int kvm_set_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *e2,
 	vcpu->arch.cpuid_nent = nent;
 
 	kvm_update_kvm_cpuid_base(vcpu);
-	kvm_update_cpuid_runtime(vcpu);
 	kvm_vcpu_after_set_cpuid(vcpu);
 
 	return 0;
-- 
2.34.1

[PATCH v3 2/4] KVM: x86: Partially allow KVM_SET_CPUID{,2} after KVM_RUN

[Vitaly Kuznetsov]

Commit feb627e8d6f6 ("KVM: x86: Forbid KVM_SET_CPUID{,2} after KVM_RUN")
forbade changing CPUID altogether but unfortunately this is not fully
compatible with existing VMMs. In particular, QEMU reuses vCPU fds for
CPU hotplug after unplug and it calls KVM_SET_CPUID2. Instead of full ban,
check whether the supplied CPUID data is equal to what was previously set.

Reported-by: Igor Mammedov <imammedo@redhat.com>
Fixes: feb627e8d6f6 ("KVM: x86: Forbid KVM_SET_CPUID{,2} after KVM_RUN")
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 arch/x86/kvm/cpuid.c | 36 ++++++++++++++++++++++++++++++++++++
 arch/x86/kvm/x86.c   | 19 -------------------
 2 files changed, 36 insertions(+), 19 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 812190a707f6..7eb046d907c6 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -119,6 +119,28 @@ static int kvm_check_cpuid(struct kvm_vcpu *vcpu,
 	return fpu_enable_guest_xfd_features(&vcpu->arch.guest_fpu, xfeatures);
 }
 
+/* Check whether the supplied CPUID data is equal to what is already set for the vCPU. */
+static int kvm_cpuid_check_equal(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *e2,
+				 int nent)
+{
+	struct kvm_cpuid_entry2 *orig;
+	int i;
+
+	if (nent != vcpu->arch.cpuid_nent)
+		return -EINVAL;
+
+	for (i = 0; i < nent; i++) {
+		orig = &vcpu->arch.cpuid_entries[i];
+		if (e2[i].function != orig->function ||
+		    e2[i].index != orig->index ||
+		    e2[i].eax != orig->eax || e2[i].ebx != orig->ebx ||
+		    e2[i].ecx != orig->ecx || e2[i].edx != orig->edx)
+			return -EINVAL;
+	}
+
+	return 0;
+}
+
 static void kvm_update_kvm_cpuid_base(struct kvm_vcpu *vcpu)
 {
 	u32 function;
@@ -313,6 +335,20 @@ static int kvm_set_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *e2,
 
 	__kvm_update_cpuid_runtime(vcpu, e2, nent);
 
+	/*
+	 * KVM does not correctly handle changing guest CPUID after KVM_RUN, as
+	 * MAXPHYADDR, GBPAGES support, AMD reserved bit behavior, etc.. aren't
+	 * tracked in kvm_mmu_page_role.  As a result, KVM may miss guest page
+	 * faults due to reusing SPs/SPTEs. In practice no sane VMM mucks with
+	 * the core vCPU model on the fly. It would've been better to forbid any
+	 * KVM_SET_CPUID{,2} calls after KVM_RUN altogether but unfortunately
+	 * some VMMs (e.g. QEMU) reuse vCPU fds for CPU hotplug/unplug and do
+	 * KVM_SET_CPUID{,2} again. To support this legacy behavior, check
+	 * whether the supplied CPUID data is equal to what's already set.
+	 */
+	if (vcpu->arch.last_vmentry_cpu != -1)
+		return kvm_cpuid_check_equal(vcpu, e2, nent);
+
 	r = kvm_check_cpuid(vcpu, e2, nent);
 	if (r)
 		return r;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 76b4803dd3bd..ff1416010728 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5230,17 +5230,6 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 		struct kvm_cpuid __user *cpuid_arg = argp;
 		struct kvm_cpuid cpuid;
 
-		/*
-		 * KVM does not correctly handle changing guest CPUID after KVM_RUN, as
-		 * MAXPHYADDR, GBPAGES support, AMD reserved bit behavior, etc.. aren't
-		 * tracked in kvm_mmu_page_role.  As a result, KVM may miss guest page
-		 * faults due to reusing SPs/SPTEs.  In practice no sane VMM mucks with
-		 * the core vCPU model on the fly, so fail.
-		 */
-		r = -EINVAL;
-		if (vcpu->arch.last_vmentry_cpu != -1)
-			goto out;
-
 		r = -EFAULT;
 		if (copy_from_user(&cpuid, cpuid_arg, sizeof(cpuid)))
 			goto out;
@@ -5251,14 +5240,6 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 		struct kvm_cpuid2 __user *cpuid_arg = argp;
 		struct kvm_cpuid2 cpuid;
 
-		/*
-		 * KVM_SET_CPUID{,2} after KVM_RUN is forbidded, see the comment in
-		 * KVM_SET_CPUID case above.
-		 */
-		r = -EINVAL;
-		if (vcpu->arch.last_vmentry_cpu != -1)
-			goto out;
-
 		r = -EFAULT;
 		if (copy_from_user(&cpuid, cpuid_arg, sizeof(cpuid)))
 			goto out;
-- 
2.34.1

[PATCH v3 3/4] KVM: selftests: Rename 'get_cpuid_test' to 'cpuid_test'

[Vitaly Kuznetsov]

In preparation to reusing the existing 'get_cpuid_test' for testing
"KVM_SET_CPUID{,2} after KVM_RUN" rename it to 'cpuid_test' to avoid
the confusion.

No functional change intended.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 tools/testing/selftests/kvm/.gitignore                        | 2 +-
 tools/testing/selftests/kvm/Makefile                          | 4 ++--
 .../selftests/kvm/x86_64/{get_cpuid_test.c => cpuid_test.c}   | 0
 3 files changed, 3 insertions(+), 3 deletions(-)
 rename tools/testing/selftests/kvm/x86_64/{get_cpuid_test.c => cpuid_test.c} (100%)

diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
index 8c129961accf..20b4c921c9a2 100644
--- a/tools/testing/selftests/kvm/.gitignore
+++ b/tools/testing/selftests/kvm/.gitignore
@@ -8,11 +8,11 @@
 /s390x/memop
 /s390x/resets
 /s390x/sync_regs_test
+/x86_64/cpuid_test
 /x86_64/cr4_cpuid_sync_test
 /x86_64/debug_regs
 /x86_64/evmcs_test
 /x86_64/emulator_error_test
-/x86_64/get_cpuid_test
 /x86_64/get_msr_index_features
 /x86_64/kvm_clock_test
 /x86_64/kvm_pv_test
diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index ee8cf2149824..ec78a8692899 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -43,11 +43,11 @@ LIBKVM_aarch64 = lib/aarch64/processor.c lib/aarch64/ucall.c lib/aarch64/handler
 LIBKVM_s390x = lib/s390x/processor.c lib/s390x/ucall.c lib/s390x/diag318_test_handler.c
 LIBKVM_riscv = lib/riscv/processor.c lib/riscv/ucall.c
 
-TEST_GEN_PROGS_x86_64 = x86_64/cr4_cpuid_sync_test
+TEST_GEN_PROGS_x86_64 = x86_64/cpuid_test
+TEST_GEN_PROGS_x86_64 += x86_64/cr4_cpuid_sync_test
 TEST_GEN_PROGS_x86_64 += x86_64/get_msr_index_features
 TEST_GEN_PROGS_x86_64 += x86_64/evmcs_test
 TEST_GEN_PROGS_x86_64 += x86_64/emulator_error_test
-TEST_GEN_PROGS_x86_64 += x86_64/get_cpuid_test
 TEST_GEN_PROGS_x86_64 += x86_64/hyperv_clock
 TEST_GEN_PROGS_x86_64 += x86_64/hyperv_cpuid
 TEST_GEN_PROGS_x86_64 += x86_64/hyperv_features
diff --git a/tools/testing/selftests/kvm/x86_64/get_cpuid_test.c b/tools/testing/selftests/kvm/x86_64/cpuid_test.c
similarity index 100%
rename from tools/testing/selftests/kvm/x86_64/get_cpuid_test.c
rename to tools/testing/selftests/kvm/x86_64/cpuid_test.c
-- 
2.34.1

[PATCH v3 4/4] KVM: selftests: Test KVM_SET_CPUID2 after KVM_RUN

[Vitaly Kuznetsov]

KVM forbids KVM_SET_CPUID2 after KVM_RUN was performed on a vCPU unless
the supplied CPUID data is equal to what was previously set. Test this.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 .../selftests/kvm/include/x86_64/processor.h  |  7 ++++
 .../selftests/kvm/lib/x86_64/processor.c      | 33 ++++++++++++++++---
 .../testing/selftests/kvm/x86_64/cpuid_test.c | 30 +++++++++++++++++
 3 files changed, 66 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
index e94ba0fc67d8..bb013d101c14 100644
--- a/tools/testing/selftests/kvm/include/x86_64/processor.h
+++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
@@ -375,6 +375,8 @@ uint64_t kvm_get_feature_msr(uint64_t msr_index);
 struct kvm_cpuid2 *kvm_get_supported_cpuid(void);
 
 struct kvm_cpuid2 *vcpu_get_cpuid(struct kvm_vm *vm, uint32_t vcpuid);
+int __vcpu_set_cpuid(struct kvm_vm *vm, uint32_t vcpuid,
+		     struct kvm_cpuid2 *cpuid);
 void vcpu_set_cpuid(struct kvm_vm *vm, uint32_t vcpuid,
 		    struct kvm_cpuid2 *cpuid);
 
@@ -418,6 +420,11 @@ uint64_t vm_get_page_table_entry(struct kvm_vm *vm, int vcpuid, uint64_t vaddr);
 void vm_set_page_table_entry(struct kvm_vm *vm, int vcpuid, uint64_t vaddr,
 			     uint64_t pte);
 
+/*
+ * get_cpuid() - find matching CPUID entry and return pointer to it.
+ */
+struct kvm_cpuid_entry2 *get_cpuid(struct kvm_cpuid2 *cpuid, uint32_t function,
+				   uint32_t index);
 /*
  * set_cpuid() - overwrites a matching cpuid entry with the provided value.
  *		 matches based on ent->function && ent->index. returns true
diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
index babb0f28575c..d61e2326dc85 100644
--- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
+++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
@@ -886,6 +886,17 @@ kvm_get_supported_cpuid_index(uint32_t function, uint32_t index)
 	return entry;
 }
 
+
+int __vcpu_set_cpuid(struct kvm_vm *vm, uint32_t vcpuid,
+		     struct kvm_cpuid2 *cpuid)
+{
+	struct vcpu *vcpu = vcpu_find(vm, vcpuid);
+
+	TEST_ASSERT(vcpu != NULL, "vcpu not found, vcpuid: %u", vcpuid);
+
+	return ioctl(vcpu->fd, KVM_SET_CPUID2, cpuid);
+}
+
 /*
  * VM VCPU CPUID Set
  *
@@ -903,12 +914,9 @@ kvm_get_supported_cpuid_index(uint32_t function, uint32_t index)
 void vcpu_set_cpuid(struct kvm_vm *vm,
 		uint32_t vcpuid, struct kvm_cpuid2 *cpuid)
 {
-	struct vcpu *vcpu = vcpu_find(vm, vcpuid);
 	int rc;
 
-	TEST_ASSERT(vcpu != NULL, "vcpu not found, vcpuid: %u", vcpuid);
-
-	rc = ioctl(vcpu->fd, KVM_SET_CPUID2, cpuid);
+	rc = __vcpu_set_cpuid(vm, vcpuid, cpuid);
 	TEST_ASSERT(rc == 0, "KVM_SET_CPUID2 failed, rc: %i errno: %i",
 		    rc, errno);
 
@@ -1384,6 +1392,23 @@ void assert_on_unhandled_exception(struct kvm_vm *vm, uint32_t vcpuid)
 	}
 }
 
+struct kvm_cpuid_entry2 *get_cpuid(struct kvm_cpuid2 *cpuid, uint32_t function,
+				   uint32_t index)
+{
+	int i;
+
+	for (i = 0; i < cpuid->nent; i++) {
+		struct kvm_cpuid_entry2 *cur = &cpuid->entries[i];
+
+		if (cur->function == function && cur->index == index)
+			return cur;
+	}
+
+	TEST_FAIL("CPUID function 0x%x index 0x%x not found ", function, index);
+
+	return NULL;
+}
+
 bool set_cpuid(struct kvm_cpuid2 *cpuid,
 	       struct kvm_cpuid_entry2 *ent)
 {
diff --git a/tools/testing/selftests/kvm/x86_64/cpuid_test.c b/tools/testing/selftests/kvm/x86_64/cpuid_test.c
index a711f83749ea..16d2465c5634 100644
--- a/tools/testing/selftests/kvm/x86_64/cpuid_test.c
+++ b/tools/testing/selftests/kvm/x86_64/cpuid_test.c
@@ -154,6 +154,34 @@ struct kvm_cpuid2 *vcpu_alloc_cpuid(struct kvm_vm *vm, vm_vaddr_t *p_gva, struct
 	return guest_cpuids;
 }
 
+static void set_cpuid_after_run(struct kvm_vm *vm, struct kvm_cpuid2 *cpuid)
+{
+	struct kvm_cpuid_entry2 *ent;
+	int rc;
+	u32 eax, ebx, x;
+
+	/* Setting unmodified CPUID is allowed */
+	rc = __vcpu_set_cpuid(vm, VCPU_ID, cpuid);
+	TEST_ASSERT(!rc, "Setting unmodified CPUID after KVM_RUN failed: %d", rc);
+
+	/* Changing CPU features is forbidden */
+	ent = get_cpuid(cpuid, 0x7, 0);
+	ebx = ent->ebx;
+	ent->ebx--;
+	rc = __vcpu_set_cpuid(vm, VCPU_ID, cpuid);
+	TEST_ASSERT(rc, "Changing CPU features should fail");
+	ent->ebx = ebx;
+
+	/* Changing MAXPHYADDR is forbidden */
+	ent = get_cpuid(cpuid, 0x80000008, 0);
+	eax = ent->eax;
+	x = eax & 0xff;
+	ent->eax = (eax & ~0xffu) | (x - 1);
+	rc = __vcpu_set_cpuid(vm, VCPU_ID, cpuid);
+	TEST_ASSERT(rc, "Changing MAXPHYADDR should fail");
+	ent->eax = eax;
+}
+
 int main(void)
 {
 	struct kvm_cpuid2 *supp_cpuid, *cpuid2;
@@ -175,5 +203,7 @@ int main(void)
 	for (stage = 0; stage < 3; stage++)
 		run_vcpu(vm, VCPU_ID, stage);
 
+	set_cpuid_after_run(vm, cpuid2);
+
 	kvm_vm_free(vm);
 }
-- 
2.34.1

Re: [PATCH v3 2/4] KVM: x86: Partially allow KVM_SET_CPUID{,2} after KVM_RUN

[Sean Christopherson]

On Tue, Jan 18, 2022, Vitaly Kuznetsov wrote:
> +/* Check whether the supplied CPUID data is equal to what is already set for the vCPU. */
> +static int kvm_cpuid_check_equal(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *e2,
> +				 int nent)
> +{
> +	struct kvm_cpuid_entry2 *orig;
> +	int i;
> +
> +	if (nent != vcpu->arch.cpuid_nent)
> +		return -EINVAL;
> +
> +	for (i = 0; i < nent; i++) {
> +		orig = &vcpu->arch.cpuid_entries[i];
> +		if (e2[i].function != orig->function ||
> +		    e2[i].index != orig->index ||
> +		    e2[i].eax != orig->eax || e2[i].ebx != orig->ebx ||
> +		    e2[i].ecx != orig->ecx || e2[i].edx != orig->edx)
> +			return -EINVAL;

This needs to check .flags for the above check on .index to be meaningful, and at
that point, can't we be even more agressive and just do?

	if (memcmp(e2, vcpu->arch.cpuid_entries, nent * sizeof(e2)))
		return -EINVAL;

	return 0;

> +	}
> +
> +	return 0;
> +}
> +
>  static void kvm_update_kvm_cpuid_base(struct kvm_vcpu *vcpu)
>  {
>  	u32 function;
> @@ -313,6 +335,20 @@ static int kvm_set_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *e2,
>  
>  	__kvm_update_cpuid_runtime(vcpu, e2, nent);
> +	/*
> +	 * KVM does not correctly handle changing guest CPUID after KVM_RUN, as
> +	 * MAXPHYADDR, GBPAGES support, AMD reserved bit behavior, etc.. aren't
> +	 * tracked in kvm_mmu_page_role.  As a result, KVM may miss guest page
> +	 * faults due to reusing SPs/SPTEs. In practice no sane VMM mucks with
> +	 * the core vCPU model on the fly. It would've been better to forbid any
> +	 * KVM_SET_CPUID{,2} calls after KVM_RUN altogether but unfortunately
> +	 * some VMMs (e.g. QEMU) reuse vCPU fds for CPU hotplug/unplug and do
> +	 * KVM_SET_CPUID{,2} again. To support this legacy behavior, check
> +	 * whether the supplied CPUID data is equal to what's already set.

This is misleading/wrong.  KVM_RUN isn't the only problematic ioctl(), it's just
the one that we decided to use to detect that userspace is being stupid.  And
forbidding KVM_SET_CPUID after KVM_RUN (or even all problematic ioctls()) wouldn't
solve problem as providing different CPUID configurations for vCPUs in a VM will
also cause the MMU to fall on its face.

> +	if (vcpu->arch.last_vmentry_cpu != -1)
> +		return kvm_cpuid_check_equal(vcpu, e2, nent);

And technically, checking last_vmentry_cpu doesn't forbid changing CPUID after
KVM_RUN, it forbids changing CPUID after successfully entering the guest (or
emulating instructions on VMX).

I realize I'm being very pedantic, as a well-intended userspace is obviously not
going to change CPUID after -EINTR or whatever.  But I do want to highlight that
this approach is by no means bulletproof, and that what is/isn't allowed with
respect to guest CPUID isn't necessarily associated with what is/isn't "safe".
In other words, this check doesn't guarantee that userspace can't misuse KVM_SET_CPUID,
and on the flip side it disallows using KVM_SET_CPUID in ways that are perfectly ok
(if userspace is careful and deliberate).

Re: [PATCH v3 2/4] KVM: x86: Partially allow KVM_SET_CPUID{,2} after KVM_RUN

[Vitaly Kuznetsov]

Sean Christopherson <seanjc@google.com> writes:

> On Tue, Jan 18, 2022, Vitaly Kuznetsov wrote:
>> +/* Check whether the supplied CPUID data is equal to what is already set for the vCPU. */
>> +static int kvm_cpuid_check_equal(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *e2,
>> +				 int nent)
>> +{
>> +	struct kvm_cpuid_entry2 *orig;
>> +	int i;
>> +
>> +	if (nent != vcpu->arch.cpuid_nent)
>> +		return -EINVAL;
>> +
>> +	for (i = 0; i < nent; i++) {
>> +		orig = &vcpu->arch.cpuid_entries[i];
>> +		if (e2[i].function != orig->function ||
>> +		    e2[i].index != orig->index ||
>> +		    e2[i].eax != orig->eax || e2[i].ebx != orig->ebx ||
>> +		    e2[i].ecx != orig->ecx || e2[i].edx != orig->edx)
>> +			return -EINVAL;
>
> This needs to check .flags for the above check on .index to be meaningful, and at
> that point, can't we be even more agressive and just do?
>
> 	if (memcmp(e2, vcpu->arch.cpuid_entries, nent * sizeof(e2)))
> 		return -EINVAL;
>
> 	return 0;
>

Sure, looks good to me.

>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  static void kvm_update_kvm_cpuid_base(struct kvm_vcpu *vcpu)
>>  {
>>  	u32 function;
>> @@ -313,6 +335,20 @@ static int kvm_set_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *e2,
>>  
>>  	__kvm_update_cpuid_runtime(vcpu, e2, nent);
>> +	/*
>> +	 * KVM does not correctly handle changing guest CPUID after KVM_RUN, as
>> +	 * MAXPHYADDR, GBPAGES support, AMD reserved bit behavior, etc.. aren't
>> +	 * tracked in kvm_mmu_page_role.  As a result, KVM may miss guest page
>> +	 * faults due to reusing SPs/SPTEs. In practice no sane VMM mucks with
>> +	 * the core vCPU model on the fly. It would've been better to forbid any
>> +	 * KVM_SET_CPUID{,2} calls after KVM_RUN altogether but unfortunately
>> +	 * some VMMs (e.g. QEMU) reuse vCPU fds for CPU hotplug/unplug and do
>> +	 * KVM_SET_CPUID{,2} again. To support this legacy behavior, check
>> +	 * whether the supplied CPUID data is equal to what's already set.
>
> This is misleading/wrong. KVM_RUN isn't the only problematic ioctl(),

Well, it wasn't me who wrote the comment about KVM_RUN :-) My addition
can be improved of course.

> it's just the one that we decided to use to detect that userspace is
> being stupid.  And forbidding KVM_SET_CPUID after KVM_RUN (or even all
> problematic ioctls()) wouldn't solve problem as providing different
> CPUID configurations for vCPUs in a VM will also cause the MMU to fall
> on its face.

True, but how do we move forward? We can either let userspace do stupid
things and (potentially) create hard-to-debug problems or we try to
cover at least some use-cases with checks (like the one we introduce
here).

Different CPUID configurations for different vCPUs is actually an
interesting case. It makes me (again) think about the
allowlist/blocklist approaches: we can easily enhance the
'vcpu->arch.last_vmentry_cpu != -1' check below and start requiring
CPUIDs to [almost] match. The question then is how to change CPUID for a
multi-vCPU guest as it will become effectively forbidden. BTW, is there
a good use-case for changing CPUIDs besides testing purposes?

>
>> +	if (vcpu->arch.last_vmentry_cpu != -1)
>> +		return kvm_cpuid_check_equal(vcpu, e2, nent);
>
> And technically, checking last_vmentry_cpu doesn't forbid changing CPUID after
> KVM_RUN, it forbids changing CPUID after successfully entering the guest (or
> emulating instructions on VMX).
>
> I realize I'm being very pedantic, as a well-intended userspace is obviously not
> going to change CPUID after -EINTR or whatever.  But I do want to highlight that
> this approach is by no means bulletproof, and that what is/isn't allowed with
> respect to guest CPUID isn't necessarily associated with what is/isn't "safe".
> In other words, this check doesn't guarantee that userspace can't misuse KVM_SET_CPUID,
> and on the flip side it disallows using KVM_SET_CPUID in ways that are perfectly ok
> (if userspace is careful and deliberate).

All true but I don't see a 'bulletproof' approach here unless we start
designing new KVM API for userspace and I don't think the problem here
is a good enough justification for that. Another approach would be to
name the "don't change CPUIDs after KVM_RUN at will" comment in the code
a good enough sentinel and hope that no real world userspace actually
does such things.

-- 
Vitaly

Re: [PATCH v3 2/4] KVM: x86: Partially allow KVM_SET_CPUID{,2} after KVM_RUN

[Sean Christopherson]

On Wed, Jan 19, 2022, Vitaly Kuznetsov wrote:
> Sean Christopherson <seanjc@google.com> writes:
> >> @@ -313,6 +335,20 @@ static int kvm_set_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *e2,
> >>  
> >>  	__kvm_update_cpuid_runtime(vcpu, e2, nent);
> >> +	/*
> >> +	 * KVM does not correctly handle changing guest CPUID after KVM_RUN, as
> >> +	 * MAXPHYADDR, GBPAGES support, AMD reserved bit behavior, etc.. aren't
> >> +	 * tracked in kvm_mmu_page_role.  As a result, KVM may miss guest page
> >> +	 * faults due to reusing SPs/SPTEs. In practice no sane VMM mucks with
> >> +	 * the core vCPU model on the fly. It would've been better to forbid any
> >> +	 * KVM_SET_CPUID{,2} calls after KVM_RUN altogether but unfortunately
> >> +	 * some VMMs (e.g. QEMU) reuse vCPU fds for CPU hotplug/unplug and do
> >> +	 * KVM_SET_CPUID{,2} again. To support this legacy behavior, check
> >> +	 * whether the supplied CPUID data is equal to what's already set.
> >
> > This is misleading/wrong. KVM_RUN isn't the only problematic ioctl(),
> 
> Well, it wasn't me who wrote the comment about KVM_RUN :-) My addition
> can be improved of course.

Don't^W^W^W shoot the messenger?  :-)

> > it's just the one that we decided to use to detect that userspace is
> > being stupid.  And forbidding KVM_SET_CPUID after KVM_RUN (or even all
> > problematic ioctls()) wouldn't solve problem as providing different
> > CPUID configurations for vCPUs in a VM will also cause the MMU to fall
> > on its face.
> 
> True, but how do we move forward? We can either let userspace do stupid
> things and (potentially) create hard-to-debug problems or we try to
> cover at least some use-cases with checks (like the one we introduce
> here).

I completely agree, and if this were an internal API or a KVM module param I
would be jumping all over the idea of restricing how it can be used.  What I don't
like is bolting on restrictions to a set of ioctl()s that have been in use for years.

> Different CPUID configurations for different vCPUs is actually an
> interesting case. It makes me (again) think about the
> allowlist/blocklist approaches: we can easily enhance the
> 'vcpu->arch.last_vmentry_cpu != -1' check below and start requiring
> CPUIDs to [almost] match. The question then is how to change CPUID for a
> multi-vCPU guest as it will become effectively forbidden. BTW, is there
> a good use-case for changing CPUIDs besides testing purposes?

No idea.  That's a big reason for my concern; we've really only got input from
QEMU, and there are plenty of users beyond QEMU.

> >> +	if (vcpu->arch.last_vmentry_cpu != -1)
> >> +		return kvm_cpuid_check_equal(vcpu, e2, nent);
> >
> > And technically, checking last_vmentry_cpu doesn't forbid changing CPUID after
> > KVM_RUN, it forbids changing CPUID after successfully entering the guest (or
> > emulating instructions on VMX).
> >
> > I realize I'm being very pedantic, as a well-intended userspace is obviously not
> > going to change CPUID after -EINTR or whatever.  But I do want to highlight that
> > this approach is by no means bulletproof, and that what is/isn't allowed with
> > respect to guest CPUID isn't necessarily associated with what is/isn't "safe".
> > In other words, this check doesn't guarantee that userspace can't misuse KVM_SET_CPUID,
> > and on the flip side it disallows using KVM_SET_CPUID in ways that are perfectly ok
> > (if userspace is careful and deliberate).
> 
> All true but I don't see a 'bulletproof' approach here unless we start
> designing new KVM API for userspace and I don't think the problem here
> is a good enough justification for that.

Yeah, agreed.

> Another approach would be to name the "don't change CPUIDs after KVM_RUN at
> will" comment in the code a good enough sentinel and hope that no real world
> userspace actually does such things.

I'm ok with that, so long as the KVM code is kept simple (a single memcmp() 
qualifies) and we are quick to revert the whole thing if it turns out there's an
existing user and/or valid use case.