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

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.

[ ... ]

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

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