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

[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

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*

[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.