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

[Sean Christopherson <seanjc@google.com>]

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.