[PATCH v3 0/2] KVM: x86: Fix missing EFER validity checks

[PATCH v3 0/2] KVM: x86: Fix missing EFER validity checks

[Yosry Ahmed]

v3 of the EFER missing validity checks on KVM_SET_SREGS* fix, split out
a larger series of EFER fixes.

v2 -> v3:
- Split out KVM_SET_SREG* fix and selftest.

v2: https://lore.kernel.org/kvm/20260706195413.1966458-1-yosry@kernel.org/

Yosry Ahmed (2):
  KVM: x86: Check EFER validity on KVM_SET_SREGS*
  KVM: selftests: Extend set_sregs test to cover EFER

 arch/x86/kvm/regs.c                           |  3 +-
 .../selftests/kvm/include/x86/processor.h     |  2 +
 .../selftests/kvm/x86/set_sregs_test.c        | 83 ++++++++++++++-----
 3 files changed, 66 insertions(+), 22 deletions(-)


base-commit: 3aec122bdcaf6f8c4ecfc7f6adf6773015ced676
-- 
2.55.0.141.g00534a21ce-goog

[PATCH v3 1/2] 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
Change-Id: I45701ec440e4fdd8f086eb70db0c0845fb0ed509
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 bd8147798cc3e..8f66438989e47 100644
--- a/arch/x86/kvm/regs.c
+++ b/arch/x86/kvm/regs.c
@@ -564,7 +564,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.141.g00534a21ce-goog

[PATCH v3 2/2] 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
Change-Id: I588e7259c6257f40640127a52ce808a0f5afa016
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.141.g00534a21ce-goog

Re: [PATCH v3 1/2] KVM: x86: Check EFER validity on KVM_SET_SREGS*

[Yosry Ahmed]

On Mon, Jul 13, 2026 at 11:02 AM Yosry Ahmed <yosry@kernel.org> wrote:
>
> 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
> Change-Id: I45701ec440e4fdd8f086eb70db0c0845fb0ed509

Forgot to drop these, sorry :/

Re: [PATCH v3 1/2] KVM: x86: Check EFER validity on KVM_SET_SREGS*

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the return value of `kvm_x86_call(set_efer)` is ignored in `__set_sregs_common()`, leading to a silent failure when nested state allocation fails.
--

--- Patch [1]: [PATCH v3 1/2] KVM: x86: Check EFER validity on KVM_SET_SREGS* ---
commit 6edd9e84461af3c58ac965b84f89b7c4985ddeb3
Author: Yosry Ahmed <yosry@kernel.org>

KVM: x86: Check EFER validity on KVM_SET_SREGS*

This commit adds checks to validate EFER against both KVM and guest caps
during KVM_SET_SREGS* writes, preventing userspace from writing bogus values
through the per-vendor set_efer callbacks.

> diff --git a/arch/x86/kvm/regs.c b/arch/x86/kvm/regs.c
> index bd8147798cc3e..8f66438989e47 100644
> --- a/arch/x86/kvm/regs.c
> +++ b/arch/x86/kvm/regs.c

[ ... ]

> @@ -564,7 +564,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,
>  			      int *mmu_reset_needed, bool update_pdptrs)
>  {

[ ... ]

>  	vcpu->arch.cr3 = sregs->cr3;
>  	kvm_register_mark_dirty(vcpu, VCPU_REG_CR3);
>  	kvm_x86_call(post_set_cr3)(vcpu, sregs->cr3);
>  
>  	*mmu_reset_needed |= vcpu->arch.efer != sregs->efer;
>  	kvm_x86_call(set_efer)(vcpu, sregs->efer);

[Severity: High]
This isn't a bug introduced by this patch, but should the return value
of kvm_x86_call() be checked here?

If a userspace process calls the KVM_SET_SREGS ioctl with EFER.SVME set,
kvm_x86_call(set_efer) could call into svm_set_efer(), which attempts to
allocate nested state via svm_allocate_nested().

If the system is under memory pressure, this allocation can fail and return
-ENOMEM. Because the return value is ignored in __set_sregs_common(),
doesn't this lead to a silent failure where KVM fails to set the guest's
EFER register but still returns success to userspace?

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

Re: [PATCH v3 1/2] KVM: x86: Check EFER validity on KVM_SET_SREGS*

[Yosry Ahmed]

On Mon, Jul 13, 2026 at 11:14 AM <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but the return value of `kvm_x86_call(set_efer)` is ignored in `__set_sregs_common()`, leading to a silent failure when nested state allocation fails.
> --
>
> --- Patch [1]: [PATCH v3 1/2] KVM: x86: Check EFER validity on KVM_SET_SREGS* ---
> commit 6edd9e84461af3c58ac965b84f89b7c4985ddeb3
> Author: Yosry Ahmed <yosry@kernel.org>
>
> KVM: x86: Check EFER validity on KVM_SET_SREGS*
>
> This commit adds checks to validate EFER against both KVM and guest caps
> during KVM_SET_SREGS* writes, preventing userspace from writing bogus values
> through the per-vendor set_efer callbacks.
>
> > diff --git a/arch/x86/kvm/regs.c b/arch/x86/kvm/regs.c
> > index bd8147798cc3e..8f66438989e47 100644
> > --- a/arch/x86/kvm/regs.c
> > +++ b/arch/x86/kvm/regs.c
>
> [ ... ]
>
> > @@ -564,7 +564,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,
> >                             int *mmu_reset_needed, bool update_pdptrs)
> >  {
>
> [ ... ]
>
> >       vcpu->arch.cr3 = sregs->cr3;
> >       kvm_register_mark_dirty(vcpu, VCPU_REG_CR3);
> >       kvm_x86_call(post_set_cr3)(vcpu, sregs->cr3);
> >
> >       *mmu_reset_needed |= vcpu->arch.efer != sregs->efer;
> >       kvm_x86_call(set_efer)(vcpu, sregs->efer);
>
> [Severity: High]
> This isn't a bug introduced by this patch, but should the return value
> of kvm_x86_call() be checked here?
>
> If a userspace process calls the KVM_SET_SREGS ioctl with EFER.SVME set,
> kvm_x86_call(set_efer) could call into svm_set_efer(), which attempts to
> allocate nested state via svm_allocate_nested().
>
> If the system is under memory pressure, this allocation can fail and return
> -ENOMEM. Because the return value is ignored in __set_sregs_common(),
> doesn't this lead to a silent failure where KVM fails to set the guest's
> EFER register but still returns success to userspace?

This seems like a real issue, and I think it's not very easy to fix
because it seems like KVM_SET_SREGS is currently atomic, we either do
all the updates or we fail. So we can't just fail in the middle of
__set_sregs_common() if set_efer() fails, we have to either undo the
previous changes or re-order the writes to do EFER first. Re-ordering
seems like it should be fine, I don't immediately see svm_set_efer()
or vmx_set_efer() depending on any of the previous updates, but I
didn't look too closely.

Anyway, this is completely orthogonal to this patch AFAICT.

Re: [PATCH v3 1/2] KVM: x86: Check EFER validity on KVM_SET_SREGS*

[Sean Christopherson]

On Mon, Jul 13, 2026, Yosry Ahmed wrote:
> On Mon, Jul 13, 2026 at 11:02 AM Yosry Ahmed <yosry@kernel.org> wrote:
> >
> > 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
> > Change-Id: I45701ec440e4fdd8f086eb70db0c0845fb0ed509
> 
> Forgot to drop these, sorry :/

By "these", I assume you mean the Change-Id here and in the next patch?  But keep
the Cc: stable@?

Re: [PATCH v3 1/2] KVM: x86: Check EFER validity on KVM_SET_SREGS*

[Yosry Ahmed]

On Wed, Jul 22, 2026 at 1:46 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Mon, Jul 13, 2026, Yosry Ahmed wrote:
> > On Mon, Jul 13, 2026 at 11:02 AM Yosry Ahmed <yosry@kernel.org> wrote:
> > >
> > > 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
> > > Change-Id: I45701ec440e4fdd8f086eb70db0c0845fb0ed509
> >
> > Forgot to drop these, sorry :/
>
> By "these", I assume you mean the Change-Id here and in the next patch?  But keep
> the Cc: stable@?

Yes, just the Change-Id.

Re: [PATCH v3 1/2] KVM: x86: Check EFER validity on KVM_SET_SREGS*

[Jim Mattson]

On Mon, Jul 13, 2026 at 11:04 AM Yosry Ahmed <yosry@kernel.org> wrote:
>
> 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.

Where is the requirement to set CPUID before SREGS documented, aside
from this commit message?

Re: [PATCH v3 1/2] KVM: x86: Check EFER validity on KVM_SET_SREGS*

[Yosry Ahmed]

On Tue, Jul 28, 2026 at 9:53 PM Jim Mattson <jmattson@google.com> wrote:
>
> On Mon, Jul 13, 2026 at 11:04 AM Yosry Ahmed <yosry@kernel.org> wrote:
> >
> > 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.
>
> Where is the requirement to set CPUID before SREGS documented, aside
> from this commit message?

I don't think so, and also coming back to this again I think the
commit message is wrong. The whole basis for doing validity checks
against guest CPUID (other than the convenience of using
kvm_valid_efer()) is cr4_guest_rsvd_bits, which is initialized based
on both KVM caps and guest CPUID.

However, cr4_guest_rsvd_bits seems to be initialized *after* CPUID is
set, so it only checks CR4 against CPUID if userspace already set
CPUID. It doesn't impose a restriction to set CPUID before SREGS, but
this patch is.

So I think this may be too restrictive. We should probably only check
against KVM caps, which was the whole motivation of this patch to
begin with (disallowing EFER.SVME if nested=0). Maybe we should just
drop the Cc:stable as it won't apply to any of the stable trees any
way, and do this on top of the kvm_caps.supported_efer_bits changes?

diff --git a/arch/x86/kvm/regs.c b/arch/x86/kvm/regs.c
index 8f66438989e47..412a37ca86ecc 100644
--- a/arch/x86/kvm/regs.c
+++ b/arch/x86/kvm/regs.c
@@ -565,7 +565,7 @@ 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_valid_efer(vcpu, sregs->efer);
+              !(sregs->efer & ~kvm_caps.supported_efer_bits);
 }

 static int __set_sregs_common(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs,

---

diff --git a/tools/testing/selftests/kvm/x86/set_sregs_test.c
b/tools/testing/selftests/kvm/x86/set_sregs_test.c
index 603226ffe4374..c07ccf0bbe85a 100644
--- a/tools/testing/selftests/kvm/x86/set_sregs_test.c
+++ b/tools/testing/selftests/kvm/x86/set_sregs_test.c
@@ -171,7 +171,6 @@ 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);

---

Sean, WDYT?

If we will change this, would you rather do it in place, a followup
fix, or a new version?

Re: [PATCH v3 1/2] KVM: x86: Check EFER validity on KVM_SET_SREGS*

[Yosry Ahmed]

On Wed, Jul 29, 2026 at 9:52 AM Yosry Ahmed <yosry@kernel.org> wrote:
>
> On Tue, Jul 28, 2026 at 9:53 PM Jim Mattson <jmattson@google.com> wrote:
> >
> > On Mon, Jul 13, 2026 at 11:04 AM Yosry Ahmed <yosry@kernel.org> wrote:
> > >
> > > 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.
> >
> > Where is the requirement to set CPUID before SREGS documented, aside
> > from this commit message?
>
> I don't think so, and also coming back to this again I think the
> commit message is wrong. The whole basis for doing validity checks
> against guest CPUID (other than the convenience of using
> kvm_valid_efer()) is cr4_guest_rsvd_bits, which is initialized based
> on both KVM caps and guest CPUID.
>
> However, cr4_guest_rsvd_bits seems to be initialized *after* CPUID is
> set, so it only checks CR4 against CPUID if userspace already set
> CPUID. It doesn't impose a restriction to set CPUID before SREGS, but
> this patch is.
>
> So I think this may be too restrictive. We should probably only check
> against KVM caps, which was the whole motivation of this patch to
> begin with (disallowing EFER.SVME if nested=0). Maybe we should just
> drop the Cc:stable as it won't apply to any of the stable trees any
> way, and do this on top of the kvm_caps.supported_efer_bits changes?
>
> diff --git a/arch/x86/kvm/regs.c b/arch/x86/kvm/regs.c
> index 8f66438989e47..412a37ca86ecc 100644
> --- a/arch/x86/kvm/regs.c
> +++ b/arch/x86/kvm/regs.c
> @@ -565,7 +565,7 @@ 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_valid_efer(vcpu, sregs->efer);
> +              !(sregs->efer & ~kvm_caps.supported_efer_bits);
>  }
>
>  static int __set_sregs_common(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs,
>
> ---
>
> diff --git a/tools/testing/selftests/kvm/x86/set_sregs_test.c
> b/tools/testing/selftests/kvm/x86/set_sregs_test.c
> index 603226ffe4374..c07ccf0bbe85a 100644
> --- a/tools/testing/selftests/kvm/x86/set_sregs_test.c
> +++ b/tools/testing/selftests/kvm/x86/set_sregs_test.c
> @@ -171,7 +171,6 @@ 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);
>
> ---
>
> Sean, WDYT?
>
> If we will change this, would you rather do it in place, a followup
> fix, or a new version?

I take this back, I think I was right the first time.
kvm_vcpu_after_set_cpuid() is called on vCPU creation (confusing?),
and looking closely at set_sregs_test seems like it specifically
verifies that CR4 bits guarded by CPUID bits cannot be set before
CPUID is set.

I think the main difference here is probably that CR4 bits that are
guarded by CPUID are more "advanced" than EFER bits?

Re: [PATCH v3 1/2] KVM: x86: Check EFER validity on KVM_SET_SREGS*

[Sean Christopherson]

On Wed, Jul 29, 2026, Yosry Ahmed wrote:
> On Wed, Jul 29, 2026 at 9:52 AM Yosry Ahmed <yosry@kernel.org> wrote:
> >
> > On Tue, Jul 28, 2026 at 9:53 PM Jim Mattson <jmattson@google.com> wrote:
> > >
> > > On Mon, Jul 13, 2026 at 11:04 AM Yosry Ahmed <yosry@kernel.org> wrote:
> > > >
> > > > 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.
> > >
> > > Where is the requirement to set CPUID before SREGS documented, aside
> > > from this commit message?

It's not, because it's not a true requirement.  And for me, this isn't about
whether or not KVM has a documented rule, it's about how likely it is that this
change will break userspace.  And for that, Yosry's statement is perfect: the
risk of breaking userspace is tiny, because unless userspace is getting creative,
it already needs to set CPUID before loading SREGS.

> > I don't think so, and also coming back to this again I think the
> > commit message is wrong. The whole basis for doing validity checks
> > against guest CPUID (other than the convenience of using
> > kvm_valid_efer()) is cr4_guest_rsvd_bits, which is initialized based
> > on both KVM caps and guest CPUID.
> >
> > However, cr4_guest_rsvd_bits seems to be initialized *after* CPUID is
> > set, so it only checks CR4 against CPUID if userspace already set
> > CPUID. It doesn't impose a restriction to set CPUID before SREGS, but
> > this patch is.
> >
> > So I think this may be too restrictive. We should probably only check
> > against KVM caps, which was the whole motivation of this patch to
> > begin with (disallowing EFER.SVME if nested=0). Maybe we should just
> > drop the Cc:stable as it won't apply to any of the stable trees any
> > way, and do this on top of the kvm_caps.supported_efer_bits changes?

...

> I take this back, I think I was right the first time.
> kvm_vcpu_after_set_cpuid() is called on vCPU creation (confusing?),

It's confusing/odd until you realize that zeroing CPUID is also "setting" CPUID.

> and looking closely at set_sregs_test seems like it specifically verifies
> that CR4 bits guarded by CPUID bits cannot be set before CPUID is set.

No, that isn't the goal.  There are two goals:

 1. Verify userspace can't set CR4 bits that aren't supported according to the
    virtual CPU model.

 2. Verify KVM doesn't try to "help" userspace by populating CPUID with non-zero
    values, e.g. so that we don't end up with a CPUID version of
    KVM_X86_QUIRK_STUFF_FEATURE_MSRS.

Combined, they effectively create the "rule" that userspace must set CPUID before
setting certain CR4 bits, but that itself is not what the test is trying to
validate.

So I 100% agree KVM's documentation is lacking, but what's lacking is a call out
that KVM disallows stuffing guest state that would violate the virtual CPU model.
I don't want to document a specific ordering of ioctls because then KVM would have
to enforce the ordering, e.g. would have to carry code to specifically reject
setting SREGS before CPUID, which would be a waste of code.

> I think the main difference here is probably that CR4 bits that are
> guarded by CPUID are more "advanced" than EFER bits?

Nah, the only "difference" is that it took us longer to notice that KVM wasn't
validating EFER.  Blame through KVM's history and you'll find the same bugs for
at least CR4.