Re: [PATCH 1/2] KVM: arm64: Fix spurious warning for benign stage 2 teardown race

On Wed, Aug 12, 2026 at 01:54:29PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> Pre-existing issues:
> - [Critical] Use-After-Free of the dynamically allocated `kvm->arch.nested_mmus` array when the MMU lock is dropped during stage-2 unmap operations.
> - [High] NULL Pointer Dereference in `kvm_vcpu_init_nested()` during concurrent VM teardown.
> --
>
> --- Patch [1]: [PATCH 1/2] KVM: arm64: Fix spurious warning for benign stage 2 teardown race ---
> commit ffda2ec0115c7506d269acccba8c1f4511410e87
> Author: Lorenzo Stoakes (ARM) <[email protected]>
>
> KVM: arm64: Fix spurious warning for benign stage 2 teardown race
>
> This patch suppresses a warning in stage2_apply_range() when the page table
> pointer is found to be NULL after reacquiring the MMU lock. It assumes that
> the NULL page table is a benign result of a concurrent teardown.
>
> > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> > index 74e7e7f7564c8..31e049ded0932 100644
> > --- a/arch/arm64/kvm/mmu.c
> > +++ b/arch/arm64/kvm/mmu.c
> > @@ -59,19 +59,25 @@ static phys_addr_t stage2_range_addr_end(phys_addr_t addr, phys_addr_t end)
> >   * long will also starve other vCPUs. We have to also make sure that the page
> >   * tables are not freed while we released the lock.
> >   */
> > -static int stage2_apply_range(struct kvm_s2_mmu *mmu, phys_addr_t addr,
> > +static int stage2_apply_range(struct kvm_s2_mmu *mmu, phys_addr_t start,
> >  			      phys_addr_t end,
> >  			      int (*fn)(struct kvm_pgtable *, u64, u64),
> >  			      bool resched)
> >  {
> >  	struct kvm *kvm = kvm_s2_mmu_to_kvm(mmu);
> > +	phys_addr_t addr = start;
> >  	int ret;
> >  	u64 next;
> >
> >  	do {
> >  		struct kvm_pgtable *pgt = mmu->pgt;
> > +		/*
> > +		 * We may be raced on PGT teardown when we release the
> > +		 * kvm->mmu_lock. That's fine as the PGT is legitimately no
> > +		 * longer present.
> > +		 */
> >  		if (!pgt)
> > -			return -EINVAL;
> > +			return resched && addr > start ? 0 : -EINVAL;
>
> [Severity: Critical]
> This is a pre-existing issue, but does suppressing this warning mask a
> regression where the mmu pointer itself is used after being freed?
>
> When kvm_nested_s2_unmap() passes an mmu pointer to stage2_apply_range(),
> that pointer references a dynamically allocated array:
>
> arch/arm64/kvm/nested.c:kvm_nested_s2_unmap() {
>     ...
>     struct kvm_s2_mmu *mmu = &kvm->arch.nested_mmus[i];
>     if (kvm_s2_mmu_valid(mmu))
>         kvm_stage2_unmap_range(mmu, ...);
> }
>
> If cond_resched_rwlock_write() drops the lock inside stage2_apply_range(),
> a concurrent VM teardown or VCPU initialization can completely free this
> array:
>
> arch/arm64/kvm/nested.c:kvm_arch_flush_shadow_all() {
>     ...
>     kvfree(kvm->arch.nested_mmus);
>     kvm->arch.nested_mmus = NULL;
> }
>
> When the lock is reacquired, the loop continues and accesses mmu->pgt
> from the now-freed memory. Is it possible that the warning was actually
> firing because we were reading a freed mmu structure?

All stuff being handled elsewhere with Marc's series AFAICT.

>
>
> [Severity: High]
> This is also a pre-existing issue, but looking at the concurrent VM teardown
> path, does kvm_vcpu_init_nested() have a NULL pointer dereference?
>
> When kvm_vcpu_init_nested() reallocates the array, it unconditionally
> updates the mmu pointer for each entry:
>
> arch/arm64/kvm/nested.c:kvm_vcpu_init_nested() {
>     ...
>     for (int i = 0; i < kvm->arch.nested_mmus_size; i++)
>         tmp[i].pgt->mmu = &tmp[i];
>     ...
> }
>
> If kvm_arch_flush_shadow_all() concurrently calls kvm_free_stage2_pgd()
> and sets pgt to NULL before the lock is acquired here, wouldn't this crash
> when dereferencing tmp[i].pgt?

This seems like what my 2nd patch handles :)

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

--
Cheers, Lorenzo