Re: [PATCH] KVM: arm64: nv: Fix life cycle of the nested_mmus array

On Wed, 12 Aug 2026 15:05:21 +0100,
"Lorenzo Stoakes (ARM)" <[email protected]> wrote:
> 
> On Tue, Aug 11, 2026 at 01:20:57PM +0100, Marc Zyngier wrote:
> > The nested_mmus array holds the shadow page tables that are used when
> > a guest is running a nested context. These structures are allocated on
> > VCPU_INIT for whole guest, which implies that they may have to be
> > relocated as the array grows.
> >
> > Should a VCPU_INIT occur whilst a vcpu is actively running an L2 and
> > that the allocation requires relocation, that vcpu will still be
> > running with a pointer to the previous structure, which will have been
> > freed.
> >
> > Fix this by turning the array of structures to an array of pointers,
> > which is now allocated at VM creation, sized to the absolute maximum
> > that KVM can handle.
> >
> > In turn, each VCPU_INIT contributes S2_MMU_PER_VCPU to the pool. No
> > reallocation is ever performed, and the life cycle of each object is
> > much clearer:
> >
> > - the nested_mmus array is allocated in kvm_init_nested(), and freed
> >   in kvm_arch_destroy_vm()
> >
> > - s2_mmu structures are allocated in kvm_vcpu_init_nested(), and freed
> >   on kvm_arch_flush_shadow_all()
> >
> > Finally, the freeing of vcpu->arch.vncr_array is made consistent
> > rather than being done on some failure paths, but not others.
> >
> > Fixes: 4f128f8e1aaa ("KVM: arm64: nv: Support multiple nested Stage-2 mmu structures")
> > Reported-by: Shen Yongchao <[email protected]>
> > Reported-by: Karl Mehltretter <[email protected]>
> > Suggested-by: Karl Mehltretter <[email protected]>
> > Link: https://lore.kernel.org/r/[email protected]
> > Signed-off-by: Marc Zyngier <[email protected]>
> 
> This addresses the same kind of stuff I had a couple of patches in my
> series for :>)
> 
> I think there are still some problems with it, see below.
> 
> Also I attach my original patch for the UAF below in case it's useful! I
> had another for the init stuff, will reply with that separately also :)
> 
> > Cc: [email protected]
> > ---
> >
> > Notes:
> >     Sending this as a first class patch, since the other approaches were even
> >     uglier than this one. I'm still displeased with kvm_arch_flush_shadow_all(),
> >     but that's a step in the direction of tightening it:
>

[...]

> > @@ -1316,16 +1308,15 @@ void kvm_nested_s2_flush(struct kvm *kvm)
> >
> >  void kvm_arch_flush_shadow_all(struct kvm *kvm)
> >  {
> > -	int i;
> > -
> > -	for (i = 0; i < kvm->arch.nested_mmus_size; i++) {
> > -		struct kvm_s2_mmu *mmu = &kvm->arch.nested_mmus[i];
> > +	for (int i = kvm->arch.nested_mmus_size - 1; i >= 0; i--) {
> > +		struct kvm_s2_mmu *mmu = kvm->arch.nested_mmus[i];
> >
> >  		if (!WARN_ON(atomic_read(&mmu->refcnt)))
> >  			kvm_free_stage2_pgd(mmu);
> > +
> > +		if ((i % S2_MMU_PER_VCPU) == 0)
> > +			kvfree(mmu);
> 
> Hmm I think that this can still be referenced if a concurrent e.g. mmu notifier
> thread doing S2 nested teardown is referencing it?

Yes, but that's a separate fix, as per the notes above.

> Maybe defer this to kvm_arch_destroy_vm() also?

Sort off. I think we need two separate things here:

- In this particular callback, keep the tearing down of the shadow
  S2s. They need to be emptied (after all, that's what the callback is
  about), but the s2_mmu structures still need to exist, just as the
  one that is embedded in the kvm structure doesn't disappear from
  under our feet.

- in kvm_arch_destroy_vm(), free the suckers, because that's the only
  safe point to do so.

> 
> >  	}
> > -	kvfree(kvm->arch.nested_mmus);
> > -	kvm->arch.nested_mmus = NULL;
> >  	kvm->arch.nested_mmus_size = 0;
> 
> This is racey (concurrent S2 teardown again) and should be
> done with the kvm->mmu_lock held I think.
> 
> In my original patch (see below) I simply did:
> 
> 	/* We may be raced by concurrent S2 teardown. */
> 	scoped_guard(write_lock, &kvm->mmu_lock)
> 		kvm->arch.nested_mmus_size = 0;

As explained above, I don't think this is the place to free the S2
MMUs at all.

I'll prepare some additional fixes for that.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.