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

[PATCH v3 0/7] 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 plugs a hole where userspace can get at the "wants_to_run" logic in
svm_set_efer() via KVM_SYNC_X86_SREGS, which is "fine" for now, but would let
a misbehaving userspace trigger the KVM_BUG_ON() added in patch 3.

Patch 3 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 4 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.

v3:
 - /s/LME/LMA in MMU role bits. [Sashiko]
 - Restore an unintentionally deleted vcpu_sregs_get(). [Sashiko]
 - Massage the KVM_SYNC_X86_SREGS changelog. [Yosry]

v2:
 - Disallowing clearing SVME via KVM_SYNC_X86_SREGS if L2 is active. [Sashiko]

v1: https://lore.kernel.org/all/20261022223052.646891-1-seanjc@google.com

Sean Christopherson (7):
  Revert "KVM: x86: Skip EFER vs. guest CPUID checks for host-initiated
    writes"
  KVM: x86: Treat EFER.LMA, not EFER.LMA, as an EFER MMU role bit
  KVM: x86: Reject clearing EFER.SVME via KVM_SYNC_X86_SREGS if L2 is
    active
  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
  KVM: selftests: Expect failure when trying to set EFER that violates
    vCPU model

 arch/x86/kvm/mmu.h                            |  2 +-
 arch/x86/kvm/msrs.c                           | 53 ++++++-----
 arch/x86/kvm/regs.c                           |  4 +
 arch/x86/kvm/svm/svm.c                        | 15 +--
 .../selftests/kvm/x86/svm_nested_efer_test.c  | 94 ++++++++++---------
 5 files changed, 93 insertions(+), 75 deletions(-)


base-commit: 7e60162b16f9da0d2fa83da5c1723b6853a0ac1d
-- 
2.56.0.385.gac671c819a-goog

[PATCH v3 1/7] 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.385.gac671c819a-goog

[PATCH v3 2/7] KVM: x86: Treat EFER.LMA, not EFER.LMA, as an EFER MMU role bit

[Sean Christopherson]

Now that KVM now longer lets userspace violate the architectural rules when
stuffing EFER, treat EFER.LMA instead of EFER.LME as the relevant EFER MMU
role bit to ensure that the MMU context is reinitialized if EFER.LMA is
toggled without EFER.LME also being toggled.  EFER.LMA is the actual bit
that is consumed by e.g. kvm_calc_cpu_role().

While the _current_ code never actually toggles EFER.LMA, that is a flaw
that will soon be fixed.  Furthermore, checking EFER.LME was essentially a
hack to fudge around the now-reverted, misguided commit 11988499e62b ("KVM:
x86: Skip EFER vs. guest CPUID checks for host-initiated writes").

Reported-by: Sashiko Bot <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/all/sashiko-outbox-187201@kernel.org
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/mmu.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
index 2ae7f9ed4cf8..2c3f777f78d6 100644
--- a/arch/x86/kvm/mmu.h
+++ b/arch/x86/kvm/mmu.h
@@ -61,7 +61,7 @@ extern bool __read_mostly eager_page_split;
 			       X86_CR4_SMEP | X86_CR4_SMAP | X86_CR4_PKE)
 
 #define KVM_MMU_CR0_ROLE_BITS (X86_CR0_PG | X86_CR0_WP)
-#define KVM_MMU_EFER_ROLE_BITS (EFER_LME | EFER_NX)
+#define KVM_MMU_EFER_ROLE_BITS (EFER_LMA | EFER_NX)
 
 static __always_inline u64 rsvd_bits(int s, int e)
 {
-- 
2.56.0.385.gac671c819a-goog

[PATCH v3 3/7] KVM: x86: Reject clearing EFER.SVME via KVM_SYNC_X86_SREGS if L2 is active

[Sean Christopherson]

Disallow clearing EFER.SVME via KVM_SYNC_X86_SREGS if L2 is active so that
svm_set_efer() can treat yanking the vCPU out of L2 during KVM_RUN as a KVM
bug.  Ideally, kvm_is_valid_sregs() itself would reject attempts to clear
SVME while L2 is active, but the risk of breaking userspace, e.g. across
save/restore, is too high.

On the other hand, yanking the vCPU out of L2 via KVM_SYNC_X86_SREGS is
already "broken" as doing so would queue a triple fault for L1 (because
"wants_to_run" will be true).

Link: https://lore.kernel.org/all/sashiko-outbox-182127@kernel.org
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/regs.c | 4 ++++
 1 file changed, 4 insertions(+)

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;
 
-- 
2.56.0.385.gac671c819a-goog

[PATCH v3 4/7] 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.385.gac671c819a-goog

[PATCH v3 5/7] 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 if the
computed EFER.LMA would be toggled, because as described above, that
_should_ be impossible.  Don't return an error if the sanity check fails,
e.g. don't inject a #GP, as setting EFER.LMA based on the current state
provides architecturally consistency.

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

diff --git a/arch/x86/kvm/msrs.c b/arch/x86/kvm/msrs.c
index c3cb1575fe66..c55402970a34 100644
--- a/arch/x86/kvm/msrs.c
+++ b/arch/x86/kvm/msrs.c
@@ -638,8 +638,17 @@ 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).
+	 */
+	WARN_ON_ONCE((efer & EFER_LMA) != (vcpu->arch.efer & EFER_LMA));
 
 	r = kvm_x86_call(set_efer)(vcpu, efer);
 	if (r) {
-- 
2.56.0.385.gac671c819a-goog

[PATCH v3 6/7] 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  | 75 ++++++++++++-------
 1 file changed, 46 insertions(+), 29 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..fa9cfe8b1242 100644
--- a/tools/testing/selftests/kvm/x86/svm_nested_efer_test.c
+++ b/tools/testing/selftests/kvm/x86/svm_nested_efer_test.c
@@ -15,22 +15,34 @@ 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.SVME in L2, got %s",
+		       ex_str(vector));
+
+	efer = rdmsr(MSR_EFER);
+	GUEST_ASSERT(efer & EFER_SVME);
+
+	l2_ran = true;
+	vmmcall();
 }
 
 static void l1_clear_efer_svme(struct svm_test_data *svm)
 {
+	struct vmcb *vmcb = svm->vmcb;
+
 	generic_svm_setup(svm, l2_clear_efer_svme);
-	run_guest(svm->vmcb, svm->vmcb_gpa);
+	run_guest(vmcb, svm->vmcb_gpa);
 
-	/* Unreachable, L1 should be shutdown */
-	GUEST_ASSERT(0);
+	GUEST_ASSERT_EQ(vmcb->control.exit_code, SVM_EXIT_VMMCALL);
+	GUEST_ASSERT(l2_ran);
+	GUEST_ASSERT(vmcb->save.efer & EFER_SVME);
+	GUEST_DONE();
 }
 
 static void l2_lmsle(void)
@@ -130,31 +142,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 +157,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 +265,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.385.gac671c819a-goog

[PATCH v3 7/7] KVM: selftests: Expect failure when trying to set EFER that violates vCPU model

[Sean Christopherson]

Adjust the EFER.LMSLE testcases to expect failure, not success, when
attempting to stuff EFER.LMSLE=1 even when the guest CPUID model is
configured with EFER_LMSLE_MBZ enabled, i.e. when it should be impossible
to set EFER.LMSLE=1.  KVM's ABI is that userspace *must* set CPUID and
feature MSRs before restoring (or stuffing) vCPU state, i.e. before writing
normal MSRs.  The previous KVM behavior of allowing usersepace to stuff
garbage was a KVM bug (that has since been fixed).

Reported-by: Sashiko Bot <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/all/sashiko-outbox-182135@kernel.org
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 .../selftests/kvm/x86/svm_nested_efer_test.c  | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 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 fa9cfe8b1242..141ef035a8c4 100644
--- a/tools/testing/selftests/kvm/x86/svm_nested_efer_test.c
+++ b/tools/testing/selftests/kvm/x86/svm_nested_efer_test.c
@@ -193,23 +193,20 @@ static void test_host_initiated_lmsle(void)
 	struct kvm_vcpu *vcpu;
 	struct kvm_vm *vm;
 	u64 efer;
+	int r;
 
 	vm = vm_create_with_one_vcpu(&vcpu, NULL);
 	vcpu_set_cpuid_feature(vcpu, X86_FEATURE_EFER_LMSLE_MBZ);
 
-	/*
-	 * EFER_LMSLE_MBZ is a guest CPUID consistency check, not a host
-	 * capability, i.e. must not be enforced against host-initiated writes,
-	 * so that userspace can set MSRs before it sets guest CPUID.
-	 */
 	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);
-	TEST_ASSERT_EQ(vcpu_get_msr(vcpu, MSR_EFER), efer | EFER_LMSLE);
+	r = _vcpu_set_msr(vcpu, MSR_EFER, efer | EFER_LMSLE);
+	TEST_ASSERT_EQ(r, 0);
+	TEST_ASSERT_EQ(vcpu_get_msr(vcpu, MSR_EFER), efer);
 
 	kvm_vm_free(vm);
-	ksft_test_result_pass("Host-initiated EFER.LMSLE=1 is allowed\n");
+	ksft_test_result_pass("KVM_SET_MSRS rejects EFER.LMSLE=1 with EFER_LMSLE_MBZ\n");
 }
 
 static void test_sregs_lmsle(void)
@@ -222,10 +219,6 @@ static void test_sregs_lmsle(void)
 	vm = vm_create_with_one_vcpu(&vcpu, NULL);
 	vcpu_set_cpuid_feature(vcpu, X86_FEATURE_EFER_LMSLE_MBZ);
 
-	/*
-	 * Unlike KVM_SET_MSRS, KVM_SET_SREGS runs the full set of guest CPUID
-	 * checks, i.e. rejects EFER.LMSLE even though it's host-initiated.
-	 */
 	vcpu_sregs_get(vcpu, &sregs);
 	TEST_ASSERT(!(sregs.efer & EFER_LMSLE), "EFER.LMSLE unexpectedly set");
 
@@ -275,7 +268,7 @@ int main(int argc, char *argv[])
 		test_sregs_lmsle();
 	} else {
 		ksft_test_result_skip("Guest EFER_LMSLE_MBZ=0\n");
-		ksft_test_result_skip("Host-initiated EFER.LMSLE=1 is allowed\n");
+		ksft_test_result_skip("KVM_SET_MSRS rejects EFER.LMSLE=1 with EFER_LMSLE_MBZ\n");
 		ksft_test_result_skip("KVM_SET_SREGS rejects EFER.LMSLE=1\n");
 	}
 
-- 
2.56.0.385.gac671c819a-goog

Re: [PATCH v3 1/7] 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:
- [Low] KVM: x86: outdated EFER_LMSLE_MBZ api.rst doc and __kvm_valid_efer() comment
--

commit e9f88969b96c2ee54c7d5d0af947bad813dd2acc
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 host-initiated writes to MSR_EFER
via set_efer() are once again validated against guest CPUID capabilities and
CR0.PG vs. EFER.LME constraints.

> 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: Low]
As reported by Sashiko on v1 [1], now that set_efer() in arch/x86/kvm/msrs.c
unconditionally calls kvm_valid_efer() (including the
X86_FEATURE_EFER_LMSLE_MBZ check) even on host-initiated MSR_EFER writes and
folds __kvm_valid_efer() into kvm_valid_efer(), should
Documentation/virt/kvm/api.rst and the comment in efer_trap() in
arch/x86/kvm/svm/svm.c also be updated?

Specifically, commit 7358d13e1f19 ("KVM: x86: Honor the guest's
EFER_LMSLE_MBZ") added the following note under "Long mode segment limits" in
Documentation/virt/kvm/api.rst:

    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.

While the selftest failure in test_host_initiated_lmsle() in
tools/testing/selftests/kvm/x86/svm_nested_efer_test.c is addressed later in
this series by commit "KVM: selftests: Expect failure when trying to set EFER
that violates vCPU model", Documentation/virt/kvm/api.rst still states that
KVM_SET_MSRS is exempt from the check.

Additionally, 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.
     */

[1] https://lore.kernel.org/r/20261022223052.646891-2-seanjc@google.com

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

Re: [PATCH v3 5/7] 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 in set_efer() via SVM state sync
--

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

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

This commit updates set_efer() to recompute EFER_LMA from EFER_LME,
is_paging(vcpu), and is_pae(vcpu) instead of carrying forward the existing
EFER_LMA value from vcpu->arch.efer, and adds a WARN_ON_ONCE() if the
recomputed EFER_LMA differs from the previous value.

> diff --git a/arch/x86/kvm/msrs.c b/arch/x86/kvm/msrs.c
> index c3cb1575fe66e..c55402970a345 100644
> --- a/arch/x86/kvm/msrs.c
> +++ b/arch/x86/kvm/msrs.c
> @@ -638,8 +638,17 @@ 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).
> +	 */
> +	WARN_ON_ONCE((efer & EFER_LMA) != (vcpu->arch.efer & EFER_LMA));

[Severity: High]
As reported by Sashiko on v1 [1], can a guest or host userspace trigger this
WARN_ON_ONCE() via SVM paths that leave EFER_LMA in vcpu->arch.efer out of
sync with EFER_LME, X86_CR0_PG, and X86_CR4_PAE?

In arch/x86/kvm/msrs.c:set_efer(), changing EFER_LME while paging is enabled
is rejected earlier:

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

As a result, the recomputed EFER_LMA always equals
((vcpu->arch.efer & EFER_LME) && is_paging(vcpu) && is_pae(vcpu)), and the
WARN_ON_ONCE() fires whenever a vCPU enters set_efer() with vcpu->arch.efer's
EFER_LMA bit out of sync with EFER_LME, X86_CR0_PG, and X86_CR4_PAE. Several
SVM paths appear to still leave EFER_LMA out of sync:

1. In arch/x86/kvm/svm/nested.c:nested_vmcb02_prepare_save() and
arch/x86/kvm/svm/svm.c:svm_set_cr0():

	/* 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);

where svm_set_cr0() only updates EFER_LMA on a transition of X86_CR0_PG
relative to the previous vcpu->arch.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 has X86_CR0_PG=1 when executing VMRUN, is_paging(vcpu) is already
true when svm_set_cr0() runs. If L1 executes VMRUN with vmcb12->save
containing EFER_SVME=1, EFER_LME=1, EFER_LMA=0, X86_CR0_PG=1, X86_CR0_PE=1,
and X86_CR4_PAE=1 (which nested_vmcb_check_save() accepts because hardware
SVM ignores EFER_LMA in the VMCB save area), L2 runs with EFER_LMA=0 in
vcpu->arch.efer while EFER_LME=1, is_paging(vcpu), and is_pae(vcpu) are all
true. A subsequent WRMSR to MSR_EFER in L2 or KVM_SET_MSRS from userspace
enters set_efer(), computes EFER_LMA=1, and triggers the WARN_ON_ONCE().

2. On RSM from SMM into nested SVM via
emulator_leave_smm() -> svm_leave_smm() -> enter_svm_guest_mode() ->
nested_vmcb02_prepare_save(), emulator_leave_smm() clears X86_CR0_PG=0
before calling svm_leave_smm(). If vmcb12->save in guest memory has
EFER_SVME=1, EFER_LME=1, EFER_LMA=1, and X86_CR0_PG=0,
nested_vmcb02_prepare_save() does not clear EFER_LMA (since EFER_LME=1) and
svm_set_cr0() does not clear EFER_LMA (since is_paging(vcpu) is already
false). Immediately afterward, rsm_load_state_64() calls
__kvm_emulate_msr_write(vcpu, MSR_EFER, smstate->efer & ~EFER_LMA) ->
set_efer(), which computes EFER_LMA=0 against vcpu->arch.efer's EFER_LMA=1
and triggers the WARN_ON_ONCE().

3. In arch/x86/kvm/svm/nested.c:svm_set_nested_state(), the check on L1's
host save state in vmcb01->save only rejects (save->efer & EFER_LMA) &&
!(save->efer & EFER_LME):

	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;

and svm_leave_smm() similarly loads vmcb01->save from guest MSR_VM_HSAVE_PA
memory without checking EFER_LMA, allowing vmcb01->save to hold EFER_LME=1,
EFER_LMA=0, X86_CR0_PG=1, and X86_CR4_PAE=1. On nested_svm_vmexit() from a
paged L2 (where is_paging(vcpu) is already true):

	svm_set_efer(vcpu, vmcb01->save.efer);
	svm_set_cr0(vcpu, vmcb01->save.cr0 | X86_CR0_PE);
	svm_set_cr4(vcpu, vmcb01->save.cr4);

L1 is left with EFER_LMA=0 in vcpu->arch.efer while EFER_LME=1, X86_CR0_PG=1,
and X86_CR4_PAE=1, so the next WRMSR to MSR_EFER or KVM_SET_MSRS triggers
the WARN_ON_ONCE().

4. On SEV-SNP AP creation via sev_snp_init_protected_guest_state() and
kvm_vcpu_reset(), KVM initializes vcpu->arch.cr0 = X86_CR0_ET,
vcpu->arch.cr4 = 0, and vcpu->arch.efer = 0 while the guest VMSA can already
have X86_CR4_PAE=1 in hardware. In 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;

If the AP traps on an MSR_EFER write setting EFER_LME=1 (while vcpu->arch.cr0
has X86_CR0_PG=0) and then traps on a CR0 write setting X86_CR0_PG=1 without
having trapped on CR4, cr_trap() -> svm_set_cr0() sets
vcpu->arch.efer |= EFER_LMA even though vcpu->arch.cr4 is still 0. A
subsequent MSR_EFER write trap before any CR4 write enters
efer_trap() -> set_efer(), sees !is_pae(vcpu), clears EFER_LMA in
vcpu->arch.efer while the guest is running in long mode, and triggers the
WARN_ON_ONCE().

[1] https://lore.kernel.org/r/sashiko-outbox-182134@kernel.org

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