Re: [PATCH v2] KVM: arm64: nv: Keep the shadow S2 MMUs at fixed addresses

On Thu, Aug 06, 2026 at 06:38:59AM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [Critical] Deterministic NULL pointer dereference and Use-After-Free race on the `nested_mmus` array after `kvm_arch_flush_shadow_all` executes.
> 
> Pre-existing issues:
> - [Critical] Deterministic NULL pointer dereference in `get_s2_mmu_nested` when `nested_mmus_size` is 0.
> --
> 
> --- Patch [1]: [PATCH v2] KVM: arm64: nv: Keep the shadow S2 MMUs at fixed addresses ---
> commit 70c0982ec06bd8ebb390c1b7ff665dae83887bb8
> Author: Karl Mehltretter <[email protected]>
> KVM: arm64: nv: Keep the shadow S2 MMUs at fixed addresses
> 
> This patch turns the nested_mmus array into a fixed-size pointer table
> allocated once during VM creation. The S2 MMUs are allocated individually
> during vCPU initialization to keep their addresses stable, fixing a
> slab-use-after-free issue.
> 
> v1: https://lore.kernel.org/r/[email protected]/
> 
> > diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
> > --- a/arch/arm64/kvm/nested.c
> > +++ b/arch/arm64/kvm/nested.c
> > @@ -90,44 +104,44 @@ int kvm_vcpu_init_nested(struct kvm_vcpu *vcpu)
> 
> [ ... ]
> 
> >  	for (i = 0; i < S2_MMU_PER_VCPU; i++) {
> >  		mmu = kzalloc_obj(*mmu, GFP_KERNEL_ACCOUNT);
> >  		if (!mmu) {
> >  			ret = -ENOMEM;
> >  			goto err_free_mmus;
> >  		}
> >  
> >  		ret = init_nested_s2_mmu(kvm, mmu);
> >  		if (ret) {
> >  			kfree(mmu);
> >  			goto err_free_vncr;
> >  		}
> >  
> >  		kvm->arch.nested_mmus[kvm->arch.nested_mmus_size + i] = mmu;
> 
> [Severity: Critical]
> Can this lead to a NULL pointer dereference or use-after-free?
> 
> The patch shifts the allocation of kvm->arch.nested_mmus to VM creation in
> kvm_init_nested(), but kvm_arch_flush_shadow_all() still frees the array and
> sets the pointer to NULL and size to 0.
> 
> If an unprivileged user process triggers kvm_mmu_notifier_release(), for
> example by passing the file descriptor to a child process and exiting,
> kvm_arch_flush_shadow_all() will free the array.
> 
> A subsequent KVM_ARM_VCPU_INIT ioctl will bypass the size check in
> kvm_vcpu_init_nested() because num_mmus is greater than 0 and
> nested_mmus_size is 0. It will allocate an MMU and write it directly to
> kvm->arch.nested_mmus[kvm->arch.nested_mmus_size + i], which dereferences
> the now NULL pointer.
> 
> Additionally, since this write happens outside mmu_lock, it could race with
> a concurrent kvm_arch_flush_shadow_all() execution and write into freed
> memory.

I don't think so. KVM_ARM_VCPU_INIT can not be run after
kvm_arch_flush_shadow_all() have started. kvm_arch_flush_shadow_all() starts
after the last use count of mm_struct is gone. Additionally vCPU/VM ioctls
require current->mm == kvm->mm. A non CLONE_VM child will fail, while a CLONE_VM
child keeps the mm_struct alive and kvm_arch_flush_shadow_all() can not run.

> 
> [ ... ]
> 
> > @@ -805,7 +819,7 @@ static struct kvm_s2_mmu *get_s2_mmu_nested(struct kvm_vcpu *vcpu)
> >  	for (i = kvm->arch.nested_mmus_next;
> >  	     i < (kvm->arch.nested_mmus_size + kvm->arch.nested_mmus_next);
> >  	     i++) {
> > -		s2_mmu = &kvm->arch.nested_mmus[i % kvm->arch.nested_mmus_size];
> > +		s2_mmu = kvm->arch.nested_mmus[i % kvm->arch.nested_mmus_size];
> >  
> >  		if (atomic_read(&s2_mmu->refcnt) == 0)
> >  			break;
> >  	}
> 
> [Severity: Critical]
> This is a pre-existing issue, but can this lead to a NULL pointer
> dereference when nested_mmus_size is 0?
> 
> If kvm_arch_flush_shadow_all() executes while a vCPU is active, it sets
> kvm->arch.nested_mmus_size to 0.
> 
> When the vCPU subsequently enters get_s2_mmu_nested(), lookup_s2_mmu()
> evaluates the empty array and returns NULL, assigning NULL to s2_mmu.
> The for loop condition i < (0 + next) evaluates to false, causing the loop
> to be completely bypassed.
> 
> The function then executes BUG_ON(atomic_read(&s2_mmu->refcnt)) just below
> this loop, which dereferences the uninitialized or NULL s2_mmu pointer.
> This can be reliably triggered by terminating a VM process while it is
> executing nested virtualization workloads.

Same as the above.

Thanks,
Wei-Lin Chang

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