Re: [PATCH v2 1/2] KVM: arm64: nv: Fix life cycle of the nested_mmus array

[sashiko-bot@kernel.org]

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

New issues:
- [High] Graceful VM teardown permanently leaks dynamically allocated `struct kvm_s2_mmu` chunks.
- [High] Post-flush nested vCPU initialization enforces constrained allocations, exhausting MMU slot limits.
- [Low] `kvmalloc_array` utilizes the undefined `struct s2_mmu *`.

Pre-existing issues:
- [High] `kvm_arch_flush_shadow_all` unconditionally frees `s2_mmu` memory chunks actively in use by running vCPUs, causing a Use-After-Free.
- [High] Lockless reading of `nested_mmus_size` leads to data races and uninitialized pointer dereferences.
- [High] Uninitialized local pointer dereference in `get_s2_mmu_nested()` if `nested_mmus_size` is 0.
--

--- Patch [1]: [PATCH v2 1/2] KVM: arm64: nv: Fix life cycle of the nested_mmus array ---
commit dd101263b02fbf52d5928987d4961bdef4c31086
Author: Marc Zyngier <maz@kernel.org>

KVM: arm64: nv: Fix life cycle of the nested_mmus array

This commit changes the nested_mmus array from an array of structures
to an array of pointers allocated at VM creation. It updates the
allocation and freeing lifecycle for nested stage-2 MMU structures to
prevent use-after-free conditions during array resizing.

Link: https://lore.kernel.org/r/20260803224405.41468-1-kmehltretter@gmail.com

> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c

[ ... ]

> @@ -337,6 +340,7 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
>  
>  	kvm_unshare_hyp(kvm, kvm + 1);
>  
> +	kvfree(kvm->arch.nested_mmus);

[Severity: High]
Does kvm_arch_destroy_vm() permanently leak the MMU chunks dynamically
allocated in kvm_vcpu_init_nested()?

Since kvm_arch_flush_shadow_all() is tied to the MMU notifier release
path and runs upon process exit, a graceful teardown that simply
closes the VM file descriptor wouldn't necessarily trigger it. This
could leave the struct kvm_s2_mmu chunks orphaned because only the
pointer array is being freed here.

>  	kvm_arm_teardown_hypercalls(kvm);
>  }
> 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

[ ... ]

> @@ -45,11 +45,15 @@ struct vncr_tlb {
>   */
>  #define S2_MMU_PER_VCPU		2
>  
> -void kvm_init_nested(struct kvm *kvm)
> +int kvm_init_nested(struct kvm *kvm)
>  {
> -	kvm->arch.nested_mmus = NULL;
> +	kvm->arch.nested_mmus = kvmalloc_array(KVM_MAX_VCPUS * S2_MMU_PER_VCPU,
> +					       sizeof(struct s2_mmu *),
> +					       GFP_KERNEL_ACCOUNT);

[Severity: Low]
Should this be sizeof(struct kvm_s2_mmu *) instead of
sizeof(struct s2_mmu *)? Even though the pointer size evaluates to the
same number of bytes, it appears to reference an undefined structure name
which might bypass compiler type checking.

>  	kvm->arch.nested_mmus_size = 0;
>  	atomic_set(&kvm->arch.vncr_tlb_count, 0);
> +
> +	return kvm->arch.nested_mmus ? 0 : -ENOMEM;
>  }

[ ... ]

> @@ -70,8 +74,9 @@ static int init_nested_s2_mmu(struct kvm *kvm, struct kvm_s2_mmu *mmu)
>  int kvm_vcpu_init_nested(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm *kvm = vcpu->kvm;
> -	struct kvm_s2_mmu *tmp;
> -	int num_mmus, ret = 0;
> +	int num_mmus;
> +
> +	lockdep_assert_held(&kvm->arch.config_lock);

[ ... ]

>  	num_mmus = atomic_read(&kvm->online_vcpus) * S2_MMU_PER_VCPU;
>  
>  	if (num_mmus > kvm->arch.nested_mmus_size) {
> -		tmp = kvcalloc(num_mmus, sizeof(*tmp), GFP_KERNEL_ACCOUNT);
> -		if (!tmp)
> -			return -ENOMEM;
> +		struct kvm_s2_mmu *tmp;
> +		int i, ret = 0;
>  
> -		write_lock(&kvm->mmu_lock);
> -
> -		if (kvm->arch.nested_mmus_size) {
> -			memcpy(tmp, kvm->arch.nested_mmus,
> -			       size_mul(sizeof(*tmp), kvm->arch.nested_mmus_size));
> +		tmp = kvcalloc(S2_MMU_PER_VCPU, sizeof(*tmp), GFP_KERNEL_ACCOUNT);
> +		if (!tmp)
> +			ret = -ENOMEM;

[Severity: High]
Does this under-provision the nested MMUs if a concurrent flush resets
nested_mmus_size to zero?

If nested_mmus_size is reset out-of-band by a flush, a subsequent vCPU
initialization will detect the deficit but only allocate S2_MMU_PER_VCPU
entries rather than the full num_mmus requirement. This could quickly
exhaust the under-provisioned pool when multiple vCPUs become active,
eventually triggering the fallback BUG_ON in get_s2_mmu_nested().

[ ... ]

> +		guard(write_lock)(&kvm->mmu_lock);
>  
> -		for (int i = kvm->arch.nested_mmus_size; i < num_mmus; i++)
> -			kvm_free_stage2_pgd(&kvm->arch.nested_mmus[i]);
> +		for (i = 0; i < S2_MMU_PER_VCPU; i++)
> +			kvm->arch.nested_mmus[i + kvm->arch.nested_mmus_size] = &tmp[i];
>  
> -		free_page((unsigned long)vcpu->arch.ctxt.vncr_array);
> -		vcpu->arch.ctxt.vncr_array = NULL;
> +		kvm->arch.nested_mmus_size += S2_MMU_PER_VCPU;
>  	}

[Severity: High]
This is a pre-existing issue, but does locklessly reading
nested_mmus_size risk dereferencing uninitialized pointers?

Because kvm_arch_flush_shadow_all() reads nested_mmus_size locklessly
(holding only the SRCU read lock) without memory barriers, the reader
could observe the incremented size before the pointer stores into the
nested_mmus array are actually visible. Could this result in a
dereference of uninitialized memory when reading the refcnt during a
flush?

>  
> -	kvm->arch.nested_mmus_size = num_mmus;
> -
>  	return 0;
>  }

[ ... ]

> @@ -823,7 +817,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;
>  	}
>  	BUG_ON(atomic_read(&s2_mmu->refcnt)); /* We have struct MMUs to spare */

[Severity: High]
This is a pre-existing issue, but can this dereference an uninitialized
local pointer if nested_mmus_size is zero?

If a vCPU attempts to execute nested code and requests a stage-2 MMU
after kvm_arch_flush_shadow_all() has reset nested_mmus_size to 0, the
search loop evaluates to false immediately. This bypasses the loop
entirely, leaving the local variable s2_mmu uninitialized when it reaches
this BUG_ON() check.

[ ... ]

> @@ -1317,16 +1311,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);

[Severity: High]
This is a pre-existing issue, but does unconditionally freeing
the MMU chunk risk a use-after-free?

While the code checks if the refcnt is zero before freeing the stage-2 PGD,
the kvfree(mmu) call happens unconditionally here. If a concurrent vCPU is
actively executing nested context code and holds a reference to the MMU,
freeing the memory backing the struct kvm_s2_mmu could result in a
use-after-free.

>  	}
> -	kvfree(kvm->arch.nested_mmus);
> -	kvm->arch.nested_mmus = NULL;
>  	kvm->arch.nested_mmus_size = 0;
>  	kvm_uninit_stage2_mmu(kvm);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260814103230.858578-1-maz@kernel.org?part=1