[PATCH 0/4] KVM: x86: More EFER.LMA vs. CR4.PAE fixes

[PATCH 0/4] KVM: x86: More EFER.LMA vs. CR4.PAE fixes

[Sean Christopherson]

Revert a change I made years ago to allow userspace to stuff EFER before
configuring guest CPUID, as configuring CPUID and feature MSRs before anything
else is now a hard requirement (though I suspect I was just a bit overzealous
at the time).  The biggest problem with that change, other than creating what
should be impossible state, is that it also bypassed this check:

	if (is_paging(vcpu) &&
	    (vcpu->arch.efer & EFER_LME) != (efer & EFER_LME))
		return 1;

which was kinda sorta intentional?  I assume the idea was that userspace could
stuff CR0 vs. EFER in any order.  Except that in doing so, userspace could get
impossible state in CR0+CR4+EFER (in the aggregate, not in any one individual
register).  KVM doesn't advertise MSR_EFER as an MSR that needs to be
saved/restored, and AFAICT no VMM manually does that, they all just use
KVM_{G,S}ET_SREGS{,2} (which is what we want).

So, hit the "I'm feeling lucky!" button and simply revert the misguided
commit.

Patch 2 is more hardening.  Rather than pend a triple fault, which is flawed
because (a) KVM still yanks the vCPU out of L2 without going through a normal
nested VM-Exit and (b) there's a window where KVM can do stuff, e.g. init the
MMU, before the triple fault request is handled, immediately synthesize a #GP.
The only reason I didn't go with a #GP the first time around is that I didn't
want to bleed EFER_SVME information into the common set_efer(), KVM effectively
doesn't allow kvm_x86_call(set_efer)() to inject faults, and the vendor version
is used for many flows, whereas set_efer() is strictly used for WRMSR emulation.

Patch 3 is also more hardening, to try and detect+prevent EFER.LMA from going
stale.

Finally, update the EFER selftest to match the new #GP behavior.

Sean Christopherson (4):
  Revert "KVM: x86: Skip EFER vs. guest CPUID checks for host-initiated
    writes"
  KVM: nSVM: Synthesize #GP instead of SHUTDOWN if L2 attempts to clear
    EFER.SVME
  KVM: x86: Recompute EFER.LMA instead of inheriting the current value
    on EFER writes
  KVM: selftests: Update nSVM EFER test to expect #GP when clearing SVME
    in L2

 arch/x86/kvm/msrs.c                           | 54 ++++++++-------
 arch/x86/kvm/svm/svm.c                        | 15 ++---
 .../selftests/kvm/x86/svm_nested_efer_test.c  | 65 +++++++++++--------
 3 files changed, 74 insertions(+), 60 deletions(-)


base-commit: 164405e8e9a0378c7e1aff2254625a6553a291f1
-- 
2.56.0.257.gf7497c7d27-goog

[PATCH 1/4] Revert "KVM: x86: Skip EFER vs. guest CPUID checks for host-initiated writes"

[Sean Christopherson]

Revert the misguided-in-hindsight decision to allow userspace to violate
architectural consistency when setting EFER as it has since been solidified
that userspace is required to configure guest CPUID prior to setting MSRs,
and skipping the CR0.PG vs. EFER.LME check in allows userspace to load an
end state of EFER.LMA=1 && EFER.LME=0, which is supposed to be impossible
and trips a variety of KVM_BUG_ON()s in the MMU.

This reverts commit 11988499e62b310f3bf6f6d0a807a06d3f9ccc96.

Fixes: 11988499e62b ("KVM: x86: Skip EFER vs. guest CPUID checks for host-initiated writes")
Cc: stable@vger.kernel.org
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/msrs.c | 30 +++++++++---------------------
 1 file changed, 9 insertions(+), 21 deletions(-)

diff --git a/arch/x86/kvm/msrs.c b/arch/x86/kvm/msrs.c
index e91650af44c2..2df42276fd82 100644
--- a/arch/x86/kvm/msrs.c
+++ b/arch/x86/kvm/msrs.c
@@ -580,8 +580,11 @@ static int do_get_feature_msr(struct kvm_vcpu *vcpu, unsigned index, u64 *data)
 				 kvm_get_feature_msr);
 }
 
-static bool __kvm_valid_efer(struct kvm_vcpu *vcpu, u64 efer)
+bool kvm_valid_efer(struct kvm_vcpu *vcpu, u64 efer)
 {
+	if (efer & ~kvm_caps.supported_efer_bits)
+		return false;
+
 	if (efer & EFER_AUTOIBRS && !guest_cpu_cap_has(vcpu, X86_FEATURE_AUTOIBRS))
 		return false;
 
@@ -610,33 +613,18 @@ static bool __kvm_valid_efer(struct kvm_vcpu *vcpu, u64 efer)
 
 	return true;
 }
-
-bool kvm_valid_efer(struct kvm_vcpu *vcpu, u64 efer)
-{
-	if (efer & ~kvm_caps.supported_efer_bits)
-		return false;
-
-	return __kvm_valid_efer(vcpu, efer);
-}
 EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_valid_efer);
 
-static int set_efer(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
+static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
 {
 	u64 old_efer = vcpu->arch.efer;
-	u64 efer = msr_info->data;
 	int r;
 
-	if (efer & ~kvm_caps.supported_efer_bits)
+	if (!kvm_valid_efer(vcpu, efer))
 		return 1;
 
-	if (!msr_info->host_initiated) {
-		if (!__kvm_valid_efer(vcpu, efer))
-			return 1;
-
-		if (is_paging(vcpu) &&
-		    (vcpu->arch.efer & EFER_LME) != (efer & EFER_LME))
-			return 1;
-	}
+	if (is_paging(vcpu) && (vcpu->arch.efer & EFER_LME) != (efer & EFER_LME))
+		return 1;
 
 	efer &= ~EFER_LMA;
 	efer |= vcpu->arch.efer & EFER_LMA;
@@ -1547,7 +1535,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		wrmsrq(MSR_IA32_FLUSH_CMD, L1D_FLUSH);
 		break;
 	case MSR_EFER:
-		return set_efer(vcpu, msr_info);
+		return set_efer(vcpu, msr_info->data);
 	case MSR_K7_HWCR: {
 		/*
 		 * Allow McStatusWrEn and TscFreqSel. (Linux guests from v3.2
-- 
2.56.0.257.gf7497c7d27-goog

[PATCH 2/4] KVM: nSVM: Synthesize #GP instead of SHUTDOWN if L2 attempts to clear EFER.SVME

[Sean Christopherson]

If L2 attempts to clear EFER.SVME (because L1 neglected to intercept writes
to EFER), synthesize a #GP instead of a triple-fault SHUTDOWN for L1, as
yanking the vCPU out of L2 and mixing the new (invalid) EFER with L2's
current state is unnecessarily risky for KVM since the TRIPLE_FAULT request
isn't fully handled until KVM attempts to enter the guest, and nothing
prevents userspace from ignoring the SHUTDOWN exit and continue running the
guest.

Apply the restriction to both guest and host (a.k.a. userspace) writes, as
unlike kvm_x86_ops.set_efer() => svm_set_efer(), set_efer() is used *only*
for targeted MSR writes, e.g. not for KVM_SET_SREGS{,2}, and KVM does NOT
advertise EFER as an MSR that needs to be saved (because it's handled via
sregs).  I.e. there is far less chance of breaking userspace that manually
restores EFER with SVME=0 while L2 is active, because in all likelihood no
such VMM exists.

Keep the sanity check in svm_set_efer() that handles the KVM_RUN case, but
escalate it to a KVM_BUG_ON() + -EIO, as it should now be unreachable.

Cc: stable@vger.kernel.org
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/msrs.c    | 12 ++++++++++++
 arch/x86/kvm/svm/svm.c | 15 +++++----------
 2 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kvm/msrs.c b/arch/x86/kvm/msrs.c
index 2df42276fd82..c3cb1575fe66 100644
--- a/arch/x86/kvm/msrs.c
+++ b/arch/x86/kvm/msrs.c
@@ -626,6 +626,18 @@ static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
 	if (is_paging(vcpu) && (vcpu->arch.efer & EFER_LME) != (efer & EFER_LME))
 		return 1;
 
+	/*
+	 * Architecturally (per the APM), clearing EFER.SVME while a guest is
+	 * running yields undefined behavior, i.e. KVM can do literally
+	 * anything.  Synthesize a #GP as that is the safest option for KVM,
+	 * as mixing the current vCPU state (from L2) with L1 and the new EFER
+	 * (if KVM simply forces the vCPU out of L2) may yield an unexpected
+	 * end result.
+	 */
+	if (is_guest_mode(vcpu) &&
+	    (vcpu->arch.efer & EFER_SVME) && !(efer & EFER_SVME))
+		return 1;
+
 	efer &= ~EFER_LMA;
 	efer |= vcpu->arch.efer & EFER_LMA;
 
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 487897ef11dd..a6ff9635677a 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -227,17 +227,12 @@ int svm_set_efer(struct kvm_vcpu *vcpu, u64 efer)
 	if ((old_efer & EFER_SVME) != (efer & EFER_SVME)) {
 		if (!(efer & EFER_SVME)) {
 			/*
-			 * Architecturally, clearing EFER.SVME while a guest is
-			 * running yields undefined behavior, i.e. KVM can do
-			 * literally anything.  Force the vCPU back into L1 as
-			 * that is the safest option for KVM, but synthesize a
-			 * triple fault (for L1!) so that KVM at least doesn't
-			 * run random L2 code in the context of L1.  Do so if
-			 * and only if the vCPU is actively running, e.g. to
-			 * avoid positives if userspace is stuffing state.
+			 * KVM is supposed to synthesize #GP if L2 attempts to
+			 * clear SVME.
 			 */
-			if (is_guest_mode(vcpu) && vcpu->wants_to_run)
-				kvm_make_request(KVM_REQ_TRIPLE_FAULT, vcpu);
+			if (KVM_BUG_ON(is_guest_mode(vcpu) &&
+				       vcpu->wants_to_run, vcpu->kvm))
+				return -EIO;
 
 			svm_leave_nested(vcpu);
 			/* #GP intercept is still needed for vmware backdoor */
-- 
2.56.0.257.gf7497c7d27-goog

[PATCH 3/4] KVM: x86: Recompute EFER.LMA instead of inheriting the current value on EFER writes

[Sean Christopherson]

Recompute EFER.LMA on bare writes to EFER (via WRMSR or KVM_SET_MSRS)
instead of carrying forward the current EFER.LMA.  While it _should_ be
impossible for EFER.LMA to end up in an inconsistent state on such a write,
KVM has had a rash of exactly those bugs, mostly because SVM is quite lax
when it comes to consistenty checks, which in turn bleeds into KVM's nested
SVM implementation.  Retaining the existing EFER is also rather confusing,
as understanding why keeping EFER.LMA is correct requires a lot of a priori
knowledge of both x86-64 and KVM.

Keep the core concept of the existing logic as a sanity check: WARN and
reject the write if the computed EFER.LMA would be toggled, because as
described above, that _should_ be impossible.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/msrs.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/msrs.c b/arch/x86/kvm/msrs.c
index c3cb1575fe66..41f3ec6eb286 100644
--- a/arch/x86/kvm/msrs.c
+++ b/arch/x86/kvm/msrs.c
@@ -638,8 +638,18 @@ static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
 	    (vcpu->arch.efer & EFER_SVME) && !(efer & EFER_SVME))
 		return 1;
 
-	efer &= ~EFER_LMA;
-	efer |= vcpu->arch.efer & EFER_LMA;
+	if ((efer & EFER_LME) && is_paging(vcpu) && is_pae(vcpu))
+		efer |= EFER_LMA;
+	else
+		efer &= ~EFER_LMA;
+
+	/*
+	 * Toggling EFER.LMA on a bare write to EFER should be impossible, as
+	 * it means KVM allowed architecturally invalid state at some point in
+	 * the past, i.e. allowed EFER.LMA=1 && (!CR0.PG || !CR4.PAE).
+	 */
+	if (WARN_ON_ONCE((efer & EFER_LMA) != (vcpu->arch.efer & EFER_LMA)))
+		return 1;
 
 	r = kvm_x86_call(set_efer)(vcpu, efer);
 	if (r) {
-- 
2.56.0.257.gf7497c7d27-goog

[PATCH 4/4] KVM: selftests: Update nSVM EFER test to expect #GP when clearing SVME in L2

[Sean Christopherson]

Adjust the nSVM EFER test to expect a #GP on WRMSR if L2 attempts to clear
EFER.SVME to match KVM's new behavior (synthesizing #GP is safer for KVM as
it doesn't require yanking the vCPU out of L2, and is fully synchronous).

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 .../selftests/kvm/x86/svm_nested_efer_test.c  | 65 +++++++++++--------
 1 file changed, 37 insertions(+), 28 deletions(-)

diff --git a/tools/testing/selftests/kvm/x86/svm_nested_efer_test.c b/tools/testing/selftests/kvm/x86/svm_nested_efer_test.c
index fa0d76a9c390..f612146d23ac 100644
--- a/tools/testing/selftests/kvm/x86/svm_nested_efer_test.c
+++ b/tools/testing/selftests/kvm/x86/svm_nested_efer_test.c
@@ -15,13 +15,18 @@ static bool l2_ran;
 static void l2_clear_efer_svme(void)
 {
 	u64 efer = rdmsr(MSR_EFER);
+	u8 vector;
 
 	/* generic_svm_setup() initializes EFER_SVME set for L2 */
 	GUEST_ASSERT(efer & EFER_SVME);
-	wrmsr(MSR_EFER, efer & ~EFER_SVME);
 
-	/* Unreachable, L1 should be shutdown */
-	GUEST_ASSERT(0);
+	vector = wrmsr_safe(MSR_EFER, efer & ~EFER_SVME);
+	__GUEST_ASSERT(vector == GP_VECTOR,
+		       "Expected #GP when clearing EFER.LME in L2, got %s",
+		       ex_str(vector));
+
+	l2_ran = true;
+	vmmcall();
 }
 
 static void l1_clear_efer_svme(struct svm_test_data *svm)
@@ -29,8 +34,7 @@ static void l1_clear_efer_svme(struct svm_test_data *svm)
 	generic_svm_setup(svm, l2_clear_efer_svme);
 	run_guest(svm->vmcb, svm->vmcb_gpa);
 
-	/* Unreachable, L1 should be shutdown */
-	GUEST_ASSERT(0);
+	GUEST_DONE();
 }
 
 static void l2_lmsle(void)
@@ -130,31 +134,10 @@ static void test_enumeration(void)
 	ksft_test_result_pass("KVM enumerates EFER_LMSLE_MBZ=%d\n", lmsle_mbz);
 }
 
-static void test_clear_efer_svme(void)
+static void test_efer_in_guest(struct kvm_vcpu *vcpu)
 {
-	struct kvm_vcpu *vcpu;
-	struct kvm_vm *vm;
-
-	vcpu = create_l1_vcpu(&vm, l1_clear_efer_svme);
-
-	vcpu_run(vcpu);
-	TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_SHUTDOWN);
-
-	kvm_vm_free(vm);
-	ksft_test_result_pass("L2 clearing EFER.SVME shuts down L1\n");
-}
-
-static void test_lmsle(bool lmsle_mbz)
-{
-	struct kvm_vcpu *vcpu;
-	struct kvm_vm *vm;
 	struct ucall uc;
 
-	vcpu = create_l1_vcpu(&vm, l1_lmsle);
-
-	vcpu_set_or_clear_cpuid_feature(vcpu, X86_FEATURE_EFER_LMSLE_MBZ,
-					lmsle_mbz);
-
 	vcpu_run(vcpu);
 	TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_IO);
 
@@ -166,6 +149,32 @@ static void test_lmsle(bool lmsle_mbz)
 	default:
 		TEST_FAIL("Unexpected ucall: %lu", uc.cmd);
 	}
+}
+
+static void test_clear_efer_svme(void)
+{
+	struct kvm_vcpu *vcpu;
+	struct kvm_vm *vm;
+
+	vcpu = create_l1_vcpu(&vm, l1_clear_efer_svme);
+
+	test_efer_in_guest(vcpu);
+
+	kvm_vm_free(vm);
+	ksft_test_result_pass("L2 clearing EFER.SVME gets #GP\n");
+}
+
+static void test_lmsle(bool lmsle_mbz)
+{
+	struct kvm_vcpu *vcpu;
+	struct kvm_vm *vm;
+
+	vcpu = create_l1_vcpu(&vm, l1_lmsle);
+
+	vcpu_set_or_clear_cpuid_feature(vcpu, X86_FEATURE_EFER_LMSLE_MBZ,
+					lmsle_mbz);
+
+	test_efer_in_guest(vcpu);
 
 	kvm_vm_free(vm);
 	ksft_test_result_pass("Guest EFER_LMSLE_MBZ=%d\n", lmsle_mbz);
@@ -248,7 +257,7 @@ int main(int argc, char *argv[])
 		test_clear_efer_svme();
 		test_lmsle(true);
 	} else {
-		ksft_test_result_skip("L2 clearing EFER.SVME shuts down L1\n");
+		ksft_test_result_skip("L2 clearing EFER.SVME gets #GP\n");
 		ksft_test_result_skip("Guest EFER_LMSLE_MBZ=1\n");
 	}
 
-- 
2.56.0.257.gf7497c7d27-goog

Re: [PATCH 1/4] Revert "KVM: x86: Skip EFER vs. guest CPUID checks for host-initiated writes"

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] kvm: x86: set_efer() revert breaks test_host_initiated_lmsle() and api.rst
--

commit f8fc3a0796b0b21e716b9ed0c1fcf3c9297d569f
Author: Sean Christopherson <seanjc@google.com>

Revert "KVM: x86: Skip EFER vs. guest CPUID checks for host-initiated writes"

This commit reverts commit 11988499e62b ("KVM: x86: Skip EFER vs. guest CPUID
checks for host-initiated writes") so that set_efer() unconditionally validates
EFER against guest CPUID capabilities and CR0.PG even for host-initiated
MSR_EFER writes.

> diff --git a/arch/x86/kvm/msrs.c b/arch/x86/kvm/msrs.c
> index e91650af44c20..2df42276fd826 100644
> --- a/arch/x86/kvm/msrs.c
> +++ b/arch/x86/kvm/msrs.c
> @@ -580,8 +580,11 @@ static int do_get_feature_msr(struct kvm_vcpu *vcpu, unsigned index, u64 *data)
>  				 kvm_get_feature_msr);
>  }
>  
> -static bool __kvm_valid_efer(struct kvm_vcpu *vcpu, u64 efer)
> +bool kvm_valid_efer(struct kvm_vcpu *vcpu, u64 efer)
>  {
> +	if (efer & ~kvm_caps.supported_efer_bits)
> +		return false;
> +
>  	if (efer & EFER_AUTOIBRS && !guest_cpu_cap_has(vcpu, X86_FEATURE_AUTOIBRS))
>  		return false;
>  
> @@ -610,33 +613,18 @@ static bool __kvm_valid_efer(struct kvm_vcpu *vcpu, u64 efer)
>  
>  	return true;
>  }
> -
> -bool kvm_valid_efer(struct kvm_vcpu *vcpu, u64 efer)
> -{
> -	if (efer & ~kvm_caps.supported_efer_bits)
> -		return false;
> -
> -	return __kvm_valid_efer(vcpu, efer);
> -}
>  EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_valid_efer);
>  
> -static int set_efer(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> +static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
>  {
>  	u64 old_efer = vcpu->arch.efer;
> -	u64 efer = msr_info->data;
>  	int r;
>  
> -	if (efer & ~kvm_caps.supported_efer_bits)
> +	if (!kvm_valid_efer(vcpu, efer))
>  		return 1;
>  
> -	if (!msr_info->host_initiated) {
> -		if (!__kvm_valid_efer(vcpu, efer))
> -			return 1;
> -
> -		if (is_paging(vcpu) &&
> -		    (vcpu->arch.efer & EFER_LME) != (efer & EFER_LME))
> -			return 1;
> -	}
> +	if (is_paging(vcpu) && (vcpu->arch.efer & EFER_LME) != (efer & EFER_LME))
> +		return 1;

[Severity: Medium]
Does reverting this check break test_host_initiated_lmsle() in
tools/testing/selftests/kvm/x86/svm_nested_efer_test.c and leave
Documentation/virt/kvm/api.rst out of date?

Commit 7358d13e1f19 ("KVM: x86: Honor the guest's EFER_LMSLE_MBZ") and commit
34f749161dab ("KVM: selftests: Add coverage for the EFER_LMSLE_MBZ defeature")
were added after commit 11988499e62b ("KVM: x86: Skip EFER vs. guest CPUID
checks for host-initiated writes"), when set_efer() skipped __kvm_valid_efer()
for host-initiated writes.

With this revert, host-initiated MSR_EFER writes via KVM_SET_MSRS take the path
kvm_arch_vcpu_ioctl() -> msr_io() -> kvm_set_msr() -> kvm_set_msr_common() ->
set_efer() -> kvm_valid_efer(), which rejects EFER_LMSLE when
X86_FEATURE_EFER_LMSLE_MBZ is set in guest CPUID:

arch/x86/kvm/msrs.c:kvm_valid_efer() {
	...
	if (efer & EFER_LMSLE &&
	    guest_cpu_cap_has(vcpu, X86_FEATURE_EFER_LMSLE_MBZ))
		return false;
}

On AMD SVM hosts that support EFER.LMSLE, test_host_initiated_lmsle() in
tools/testing/selftests/kvm/x86/svm_nested_efer_test.c sets
X86_FEATURE_EFER_LMSLE_MBZ and then asserts that a host-initiated write of
EFER_LMSLE succeeds:

tools/testing/selftests/kvm/x86/svm_nested_efer_test.c:test_host_initiated_lmsle() {
	...
	vcpu_set_cpuid_feature(vcpu, X86_FEATURE_EFER_LMSLE_MBZ);
	...
	vcpu_set_msr(vcpu, MSR_EFER, efer | EFER_LMSLE);
	TEST_ASSERT_EQ(vcpu_get_msr(vcpu, MSR_EFER), efer | EFER_LMSLE);
}

Because set_efer() now returns 1 on that write, vcpu_set_msr() fails and aborts
the selftest (and neither this patch nor follow-up patch 4/4 in the series
updates test_host_initiated_lmsle() or the related comment in
test_sregs_lmsle()).

In addition, Documentation/virt/kvm/api.rst under "Long mode segment limits"
still states:

  Note, ``KVM_SET_MSRS`` is exempt from the check, as host-initiated MSR writes
  skip guest CPUID checks so that userspace can set MSRs before it sets guest
  CPUID.  ``KVM_SET_SREGS`` and nested VMRUN are not exempt.

And efer_trap() in arch/x86/kvm/svm/svm.c still references the removed
__kvm_valid_efer() helper in its comment:

  * Clear the EFER_SVME bit from EFER. The SVM code always sets this
  * bit in svm_set_efer(), but __kvm_valid_efer() checks it against
  * whether the guest has X86_FEATURE_SVM - this avoids a failure if
  * the guest doesn't have X86_FEATURE_SVM.

Should test_host_initiated_lmsle(), Documentation/virt/kvm/api.rst, and the
comment in efer_trap() be updated to match the restored set_efer() behavior?

[ ... ]

>
>  	efer &= ~EFER_LMA;
>  	efer |= vcpu->arch.efer & EFER_LMA;

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022223052.646891-1-seanjc@google.com?part=1

Re: [PATCH 2/4] KVM: nSVM: Synthesize #GP instead of SHUTDOWN if L2 attempts to clear EFER.SVME

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] kvm: userspace-reachable KVM_BUG_ON() in svm_set_efer() via KVM_SYNC_X86_SREGS
--

commit fd2bbc971d026eddf8121873792d458775b0e3cb
Author: Sean Christopherson <seanjc@google.com>

KVM: nSVM: Synthesize #GP instead of SHUTDOWN if L2 attempts to clear EFER.SVME

This commit synthesizes a #GP in set_efer() when clearing EFER_SVME while
in guest mode instead of forcing a triple fault shutdown in svm_set_efer().
It also converts the check in svm_set_efer() for clearing EFER_SVME while
is_guest_mode(vcpu) and vcpu->wants_to_run are true into a KVM_BUG_ON()
returning -EIO.

> diff --git a/arch/x86/kvm/msrs.c b/arch/x86/kvm/msrs.c
> index 2df42276fd826..c3cb1575fe66e 100644
> --- a/arch/x86/kvm/msrs.c
> +++ b/arch/x86/kvm/msrs.c
> @@ -626,6 +626,18 @@ static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
>  	if (is_paging(vcpu) && (vcpu->arch.efer & EFER_LME) != (efer & EFER_LME))
>  		return 1;
>  
> +	/*
> +	 * Architecturally (per the APM), clearing EFER.SVME while a guest is
> +	 * running yields undefined behavior, i.e. KVM can do literally
> +	 * anything.  Synthesize a #GP as that is the safest option for KVM,
> +	 * as mixing the current vCPU state (from L2) with L1 and the new EFER
> +	 * (if KVM simply forces the vCPU out of L2) may yield an unexpected
> +	 * end result.
> +	 */
> +	if (is_guest_mode(vcpu) &&
> +	    (vcpu->arch.efer & EFER_SVME) && !(efer & EFER_SVME))
> +		return 1;
> +
>  	efer &= ~EFER_LMA;
>  	efer |= vcpu->arch.efer & EFER_LMA;
>  
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 487897ef11dd6..a6ff9635677a1 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -227,17 +227,12 @@ int svm_set_efer(struct kvm_vcpu *vcpu, u64 efer)
>  	if ((old_efer & EFER_SVME) != (efer & EFER_SVME)) {
>  		if (!(efer & EFER_SVME)) {
>  			/*
> -			 * Architecturally, clearing EFER.SVME while a guest is
> -			 * running yields undefined behavior, i.e. KVM can do
> -			 * literally anything.  Force the vCPU back into L1 as
> -			 * that is the safest option for KVM, but synthesize a
> -			 * triple fault (for L1!) so that KVM at least doesn't
> -			 * run random L2 code in the context of L1.  Do so if
> -			 * and only if the vCPU is actively running, e.g. to
> -			 * avoid positives if userspace is stuffing state.
> +			 * KVM is supposed to synthesize #GP if L2 attempts to
> +			 * clear SVME.
>  			 */
> -			if (is_guest_mode(vcpu) && vcpu->wants_to_run)
> -				kvm_make_request(KVM_REQ_TRIPLE_FAULT, vcpu);
> +			if (KVM_BUG_ON(is_guest_mode(vcpu) &&
> +				       vcpu->wants_to_run, vcpu->kvm))
> +				return -EIO;
>  
>  			svm_leave_nested(vcpu);
>  			/* #GP intercept is still needed for vmware backdoor */

[Severity: High]
Can userspace still reach this KVM_BUG_ON() during KVM_RUN via
KVM_SYNC_X86_SREGS when a vCPU is in guest mode?

In virt/kvm/kvm_main.c:kvm_vcpu_ioctl(), vcpu->wants_to_run is set before
calling kvm_arch_vcpu_ioctl_run():

	vcpu->wants_to_run = !READ_ONCE(vcpu->run->immediate_exit__unsafe);
	r = kvm_arch_vcpu_ioctl_run(vcpu);
	vcpu->wants_to_run = false;

Then arch/x86/kvm/x86.c:kvm_arch_vcpu_ioctl_run() syncs dirty registers
from userspace while vcpu->wants_to_run is already true:

	if (kvm_run->kvm_dirty_regs) {
		r = sync_regs(vcpu);
		if (r != 0)
			goto out;
	}

When KVM_SYNC_X86_SREGS is set in kvm_dirty_regs, the call chain
sync_regs() -> kvm_run_sync_regs_from_user() -> __set_sregs() ->
__set_sregs_common() in arch/x86/kvm/regs.c bypasses set_efer() in
arch/x86/kvm/msrs.c and calls svm_set_efer() directly:

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

If userspace clears EFER_SVME in vcpu->run->s.regs.sregs.efer while
is_guest_mode(vcpu) is true, wouldn't svm_set_efer() trigger the
KVM_BUG_ON() and return -EIO (which __set_sregs_common() ignores) after
already updating vcpu->arch.efer = efer and without calling
svm_leave_nested(vcpu)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022223052.646891-1-seanjc@google.com?part=2

Re: [PATCH 3/4] KVM: x86: Recompute EFER.LMA instead of inheriting the current value on EFER writes

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] KVM: x86: guest-triggerable WARN_ON_ONCE and #GP in set_efer()
--

commit f0c2fa127e8b22e38db4ba12c6a113c1f2ed48a1
Author: Sean Christopherson <seanjc@google.com>

KVM: x86: Recompute EFER.LMA instead of inheriting the current value on EFER writes

This commit recomputes EFER.LMA from EFER.LME, CR0.PG, and CR4.PAE in
set_efer() instead of inheriting the existing EFER.LMA bit from
vcpu->arch.efer. It also adds a WARN_ON_ONCE() check that rejects the write
if the recomputed EFER.LMA differs from the current EFER.LMA in
vcpu->arch.efer.

> diff --git a/arch/x86/kvm/msrs.c b/arch/x86/kvm/msrs.c
> index c3cb1575fe66e..41f3ec6eb2866 100644
> --- a/arch/x86/kvm/msrs.c
> +++ b/arch/x86/kvm/msrs.c
> @@ -638,8 +638,18 @@ static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
>  	    (vcpu->arch.efer & EFER_SVME) && !(efer & EFER_SVME))
>  		return 1;
>  
> -	efer &= ~EFER_LMA;
> -	efer |= vcpu->arch.efer & EFER_LMA;
> +	if ((efer & EFER_LME) && is_paging(vcpu) && is_pae(vcpu))
> +		efer |= EFER_LMA;
> +	else
> +		efer &= ~EFER_LMA;
> +
> +	/*
> +	 * Toggling EFER.LMA on a bare write to EFER should be impossible, as
> +	 * it means KVM allowed architecturally invalid state at some point in
> +	 * the past, i.e. allowed EFER.LMA=1 && (!CR0.PG || !CR4.PAE).
> +	 */
> +	if (WARN_ON_ONCE((efer & EFER_LMA) != (vcpu->arch.efer & EFER_LMA)))
> +		return 1;

[Severity: High]
Can a guest or host userspace trigger this WARN_ON_ONCE() and get a spurious
#GP (or failed RSM / KVM_SET_MSRS) via SVM paths that still leave EFER_LMA
in vcpu->arch.efer out of sync with (EFER_LME && CR0.PG && CR4.PAE)?

Earlier in arch/x86/kvm/msrs.c:set_efer(), KVM rejects changing EFER_LME
while paging is enabled:

arch/x86/kvm/msrs.c:set_efer() {
	...
	if (is_paging(vcpu) && (vcpu->arch.efer & EFER_LME) != (efer & EFER_LME))
		return 1;
	...
}

Because of that check, the recomputed EFER_LMA in set_efer() is always equal
to ((vcpu->arch.efer & EFER_LME) && is_paging(vcpu) && is_pae(vcpu)). The new
WARN_ON_ONCE() therefore triggers whenever a vCPU enters set_efer() with
vcpu->arch.efer & EFER_LMA out of sync with those bits.

There appear to be three SVM paths where that still happens:

1. Nested SVM VMRUN and RSM into L2:

Architecturally on SVM, VMRUN ignores EFER.LMA in the VMCB save area and sets
EFER.LMA from (EFER.LME && CR0.PG) in hardware, so nested_vmcb_check_save()
does not require save->efer & EFER_LMA to match (EFER_LME && CR0.PG):

arch/x86/kvm/svm/nested.c:nested_vmcb_check_save() {
	...
	/*
	 * These checks are also performed by KVM_SET_SREGS,
	 * except that EFER.LMA is not checked by SVM against
	 * CR0.PG && EFER.LME.
	 */
	if ((save->efer & EFER_LME) && (save->cr0 & X86_CR0_PG)) {
		if (CC(!(save->cr4 & X86_CR4_PAE)) ||
		    CC(!(save->cr0 & X86_CR0_PE)) ||
		    CC(!kvm_vcpu_is_legal_cr3(vcpu, save->cr3)))
			return false;
	...
}

However, nested_vmcb02_prepare_save() only clears EFER_LMA when EFER_LME is
0, and relies on svm_set_cr0() to update EFER_LMA:

arch/x86/kvm/svm/nested.c:nested_vmcb02_prepare_save() {
	...
	/* SVM ignores EFER.LMA if EFER.LME=0 (instead of failing VMRUN). */
	if (!(svm->nested.save.efer & EFER_LME))
		svm->nested.save.efer &= ~EFER_LMA;

	svm_set_efer(vcpu, svm->nested.save.efer);

	svm_set_cr0(vcpu, svm->nested.save.cr0);
	svm_set_cr4(vcpu, svm->nested.save.cr4);
	...
}

And svm_set_cr0() only updates EFER_LMA in vcpu->arch.efer when CR0.PG
transitions relative to the previous vcpu->arch.cr0:

arch/x86/kvm/svm/svm.c:svm_set_cr0() {
	...
	if (vcpu->arch.efer & EFER_LME) {
		if (!is_paging(vcpu) && (cr0 & X86_CR0_PG)) {
			vcpu->arch.efer |= EFER_LMA;
			if (!vcpu->arch.guest_state_protected)
				svm->vmcb->save.efer |= EFER_LMA | EFER_LME;
		}

		if (is_paging(vcpu) && !(cr0 & X86_CR0_PG)) {
			vcpu->arch.efer &= ~EFER_LMA;
			if (!vcpu->arch.guest_state_protected)
				svm->vmcb->save.efer &= ~(EFER_LMA | EFER_LME);
		}
	}
	...
}

Because L1 must have CR0.PG=1 to execute VMRUN, is_paging(vcpu) is already
true when svm_set_cr0() runs during VMRUN. If vmcb12 has CR0.PG=1, CR4.PAE=1,
EFER.LME=1, and EFER.LMA=0, svm_set_cr0() never sets EFER_LMA, leaving L2
running in 64-bit mode with EFER_LMA=0 in vcpu->arch.efer. When L2 executes a
valid WRMSR(MSR_EFER) (for example setting EFER_NX or EFER_SCE) or userspace
calls KVM_SET_MSRS, set_efer() computes EFER_LMA=1, triggers WARN_ON_ONCE(),
and injects a #GP into L2.

Conversely, during RSM to L2 via emulator_leave_smm() -> svm_leave_smm() ->
enter_svm_guest_mode() -> nested_vmcb02_prepare_save(), emulator_leave_smm()
clears CR0.PG before calling svm_leave_smm(). If vmcb12 has EFER.LME=1,
EFER.LMA=1, and CR0.PG=0, is_paging(vcpu) is already false when svm_set_cr0()
runs, leaving EFER_LMA=1 with CR0.PG=0. When rsm_load_state_64() then calls
__kvm_emulate_msr_write(vcpu, MSR_EFER, smstate->efer & ~EFER_LMA) ->
set_efer(), set_efer() computes EFER_LMA=0, triggers WARN_ON_ONCE(), and
aborts RSM with X86EMUL_UNHANDLEABLE.

2. KVM_SET_NESTED_STATE and svm_leave_smm() followed by #VMEXIT:

In arch/x86/kvm/svm/nested.c:svm_set_nested_state(), L1's saved host state in
vmcb01->save is only checked for ((save->efer & EFER_LMA) &&
!(save->efer & EFER_LME)) (and svm_leave_smm() copies vmcb01->save from
guest MSR_VM_HSAVE_PA memory without checking EFER_LMA):

arch/x86/kvm/svm/nested.c:svm_set_nested_state() {
	...
	if (!(save->cr0 & X86_CR0_PG) ||
	    !(save->cr0 & X86_CR0_PE) ||
	    (save->rflags & X86_EFLAGS_VM) ||
	    ((save->efer & EFER_LMA) && !(save->efer & EFER_LME)) ||
	    !nested_vmcb_check_save(vcpu, &save_cached, false))
		goto out_free;
	...
}

If vmcb01->save has EFER.LME=1, EFER.LMA=0, CR0.PG=1, and CR4.PAE=1, then on
nested_svm_vmexit() from a paged L2 (where is_paging(vcpu) is already true),
svm_set_efer() and svm_set_cr0() leave L1 with EFER_LMA=0 while EFER_LME=1,
CR0.PG=1, and CR4.PAE=1, causing the next WRMSR(MSR_EFER) or KVM_SET_MSRS to
trigger the WARN_ON_ONCE() in set_efer().

3. SEV-ES and SEV-SNP write traps:

In arch/x86/kvm/svm/svm.c:cr_trap(), SVM_EXIT_CR0_WRITE_TRAP and
SVM_EXIT_CR4_WRITE_TRAP call svm_set_cr0() and svm_set_cr4() directly without
checking consistency between EFER.LMA, CR0.PG, and CR4.PAE:

arch/x86/kvm/svm/svm.c:cr_trap() {
	...
	case 0:
		old_value = kvm_read_cr0(vcpu);
		svm_set_cr0(vcpu, new_value);

		kvm_post_set_cr0(vcpu, old_value, new_value);
		break;
	case 4:
		old_value = kvm_read_cr4(vcpu);
		svm_set_cr4(vcpu, new_value);

		kvm_post_set_cr4(vcpu, old_value, new_value);
		break;
	...
}

Because svm_set_cr0() sets EFER_LMA without checking is_pae(vcpu) (for
example on an SEV-SNP AP created via SVM_VMGEXIT_AP_CREATE where
sev_snp_init_protected_guest_state() calls kvm_vcpu_reset() and clears
vcpu->arch.cr4 while the VMSA has CR4.PAE=1), or if a guest clears CR4.PAE
observed via cr_trap() while EFER_LMA=1, a subsequent efer_trap() ->
set_efer() sees EFER_LMA=1 with !is_pae(vcpu) and triggers the WARN_ON_ONCE()
and #GP.

>  
>  	r = kvm_x86_call(set_efer)(vcpu, efer);
>  	if (r) {

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022223052.646891-1-seanjc@google.com?part=3

Re: [PATCH 4/4] KVM: selftests: Update nSVM EFER test to expect #GP when clearing SVME in L2

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] KVM: selftests: l1_clear_efer_svme() misses L2 exit and completion checks
- [Medium] KVM: selftests: test_host_initiated_lmsle() not updated for EFER CPUID check
- [Low] KVM: selftests: l2_clear_efer_svme() assert message says EFER.LME not SVME
--

commit bfd1afa646e17ebc8c26cbfd62dcc99d73a0c1f3
Author: Sean Christopherson <seanjc@google.com>

KVM: selftests: Update nSVM EFER test to expect #GP when clearing SVME in L2

This commit updates svm_nested_efer_test to expect a #GP fault in L2 when
clearing EFER.SVME via WRMSR instead of a triple-fault shutdown of L1, and
factors out a helper to run the guest EFER test cases.

> diff --git a/tools/testing/selftests/kvm/x86/svm_nested_efer_test.c b/tools/testing/selftests/kvm/x86/svm_nested_efer_test.c
> index fa0d76a9c390b..f612146d23ac2 100644
> --- a/tools/testing/selftests/kvm/x86/svm_nested_efer_test.c
> +++ b/tools/testing/selftests/kvm/x86/svm_nested_efer_test.c
> @@ -15,13 +15,18 @@ static bool l2_ran;
>  static void l2_clear_efer_svme(void)
>  {
>  	u64 efer = rdmsr(MSR_EFER);
> +	u8 vector;
>  
>  	/* generic_svm_setup() initializes EFER_SVME set for L2 */
>  	GUEST_ASSERT(efer & EFER_SVME);
> -	wrmsr(MSR_EFER, efer & ~EFER_SVME);
>  
> -	/* Unreachable, L1 should be shutdown */
> -	GUEST_ASSERT(0);
> +	vector = wrmsr_safe(MSR_EFER, efer & ~EFER_SVME);
> +	__GUEST_ASSERT(vector == GP_VECTOR,
> +		       "Expected #GP when clearing EFER.LME in L2, got %s",
> +		       ex_str(vector));

[Severity: Low]
In l2_clear_efer_svme(), should the __GUEST_ASSERT() format string say
EFER.SVME instead of EFER.LME when wrmsr_safe(MSR_EFER, efer & ~EFER_SVME)
returns a vector other than GP_VECTOR?

> +
> +	l2_ran = true;
> +	vmmcall();
>  }
>  
>  static void l1_clear_efer_svme(struct svm_test_data *svm)
> @@ -29,8 +34,7 @@ static void l1_clear_efer_svme(struct svm_test_data *svm)
>  	generic_svm_setup(svm, l2_clear_efer_svme);
>  	run_guest(svm->vmcb, svm->vmcb_gpa);
>  
> -	/* Unreachable, L1 should be shutdown */
> -	GUEST_ASSERT(0);
> +	GUEST_DONE();
>  }

[Severity: Medium]
When test_clear_efer_svme() runs l1_clear_efer_svme(), run_guest() enters
l2_clear_efer_svme(), which sets l2_ran = true and executes vmmcall().
However, when run_guest() returns to l1_clear_efer_svme(), it immediately
calls GUEST_DONE() without checking l2_ran or svm->vmcb->control.exit_code
(unlike l1_lmsle()):

tools/testing/selftests/kvm/x86/svm_nested_efer_test.c:l1_lmsle() {
	...
	} else {
		GUEST_ASSERT_EQ(vmcb->control.exit_code, SVM_EXIT_VMMCALL);
		GUEST_ASSERT(l2_ran);
		GUEST_ASSERT(vmcb->save.efer & EFER_LMSLE);
	}
}

If VMRUN fails or if clearing EFER.SVME in L2 triggers a nested VM-Exit to L1
instead of injecting #GP into L2, wouldn't run_guest() return to
l1_clear_efer_svme() with l2_ran == false and still pass the test? Also,
should l2_clear_efer_svme() re-read MSR_EFER after wrmsr_safe() to verify
that EFER_SVME remained set?

[ ... ]

> @@ -248,7 +257,7 @@ int main(int argc, char *argv[])
>  		test_clear_efer_svme();
>  		test_lmsle(true);
>  	} else {
> -		ksft_test_result_skip("L2 clearing EFER.SVME shuts down L1\n");
> +		ksft_test_result_skip("L2 clearing EFER.SVME gets #GP\n");
>  		ksft_test_result_skip("Guest EFER_LMSLE_MBZ=1\n");
>  	}
>  

[Severity: Medium]
Earlier in this series, commit f8fc3a0796b0 ("Revert \"KVM: x86: Skip EFER
vs. guest CPUID checks for host-initiated writes\"") updated set_efer() in
arch/x86/kvm/msrs.c to enforce kvm_valid_efer() unconditionally on all
MSR_EFER writes, including host-initiated writes:

arch/x86/kvm/msrs.c:set_efer() {
	...
	if (!kvm_valid_efer(vcpu, efer))
		return 1;
	...
}

Because kvm_valid_efer() rejects EFER_LMSLE when X86_FEATURE_EFER_LMSLE_MBZ
is set in guest CPUID, won't main() -> test_host_initiated_lmsle() in
tools/testing/selftests/kvm/x86/svm_nested_efer_test.c now fail on hosts
where has_lmsle is true?

tools/testing/selftests/kvm/x86/svm_nested_efer_test.c:test_host_initiated_lmsle() {
	...
	vcpu_set_cpuid_feature(vcpu, X86_FEATURE_EFER_LMSLE_MBZ);
	...
	efer = vcpu_get_msr(vcpu, MSR_EFER);
	TEST_ASSERT(!(efer & EFER_LMSLE), "EFER.LMSLE unexpectedly set");

	vcpu_set_msr(vcpu, MSR_EFER, efer | EFER_LMSLE);
	...
}

Does test_host_initiated_lmsle() also need to be updated or removed to expect
host-initiated KVM_SET_MSRS writes to reject EFER_LMSLE when
X86_FEATURE_EFER_LMSLE_MBZ is set?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022223052.646891-1-seanjc@google.com?part=4

Re: [PATCH 3/4] KVM: x86: Recompute EFER.LMA instead of inheriting the current value on EFER writes

[Yosry Ahmed]

On Thu, Oct 22, 2026 at 3:31 PM Sean Christopherson <seanjc@google.com> wrote:
>
> Recompute EFER.LMA on bare writes to EFER (via WRMSR or KVM_SET_MSRS)
> instead of carrying forward the current EFER.LMA.  While it _should_ be
> impossible for EFER.LMA to end up in an inconsistent state on such a write,
> KVM has had a rash of exactly those bugs, mostly because SVM is quite lax
> when it comes to consistenty checks, which in turn bleeds into KVM's nested
> SVM implementation.  Retaining the existing EFER is also rather confusing,
> as understanding why keeping EFER.LMA is correct requires a lot of a priori
> knowledge of both x86-64 and KVM.
>
> Keep the core concept of the existing logic as a sanity check: WARN and
> reject the write if the computed EFER.LMA would be toggled, because as
> described above, that _should_ be impossible.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/msrs.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/msrs.c b/arch/x86/kvm/msrs.c
> index c3cb1575fe66..41f3ec6eb286 100644
> --- a/arch/x86/kvm/msrs.c
> +++ b/arch/x86/kvm/msrs.c
> @@ -638,8 +638,18 @@ static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
>             (vcpu->arch.efer & EFER_SVME) && !(efer & EFER_SVME))
>                 return 1;
>
> -       efer &= ~EFER_LMA;
> -       efer |= vcpu->arch.efer & EFER_LMA;
> +       if ((efer & EFER_LME) && is_paging(vcpu) && is_pae(vcpu))
> +               efer |= EFER_LMA;
> +       else
> +               efer &= ~EFER_LMA;

Does this mean we can effectively revert this patch:
https://lore.kernel.org/kvm/20260902232028.2767071-3-seanjc@google.com/?

> +
> +       /*
> +        * Toggling EFER.LMA on a bare write to EFER should be impossible, as
> +        * it means KVM allowed architecturally invalid state at some point in
> +        * the past, i.e. allowed EFER.LMA=1 && (!CR0.PG || !CR4.PAE).
> +        */
> +       if (WARN_ON_ONCE((efer & EFER_LMA) != (vcpu->arch.efer & EFER_LMA)))
> +               return 1;
>
>         r = kvm_x86_call(set_efer)(vcpu, efer);
>         if (r) {
> --
> 2.56.0.257.gf7497c7d27-goog
>
>

Re: [PATCH 3/4] KVM: x86: Recompute EFER.LMA instead of inheriting the current value on EFER writes

[Sean Christopherson]

On Fri, Oct 23, 2026, Yosry Ahmed wrote:
> On Thu, Oct 22, 2026 at 3:31 PM Sean Christopherson <seanjc@google.com> wrote:
> >
> > Recompute EFER.LMA on bare writes to EFER (via WRMSR or KVM_SET_MSRS)
> > instead of carrying forward the current EFER.LMA.  While it _should_ be
> > impossible for EFER.LMA to end up in an inconsistent state on such a write,
> > KVM has had a rash of exactly those bugs, mostly because SVM is quite lax
> > when it comes to consistenty checks, which in turn bleeds into KVM's nested
> > SVM implementation.  Retaining the existing EFER is also rather confusing,
> > as understanding why keeping EFER.LMA is correct requires a lot of a priori
> > knowledge of both x86-64 and KVM.
> >
> > Keep the core concept of the existing logic as a sanity check: WARN and
> > reject the write if the computed EFER.LMA would be toggled, because as
> > described above, that _should_ be impossible.
> >
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > ---
> >  arch/x86/kvm/msrs.c | 14 ++++++++++++--
> >  1 file changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/kvm/msrs.c b/arch/x86/kvm/msrs.c
> > index c3cb1575fe66..41f3ec6eb286 100644
> > --- a/arch/x86/kvm/msrs.c
> > +++ b/arch/x86/kvm/msrs.c
> > @@ -638,8 +638,18 @@ static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
> >             (vcpu->arch.efer & EFER_SVME) && !(efer & EFER_SVME))
> >                 return 1;
> >
> > -       efer &= ~EFER_LMA;
> > -       efer |= vcpu->arch.efer & EFER_LMA;
> > +       if ((efer & EFER_LME) && is_paging(vcpu) && is_pae(vcpu))
> > +               efer |= EFER_LMA;
> > +       else
> > +               efer &= ~EFER_LMA;
> 
> Does this mean we can effectively revert this patch:
> https://lore.kernel.org/kvm/20260902232028.2767071-3-seanjc@google.com/?

No, because that flow goes straight through svm_set_efer(), i.e. doesn't hit this
path:

+	/* SVM ignores EFER.LMA if EFER.LME=0 (instead of failing VMRUN). */
+	if (!(svm->nested.save.efer & EFER_LME))
+		svm->nested.save.efer &= ~EFER_LMA;
+
 	svm_set_efer(vcpu, svm->nested.save.efer);

Re: [PATCH 3/4] KVM: x86: Recompute EFER.LMA instead of inheriting the current value on EFER writes

[Sean Christopherson]

On Thu, Oct 22, 2026, sashiko-bot@kernel.org wrote:
> > diff --git a/arch/x86/kvm/msrs.c b/arch/x86/kvm/msrs.c
> > index c3cb1575fe66e..41f3ec6eb2866 100644
> > --- a/arch/x86/kvm/msrs.c
> > +++ b/arch/x86/kvm/msrs.c
> > @@ -638,8 +638,18 @@ static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
> >  	    (vcpu->arch.efer & EFER_SVME) && !(efer & EFER_SVME))
> >  		return 1;
> >  
> > -	efer &= ~EFER_LMA;
> > -	efer |= vcpu->arch.efer & EFER_LMA;
> > +	if ((efer & EFER_LME) && is_paging(vcpu) && is_pae(vcpu))
> > +		efer |= EFER_LMA;
> > +	else
> > +		efer &= ~EFER_LMA;
> > +
> > +	/*
> > +	 * Toggling EFER.LMA on a bare write to EFER should be impossible, as
> > +	 * it means KVM allowed architecturally invalid state at some point in
> > +	 * the past, i.e. allowed EFER.LMA=1 && (!CR0.PG || !CR4.PAE).
> > +	 */
> > +	if (WARN_ON_ONCE((efer & EFER_LMA) != (vcpu->arch.efer & EFER_LMA)))
> > +		return 1;
> 
> [Severity: High]
> Can a guest or host userspace trigger this WARN_ON_ONCE() and get a spurious
> #GP (or failed RSM / KVM_SET_MSRS) via SVM paths that still leave EFER_LMA
> in vcpu->arch.efer out of sync with (EFER_LME && CR0.PG && CR4.PAE)?

LOL, followed by a freaking disseration.

> Earlier in arch/x86/kvm/msrs.c:set_efer(), KVM rejects changing EFER_LME
> while paging is enabled:
> 
> arch/x86/kvm/msrs.c:set_efer() {
> 	...
> 	if (is_paging(vcpu) && (vcpu->arch.efer & EFER_LME) != (efer & EFER_LME))
> 		return 1;
> 	...
> }
> 
> Because of that check, the recomputed EFER_LMA in set_efer() is always equal
> to ((vcpu->arch.efer & EFER_LME) && is_paging(vcpu) && is_pae(vcpu)). The new
> WARN_ON_ONCE() therefore triggers whenever a vCPU enters set_efer() with
> vcpu->arch.efer & EFER_LMA out of sync with those bits.
> 
> There appear to be three SVM paths where that still happens:
> 
> 1. Nested SVM VMRUN and RSM into L2:
> 
> Architecturally on SVM, VMRUN ignores EFER.LMA in the VMCB save area and sets
> EFER.LMA from (EFER.LME && CR0.PG) in hardware, so nested_vmcb_check_save()
> does not require save->efer & EFER_LMA to match (EFER_LME && CR0.PG):
> 
> arch/x86/kvm/svm/nested.c:nested_vmcb_check_save() {
> 	...
> 	/*
> 	 * These checks are also performed by KVM_SET_SREGS,
> 	 * except that EFER.LMA is not checked by SVM against
> 	 * CR0.PG && EFER.LME.
> 	 */
> 	if ((save->efer & EFER_LME) && (save->cr0 & X86_CR0_PG)) {
> 		if (CC(!(save->cr4 & X86_CR4_PAE)) ||
> 		    CC(!(save->cr0 & X86_CR0_PE)) ||
> 		    CC(!kvm_vcpu_is_legal_cr3(vcpu, save->cr3)))
> 			return false;
> 	...
> }
> 
> However, nested_vmcb02_prepare_save() only clears EFER_LMA when EFER_LME is
> 0, and relies on svm_set_cr0() to update EFER_LMA:
> 
> arch/x86/kvm/svm/nested.c:nested_vmcb02_prepare_save() {
> 	...
> 	/* SVM ignores EFER.LMA if EFER.LME=0 (instead of failing VMRUN). */
> 	if (!(svm->nested.save.efer & EFER_LME))
> 		svm->nested.save.efer &= ~EFER_LMA;
> 
> 	svm_set_efer(vcpu, svm->nested.save.efer);
> 
> 	svm_set_cr0(vcpu, svm->nested.save.cr0);
> 	svm_set_cr4(vcpu, svm->nested.save.cr4);
> 	...
> }
> 
> And svm_set_cr0() only updates EFER_LMA in vcpu->arch.efer when CR0.PG
> transitions relative to the previous vcpu->arch.cr0:
> 
> arch/x86/kvm/svm/svm.c:svm_set_cr0() {
> 	...
> 	if (vcpu->arch.efer & EFER_LME) {
> 		if (!is_paging(vcpu) && (cr0 & X86_CR0_PG)) {
> 			vcpu->arch.efer |= EFER_LMA;
> 			if (!vcpu->arch.guest_state_protected)
> 				svm->vmcb->save.efer |= EFER_LMA | EFER_LME;
> 		}
> 
> 		if (is_paging(vcpu) && !(cr0 & X86_CR0_PG)) {
> 			vcpu->arch.efer &= ~EFER_LMA;
> 			if (!vcpu->arch.guest_state_protected)
> 				svm->vmcb->save.efer &= ~(EFER_LMA | EFER_LME);
> 		}
> 	}
> 	...
> }
> 
> Because L1 must have CR0.PG=1 to execute VMRUN, is_paging(vcpu) is already
> true when svm_set_cr0() runs during VMRUN. If vmcb12 has CR0.PG=1, CR4.PAE=1,
> EFER.LME=1, and EFER.LMA=0, svm_set_cr0() never sets EFER_LMA, leaving L2
> running in 64-bit mode with EFER_LMA=0 in vcpu->arch.efer. When L2 executes a
> valid WRMSR(MSR_EFER) (for example setting EFER_NX or EFER_SCE) or userspace
> calls KVM_SET_MSRS, set_efer() computes EFER_LMA=1, triggers WARN_ON_ONCE(),
> and injects a #GP into L2.

This looks like a KVM bug, i.e. we want the WARN and should fix the underlying
bug.

> Conversely, during RSM to L2 via emulator_leave_smm() -> svm_leave_smm() ->
> enter_svm_guest_mode() -> nested_vmcb02_prepare_save(), emulator_leave_smm()
> clears CR0.PG before calling svm_leave_smm(). If vmcb12 has EFER.LME=1,
> EFER.LMA=1, and CR0.PG=0, is_paging(vcpu) is already false when svm_set_cr0()
> runs, leaving EFER_LMA=1 with CR0.PG=0. When rsm_load_state_64() then calls
> __kvm_emulate_msr_write(vcpu, MSR_EFER, smstate->efer & ~EFER_LMA) ->
> set_efer(), set_efer() computes EFER_LMA=0, triggers WARN_ON_ONCE(), and
> aborts RSM with X86EMUL_UNHANDLEABLE.

Same here?

> 2. KVM_SET_NESTED_STATE and svm_leave_smm() followed by #VMEXIT:
> 
> In arch/x86/kvm/svm/nested.c:svm_set_nested_state(), L1's saved host state in
> vmcb01->save is only checked for ((save->efer & EFER_LMA) &&
> !(save->efer & EFER_LME)) (and svm_leave_smm() copies vmcb01->save from
> guest MSR_VM_HSAVE_PA memory without checking EFER_LMA):
> 
> arch/x86/kvm/svm/nested.c:svm_set_nested_state() {
> 	...
> 	if (!(save->cr0 & X86_CR0_PG) ||
> 	    !(save->cr0 & X86_CR0_PE) ||
> 	    (save->rflags & X86_EFLAGS_VM) ||
> 	    ((save->efer & EFER_LMA) && !(save->efer & EFER_LME)) ||
> 	    !nested_vmcb_check_save(vcpu, &save_cached, false))
> 		goto out_free;
> 	...
> }
> 
> If vmcb01->save has EFER.LME=1, EFER.LMA=0, CR0.PG=1, and CR4.PAE=1, then on
> nested_svm_vmexit() from a paged L2 (where is_paging(vcpu) is already true),
> svm_set_efer() and svm_set_cr0() leave L1 with EFER_LMA=0 while EFER_LME=1,
> CR0.PG=1, and CR4.PAE=1, causing the next WRMSR(MSR_EFER) or KVM_SET_MSRS to
> trigger the WARN_ON_ONCE() in set_efer().

And here.

> 3. SEV-ES and SEV-SNP write traps:
> 
> In arch/x86/kvm/svm/svm.c:cr_trap(), SVM_EXIT_CR0_WRITE_TRAP and
> SVM_EXIT_CR4_WRITE_TRAP call svm_set_cr0() and svm_set_cr4() directly without
> checking consistency between EFER.LMA, CR0.PG, and CR4.PAE:
> 
> arch/x86/kvm/svm/svm.c:cr_trap() {
> 	...
> 	case 0:
> 		old_value = kvm_read_cr0(vcpu);
> 		svm_set_cr0(vcpu, new_value);
> 
> 		kvm_post_set_cr0(vcpu, old_value, new_value);
> 		break;
> 	case 4:
> 		old_value = kvm_read_cr4(vcpu);
> 		svm_set_cr4(vcpu, new_value);
> 
> 		kvm_post_set_cr4(vcpu, old_value, new_value);
> 		break;
> 	...
> }
> 
> Because svm_set_cr0() sets EFER_LMA without checking is_pae(vcpu) (for
> example on an SEV-SNP AP created via SVM_VMGEXIT_AP_CREATE where
> sev_snp_init_protected_guest_state() calls kvm_vcpu_reset() and clears
> vcpu->arch.cr4 while the VMSA has CR4.PAE=1), or if a guest clears CR4.PAE
> observed via cr_trap() while EFER_LMA=1, a subsequent efer_trap() ->
> set_efer() sees EFER_LMA=1 with !is_pae(vcpu) and triggers the WARN_ON_ONCE()
> and #GP.

Ugh.  I _want_ this to be a KVM bug, e.g. as is already pointed out by the comment:

	 * Clear EFER_LMSLE for a related reason: EFER writes are *trapped*,
	 * not intercepted, i.e. hardware has already committed the write by
	 * the time KVM gains control, and the trap is enabled if and only if
	 * the guest is SEV-ES, whose EFER lives in the encrypted VMSA and so
	 * can't be fixed up by KVM.  Rejecting EFER.LMSLE=1 would inject a #GP
	 * *and* leave EFER.LMSLE set in the guest, which is strictly worse
	 * than honoring a write that hardware itself allowed.

"rejecting" a value and injecting a #GP is completely pointless.  The guest already
has the "bad" value in EFER, and a misbehaving guest can simply eat the #GP.

But the issue is really just that KVM simply doesn't have visibility into EFER.LMA
being set.  Heh, which technically makes this a bug fix, since IIUC, the current
code will never set EFER.LMA in this case?

I can't think of any way to do the right thing other than to rely on hardware to
not screw up?  E.g.

diff --git a/arch/x86/kvm/msrs.c b/arch/x86/kvm/msrs.c
index 41f3ec6eb286..3a6a0ebf1932 100644
--- a/arch/x86/kvm/msrs.c
+++ b/arch/x86/kvm/msrs.c
@@ -648,7 +648,8 @@ static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
 	 * it means KVM allowed architecturally invalid state at some point in
 	 * the past, i.e. allowed EFER.LMA=1 && (!CR0.PG || !CR4.PAE).
 	 */
-	if (WARN_ON_ONCE((efer & EFER_LMA) != (vcpu->arch.efer & EFER_LMA)))
+	if (WARN_ON_ONCE((efer & EFER_LMA) != (vcpu->arch.efer & EFER_LMA) &&
+			 !vcpu->kvm->arch.has_protected_state))
 		return 1;
 
 	r = kvm_x86_call(set_efer)(vcpu, efer);


Hmm, though all of these cases make me wonder if we'd better off with just a WARN,
i.e. no #GP, since the goal is to enforce a consistent model, which is what the
new code does.

	/*
	 * Toggling EFER.LMA on a bare write to EFER should be impossible, as
	 * it means KVM allowed architecturally invalid state at some point in
	 * the past, i.e. allowed EFER.LMA=1 && (!CR0.PG || !CR4.PAE).
	 */
	WARN_ON_ONCE((efer & EFER_LMA) != (vcpu->arch.efer & EFER_LMA) &&
		     !vcpu->kvm->arch.has_protected_state);

Re: [PATCH 3/4] KVM: x86: Recompute EFER.LMA instead of inheriting the current value on EFER writes

[Sean Christopherson]

On Mon, Oct 26, 2026, Sean Christopherson wrote:
> On Thu, Oct 22, 2026, sashiko-bot@kernel.org wrote:
> > Because svm_set_cr0() sets EFER_LMA without checking is_pae(vcpu) (for
> > example on an SEV-SNP AP created via SVM_VMGEXIT_AP_CREATE where
> > sev_snp_init_protected_guest_state() calls kvm_vcpu_reset() and clears
> > vcpu->arch.cr4 while the VMSA has CR4.PAE=1), 

Wait, no, kvm_vcpu_reset() also immediately clears EFER:

	kvm_x86_call(set_cr4)(vcpu, 0);
	kvm_x86_call(set_efer)(vcpu, 0);  <===

and svm_set_efer() sets vcpu->arch.efer verbatim.

> > or if a guest clears CR4.PAE observed via cr_trap() while EFER_LMA=1,

And I'm pretty sure Sashiko is wrong on this one too, as this can't happen.
Clearing CR4.PAE while long mode is active results in a #GP, and hardware should
enforcee that when MOV CR4 intercepts are trap-like.

So I don't think the vcpu->kvm->arch.has_protected_state waiver is needed (though
I do think it makes sense to WARN and continute, not WARN and reject).

> a subsequent efer_trap() ->
> > set_efer() sees EFER_LMA=1 with !is_pae(vcpu) and triggers the WARN_ON_ONCE()
> > and #GP.
> 
> Ugh.  I _want_ this to be a KVM bug, e.g. as is already pointed out by the comment:
> 
> 	 * Clear EFER_LMSLE for a related reason: EFER writes are *trapped*,
> 	 * not intercepted, i.e. hardware has already committed the write by
> 	 * the time KVM gains control, and the trap is enabled if and only if
> 	 * the guest is SEV-ES, whose EFER lives in the encrypted VMSA and so
> 	 * can't be fixed up by KVM.  Rejecting EFER.LMSLE=1 would inject a #GP
> 	 * *and* leave EFER.LMSLE set in the guest, which is strictly worse
> 	 * than honoring a write that hardware itself allowed.
> 
> "rejecting" a value and injecting a #GP is completely pointless.  The guest already
> has the "bad" value in EFER, and a misbehaving guest can simply eat the #GP.
> 
> But the issue is really just that KVM simply doesn't have visibility into EFER.LMA
> being set.  Heh, which technically makes this a bug fix, since IIUC, the current
> code will never set EFER.LMA in this case?
> 
> I can't think of any way to do the right thing other than to rely on hardware to
> not screw up?  E.g.
> 
> diff --git a/arch/x86/kvm/msrs.c b/arch/x86/kvm/msrs.c
> index 41f3ec6eb286..3a6a0ebf1932 100644
> --- a/arch/x86/kvm/msrs.c
> +++ b/arch/x86/kvm/msrs.c
> @@ -648,7 +648,8 @@ static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
>  	 * it means KVM allowed architecturally invalid state at some point in
>  	 * the past, i.e. allowed EFER.LMA=1 && (!CR0.PG || !CR4.PAE).
>  	 */
> -	if (WARN_ON_ONCE((efer & EFER_LMA) != (vcpu->arch.efer & EFER_LMA)))
> +	if (WARN_ON_ONCE((efer & EFER_LMA) != (vcpu->arch.efer & EFER_LMA) &&
> +			 !vcpu->kvm->arch.has_protected_state))
>  		return 1;
>  
>  	r = kvm_x86_call(set_efer)(vcpu, efer);
> 
> 
> Hmm, though all of these cases make me wonder if we'd better off with just a WARN,
> i.e. no #GP, since the goal is to enforce a consistent model, which is what the
> new code does.
> 
> 	/*
> 	 * Toggling EFER.LMA on a bare write to EFER should be impossible, as
> 	 * it means KVM allowed architecturally invalid state at some point in
> 	 * the past, i.e. allowed EFER.LMA=1 && (!CR0.PG || !CR4.PAE).
> 	 */
> 	WARN_ON_ONCE((efer & EFER_LMA) != (vcpu->arch.efer & EFER_LMA) &&
> 		     !vcpu->kvm->arch.has_protected_state);
> 

Re: [PATCH 2/4] KVM: nSVM: Synthesize #GP instead of SHUTDOWN if L2 attempts to clear EFER.SVME

[Sean Christopherson]

On Thu, Oct 22, 2026, sashiko-bot@kernel.org wrote:
> > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> > index 487897ef11dd6..a6ff9635677a1 100644
> > --- a/arch/x86/kvm/svm/svm.c
> > +++ b/arch/x86/kvm/svm/svm.c
> > @@ -227,17 +227,12 @@ int svm_set_efer(struct kvm_vcpu *vcpu, u64 efer)
> >  	if ((old_efer & EFER_SVME) != (efer & EFER_SVME)) {
> >  		if (!(efer & EFER_SVME)) {
> >  			/*
> > -			 * Architecturally, clearing EFER.SVME while a guest is
> > -			 * running yields undefined behavior, i.e. KVM can do
> > -			 * literally anything.  Force the vCPU back into L1 as
> > -			 * that is the safest option for KVM, but synthesize a
> > -			 * triple fault (for L1!) so that KVM at least doesn't
> > -			 * run random L2 code in the context of L1.  Do so if
> > -			 * and only if the vCPU is actively running, e.g. to
> > -			 * avoid positives if userspace is stuffing state.
> > +			 * KVM is supposed to synthesize #GP if L2 attempts to
> > +			 * clear SVME.
> >  			 */
> > -			if (is_guest_mode(vcpu) && vcpu->wants_to_run)
> > -				kvm_make_request(KVM_REQ_TRIPLE_FAULT, vcpu);
> > +			if (KVM_BUG_ON(is_guest_mode(vcpu) &&
> > +				       vcpu->wants_to_run, vcpu->kvm))
> > +				return -EIO;
> >  
> >  			svm_leave_nested(vcpu);
> >  			/* #GP intercept is still needed for vmware backdoor */
> 
> [Severity: High]
> Can userspace still reach this KVM_BUG_ON() during KVM_RUN via
> KVM_SYNC_X86_SREGS when a vCPU is in guest mode?
> 
> In virt/kvm/kvm_main.c:kvm_vcpu_ioctl(), vcpu->wants_to_run is set before
> calling kvm_arch_vcpu_ioctl_run():
> 
> 	vcpu->wants_to_run = !READ_ONCE(vcpu->run->immediate_exit__unsafe);
> 	r = kvm_arch_vcpu_ioctl_run(vcpu);
> 	vcpu->wants_to_run = false;
> 
> Then arch/x86/kvm/x86.c:kvm_arch_vcpu_ioctl_run() syncs dirty registers
> from userspace while vcpu->wants_to_run is already true:
> 
> 	if (kvm_run->kvm_dirty_regs) {
> 		r = sync_regs(vcpu);
> 		if (r != 0)
> 			goto out;
> 	}
> 
> When KVM_SYNC_X86_SREGS is set in kvm_dirty_regs, the call chain
> sync_regs() -> kvm_run_sync_regs_from_user() -> __set_sregs() ->
> __set_sregs_common() in arch/x86/kvm/regs.c bypasses set_efer() in
> arch/x86/kvm/msrs.c and calls svm_set_efer() directly:
> 
> 	*mmu_reset_needed |= vcpu->arch.efer != sregs->efer;
> 	kvm_x86_call(set_efer)(vcpu, sregs->efer);
> 
> If userspace clears EFER_SVME in vcpu->run->s.regs.sregs.efer while
> is_guest_mode(vcpu) is true, wouldn't svm_set_efer() trigger the
> KVM_BUG_ON() and return -EIO (which __set_sregs_common() ignores) after
> already updating vcpu->arch.efer = efer and without calling
> svm_leave_nested(vcpu)?

Oof.  I think the right answer is to straight up disallow that?

diff --git a/arch/x86/kvm/regs.c b/arch/x86/kvm/regs.c
index 3621f1a47d37..e53496fdeb82 100644
--- a/arch/x86/kvm/regs.c
+++ b/arch/x86/kvm/regs.c
@@ -738,6 +738,10 @@ int kvm_run_sync_regs_from_user(struct kvm_vcpu *vcpu)
 	if (vcpu->run->kvm_dirty_regs & KVM_SYNC_X86_SREGS) {
 		struct kvm_sregs sregs = vcpu->run->s.regs.sregs;
 
+		if (is_guest_mode(vcpu) &&
+		    (vcpu->arch.efer & EFER_SVME) && !(sregs.efer & EFER_SVME))
+			return -EINVAL;
+
 		if (__set_sregs(vcpu, &sregs))
 			return -EINVAL;

Ideally, kvm_is_valid_sregs() would disallow that, but the risk of breaking
userspace, e.g. on save/restore, is too high.  Whereas I really, really hope no
one is using KVM_SYNC_X86_SREGS to yank the vCPU out of L2 during save/restore.