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