Re: [PATCH v3 2/2] KVM: arm64: nv: Delay freeing of shadow S2 structures until VM destruction

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Preserving nested MMU containers with freed page tables leads to NULL pointer dereference.

Pre-existing issues:
- [High] Permanent memory leak of nested stage-2 page tables and per-CPU state on VM destruction.
- [High] Data race between lockless array iteration and array population causes uninitialized pointer dereference.
--

--- Patch [2]: [PATCH v3 2/2] KVM: arm64: nv: Delay freeing of shadow S2 structures until VM destruction ---
commit 46c2225152ae78e4009a13ad7c3bcc3e69fbab04
Author: Marc Zyngier <[email protected]>

KVM: arm64: nv: Delay freeing of shadow S2 structures until VM destruction

We free the shadow S2 structures from kvm_arch_flush_shadow_all(), which
is a Bad Idea(tm). Freeing the page tables is fair game (this is what
this callback is for), but freeing the container that could still be
referenced by another part of the system is not great.

Instead, grow separate destructors that gets called when we tear the VM
down for good. From there, we can nuke both the individual MMUs as well
as the global array that points to them, safe in the knowledge that the
vcpus themselves have been destroyed already.

> diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
> index 9070d5bc5af03..bc2b58e583b52 100644
> --- a/arch/arm64/kvm/nested.c
> +++ b/arch/arm64/kvm/nested.c

[ ... ]

> @@ -56,6 +56,15 @@ int kvm_init_nested(struct kvm *kvm)
>  	return kvm->arch.nested_mmus ? 0 : -ENOMEM;
>  }
>  
> +void kvm_destroy_nested(struct kvm *kvm)
> +{
> +	for (int i = 0; i < kvm->arch.nested_mmus_size; i+= S2_MMU_PER_VCPU)
> +		kvfree(kvm->arch.nested_mmus[i]);

[Severity: High]
This is a pre-existing issue, but does this loop permanently leak the nested
stage-2 page tables? The loop blindly calls kvfree() without first calling
kvm_free_stage2_pgd() to recursively free the page tables and per-CPU
allocations.

If a user creates a KVM VM, runs nested vCPUs to allocate stage-2 page tables,
and then closes the KVM file descriptor, the KVM VM teardown calls
kvm_destroy_nested(). Because it frees the container structures but forgets to
call kvm_free_stage2_pgd(), the pgt and last_vcpu_ran allocations are
permanently leaked.

> +
> +	kvm->arch.nested_mmus_size = 0;
> +	kvfree(kvm->arch.nested_mmus);
> +}
> +
>  static int init_nested_s2_mmu(struct kvm *kvm, struct kvm_s2_mmu *mmu)
>  {

[ ... ]

> @@ -1311,16 +1320,12 @@ void kvm_nested_s2_flush(struct kvm *kvm)
>  
>  void kvm_arch_flush_shadow_all(struct kvm *kvm)
>  {
> -	for (int i = kvm->arch.nested_mmus_size - 1; i >= 0; i--) {
> +	for (int i = 0; i < kvm->arch.nested_mmus_size; i++) {
>  		struct kvm_s2_mmu *mmu = kvm->arch.nested_mmus[i];
>  
>  		if (!WARN_ON(atomic_read(&mmu->refcnt)))

[Severity: High]
This isn't a bug introduced by this patch, but can this lockless array
iteration race with array population and cause an uninitialized pointer
dereference?

While one thread executes KVM_ARM_VCPU_INIT and appends newly allocated MMU
pointers to the nested_mmus array under mmu_lock in kvm_vcpu_init_nested():

	for (i = 0; i < S2_MMU_PER_VCPU; i++)
		kvm->arch.nested_mmus[i + kvm->arch.nested_mmus_size] = &tmp[i];

	kvm->arch.nested_mmus_size += S2_MMU_PER_VCPU;

Another process context (like OOM killer or thread exit) can trigger
mmu_notifier_release -> kvm_arch_flush_shadow_all().

kvm_arch_flush_shadow_all() iterates over nested_mmus reading
nested_mmus_size without mmu_lock or memory barriers. Because kvmalloc_objs
does not zero memory, the lockless reader can observe an incremented size but
a garbage pointer, causing a crash on atomic_read(&mmu->refcnt).

>  			kvm_free_stage2_pgd(mmu);

[Severity: High]
Since this patch now preserves the nested MMU containers, does this lead to a
NULL pointer dereference?

The array elements are kept, but kvm_free_stage2_pgd(mmu) frees their page
tables and sets mmu->pgt to NULL. If an application shares a KVM VM file
descriptor across processes and the process holding the mm_struct exits, it
fires mmu_notifier_release -> kvm_arch_flush_shadow_all().

If the surviving process calls KVM_RUN, get_s2_mmu_nested() can reuse the
container since its refcnt is 0:

	s2_mmu = kvm->arch.nested_mmus[i % kvm->arch.nested_mmus_size];

	if (atomic_read(&s2_mmu->refcnt) == 0)
		break;

However, because there is no logic to reallocate the missing page table tree,
a subsequent stage-2 fault will dereference mmu->pgt, panicking the kernel.

> -
> -		if ((i % S2_MMU_PER_VCPU) == 0)
> -			kvfree(mmu);
>  	}
> -	kvm->arch.nested_mmus_size = 0;
>  	kvm_uninit_stage2_mmu(kvm);
>  }

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