[PATCH 0/4] KVM: x86/mmu: Don't sync cached roots created for a different MMU context

[PATCH 0/4] KVM: x86/mmu: Don't sync cached roots created for a different MMU context

[Yosry Ahmed]

When invalidating an address, KVM walks and syncs cached roots using the
current MMU context, but cached roots may have been created for a
different context after a nested transition.  With TDP disabled, this
lets a guest trigger a BUG_ON() and crash the host (see patch 3 for
details).

Patch 1 downgrades the BUG_ON() to a WARN_ON_ONCE(), patch 2 is prep
work, patch 3 is the fix, and patch 4 drops a TODO that isn't worth
pursuing.

Sashiko flagged the issue while reviewing the INVLPGA emulation patch in
my nested SVM TLB series [1].  The host crash is easy to reproduce with an
AI-generated repro (with TDP disabled), and no longer triggers with this
series.

[1] https://lore.kernel.org/kvm/sashiko-outbox-163479@kernel.org/

Yosry Ahmed (4):
  KVM: x86/mmu: WARN, don't BUG, when walking a cached root with a PAE
    MMU
  KVM: x86/mmu: Drop for_each_shadow_entry_using_root()
  KVM: x86/mmu: Don't sync cached roots created for a different MMU
    context
  KVM: nVMX: Drop the TODO to sync only the affected SPTEs for INVVPID

 arch/x86/kvm/mmu/mmu.c    | 76 ++++++++++++++++++++++++++++-----------
 arch/x86/kvm/vmx/nested.c |  2 --
 2 files changed, 55 insertions(+), 23 deletions(-)


base-commit: 6bd2905303c58679e303941b7d8ae8c074cd95ce
-- 
2.56.0.385.gd3acb90ef8-goog

[PATCH 1/4] KVM: x86/mmu: WARN, don't BUG, when walking a cached root with a PAE MMU

[Yosry Ahmed]

Replace the BUG_ON() in shadow_walk_init_using_root() that guards against
walking a cached (previous) root while the MMU is configured for PAE
paging with a WARN_ON_ONCE(), and terminate the walk if the WARN fires.
Walking a cached root with a PAE MMU is a KVM bug, as PAE roots are never
cached, but there's no need to bring down the host, as KVM can simply
bail from the walk.

Note, the BUG_ON() is currently reachable by a guest if TDP is disabled,
as KVM walks cached roots even if they were created for a different MMU
context, e.g. if L2 executes INVLPG while using PAE paging, L1 doesn't
intercept INVLPG, and L1's 64-bit root is cached.

Add a common error label for terminating the walk, and use it for the
existing invalid/dummy root check as well.

Fixes: 7eb77e9f5fcf ("kvm: x86: Add a root_hpa parameter to kvm_mmu->invlpg()")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Yosry Ahmed <yosry@kernel.org>
---
 arch/x86/kvm/mmu/mmu.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index aadd5bdc2c97a..126faef5a6476 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -2558,10 +2558,8 @@ static void shadow_walk_init_using_root(struct kvm_shadow_walk_iterator *iterato
 	iterator->addr = addr;
 	iterator->shadow_addr = root;
 
-	if (WARN_ON_ONCE(!VALID_PAGE(root)) || kvm_mmu_is_dummy_root(root)) {
-		iterator->level = 0;
-		return;
-	}
+	if (WARN_ON_ONCE(!VALID_PAGE(root)) || kvm_mmu_is_dummy_root(root))
+		goto error;
 
 	iterator->level = vcpu->arch.mmu->root_role.level;
 
@@ -2575,7 +2573,8 @@ static void shadow_walk_init_using_root(struct kvm_shadow_walk_iterator *iterato
 		 * prev_root is currently only used for 64-bit hosts. So only
 		 * the active root_hpa is valid here.
 		 */
-		BUG_ON(root != vcpu->arch.mmu->root.hpa);
+		if (WARN_ON_ONCE(root != vcpu->arch.mmu->root.hpa))
+			goto error;
 
 		iterator->shadow_addr
 			= vcpu->arch.mmu->pae_root[(addr >> 30) & 3];
@@ -2584,6 +2583,10 @@ static void shadow_walk_init_using_root(struct kvm_shadow_walk_iterator *iterato
 		if (!iterator->shadow_addr)
 			iterator->level = 0;
 	}
+	return;
+
+error:
+	iterator->level = 0;
 }
 
 static void shadow_walk_init(struct kvm_shadow_walk_iterator *iterator,
-- 
2.56.0.385.gd3acb90ef8-goog

[PATCH 2/4] KVM: x86/mmu: Drop for_each_shadow_entry_using_root()

[Yosry Ahmed]

Drop for_each_shadow_entry_using_root() and open code the walk in
__kvm_mmu_invalidate_addr(), the macro's only user, in preparation for
handling failure to initialize the walk, e.g. if a cached root was created
for a different MMU context and so can't be walked.

No functional change intended.

Suggested-by: Sean Christopherson <seanjc@google.com>
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Yosry Ahmed <yosry@kernel.org>
---
 arch/x86/kvm/mmu/mmu.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 126faef5a6476..2fc23a36433c5 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -166,12 +166,6 @@ struct kvm_shadow_walk_iterator {
 	unsigned index;
 };
 
-#define for_each_shadow_entry_using_root(_vcpu, _root, _addr, _walker)     \
-	for (shadow_walk_init_using_root(&(_walker), (_vcpu),              \
-					 (_root), (_addr));                \
-	     shadow_walk_okay(&(_walker));			           \
-	     shadow_walk_next(&(_walker)))
-
 #define for_each_shadow_entry(_vcpu, _addr, _walker)            \
 	for (shadow_walk_init(&(_walker), _vcpu, _addr);	\
 	     shadow_walk_okay(&(_walker));			\
@@ -6754,7 +6748,8 @@ static void __kvm_mmu_invalidate_addr(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu
 		return;
 
 	write_lock(&vcpu->kvm->mmu_lock);
-	for_each_shadow_entry_using_root(vcpu, root_hpa, addr, iterator) {
+	shadow_walk_init_using_root(&iterator, vcpu, root_hpa, addr);
+	for ( ; shadow_walk_okay(&iterator); shadow_walk_next(&iterator)) {
 		struct kvm_mmu_page *sp = sptep_to_sp(iterator.sptep);
 
 		if (sp->unsync) {
-- 
2.56.0.385.gd3acb90ef8-goog

[PATCH 3/4] KVM: x86/mmu: Don't sync cached roots created for a different MMU context

[Yosry Ahmed]

When invalidating an address (e.g. emulating INVLPG), KVM walks and
syncs the cached (previous) roots in addition to the current root.
The walk uses the current MMU's root level, and the sync uses the
current MMU's sync_spte() and page table walker, i.e. KVM assumes that
cached roots were created for the current MMU context.

This assumption does not hold for nested virtualization.  Nested
transitions re-initialize the MMU and load a new PGD without unloading
cached roots, so the cached roots may belong to a different MMU context
(e.g. a different paging mode).  Outside of nested transitions, this is
not possible, as changing CR0, CR4, or EFER in a way that affects the
MMU context resets the MMU, unloading all roots.

For example, with shadow paging, if a 64-bit L1 runs a 32-bit PAE L2,
L1's 4-level root is cached while L2 is running with a PAE root.  If L2
executes INVLPG and L1 doesn't intercept it, KVM walks L1's root using
L2's PAE root level, which hits the WARN (previously BUG) in
shadow_walk_init_using_root().  Similarly, with nested EPT,
nested_ept_invalidate_addr() can walk a cached root at the wrong level
if L1 uses the same EPT root with a different page-walk length.

Note, the WARN is only reachable if TDP is disabled, as KVM only syncs
the root_mmu's cached roots if TDP is disabled, and the only other flow
that syncs cached roots, nested_ept_invalidate_addr(), never uses a PAE
MMU.

Only walk cached roots if their role matches the current MMU's root role,
and sync the rest by freeing them if they have unsync children.  Check
the role in shadow_walk_init_using_root(), i.e. with mmu_lock held, as
zapping a root (e.g. due to a guest write to the page table it shadows)
marks it invalid, changing its role.

Note, comparing the full role, including guest_mode, doesn't affect
nested EPT fault injection (nested_ept_invalidate_addr()), as all of the
guest_mmu's roots, and its root role, have guest_mode set.  The only
casualty is nested virtualization with TDP disabled, where L1 and L2
share the root_mmu, i.e. their roots differ in guest_mode even if they're
otherwise compatible, e.g. INVLPG in L1 will now free L2's unsync roots
instead of syncing them.  But nested with TDP disabled isn't worth
optimizing.

Opportunistically WARN if the current root can't be invalidated, and
free it, even though that should be impossible.

Fixes: 7eb77e9f5fcf ("kvm: x86: Add a root_hpa parameter to kvm_mmu->invlpg()")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Yosry Ahmed <yosry@kernel.org>
---
 arch/x86/kvm/mmu/mmu.c | 56 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 46 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 2fc23a36433c5..0b78c56f771b7 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -2547,7 +2547,8 @@ static void shadow_walk_init_using_root(struct kvm_shadow_walk_iterator *iterato
 					struct kvm_vcpu *vcpu, hpa_t root,
 					u64 addr)
 {
-	struct kvm_pagewalk *w = vcpu->arch.mmu->w;
+	struct kvm_mmu *mmu = vcpu->arch.mmu;
+	struct kvm_pagewalk *w = mmu->w;
 
 	iterator->addr = addr;
 	iterator->shadow_addr = root;
@@ -2555,6 +2556,18 @@ static void shadow_walk_init_using_root(struct kvm_shadow_walk_iterator *iterato
 	if (WARN_ON_ONCE(!VALID_PAGE(root)) || kvm_mmu_is_dummy_root(root))
 		goto error;
 
+	/*
+	 * Cached roots are walked using the current MMU context, and so can
+	 * only be walked if they were created for the same context, which
+	 * isn't guaranteed as nested transitions don't unload cached roots.
+	 */
+	if (root != mmu->root.hpa) {
+		struct kvm_mmu_page *sp = root_to_sp(root);
+
+		if (WARN_ON_ONCE(!sp) || sp->role.word != mmu->root_role.word)
+			goto error;
+	}
+
 	iterator->level = vcpu->arch.mmu->root_role.level;
 
 	if (iterator->level >= PT64_ROOT_4LEVEL &&
@@ -6729,7 +6742,11 @@ void kvm_mmu_print_sptes(struct kvm_vcpu *vcpu, gpa_t gpa, const char *msg)
 }
 EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_mmu_print_sptes);
 
-static void __kvm_mmu_invalidate_addr(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
+/*
+ * Returns false if the address cannot be invalidated, and the caller needs to
+ * free the entire root instead.
+ */
+static bool __kvm_mmu_invalidate_addr(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
 				      u64 addr, hpa_t root_hpa)
 {
 	struct kvm_shadow_walk_iterator iterator;
@@ -6742,13 +6759,23 @@ static void __kvm_mmu_invalidate_addr(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu
 	 * this is ever used to sync the guest_mmu, e.g. to emulate INVEPT.
 	 */
 	if (WARN_ON_ONCE(mmu != vcpu->arch.mmu))
-		return;
+		return false;
 
 	if (!VALID_PAGE(root_hpa))
-		return;
+		return true;
 
-	write_lock(&vcpu->kvm->mmu_lock);
+	guard(write_lock)(&vcpu->kvm->mmu_lock);
 	shadow_walk_init_using_root(&iterator, vcpu, root_hpa, addr);
+
+	/*
+	 * Sync cached roots that can't be walked by freeing them if they have
+	 * unsync children.  Roots that aren't backed by shadow pages (PAE and
+	 * dummy roots) are never cached, and failing to walk them means that
+	 * nothing is mapped at @addr, e.g. the relevant PDPTE isn't present.
+	 */
+	if (!shadow_walk_okay(&iterator))
+		return !root_to_sp(root_hpa) || !is_unsync_root(root_hpa);
+
 	for ( ; shadow_walk_okay(&iterator); shadow_walk_next(&iterator)) {
 		struct kvm_mmu_page *sp = sptep_to_sp(iterator.sptep);
 
@@ -6764,12 +6791,13 @@ static void __kvm_mmu_invalidate_addr(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu
 		if (!sp->unsync_children)
 			break;
 	}
-	write_unlock(&vcpu->kvm->mmu_lock);
+	return true;
 }
 
 void kvm_mmu_invalidate_addr(struct kvm_vcpu *vcpu, struct kvm_pagewalk *w,
 			     u64 addr, unsigned long roots)
 {
+	unsigned long roots_to_free = 0;
 	struct kvm_mmu *mmu;
 	int i;
 
@@ -6795,13 +6823,21 @@ void kvm_mmu_invalidate_addr(struct kvm_vcpu *vcpu, struct kvm_pagewalk *w,
 	if (!mmu->sync_spte)
 		return;
 
-	if (roots & KVM_MMU_ROOT_CURRENT)
-		__kvm_mmu_invalidate_addr(vcpu, mmu, addr, mmu->root.hpa);
+	if (roots & KVM_MMU_ROOT_CURRENT) {
+		if (WARN_ON_ONCE(!__kvm_mmu_invalidate_addr(vcpu, mmu, addr, mmu->root.hpa)))
+			roots_to_free |= KVM_MMU_ROOT_CURRENT;
+	}
 
 	for (i = 0; i < KVM_MMU_NUM_PREV_ROOTS; i++) {
-		if (roots & KVM_MMU_ROOT_PREVIOUS(i))
-			__kvm_mmu_invalidate_addr(vcpu, mmu, addr, mmu->prev_roots[i].hpa);
+		if (!(roots & KVM_MMU_ROOT_PREVIOUS(i)))
+			continue;
+
+		if (!__kvm_mmu_invalidate_addr(vcpu, mmu, addr, mmu->prev_roots[i].hpa))
+			roots_to_free |= KVM_MMU_ROOT_PREVIOUS(i);
 	}
+
+	if (roots_to_free)
+		kvm_mmu_free_roots(vcpu->kvm, mmu, roots_to_free);
 }
 EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_mmu_invalidate_addr);
 
-- 
2.56.0.385.gd3acb90ef8-goog

[PATCH 4/4] KVM: nVMX: Drop the TODO to sync only the affected SPTEs for INVVPID

[Yosry Ahmed]

Drop the TODO in handle_invvpid() about syncing only the affected SPTEs
when emulating single-address INVVPID with EPT disabled, instead of
freeing all of L2's roots.  Nested virtualization with EPT disabled is a
niche configuration, and no one cares about the performance of INVVPID
emulation in that setup.  If someone does care, they'll find the
overhead by profiling, whereas the TODO is more likely to send someone
chasing an optimization that isn't worth the effort.

Assisted-by: LLM
Signed-off-by: Yosry Ahmed <yosry@kernel.org>
---
 arch/x86/kvm/vmx/nested.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 9b0bfa2f854cf..57458b8455a41 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -6159,8 +6159,6 @@ static int handle_invvpid(struct kvm_vcpu *vcpu)
 	 *
 	 * Note, this operates on root_mmu, not guest_mmu, as L1 and L2 share
 	 * an MMU when EPT is disabled.
-	 *
-	 * TODO: sync only the affected SPTEs for INVDIVIDUAL_ADDR.
 	 */
 	if (!enable_ept)
 		kvm_mmu_free_guest_mode_roots(vcpu->kvm, &vcpu->arch.root_mmu);
-- 
2.56.0.385.gd3acb90ef8-goog