Re: [PATCH 2/2] KVM: arm64: nv: Fix null ptr deref in kvm_nested_s2_unmap() on S2 teardown

On Wed, Aug 12, 2026 at 02:01:01PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>
> Pre-existing issues:
> - [Critical] Use-After-Free on `kvm->arch.nested_mmus` array due to concurrent teardown.

Addressed by Marc's changes + discussed over there.

> - [Critical] Incomplete Fix: `kvm_nested_s2_wp()` is vulnerable to the same NULL pointer dereference and UAF.

OK interesting!

> - [High] Data Race: `kvm->arch.nested_mmus_size` updated locklessly causing uninitialized memory reads.

Addressed by Marc's changes + discussed over there.

> --
>
> --- Patch [2]: [PATCH 2/2] KVM: arm64: nv: Fix null ptr deref in kvm_nested_s2_unmap() on S2 teardown ---
> commit 04c9d62ec51b0000a28c8099341f6a2d31ce7848
> Author: Lorenzo Stoakes (ARM) <[email protected]>
>
> KVM: arm64: nv: Fix null ptr deref in kvm_nested_s2_unmap() on S2 teardown
>
> This commit adds a NULL check for kvm->arch.mmu.pgt in kvm_nested_s2_unmap()
> to prevent a NULL pointer dereference. This happens when the MMU lock is
> dropped during stage 2 teardown, allowing concurrent progress that clears the
> page table.
>
> > diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
> > index 17123f0b6daba..9fc0b1696dcb6 100644
> > --- a/arch/arm64/kvm/nested.c
> > +++ b/arch/arm64/kvm/nested.c
> > @@ -1295,7 +1295,11 @@ void kvm_nested_s2_unmap(struct kvm *kvm, bool may_block)
> >  			kvm_stage2_unmap_range(mmu, 0, kvm_phys_size(mmu), may_block);
>
> [Severity: Critical]
> This isn't a bug introduced by this patch, but does this loop contain a
> Use-After-Free vulnerability?
>
> While iterating over the nested_mmus array in kvm_nested_s2_unmap(),
> kvm_stage2_unmap_range() can drop kvm->mmu_lock inside stage2_apply_range().
>
> Concurrently, if kvm_mmu_notifier_release() calls kvm_arch_flush_shadow_all(),
> it frees the kvm->arch.nested_mmus array unconditionally and without taking
> kvm->mmu_lock:
>
> kvm_arch_flush_shadow_all() {
>     ...
>     kvfree(kvm->arch.nested_mmus);
>     ...
> }
>
> If the array is freed during the lock-drop window, the local mmu pointer
> becomes dangling, and accessing it when the lock is reacquired could result
> in a Use-After-Free.
>
> >  	}
> >
> > -	kvm_invalidate_vncr_ipa(kvm, 0, BIT(kvm->arch.mmu.pgt->ia_bits));
> > +	/* NULL pgt should only be possible if raced when mmu_lock dropped. */
> > +	if (kvm->arch.mmu.pgt)
> > +		kvm_invalidate_vncr_ipa(kvm, 0, BIT(kvm->arch.mmu.pgt->ia_bits));
> > +	else
> > +		WARN_ON(!may_block);
> >  }
>
> [Severity: Critical]
> This is a pre-existing issue, but does kvm_nested_s2_wp() suffer from the exact
> same NULL pointer dereference and Use-After-Free vulnerability?
>
> Like kvm_nested_s2_unmap(), kvm_nested_s2_wp() iterates over the nested_mmus
> array and drops mmu_lock via stage2_apply_range_resched(). When it resumes
> and exits the loop, it also unconditionally executes
> BIT(kvm->arch.mmu.pgt->ia_bits).
>
> Should the same fix be applied to kvm_nested_s2_wp() to prevent a guaranteed
> NULL pointer dereference when kvm_arch_flush_shadow_all() clears the page
> table in the window where the lock is dropped?

Yeah this seems valid actually, will note and can send as a follow-up?

>
> [Severity: High]
> This isn't a bug introduced by this patch, but is there a data race on
> kvm->arch.nested_mmus_size?
>
> In kvm_vcpu_init_nested(), nested_mmus_size is updated outside of
> kvm->mmu_lock and without memory barriers:
>
> kvm_vcpu_init_nested() {
>     ...
>     kvm->arch.nested_mmus_size = num_mmus;
>     ...
> }
>
> A concurrent reader like kvm_nested_s2_unmap() can see the updated size
> before the stores initializing the new array elements become visible. Because
> the new array is allocated with kvcalloc(), an uninitialized/zeroed
> mmu->tlb_vttbr causes kvm_s2_mmu_valid() to incorrectly evaluate to TRUE.
>
> Will this cause the reader to process the zeroed mmu structure, passing it
> to kvm_stage2_unmap_range(), which would dereference a NULL mmu->pgt or
> access a partially initialized pgt?
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2

--
Cheers, Lorenzo