Re: [PATCH v2 05/10] KVM: Destroy memslots immediately after mmu_notifiers are unregistered

[Sean Christopherson <seanjc@google.com>]

On Fri, Oct 02, 2026, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Critical] kvm: x86/mmu: Kernel panic via NULL pointer dereference in gfn_to_rmap() during VM destruction
> --
> 
> commit 51cc08e54e96c8c842b1348871cdf24648cdd20a
> Author: Sean Christopherson <seanjc@google.com>
> 
> KVM: Destroy memslots immediately after mmu_notifiers are unregistered
> 
> This commit installs dummy, empty memslots immediately after unregistering
> KVM's mmu_notifier during VM destruction. It aims to harden against
> accessing memory from the wrong address space when tearing down a VM.
> 
> > Instally dummy, empty memslots immediately after unregistering KVM's
> 
> This isn't a bug, but there is a typo (Instally) here.
> 
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index f368240aa1cdd..770a2c3bd4458 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> [ ... ]
> > @@ -1302,12 +1321,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);
> 
> [Severity: Critical]
> Does calling kvm_destroy_memslots() before kvm_arch_destroy_vm() risk a NULL
> pointer dereference?
> 
> If a user creates a VM, maps memory into it (creating leaf SPTEs),
> and closes the VM file descriptor without terminating the process
> (mm_users remains > 0), mmu_notifier_unregister() is called.
> 
> However, it does not invoke the release callback because the process's
> mm is still alive.

Ha!  Sashiko: 100, Sean: 1

mmu_notifier_unregister() invokes the subscription's ->release() callback.  FWIW,
at one point in the past there was a no_release() variant, but it got dropped by
commit c96245148c1e ("mm/mmu_notifiers: remove unregister_no_release").

  void mmu_notifier_unregister(struct mmu_notifier *subscription,
			     struct mm_struct *mm)
  {
	BUG_ON(atomic_read(&mm->mm_count) <= 0);

	if (!hlist_unhashed(&subscription->hlist)) {
		/*
		 * SRCU here will force exit_mmap to wait for ->release to
		 * finish before freeing the pages.
		 */
		int id;

		id = srcu_read_lock(&srcu);
		/*
		 * exit_mmap will block in mmu_notifier_release to guarantee
		 * that ->release is called before freeing the pages.
		 */
		if (subscription->ops->release)
			subscription->ops->release(subscription, mm);   <=========== TAKE THAT SASHIKO
		srcu_read_unlock(&srcu, id);

		spin_lock(&mm->notifier_subscriptions->lock);
		/*
		 * Can not use list_del_rcu() since __mmu_notifier_release
		 * can delete it before we hold the lock.
		 */
		hlist_del_init_rcu(&subscription->hlist);
		spin_unlock(&mm->notifier_subscriptions->lock);
	}

	/*
	 * Wait for any running method to finish, of course including
	 * ->release if it was run by mmu_notifier_release instead of us.
	 */
	synchronize_srcu(&srcu);

	BUG_ON(atomic_read(&mm->mm_count) <= 0);

	mmdrop(mm);
  }
  EXPORT_SYMBOL_GPL(mmu_notifier_unregister);

> As a result, kvm_arch_flush_shadow_all() is skipped and shadow pages
> remain intact.
> 
> Next, kvm_destroy_memslots() replaces the active memslots with
> kvm_empty_memslots.
> 
> Then, kvm_arch_destroy_vm() tears down the vCPUs via
> kvm_destroy_vcpus(), calling kvm_mmu_unload(), which recursively zaps
> the remaining shadow pages.
> 
> When leaf SPTEs are dropped, rmap_remove() looks up the memslot for
> the GFN:
> 
> arch/x86/kvm/mmu/mmu.c:rmap_remove() {
>     ...
>     slot = __gfn_to_memslot(slots, gfn);
>     rmap_head = gfn_to_rmap(gfn, sp->role.level, slot);
>     ...
> }
> 
> Since the memslots are now empty, __gfn_to_memslot() returns NULL,
> which is then unconditionally dereferenced by gfn_to_rmap():
> 
> arch/x86/kvm/mmu/mmu.c:gfn_to_rmap() {
>     ...
>     idx = gfn_to_index(gfn, slot->base_gfn, level);
>     return &slot->arch.rmap[level - PG_LEVEL_4K][idx];
> }
> 
> Could this lead to a host kernel panic during VM destruction?
> 
> >  	kvm_destroy_devices(kvm);
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20261001202234.3794060-1-seanjc@google.com?part=5