[PATCH 0/7] KVM: x86: EFER validity fixes and cleanups

[PATCH 0/7] KVM: x86: EFER validity fixes and cleanups

[Yosry Ahmed]

A couple of bug fixes for EFER validity checks, namely not doing any on
KVM_SET_SREGS*, and incorrectly preserving EFER.SVME and EFER.LMSLE
state after module unload+load.

The rest of the series reworks EFER supported bits and moves them into
kvm_caps, ensuring they are always reinitialized on vendor module init.
The final patch adds test cases to set_sregs that catch the bug fixed by
patch 1.

Yosry Ahmed (7):
  KVM: x86: Check EFER validity on KVM_SET_SREGS*
  KVM: SVM: Disallow EFER.SVME and EFER.LSMLE if nested is disabled
  KVM: x86: Disallow EFER.LME and EFER.LMA if long mode is not supported
  KVM: x86: Add a per-vendor callback to setup EFER caps
  KVM: x86: Reverse the polarity of efer_reserved_bits
  KVM: x86: Move supported EFER bits to kvm_caps
  KVM: selftests: Extend set_sregs test to cover EFER

 arch/x86/include/asm/kvm-x86-ops.h            |  1 +
 arch/x86/include/asm/kvm_host.h               |  3 +
 arch/x86/kvm/msrs.c                           | 21 +----
 arch/x86/kvm/msrs.h                           |  1 +
 arch/x86/kvm/regs.c                           |  3 +-
 arch/x86/kvm/svm/svm.c                        | 14 +++-
 arch/x86/kvm/x86.c                            | 18 ++--
 .../selftests/kvm/include/x86/processor.h     |  2 +
 .../selftests/kvm/x86/set_sregs_test.c        | 83 ++++++++++++++-----
 9 files changed, 96 insertions(+), 50 deletions(-)


base-commit: 50406d35f5635e1cc523e61409d57e851b5f5df8
-- 
2.55.0.rc0.799.gd6f94ed593-goog

[PATCH 1/7] KVM: x86: Check EFER validity on KVM_SET_SREGS*

[Yosry Ahmed]

When handling userspace SREGS writes, check the validity of EFER (i.e.
allowed bits) before writing the new value of EFER through the
per-vendor set_efer callbacks. This prevents userspace from writing
bogus values (e.g. EFER.SVME=1 with nested=0).

Note: on KVM_SET_MSRS, KVM only checks EFER validity in terms of KVM
caps, not guest caps, so it is possible to set EFER bits that are
supported by KVM but not by the guest CPUID. Potentially allowing
userspace to set msrs before CPUID.

However, for KVM_SET_SREGS*, check the validity of the set bits against
both KVM and guest caps. This is consistent with other validity checks
(e.g. for CR4) that check validity against guest caps, which already
imposes the need to set CPUID before SREGS.

Cc: stable@vger.kernel.org
Signed-off-by: Yosry Ahmed <yosry@kernel.org>
---
 arch/x86/kvm/regs.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/regs.c b/arch/x86/kvm/regs.c
index d2caf5a67dba4..94c4e4e41868f 100644
--- a/arch/x86/kvm/regs.c
+++ b/arch/x86/kvm/regs.c
@@ -563,7 +563,8 @@ static bool kvm_is_valid_sregs(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs)
 	}
 
 	return kvm_is_valid_cr4(vcpu, sregs->cr4) &&
-	       kvm_is_valid_cr0(vcpu, sregs->cr0);
+	       kvm_is_valid_cr0(vcpu, sregs->cr0) &&
+	       kvm_valid_efer(vcpu, sregs->efer);
 }
 
 static int __set_sregs_common(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs,
-- 
2.55.0.rc0.799.gd6f94ed593-goog

[PATCH 2/7] KVM: SVM: Disallow EFER.SVME and EFER.LSMLE if nested is disabled

[Yosry Ahmed]

Explicitly disallow setting EFER.SVME and EFER.LSMLE if nested
virtualization is disabled on SVM, to prevent the bits remaining allowed
if kvm_amd is loaded with nested=1 and then reloaded with nested=0.

This is a minimal fix for the benefit of stable backports, which will be
followed by a more systematic fix (moving efer_reserved_bits to
kvm_caps).

Cc: stable@vger.kernel.org
Signed-off-by: Yosry Ahmed <yosry@kernel.org>
---
 arch/x86/kvm/msrs.c    | 8 +++++++-
 arch/x86/kvm/msrs.h    | 1 +
 arch/x86/kvm/svm/svm.c | 3 +++
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/msrs.c b/arch/x86/kvm/msrs.c
index c230b18d87e38..45170df0ce40b 100644
--- a/arch/x86/kvm/msrs.c
+++ b/arch/x86/kvm/msrs.c
@@ -660,10 +660,16 @@ static int set_efer(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 
 void kvm_enable_efer_bits(u64 mask)
 {
-       efer_reserved_bits &= ~mask;
+	efer_reserved_bits &= ~mask;
 }
 EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_enable_efer_bits);
 
+void kvm_disable_efer_bits(u64 mask)
+{
+	efer_reserved_bits |= mask;
+}
+EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_disable_efer_bits);
+
 bool kvm_msr_allowed(struct kvm_vcpu *vcpu, u32 index, u32 type)
 {
 	struct kvm_x86_msr_filter *msr_filter;
diff --git a/arch/x86/kvm/msrs.h b/arch/x86/kvm/msrs.h
index b698983e37fb6..89f10447cdddf 100644
--- a/arch/x86/kvm/msrs.h
+++ b/arch/x86/kvm/msrs.h
@@ -59,6 +59,7 @@ int kvm_get_reg_list(struct kvm_vcpu *vcpu,
 		     struct kvm_reg_list __user *user_list);
 
 void kvm_enable_efer_bits(u64);
+void kvm_disable_efer_bits(u64);
 bool kvm_valid_efer(struct kvm_vcpu *vcpu, u64 efer);
 int kvm_emulate_msr_read(struct kvm_vcpu *vcpu, u32 index, u64 *data);
 int kvm_emulate_msr_write(struct kvm_vcpu *vcpu, u32 index, u64 data);
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index ef69a51ab27f9..1d51500238462 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -5645,6 +5645,9 @@ static __init int svm_hardware_setup(void)
 		r = nested_svm_init_msrpm_merge_offsets();
 		if (r)
 			return r;
+	} else {
+		kvm_disable_efer_bits(EFER_SVME);
+		kvm_disable_efer_bits(EFER_LMSLE);
 	}
 
 	/*
-- 
2.55.0.rc0.799.gd6f94ed593-goog

[PATCH 3/7] KVM: x86: Disallow EFER.LME and EFER.LMA if long mode is not supported

[Yosry Ahmed]

Remove EFER.LME and EFER.LMA from EFER reserved bits only if long mode
is actually supported. KVM does check long-mode support before allowing
the bits for guest writes and userspace writes through KVM_SET_SREGS*
(in __kvm_valid_efer()), but userspace writes through KVM_SET_MSRS only
check reserved bits.

A nice side effect (and the true motivation) is getting rid of
the #ifdeffery when initializing efer_reserved_bits.

Signed-off-by: Yosry Ahmed <yosry@kernel.org>
---
 arch/x86/kvm/msrs.c | 10 +---------
 arch/x86/kvm/x86.c  |  3 +++
 2 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kvm/msrs.c b/arch/x86/kvm/msrs.c
index 45170df0ce40b..17d4c813a9e8a 100644
--- a/arch/x86/kvm/msrs.c
+++ b/arch/x86/kvm/msrs.c
@@ -19,16 +19,8 @@ bool __read_mostly report_ignored_msrs = true;
 module_param(report_ignored_msrs, bool, 0644);
 EXPORT_SYMBOL_FOR_KVM_INTERNAL(report_ignored_msrs);
 
-/* EFER defaults:
- * - enable syscall per default because its emulated by KVM
- * - enable LME and LMA per default on 64 bit KVM
- */
-#ifdef CONFIG_X86_64
-static
-u64 __read_mostly efer_reserved_bits = ~((u64)(EFER_SCE | EFER_LME | EFER_LMA));
-#else
+/* Enable syscall by default because its emulated by KVM */
 static u64 __read_mostly efer_reserved_bits = ~((u64)EFER_SCE);
-#endif
 
 #define MAX_IO_MSRS 256
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 8dbc0fa302a8b..a0b2c40d93c21 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6892,6 +6892,9 @@ EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_setup_xss_caps);
 
 static void kvm_setup_efer_caps(void)
 {
+	if (kvm_cpu_cap_has(X86_FEATURE_LM))
+		kvm_enable_efer_bits(EFER_LME | EFER_LMA);
+
 	if (kvm_cpu_cap_has(X86_FEATURE_NX))
 		kvm_enable_efer_bits(EFER_NX);
 
-- 
2.55.0.rc0.799.gd6f94ed593-goog

[PATCH 4/7] KVM: x86: Add a per-vendor callback to setup EFER caps

[Yosry Ahmed]

Move handling EFER.SVME and EFER.LMSLE from hardware setup to a new
optional per-vendor callback invoked from kvm_setup_efer_caps(). This
centralizes allowed EFER bits handling to kvm_setup_efer_caps(),
facilitating following changes to move efer_reserved_bits into kvm_caps.

Move the call to kvm_setup_efer_caps() after per-vendor ops are
initialized.

No functional change intended.

Signed-off-by: Yosry Ahmed <yosry@kernel.org>
---
 arch/x86/include/asm/kvm-x86-ops.h |  1 +
 arch/x86/include/asm/kvm_host.h    |  1 +
 arch/x86/kvm/svm/svm.c             | 20 +++++++++++++-------
 arch/x86/kvm/x86.c                 |  6 ++++--
 4 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
index 83dc5086138b3..d587242dcab56 100644
--- a/arch/x86/include/asm/kvm-x86-ops.h
+++ b/arch/x86/include/asm/kvm-x86-ops.h
@@ -45,6 +45,7 @@ KVM_X86_OP_OPTIONAL(post_set_cr3)
 KVM_X86_OP(is_valid_cr4)
 KVM_X86_OP(set_cr4)
 KVM_X86_OP(set_efer)
+KVM_X86_OP_OPTIONAL(setup_efer_caps)
 KVM_X86_OP(get_idt)
 KVM_X86_OP(set_idt)
 KVM_X86_OP(get_gdt)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index d8700eb848b45..0efd93f961df4 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1730,6 +1730,7 @@ struct kvm_x86_ops {
 	bool (*is_valid_cr4)(struct kvm_vcpu *vcpu, unsigned long cr4);
 	void (*set_cr4)(struct kvm_vcpu *vcpu, unsigned long cr4);
 	int (*set_efer)(struct kvm_vcpu *vcpu, u64 efer);
+	void (*setup_efer_caps)(void);
 	void (*get_idt)(struct kvm_vcpu *vcpu, struct desc_ptr *dt);
 	void (*set_idt)(struct kvm_vcpu *vcpu, struct desc_ptr *dt);
 	void (*get_gdt)(struct kvm_vcpu *vcpu, struct desc_ptr *dt);
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 1d51500238462..747f1e31a3622 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -274,6 +274,18 @@ int svm_set_efer(struct kvm_vcpu *vcpu, u64 efer)
 	return 0;
 }
 
+static void svm_setup_efer_caps(void)
+{
+	if (nested) {
+		kvm_enable_efer_bits(EFER_SVME);
+		if (!boot_cpu_has(X86_FEATURE_EFER_LMSLE_MBZ))
+			kvm_enable_efer_bits(EFER_LMSLE);
+	} else {
+		kvm_disable_efer_bits(EFER_SVME);
+		kvm_disable_efer_bits(EFER_LMSLE);
+	}
+}
+
 static u32 svm_get_interrupt_shadow(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
@@ -5364,6 +5376,7 @@ struct kvm_x86_ops svm_x86_ops __initdata = {
 	.is_valid_cr4 = svm_is_valid_cr4,
 	.set_cr4 = svm_set_cr4,
 	.set_efer = svm_set_efer,
+	.setup_efer_caps = svm_setup_efer_caps,
 	.get_idt = svm_get_idt,
 	.set_idt = svm_set_idt,
 	.get_gdt = svm_get_gdt,
@@ -5638,16 +5651,9 @@ static __init int svm_hardware_setup(void)
 
 	if (nested) {
 		pr_info("Nested Virtualization enabled\n");
-		kvm_enable_efer_bits(EFER_SVME);
-		if (!boot_cpu_has(X86_FEATURE_EFER_LMSLE_MBZ))
-			kvm_enable_efer_bits(EFER_LMSLE);
-
 		r = nested_svm_init_msrpm_merge_offsets();
 		if (r)
 			return r;
-	} else {
-		kvm_disable_efer_bits(EFER_SVME);
-		kvm_disable_efer_bits(EFER_LMSLE);
 	}
 
 	/*
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index a0b2c40d93c21..a297a77469b38 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6903,6 +6903,8 @@ static void kvm_setup_efer_caps(void)
 
 	if (kvm_cpu_cap_has(X86_FEATURE_AUTOIBRS))
 		kvm_enable_efer_bits(EFER_AUTOIBRS);
+
+	kvm_x86_call(setup_efer_caps)();
 }
 
 static inline void kvm_ops_update(struct kvm_x86_init_ops *ops)
@@ -7041,13 +7043,13 @@ int kvm_x86_vendor_init(struct kvm_x86_init_ops *ops)
 	if (r != 0)
 		goto out_mmu_exit;
 
-	kvm_setup_efer_caps();
-
 	enable_device_posted_irqs &= enable_apicv &&
 				     irq_remapping_cap(IRQ_POSTING_CAP);
 
 	kvm_ops_update(ops);
 
+	kvm_setup_efer_caps();
+
 	for_each_online_cpu(cpu) {
 		smp_call_function_single(cpu, kvm_x86_check_cpu_compat, &r, 1);
 		if (r < 0)
-- 
2.55.0.rc0.799.gd6f94ed593-goog

[PATCH 5/7] KVM: x86: Reverse the polarity of efer_reserved_bits

[Yosry Ahmed]

In preparation for moving efer_reserved_bits into kvm_caps, reverse its
polarity and make it efer_supported_bits, to be more consistent with
other fields in kvm_caps.

No functional change intended.

Signed-off-by: Yosry Ahmed <yosry@kernel.org>
---
 arch/x86/kvm/msrs.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/msrs.c b/arch/x86/kvm/msrs.c
index 17d4c813a9e8a..b9a16a27f6c23 100644
--- a/arch/x86/kvm/msrs.c
+++ b/arch/x86/kvm/msrs.c
@@ -20,7 +20,7 @@ module_param(report_ignored_msrs, bool, 0644);
 EXPORT_SYMBOL_FOR_KVM_INTERNAL(report_ignored_msrs);
 
 /* Enable syscall by default because its emulated by KVM */
-static u64 __read_mostly efer_reserved_bits = ~((u64)EFER_SCE);
+static u64 __read_mostly efer_supported_bits = ((u64)EFER_SCE);
 
 #define MAX_IO_MSRS 256
 
@@ -606,7 +606,7 @@ static bool __kvm_valid_efer(struct kvm_vcpu *vcpu, u64 efer)
 }
 bool kvm_valid_efer(struct kvm_vcpu *vcpu, u64 efer)
 {
-	if (efer & efer_reserved_bits)
+	if (!(efer & efer_supported_bits))
 		return false;
 
 	return __kvm_valid_efer(vcpu, efer);
@@ -619,7 +619,7 @@ static int set_efer(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	u64 efer = msr_info->data;
 	int r;
 
-	if (efer & efer_reserved_bits)
+	if (!(efer & efer_supported_bits))
 		return 1;
 
 	if (!msr_info->host_initiated) {
@@ -652,13 +652,13 @@ static int set_efer(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 
 void kvm_enable_efer_bits(u64 mask)
 {
-	efer_reserved_bits &= ~mask;
+	efer_supported_bits |= mask;
 }
 EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_enable_efer_bits);
 
 void kvm_disable_efer_bits(u64 mask)
 {
-	efer_reserved_bits |= mask;
+	efer_supported_bits &= ~mask;
 }
 EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_disable_efer_bits);
 
-- 
2.55.0.rc0.799.gd6f94ed593-goog

[PATCH 6/7] KVM: x86: Move supported EFER bits to kvm_caps

[Yosry Ahmed]

Supported EFER bits naturally fits into kvm_caps because it needs to be
recomputed during vendor init (e.g. to account for EFER.SVME being
allowed/disallowed based on nested being enabled/disabled).

Move efer_supported_bits into kvm_caps as supported_efer_bits (for
naming consistency), and reinitialize it at the beginning of
kvm_setup_efer_caps(), removing the need to clear unsupported bits in
vendor code (e.g. EFER.SVME).

As the bitmask is now globally visible as part of kvm_caps, there's
little use for helpers to enable/disable specific bits, so drop them and
open-code updates to kvm_caps.supported_efer_bits.

No functional change intended.

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Yosry Ahmed <yosry@kernel.org>
---
 arch/x86/include/asm/kvm_host.h |  2 ++
 arch/x86/kvm/msrs.c             | 19 ++-----------------
 arch/x86/kvm/svm/svm.c          |  7 ++-----
 arch/x86/kvm/x86.c              | 11 +++++++----
 4 files changed, 13 insertions(+), 26 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 0efd93f961df4..40be39fea39b9 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -329,6 +329,8 @@ struct kvm_caps {
 	u64 supported_xss;
 	u64 supported_perf_cap;
 
+	u64 supported_efer_bits;
+
 	u64 supported_quirks;
 	u64 inapplicable_quirks;
 };
diff --git a/arch/x86/kvm/msrs.c b/arch/x86/kvm/msrs.c
index b9a16a27f6c23..a39c73088f1fb 100644
--- a/arch/x86/kvm/msrs.c
+++ b/arch/x86/kvm/msrs.c
@@ -19,9 +19,6 @@ bool __read_mostly report_ignored_msrs = true;
 module_param(report_ignored_msrs, bool, 0644);
 EXPORT_SYMBOL_FOR_KVM_INTERNAL(report_ignored_msrs);
 
-/* Enable syscall by default because its emulated by KVM */
-static u64 __read_mostly efer_supported_bits = ((u64)EFER_SCE);
-
 #define MAX_IO_MSRS 256
 
 struct msr_bitmap_range {
@@ -606,7 +603,7 @@ static bool __kvm_valid_efer(struct kvm_vcpu *vcpu, u64 efer)
 }
 bool kvm_valid_efer(struct kvm_vcpu *vcpu, u64 efer)
 {
-	if (!(efer & efer_supported_bits))
+	if (!(efer & kvm_caps.supported_efer_bits))
 		return false;
 
 	return __kvm_valid_efer(vcpu, efer);
@@ -619,7 +616,7 @@ static int set_efer(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	u64 efer = msr_info->data;
 	int r;
 
-	if (!(efer & efer_supported_bits))
+	if (!(efer & kvm_caps.supported_efer_bits))
 		return 1;
 
 	if (!msr_info->host_initiated) {
@@ -650,18 +647,6 @@ static int set_efer(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	return 0;
 }
 
-void kvm_enable_efer_bits(u64 mask)
-{
-	efer_supported_bits |= mask;
-}
-EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_enable_efer_bits);
-
-void kvm_disable_efer_bits(u64 mask)
-{
-	efer_supported_bits &= ~mask;
-}
-EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_disable_efer_bits);
-
 bool kvm_msr_allowed(struct kvm_vcpu *vcpu, u32 index, u32 type)
 {
 	struct kvm_x86_msr_filter *msr_filter;
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 747f1e31a3622..e755f43f4376e 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -277,12 +277,9 @@ int svm_set_efer(struct kvm_vcpu *vcpu, u64 efer)
 static void svm_setup_efer_caps(void)
 {
 	if (nested) {
-		kvm_enable_efer_bits(EFER_SVME);
+		kvm_caps.supported_efer_bits |= EFER_SVME;
 		if (!boot_cpu_has(X86_FEATURE_EFER_LMSLE_MBZ))
-			kvm_enable_efer_bits(EFER_LMSLE);
-	} else {
-		kvm_disable_efer_bits(EFER_SVME);
-		kvm_disable_efer_bits(EFER_LMSLE);
+			kvm_caps.supported_efer_bits |= EFER_LMSLE;
 	}
 }
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index a297a77469b38..5cedaa8409f8d 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6892,17 +6892,20 @@ EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_setup_xss_caps);
 
 static void kvm_setup_efer_caps(void)
 {
+	/* Enable syscall by default because its emulated by KVM */
+	kvm_caps.supported_efer_bits = (u64)EFER_SCE;
+
 	if (kvm_cpu_cap_has(X86_FEATURE_LM))
-		kvm_enable_efer_bits(EFER_LME | EFER_LMA);
+		kvm_caps.supported_efer_bits |= (EFER_LME | EFER_LMA);
 
 	if (kvm_cpu_cap_has(X86_FEATURE_NX))
-		kvm_enable_efer_bits(EFER_NX);
+		kvm_caps.supported_efer_bits |= EFER_NX;
 
 	if (kvm_cpu_cap_has(X86_FEATURE_FXSR_OPT))
-		kvm_enable_efer_bits(EFER_FFXSR);
+		kvm_caps.supported_efer_bits |= EFER_FFXSR;
 
 	if (kvm_cpu_cap_has(X86_FEATURE_AUTOIBRS))
-		kvm_enable_efer_bits(EFER_AUTOIBRS);
+		kvm_caps.supported_efer_bits |= EFER_AUTOIBRS;
 
 	kvm_x86_call(setup_efer_caps)();
 }
-- 
2.55.0.rc0.799.gd6f94ed593-goog

[PATCH 7/7] KVM: selftests: Extend set_sregs test to cover EFER

[Yosry Ahmed]

Extend the set_sregs test to cover various bits in EFER. Update
TEST_INVALID_CR_BIT() to operate on EFER as well as CRx (and rename it
accordingly). Add test cases to check that EFER bits are disallowed
without the relevant CPUID enablement.

Assisted-by: Gemini:unknown-version
Signed-off-by: Yosry Ahmed <yosry@kernel.org>
---
 .../selftests/kvm/include/x86/processor.h     |  2 +
 .../selftests/kvm/x86/set_sregs_test.c        | 83 ++++++++++++++-----
 2 files changed, 64 insertions(+), 21 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/x86/processor.h b/tools/testing/selftests/kvm/include/x86/processor.h
index 7d3a27bc0d842..b161174ece453 100644
--- a/tools/testing/selftests/kvm/include/x86/processor.h
+++ b/tools/testing/selftests/kvm/include/x86/processor.h
@@ -208,6 +208,7 @@ struct kvm_x86_cpu_feature {
 #define	X86_FEATURE_PERFCTR_NB		KVM_X86_CPU_FEATURE(0x80000001, 0, ECX, 24)
 #define	X86_FEATURE_PERFCTR_LLC		KVM_X86_CPU_FEATURE(0x80000001, 0, ECX, 28)
 #define	X86_FEATURE_NX			KVM_X86_CPU_FEATURE(0x80000001, 0, EDX, 20)
+#define	X86_FEATURE_FXSR_OPT		KVM_X86_CPU_FEATURE(0x80000001, 0, EDX, 25)
 #define	X86_FEATURE_GBPAGES		KVM_X86_CPU_FEATURE(0x80000001, 0, EDX, 26)
 #define	X86_FEATURE_RDTSCP		KVM_X86_CPU_FEATURE(0x80000001, 0, EDX, 27)
 #define	X86_FEATURE_LM			KVM_X86_CPU_FEATURE(0x80000001, 0, EDX, 29)
@@ -226,6 +227,7 @@ struct kvm_x86_cpu_feature {
 #define X86_FEATURE_SEV			KVM_X86_CPU_FEATURE(0x8000001F, 0, EAX, 1)
 #define X86_FEATURE_SEV_ES		KVM_X86_CPU_FEATURE(0x8000001F, 0, EAX, 3)
 #define X86_FEATURE_SEV_SNP		KVM_X86_CPU_FEATURE(0x8000001F, 0, EAX, 4)
+#define	X86_FEATURE_AUTOIBRS		KVM_X86_CPU_FEATURE(0x80000021, 0, EAX, 8)
 #define	X86_FEATURE_GP_ON_USER_CPUID	KVM_X86_CPU_FEATURE(0x80000021, 0, EAX, 17)
 #define	X86_FEATURE_PERFMON_V2		KVM_X86_CPU_FEATURE(0x80000022, 0, EAX, 0)
 #define	X86_FEATURE_LBR_PMC_FREEZE	KVM_X86_CPU_FEATURE(0x80000022, 0, EAX, 2)
diff --git a/tools/testing/selftests/kvm/x86/set_sregs_test.c b/tools/testing/selftests/kvm/x86/set_sregs_test.c
index 8e654cc9ab168..562afab378d11 100644
--- a/tools/testing/selftests/kvm/x86/set_sregs_test.c
+++ b/tools/testing/selftests/kvm/x86/set_sregs_test.c
@@ -21,20 +21,20 @@
 #include "kvm_util.h"
 #include "processor.h"
 
-#define TEST_INVALID_CR_BIT(vcpu, cr, orig, bit)				\
+#define TEST_INVALID_SREG_BIT(vcpu, reg, orig, bit)				\
 do {										\
 	struct kvm_sregs new;							\
 	int rc;									\
 										\
 	/* Skip the sub-test, the feature/bit is supported. */			\
-	if (orig.cr & bit)							\
+	if (orig.reg & bit)							\
 		break;								\
 										\
-	memcpy(&new, &orig, sizeof(sregs));					\
-	new.cr |= bit;								\
+	memcpy(&new, &orig, sizeof(new));					\
+	new.reg |= bit;								\
 										\
 	rc = _vcpu_sregs_set(vcpu, &new);					\
-	TEST_ASSERT(rc, "KVM allowed invalid " #cr " bit (0x%lx)", bit);	\
+	TEST_ASSERT(rc, "KVM allowed invalid " #reg " bit (0x%llx)", (unsigned long long)bit); \
 										\
 	/* Sanity check that KVM didn't change anything. */			\
 	vcpu_sregs_get(vcpu, &new);						\
@@ -46,6 +46,8 @@ do {										\
 				X86_CR4_MCE | X86_CR4_PGE | X86_CR4_PCE |	\
 				X86_CR4_OSFXSR | X86_CR4_OSXMMEXCPT)
 
+#define KVM_ALWAYS_ALLOWED_EFER EFER_SCE
+
 static u64 calc_supported_cr4_feature_bits(void)
 {
 	u64 cr4 = KVM_ALWAYS_ALLOWED_CR4;
@@ -74,6 +76,24 @@ static u64 calc_supported_cr4_feature_bits(void)
 	return cr4;
 }
 
+static u64 calc_supported_efer_feature_bits(void)
+{
+	u64 efer = KVM_ALWAYS_ALLOWED_EFER;
+
+	if (kvm_cpu_has(X86_FEATURE_LM))
+		efer |= (EFER_LME | EFER_LMA);
+	if (kvm_cpu_has(X86_FEATURE_NX))
+		efer |= EFER_NX;
+	if (kvm_cpu_has(X86_FEATURE_SVM))
+		efer |= EFER_SVME;
+	if (kvm_cpu_has(X86_FEATURE_FXSR_OPT))
+		efer |= EFER_FFXSR;
+	if (kvm_cpu_has(X86_FEATURE_AUTOIBRS))
+		efer |= EFER_AUTOIBRS;
+
+	return efer;
+}
+
 static void test_cr_bits(struct kvm_vcpu *vcpu, u64 cr4)
 {
 	struct kvm_sregs sregs;
@@ -96,26 +116,45 @@ static void test_cr_bits(struct kvm_vcpu *vcpu, u64 cr4)
 		    (sregs.cr4 & X86_CR4_PKE) ? "set" : "clear");
 
 	vcpu_sregs_get(vcpu, &sregs);
-	TEST_ASSERT(sregs.cr4 == cr4, "sregs.CR4 (0x%llx) != CR4 (0x%lx)",
-		    sregs.cr4, cr4);
-
-	TEST_INVALID_CR_BIT(vcpu, cr4, sregs, X86_CR4_UMIP);
-	TEST_INVALID_CR_BIT(vcpu, cr4, sregs, X86_CR4_LA57);
-	TEST_INVALID_CR_BIT(vcpu, cr4, sregs, X86_CR4_VMXE);
-	TEST_INVALID_CR_BIT(vcpu, cr4, sregs, X86_CR4_SMXE);
-	TEST_INVALID_CR_BIT(vcpu, cr4, sregs, X86_CR4_FSGSBASE);
-	TEST_INVALID_CR_BIT(vcpu, cr4, sregs, X86_CR4_PCIDE);
-	TEST_INVALID_CR_BIT(vcpu, cr4, sregs, X86_CR4_OSXSAVE);
-	TEST_INVALID_CR_BIT(vcpu, cr4, sregs, X86_CR4_SMEP);
-	TEST_INVALID_CR_BIT(vcpu, cr4, sregs, X86_CR4_SMAP);
-	TEST_INVALID_CR_BIT(vcpu, cr4, sregs, X86_CR4_PKE);
+	TEST_ASSERT_EQ(sregs.cr4, cr4);
+
+	TEST_INVALID_SREG_BIT(vcpu, cr4, sregs, X86_CR4_UMIP);
+	TEST_INVALID_SREG_BIT(vcpu, cr4, sregs, X86_CR4_LA57);
+	TEST_INVALID_SREG_BIT(vcpu, cr4, sregs, X86_CR4_VMXE);
+	TEST_INVALID_SREG_BIT(vcpu, cr4, sregs, X86_CR4_SMXE);
+	TEST_INVALID_SREG_BIT(vcpu, cr4, sregs, X86_CR4_FSGSBASE);
+	TEST_INVALID_SREG_BIT(vcpu, cr4, sregs, X86_CR4_PCIDE);
+	TEST_INVALID_SREG_BIT(vcpu, cr4, sregs, X86_CR4_OSXSAVE);
+	TEST_INVALID_SREG_BIT(vcpu, cr4, sregs, X86_CR4_SMEP);
+	TEST_INVALID_SREG_BIT(vcpu, cr4, sregs, X86_CR4_SMAP);
+	TEST_INVALID_SREG_BIT(vcpu, cr4, sregs, X86_CR4_PKE);
 
 	for (i = 32; i < 64; i++)
-		TEST_INVALID_CR_BIT(vcpu, cr0, sregs, BIT(i));
+		TEST_INVALID_SREG_BIT(vcpu, cr0, sregs, BIT(i));
 
 	/* NW without CD is illegal, as is PG without PE. */
-	TEST_INVALID_CR_BIT(vcpu, cr0, sregs, X86_CR0_NW);
-	TEST_INVALID_CR_BIT(vcpu, cr0, sregs, X86_CR0_PG);
+	TEST_INVALID_SREG_BIT(vcpu, cr0, sregs, X86_CR0_NW);
+	TEST_INVALID_SREG_BIT(vcpu, cr0, sregs, X86_CR0_PG);
+}
+
+static void test_efer_bits(struct kvm_vcpu *vcpu, u64 efer)
+{
+	struct kvm_sregs sregs;
+	int rc;
+
+	vcpu_sregs_get(vcpu, &sregs);
+	sregs.efer |= efer;
+	rc = _vcpu_sregs_set(vcpu, &sregs);
+	TEST_ASSERT(!rc, "Failed to set supported EFER bits (0x%llx)", sregs.efer);
+
+	vcpu_sregs_get(vcpu, &sregs);
+	TEST_ASSERT_EQ(sregs.efer, efer);
+
+	TEST_INVALID_SREG_BIT(vcpu, efer, sregs, EFER_LME);
+	TEST_INVALID_SREG_BIT(vcpu, efer, sregs, EFER_NX);
+	TEST_INVALID_SREG_BIT(vcpu, efer, sregs, EFER_SVME);
+	TEST_INVALID_SREG_BIT(vcpu, efer, sregs, EFER_FFXSR);
+	TEST_INVALID_SREG_BIT(vcpu, efer, sregs, EFER_AUTOIBRS);
 }
 
 int main(int argc, char *argv[])
@@ -132,6 +171,7 @@ int main(int argc, char *argv[])
 	 */
 	vm = vm_create_barebones();
 	vcpu = __vm_vcpu_add(vm, 0);
+	test_efer_bits(vcpu, KVM_ALWAYS_ALLOWED_EFER);
 	test_cr_bits(vcpu, KVM_ALWAYS_ALLOWED_CR4);
 	kvm_vm_free(vm);
 
@@ -151,6 +191,7 @@ int main(int argc, char *argv[])
 		    sregs.apic_base);
 
 	test_cr_bits(vcpu, calc_supported_cr4_feature_bits());
+	test_efer_bits(vcpu, calc_supported_efer_feature_bits());
 
 	kvm_vm_free(vm);
 
-- 
2.55.0.rc0.799.gd6f94ed593-goog

Re: [PATCH 5/7] KVM: x86: Reverse the polarity of efer_reserved_bits

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Incorrect bitwise logic in EFER validation rejects valid zero values (breaking guest boot) and allows guests to set reserved bits (triggering VMENTRY failures).
--

commit 4bd740e43933efd5aaca18b2bd83e1696dbbd7dd
Author: Yosry Ahmed <yosry@kernel.org>

KVM: x86: Reverse the polarity of efer_reserved_bits

This commit reverses the polarity of the EFER validation variable, renaming
efer_reserved_bits to efer_supported_bits to align with other fields in
kvm_caps. The stated intent is that no functional change is introduced.

> diff --git a/arch/x86/kvm/msrs.c b/arch/x86/kvm/msrs.c
> index 17d4c813a9e8a..b9a16a27f6c23 100644
> --- a/arch/x86/kvm/msrs.c
> +++ b/arch/x86/kvm/msrs.c

[ ... ]

> @@ -606,7 +606,7 @@ static bool __kvm_valid_efer(struct kvm_vcpu *vcpu, u64 efer)
>  }
>  bool kvm_valid_efer(struct kvm_vcpu *vcpu, u64 efer)
>  {
> -	if (efer & efer_reserved_bits)
> +	if (!(efer & efer_supported_bits))

[Severity: High]
Does this bitwise logic inadvertently reject valid zero values and allow
guests to set reserved bits?

If a guest writes 0 to the EFER MSR, (efer & efer_supported_bits) evaluates
to 0, and the negation makes this condition true, improperly rejecting it.

Additionally, if a guest writes an EFER value containing both a supported
bit and a reserved bit, the condition evaluates to false, allowing the
reserved bit to be set. 

Should this check instead be (efer & ~efer_supported_bits)?

>  		return false;
>  
>  	return __kvm_valid_efer(vcpu, efer);
> @@ -619,7 +619,7 @@ static int set_efer(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  	u64 efer = msr_info->data;
>  	int r;
>  
> -	if (efer & efer_reserved_bits)
> +	if (!(efer & efer_supported_bits))

[Severity: High]
As with kvm_valid_efer() above, does this logic incorrectly reject a valid
clearing of EFER and accept invalid reserved bits if at least one supported
bit is also present?

>  		return 1;
>  
>  	if (!msr_info->host_initiated) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260630234716.3039031-1-yosry@kernel.org?part=5

Re: [PATCH 5/7] KVM: x86: Reverse the polarity of efer_reserved_bits

[Yosry Ahmed]

On Tue, Jun 30, 2026 at 4:52 PM <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Incorrect bitwise logic in EFER validation rejects valid zero values (breaking guest boot) and allows guests to set reserved bits (triggering VMENTRY failures).
> --
>
> commit 4bd740e43933efd5aaca18b2bd83e1696dbbd7dd
> Author: Yosry Ahmed <yosry@kernel.org>
>
> KVM: x86: Reverse the polarity of efer_reserved_bits
>
> This commit reverses the polarity of the EFER validation variable, renaming
> efer_reserved_bits to efer_supported_bits to align with other fields in
> kvm_caps. The stated intent is that no functional change is introduced.
>
> > diff --git a/arch/x86/kvm/msrs.c b/arch/x86/kvm/msrs.c
> > index 17d4c813a9e8a..b9a16a27f6c23 100644
> > --- a/arch/x86/kvm/msrs.c
> > +++ b/arch/x86/kvm/msrs.c
>
> [ ... ]
>
> > @@ -606,7 +606,7 @@ static bool __kvm_valid_efer(struct kvm_vcpu *vcpu, u64 efer)
> >  }
> >  bool kvm_valid_efer(struct kvm_vcpu *vcpu, u64 efer)
> >  {
> > -     if (efer & efer_reserved_bits)
> > +     if (!(efer & efer_supported_bits))
>
> [Severity: High]
> Does this bitwise logic inadvertently reject valid zero values and allow
> guests to set reserved bits?
>
> If a guest writes 0 to the EFER MSR, (efer & efer_supported_bits) evaluates
> to 0, and the negation makes this condition true, improperly rejecting it.
>
> Additionally, if a guest writes an EFER value containing both a supported
> bit and a reserved bit, the condition evaluates to false, allowing the
> reserved bit to be set.
>
> Should this check instead be (efer & ~efer_supported_bits)?

Yes, I completely screwed this up :)

The tests don't fail because we only have tests for SREGS setting EFER
(added in this series), and __kvm_valid_efer() still properly catches
invalid bits for this path (because KVM caps == guest caps).

Re: [PATCH 6/7] KVM: x86: Move supported EFER bits to kvm_caps

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Incorrect bitwise logic in EFER validation rejects valid EFER=0 and allows setting reserved EFER bits.
--

commit 2ebe510bc1d7884420361ec9d7613318508f1d94
Author: Yosry Ahmed <yosry@kernel.org>

KVM: x86: Move supported EFER bits to kvm_caps

This commit moves efer_supported_bits into kvm_caps as supported_efer_bits
for consistency, allowing it to be recomputed during vendor initialization.
It removes helper functions for enabling or disabling specific bits, choosing
to open-code updates directly to the new bitmask.

> diff --git a/arch/x86/kvm/msrs.c b/arch/x86/kvm/msrs.c
> --- a/arch/x86/kvm/msrs.c
> +++ b/arch/x86/kvm/msrs.c
> @@ -606,7 +603,7 @@ static bool __kvm_valid_efer(struct kvm_vcpu *vcpu, u64 efer)
>  }
>  bool kvm_valid_efer(struct kvm_vcpu *vcpu, u64 efer)
>  {
> -	if (!(efer & efer_supported_bits))
> +	if (!(efer & kvm_caps.supported_efer_bits))

[Severity: High]
Does this bitwise logic incorrectly reject an efer value of 0?

If userspace or a guest completely clears efer (e.g., setting it to 0 when
returning to real mode), efer & kvm_caps.supported_efer_bits evaluates to 0,
making the ! condition true. This would reject the valid state.

Additionally, does this allow writing unsupported reserved bits? If a guest
sets a supported bit alongside arbitrary reserved bits (e.g.,
EFER_SCE | BIT_ULL(63)), the bitwise AND yields a non-zero value, meaning
this check passes.

Since __kvm_valid_efer() only validates specific feature bits and ignores
unknown bits, could these reserved bits be written directly to vCPU hardware
state?

>  		return false;
>  
>  	return __kvm_valid_efer(vcpu, efer);
> @@ -619,7 +616,7 @@ static int set_efer(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  	u64 efer = msr_info->data;
>  	int r;
>  
> -	if (!(efer & efer_supported_bits))
> +	if (!(efer & kvm_caps.supported_efer_bits))

[Severity: High]
Does this identically flawed check also block the guest from clearing its efer
via wrmsr, and allow writing reserved bits directly to hardware?

Should this check be if (efer & ~kvm_caps.supported_efer_bits) instead to
correctly verify that no unsupported bits are set?

>  		return 1;
>  
>  	if (!msr_info->host_initiated) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260630234716.3039031-1-yosry@kernel.org?part=6

Re: [PATCH 5/7] KVM: x86: Reverse the polarity of efer_reserved_bits

[Yosry Ahmed]

On Tue, Jun 30, 2026 at 4:54 PM Yosry Ahmed <yosry@kernel.org> wrote:
>
> On Tue, Jun 30, 2026 at 4:52 PM <sashiko-bot@kernel.org> wrote:
> >
> > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> > - [High] Incorrect bitwise logic in EFER validation rejects valid zero values (breaking guest boot) and allows guests to set reserved bits (triggering VMENTRY failures).
> > --
> >
> > commit 4bd740e43933efd5aaca18b2bd83e1696dbbd7dd
> > Author: Yosry Ahmed <yosry@kernel.org>
> >
> > KVM: x86: Reverse the polarity of efer_reserved_bits
> >
> > This commit reverses the polarity of the EFER validation variable, renaming
> > efer_reserved_bits to efer_supported_bits to align with other fields in
> > kvm_caps. The stated intent is that no functional change is introduced.
> >
> > > diff --git a/arch/x86/kvm/msrs.c b/arch/x86/kvm/msrs.c
> > > index 17d4c813a9e8a..b9a16a27f6c23 100644
> > > --- a/arch/x86/kvm/msrs.c
> > > +++ b/arch/x86/kvm/msrs.c
> >
> > [ ... ]
> >
> > > @@ -606,7 +606,7 @@ static bool __kvm_valid_efer(struct kvm_vcpu *vcpu, u64 efer)
> > >  }
> > >  bool kvm_valid_efer(struct kvm_vcpu *vcpu, u64 efer)
> > >  {
> > > -     if (efer & efer_reserved_bits)
> > > +     if (!(efer & efer_supported_bits))
> >
> > [Severity: High]
> > Does this bitwise logic inadvertently reject valid zero values and allow
> > guests to set reserved bits?
> >
> > If a guest writes 0 to the EFER MSR, (efer & efer_supported_bits) evaluates
> > to 0, and the negation makes this condition true, improperly rejecting it.
> >
> > Additionally, if a guest writes an EFER value containing both a supported
> > bit and a reserved bit, the condition evaluates to false, allowing the
> > reserved bit to be set.
> >
> > Should this check instead be (efer & ~efer_supported_bits)?
>
> Yes, I completely screwed this up :)
>
> The tests don't fail because we only have tests for SREGS setting EFER
> (added in this series), and __kvm_valid_efer() still properly catches
> invalid bits for this path (because KVM caps == guest caps).

For the record, I was curious why my hacky AI-generated reproducer
didn't fail with this bug.

Turns out it was trying to write efer=EFER.SVME, which means that:
(a) If nested=1, (efer & efer_supported_bits) would be 1, and
set_efer() would succeed as expected.
(b) If nested=0, (efer & efer_supported_bits) would be 0, and
set_efer() would (accidentally) fail as expected.

I updated the reproducer to read the value of EFER first, then write
efer=(old_efer | EFER.SVME). With nested=0, setting EFER.SVME would
(unexpectedly) succeed with the incorrect check above, because (efer &
efer_supported_bits) is non-zero. Fixing the check to be (efer &
~efer_supported_bits) makes the reproducer pass as expected.