Re: [PATCH] KVM: nVMX: Don't flush shadow VMCS12 to guest memory during vCPU teardown

[Sean Christopherson <seanjc@google.com>]

On Tue, Sep 08, 2026, Jim Mattson wrote:
> On Tue, Sep 8, 2026 at 10:35 AM James Houghton <jthoughton@google.com> wrote:
> >
> > On Tue, Sep 8, 2026 at 6:54 AM Jim Mattson <jmattson@google.com> wrote:
> > >
> > > When a vCPU is destroyed while L2 is active, KVM synthesizes a nested
> > > VM-Exit, which flushes the cached shadow VMCS12 back to guest memory:
> > >
> > >   vmx_vcpu_free()
> > >   |-> nested_vmx_free_vcpu()
> > >       |-> vmx_leave_nested()
> > >           |-> nested_vmx_vmexit(vcpu, -1, 0, 0)
> > >               |-> nested_flush_cached_shadow_vmcs12()
> > >                   |-> kvm_write_guest_cached()
> > >                       |-> __copy_to_user(ghc->hva, ...)
> > >
> > > During process exit, do_exit() calls exit_mm() before closing file
> > > descriptors, so vCPU destruction runs with current->mm == NULL on a
> > > borrowed lazy TLB active_mm. If the borrowed address space has a writable
> > > mapping at ghc->hva, __copy_to_user() corrupts an unrelated task's memory.
> > >
> > > Skip the flush when KVM synthesizes a VM-exit with vm_exit_reason == -1
> > > (e.g. during vCPU teardown or SMM entry). In these paths, KVM forces the
> > > vCPU out of guest mode internally--no architectural VM-exit is delivered
> > > to L1.
> > >
> > > Fixes: 61ada7488ffd ("KVM: nVMX: Cache shadow vmcs12 on VMEntry and flush to memory on VMExit")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Jim Mattson <jmattson@google.com>
> >
> > Hi Jim,
> >
> > This patch looks good for a stable backport, but I feel this kind of
> > bug could easily happen again with the current API.
> >
> > One simple thing that would have prevented this would be to:
> > 1. Change kvm_write_guest_cached() (and related) to take the vcpu
> > instead of the kvm, and
> > 2. Check that vcpu->mm == current->mm before doing the uaccess.

The mm pointer is store in "kvm", not in "kvm_vcpu", i.e. this sort of hardening
shouldn't require modifying the callsites.

> > This wouldn't be as suitable for a backport, but I think it's better
> > for preventing similar classes of bugs. I'm sure you (and
> > Paolo/Sean/others) will have better ideas. What do you think?
> 
> There seems to be some awareness of this issue within KVM, so that may
> not be necessary. Also, as Sashiko points out, we have issues with
> reads as well as writes. I don't know how many choke points we'd have
> to test.
> 
> AFAICT (with AI assistance), all of the current problems are rooted in
> the following call to nested_vmx_vmexit():
> 
> void vmx_leave_nested(struct kvm_vcpu *vcpu)
> {
>         if (is_guest_mode(vcpu)) {
>                 vcpu->arch.nested_run_pending = 0;
>                 nested_vmx_vmexit(vcpu, -1, 0, 0);
>         }
>         free_nested(vcpu);
> }
> 
> On the SVM side, there is no comparable faux VM-exit. Instead of
> calling nested_svm_vmexit(), the teardown of SVM nested state is an
> open-coded sequence. Maybe something like that would work better for
> VMX?

Yes, vmx_leave_nested()'s use of nested_vmx_vmexit() is an endless source of pain
and needs to be rewritten.

However, the bigger flaw is that the memslots are still valid when the VM is
being destroyed.  The hardening James suggested above really should be hardening,
not the primary mechanism for ensuring correctness.

Manually freeing each memslot one-by-one isn't a great option, because the latency
introduced by each synchronize_srcu_expedited() call would be rather absurd.  But
once KVM unregisters its mmu_notifier, i.e. once kvm_mmu_notifier_release() and
thus kvm_flush_shadow_all() runs, all indirect references to memslots need to be
gone.  And by "indirect references" I mean code in KVM that relies on a memslot
existing and being reachable, e.g. x86's rmaps and shadow page accounting.

KVM is infuriatingly close to enforcing that already, as only kvm_arch_destroy_vm()
and kvm_destroy_devices() run between unregistering the mmu_notifier and freeing
all memslot metadata.

	mmu_notifier_unregister(&kvm->mmu_notifier, kvm->mm);
	/*
	 * At this point, pending calls to invalidate_range_start()
	 * have completed but no more MMU notifiers will run, so
	 * mn_active_invalidate_count may remain unbalanced.
	 * No threads can be waiting in kvm_swap_active_memslots() as the
	 * last reference on KVM has been dropped, but freeing
	 * memslots would deadlock without this manual intervention.
	 *
	 * If the count isn't unbalanced, i.e. KVM did NOT unregister its MMU
	 * notifier between a start() and end(), then there shouldn't be any
	 * in-progress invalidations.
	 */
	WARN_ON(rcuwait_active(&kvm->mn_memslots_update_rcuwait));
	if (kvm->mn_active_invalidate_count)
		kvm->mn_active_invalidate_count = 0;
	else
		WARN_ON(kvm->mmu_invalidate_in_progress);
	kvm_arch_destroy_vm(kvm);
	kvm_destroy_devices(kvm);
	for (i = 0; i < kvm_arch_nr_memslot_as_ids(kvm); i++) {
		kvm_free_memslots(kvm, &kvm->__memslots[i][0]);
		kvm_free_memslots(kvm, &kvm->__memslots[i][1]);  <=== KVM will be very sad after this

	}

Destroying memslots before kvm_destroy_devices() is a-ok, all implementations do
nothing more than kvm_io_bus_unregister_dev() (a nop at this point in the VM's
lifecycle, as buses are already destroyed), and free of memory.

Unfortunately, kvm_arch_destroy_vm() is practically infeasible to audit.  But, we
don't need to audit that code, we just need to audit kvm_arch_flush_shadow_memslot(),
because if there are memslot references that are dropped by flush_shadow_memslot()
but not flush_shadow_all(), then KVM is already buggy and vulnerable.

*sigh*

And of course PPC is a disaster and does literally nothing on flush_shadow_all().
Doubly hilarious, the worst offender seems to be kvmhv_release_all_nested().  As
a quick and dirty fix, I think we can simply iterate over memslots and manually
flush each one?

Which, amazingly, appears to be safe even though kvmppc_uvmem_drop_pages() takes
mmap_lock, as exit_mmap()'s call to mmu_notifier_release() is (thanfully) super
obviously done without hold mmap_lock.

	/* mm's last user has gone, and its about to be pulled down */
	mmu_notifier_release(mm);

	mmap_read_lock(mm);

diff --git arch/powerpc/include/asm/kvm_host.h arch/powerpc/include/asm/kvm_host.h
index 2d139c807577..1c8d9d7360e9 100644
--- arch/powerpc/include/asm/kvm_host.h
+++ arch/powerpc/include/asm/kvm_host.h
@@ -903,7 +903,6 @@ struct kvm_vcpu_arch {
 #define __KVM_HAVE_CREATE_DEVICE
 
 static inline void kvm_arch_memslots_updated(struct kvm *kvm, u64 gen) {}
-static inline void kvm_arch_flush_shadow_all(struct kvm *kvm) {}
 static inline void kvm_arch_vcpu_blocking(struct kvm_vcpu *vcpu) {}
 static inline void kvm_arch_vcpu_unblocking(struct kvm_vcpu *vcpu) {}
 
diff --git arch/powerpc/kvm/powerpc.c arch/powerpc/kvm/powerpc.c
index 9194cf492d1c..2b88fb9e9bc4 100644
--- arch/powerpc/kvm/powerpc.c
+++ arch/powerpc/kvm/powerpc.c
@@ -764,6 +764,17 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
 	kvmppc_core_commit_memory_region(kvm, old, new, change);
 }
 
+void kvm_arch_flush_shadow_all(struct kvm *kvm)
+{
+	struct kvm_memory_slot *memslot;
+	struct kvm_memslots *slots;
+	int bkt;
+
+	slots = kvm_memslots(kvm);
+	kvm_for_each_memslot(memslot, bkt, slots)
+		kvmppc_core_flush_memslot(kvm, slot);
+}
+
 void kvm_arch_flush_shadow_memslot(struct kvm *kvm,
 				   struct kvm_memory_slot *slot)
 {

If the above works for PPC, then KVM can nuke memslots before calling into
kvm_arch_destroy_vm().  x86's asinine memslot deletion in kvm_arch_destroy_vm()
needs to be addressed, but that code exists purely to do vm_munmap(), and can
and should be moved to kvm_arch_free_memslot().

All that said, I'm not sure this aggressive fix is the right thing to send to
stable@.  For that, James' suggestion of hardening KVM's usage of
__copy_{to,from}_user() seems like the best blend of being comprehensive without
being overly invasive/risky.

So, as an immediate set of changes, what if we do this over ~5 patches, with patches
1 and 2 tagged for stable@?

  1. Add kvm_copy_{to,from}_user{,_inatomic)() and return -EFAULT if current->mm
     is not kvm->mm.
  2. Hack-a-fix PPC's kvm_arch_flush_shadow_all().
  3. Do x86's vm_munmap() in kvm_arch_free_memslot().
  4. Nuke memslots before calling kvm_arch_destroy_vm().
  5. Change the current->mm checks in kvm_copy_{to,from}_user{,_inatomic)() to
     WARN_ON_ONCE() on failure.

And then in the near-ish future, take things a step further and do:

  6. Fix the vmx_leave_nested() trainwreck.
  7. Harden the common kvm_{read,write}_guest family of APIs even further by
     adding an early WARN_ON_ONCE() on current->mm != kvm->mm, i.e. to detect
     bad KVM behavior as additional defense-in-depth.

The diff for #3 and #4 (lightly tested on x86):

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 4b3681796c75..f05ba2d1364d 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -9925,7 +9925,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
  * @size > 0 to install a new slot, while @size == 0 to uninstall a
  * slot.  The return code can be one of the following:
  *
- *   HVA:           on success (uninstall will return a bogus HVA)
+ *   HVA:           on success (uninstall will return a NULL HVA)
  *   -errno:        on error
  *
  * The caller should always use IS_ERR() to check the return value
@@ -9938,10 +9938,10 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
 void __user * __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa,
 				      u32 size)
 {
-	int i, r;
-	unsigned long hva, old_npages;
 	struct kvm_memslots *slots = kvm_memslots(kvm);
 	struct kvm_memory_slot *slot;
+	unsigned long hva;
+	int i, r;
 
 	lockdep_assert_held(&kvm->slots_lock);
 
@@ -9965,8 +9965,7 @@ void __user * __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa,
 		if (!slot || !slot->npages)
 			return NULL;
 
-		old_npages = slot->npages;
-		hva = slot->userspace_addr;
+		hva = 0;
 	}
 
 	for (i = 0; i < kvm_arch_nr_memslot_as_ids(kvm); i++) {
@@ -9982,9 +9981,6 @@ void __user * __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa,
 			return ERR_PTR_USR(r);
 	}
 
-	if (!size)
-		vm_munmap(hva, old_npages * PAGE_SIZE);
-
 	return (void __user *)hva;
 }
 EXPORT_SYMBOL_FOR_KVM_INTERNAL(__x86_set_memory_region);
@@ -10013,20 +10009,6 @@ void kvm_arch_pre_destroy_vm(struct kvm *kvm)
 
 void kvm_arch_destroy_vm(struct kvm *kvm)
 {
-	if (current->mm == kvm->mm) {
-		/*
-		 * Free memory regions allocated on behalf of userspace,
-		 * unless the memory map has changed due to process exit
-		 * or fd copying.
-		 */
-		mutex_lock(&kvm->slots_lock);
-		__x86_set_memory_region(kvm, APIC_ACCESS_PAGE_PRIVATE_MEMSLOT,
-					0, 0);
-		__x86_set_memory_region(kvm, IDENTITY_PAGETABLE_PRIVATE_MEMSLOT,
-					0, 0);
-		__x86_set_memory_region(kvm, TSS_PRIVATE_MEMSLOT, 0, 0);
-		mutex_unlock(&kvm->slots_lock);
-	}
 	if (kvm->arch.created_mediated_pmu)
 		perf_release_mediated_pmu();
 	kvm_destroy_vcpus(kvm);
@@ -10066,6 +10048,16 @@ void kvm_arch_free_memslot(struct kvm *kvm, struct kvm_memory_slot *slot)
 	}
 
 	kvm_page_track_free_memslot(slot);
+
+	/*
+	 * Free memory regions allocated on behalf of userspace, unless the
+	 * memory map has changed due to process exit or fd copying.  Leak the
+	 * mapping on failure, e.g. if the task is killed, worst case scenario,
+	 * the page(s) will be reclaimed when the process exits.
+	 */
+	if (current->mm == kvm->mm && slot->id >= KVM_USER_MEM_SLOTS &&
+	    !WARN_ON_ONCE(!slot->npages))
+		vm_munmap(slot->userspace_addr, slot->npages * PAGE_SIZE);
 }
 
 int memslot_rmap_alloc(struct kvm_memory_slot *slot, unsigned long npages)
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 03bfc92864b6..bed48f70b2d8 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -1693,6 +1693,7 @@ int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu);
 bool kvm_arch_dy_runnable(struct kvm_vcpu *vcpu);
 bool kvm_arch_dy_has_pending_interrupt(struct kvm_vcpu *vcpu);
 bool kvm_arch_vcpu_preempted_in_kernel(struct kvm_vcpu *vcpu);
+void kvm_arch_destroy_memslots(struct kvm *kvm);
 void kvm_arch_pre_destroy_vm(struct kvm *kvm);
 void kvm_arch_create_vm_debugfs(struct kvm *kvm);
 
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 65eb26a0520d..899bf5970434 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -945,23 +945,36 @@ static void kvm_free_memslot(struct kvm *kvm, struct kvm_memory_slot *slot)
 	kfree(slot);
 }
 
-static void kvm_free_memslots(struct kvm *kvm, struct kvm_memslots *slots)
+static const struct kvm_memslots kvm_empty_memslots = {
+	.generation = -1ull,
+	.hva_tree = RB_ROOT_CACHED,
+	.gfn_tree = RB_ROOT,
+	.id_hash[0 ... (ARRAY_SIZE(kvm_empty_memslots.id_hash) - 1)] = HLIST_HEAD_INIT,
+	.node_idx = 0,
+};
+
+static void kvm_destroy_memslots(struct kvm *kvm)
 {
 	struct hlist_node *idnode;
 	struct kvm_memory_slot *memslot;
-	int bkt;
+	int bkt, i;
+
+	mutex_lock(&kvm->slots_lock);
+	for (i = 0; i < kvm_arch_nr_memslot_as_ids(kvm); i++)
+		rcu_assign_pointer(kvm->memslots[i], &kvm_empty_memslots);
+
+	synchronize_srcu_expedited(&kvm->srcu);
+	mutex_unlock(&kvm->slots_lock);
 
 	/*
 	 * The same memslot objects live in both active and inactive sets,
-	 * arbitrarily free using index '1' so the second invocation of this
-	 * function isn't operating over a structure with dangling pointers
-	 * (even though this function isn't actually touching them).
+	 * arbitrarily free using index '1'.
 	 */
-	if (!slots->node_idx)
-		return;
-
-	hash_for_each_safe(slots->id_hash, bkt, idnode, memslot, id_node[1])
-		kvm_free_memslot(kvm, memslot);
+	for (i = 0; i < kvm_arch_nr_memslot_as_ids(kvm); i++) {
+		hash_for_each_safe(kvm->__memslots[i][1].id_hash, bkt, idnode,
+				   memslot, id_node[1])
+			kvm_free_memslot(kvm, memslot);
+	}
 }
 
 static umode_t kvm_stats_debugfs_mode(const struct kvm_stats_desc *desc)
@@ -1292,12 +1305,10 @@ static void kvm_destroy_vm(struct kvm *kvm)
 		kvm->mn_active_invalidate_count = 0;
 	else
 		WARN_ON(kvm->mmu_invalidate_in_progress);
+	kvm_destroy_memslots(kvm);
+
 	kvm_arch_destroy_vm(kvm);
 	kvm_destroy_devices(kvm);
-	for (i = 0; i < kvm_arch_nr_memslot_as_ids(kvm); i++) {
-		kvm_free_memslots(kvm, &kvm->__memslots[i][0]);
-		kvm_free_memslots(kvm, &kvm->__memslots[i][1]);
-	}
 	cleanup_srcu_struct(&kvm->irq_srcu);
 	srcu_barrier(&kvm->srcu);
 	cleanup_srcu_struct(&kvm->srcu);
@@ -2005,6 +2016,9 @@ static int kvm_set_memory_region(struct kvm *kvm,
 
 	lockdep_assert_held(&kvm->slots_lock);
 
+	if (WARN_ON_ONCE(!refcount_read(&kvm->users_count)))
+		return -EIO;
+
 	r = check_memory_region_flags(kvm, mem);
 	if (r)
 		return r;