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

[sashiko-bot@kernel.org]

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.

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