Re: [PATCH v2 2/2] KVM: arm64: nv: Fix null ptr deref on nested wp/unmap, teardown race

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

Pre-existing issues:
- [High] Use-After-Free of `kvm->arch.nested_mmus` array during nested page table walks
- [High] Memory Leak of `kvm->arch.nested_mmus` on VM teardown via fd closure
--

--- Patch [2]: [PATCH v2 2/2] KVM: arm64: nv: Fix null ptr deref on nested wp/unmap, teardown race ---
commit 82456f58d55471e0202bc28188ae985c14d4be0a
Author: Lorenzo Stoakes (ARM) <[email protected]>

KVM: arm64: nv: Fix null ptr deref on nested wp/unmap, teardown race

This commit fixes a NULL pointer dereference in the nested page table walk
paths. It resolves a race condition where concurrent stage 2 teardown can set
the page table pointer to NULL while the MMU lock is temporarily dropped.

> diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
> index 17123f0b6daba..f69722e1592a5 100644
> --- a/arch/arm64/kvm/nested.c
> +++ b/arch/arm64/kvm/nested.c

[ ... ]

> @@ -1295,7 +1306,7 @@ void kvm_nested_s2_unmap(struct kvm *kvm, bool may_block)
>  			kvm_stage2_unmap_range(mmu, 0, kvm_phys_size(mmu), may_block);

[Severity: High]
This isn't a bug introduced by this patch, but can this lead to a
use-after-free of the mmu pointer?

When we pass the array element pointer down the call chain:

kvm_nested_s2_unmap()
  -> kvm_stage2_unmap_range()
    -> __unmap_stage2_range()
      -> stage2_apply_range()

stage2_apply_range() can drop kvm->mmu_lock by calling
cond_resched_rwlock_write().

Concurrently, kvm_vcpu_init_nested() can reallocate and free the
kvm->arch.nested_mmus array, or kvm_arch_flush_shadow_all() can free it
locklessly. When stage2_apply_range() reacquires the lock, could it
dereference the dangling mmu pointer?

[Severity: High]
This is a pre-existing issue, but does kvm->arch.nested_mmus leak during VM
teardown via fd closure?

It looks like the array is currently only freed in kvm_arch_flush_shadow_all(),
which is triggered exclusively by the MMU notifier's release hook.

If a VM is closed via file descriptor, mmu_notifier_unregister() is called
which does not invoke the release callback. Since kvm_arch_destroy_vm() doesn't
appear to clean up the nested_mmus array, is this memory permanently orphaned?

>  	}
>  
> -	kvm_invalidate_vncr_ipa(kvm, 0, BIT(kvm->arch.mmu.pgt->ia_bits));
> +	kvm_invalidate_vncr_ipa_all(kvm);
>  }

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