Re: [PATCH v2 2/2] KVM: arm64: nv: Delay freeing of shadow S2 structures until VM destruction

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Reading KVM stage-2 debugfs files after `mmu_notifier_release` causes a NULL pointer dereference or Use-After-Free (UAF).
--

--- Patch [2]: [PATCH v2 2/2] KVM: arm64: nv: Delay freeing of shadow S2 structures until VM destruction ---
commit 0e0fd6e2861a90f62fede091e967d2714a276ba0
Author: Marc Zyngier <[email protected]>
Subject: KVM: arm64: nv: Delay freeing of shadow S2 structures until VM destruction

This commit prevents the early freeing of the shadow stage-2 MMU container
structures from kvm_arch_flush_shadow_all(). It introduces a separate
destructor invoked during VM teardown, while keeping the page table freeing
in the original flush callback.

> diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
> index 99cd2ea61a220..db1824067dbdf 100644
> --- a/arch/arm64/kvm/nested.c
> +++ b/arch/arm64/kvm/nested.c
[ ... ]
> @@ -56,6 +56,27 @@ int kvm_init_nested(struct kvm *kvm)
>  	return kvm->arch.nested_mmus ? 0 : -ENOMEM;
>  }
>  
> +static void kvm_uninit_shadow_stage2_mmu(struct kvm *kvm)
> +{
> +	for (int i = 0; i < kvm->arch.nested_mmus_size; i++) {
> +		struct kvm_s2_mmu *mmu = kvm->arch.nested_mmus[i];
> +
> +		if (!WARN_ON(atomic_read(&mmu->refcnt)))
> +			kvm_free_stage2_pgd(mmu);

[Severity: High]
This is a pre-existing issue, but does reading the KVM stage-2 debugfs files
after mmu_notifier_release cause a NULL pointer dereference or use-after-free?

While this patch prevents the use-after-free of the mmu container itself, it
appears mmu->pgt is still vulnerable. When a process exits,
mmu_notifier_release triggers kvm_arch_flush_shadow_all(), which calls down
to kvm_free_stage2_pgd().

kvm_free_stage2_pgd() sets mmu->pgt to NULL and frees the page table
structure:

arch/arm64/kvm/mmu.c:kvm_free_stage2_pgd() {
    write_lock(&kvm->mmu_lock);
    pgt = mmu->pgt;
    if (pgt) {
        mmu->pgd_phys = 0;
        mmu->pgt = NULL;
        ...
    }
    ...
    write_unlock(&kvm->mmu_lock);
    if (pgt) {
        kvm_stage2_destroy(pgt);
        kfree(pgt);
    }
}

However, the KVM VM file descriptor and its debugfs dentries can remain alive.
If a process holding the VM fd reads debugfs files like ipa_range or
stage2_levels, functions like kvm_pgtable_debugfs_open() or
kvm_ptdump_parser_create() will read mmu->pgt without holding kvm->mmu_lock:

arch/arm64/kvm/ptdump.c:kvm_ptdump_parser_create() {
    struct kvm_pgtable *pgtable = mmu->pgt;
    ...
    ret = kvm_ptdump_build_levels(&st->level[0], pgtable->start_level);
    ...
}

Could a concurrent read of mmu->pgt either dereference a NULL pointer, or
pass a soon-to-be-freed page table pointer to the seq_file show callbacks
such as kvm_pgtable_range_show()?

> +	}
> +}
> +
> +void kvm_destroy_nested(struct kvm *kvm)
> +{
> +	kvm_uninit_shadow_stage2_mmu(kvm);
> +
> +	for (int i = 0; i < kvm->arch.nested_mmus_size; i+= S2_MMU_PER_VCPU)
> +		kvfree(kvm->arch.nested_mmus[i]);
> +
> +	kvm->arch.nested_mmus_size = 0;
> +	kvfree(kvm->arch.nested_mmus);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2