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

On Thu, Aug 13, 2026 at 04:08:54PM +0100, Marc Zyngier wrote:
> 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.

OK fair enough, missed that!

>
> > 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.

Yes that makes sense.

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

Yes, absolutely.

>
> >
> > >  	}
> > > -	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.

Ah I guess you would need to keep the nested_mmus_size around anyway if you
want to individually free the mmus, makes sense.

>
> I'll prepare some additional fixes for that.

Cool, feel free to cc- me on those would be happy to help with review +
this is all helpful for learning more about the subsystem :)

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

--
Cheers, Lorenzo