Re: [PATCH v4 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 1 potential issue(s) to consider:
- [High] Data race in `kvm_arch_flush_shadow_all()` leads to garbage pointer dereference and kernel panic
--

commit 87dd1db0613c9f0d7e10bf7593d998a853fa1c02
Author: Marc Zyngier <maz@kernel.org>

KVM: arm64: nv: Fix life cycle of the nested_mmus array
    
This commit alters the life cycle of the nested_mmus array to allocate
an array of pointers at VM creation time. It avoids reallocation during
VCPU_INIT by sizing the array to the absolute maximum KVM can handle.

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

> diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
> index 3c4fc566eafc6..0a32d6bd3f6c6 100644
> --- 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_objs(struct kvm_s2_mmu *,
> +					      KVM_MAX_VCPUS * S2_MMU_PER_VCPU,
> +					      GFP_KERNEL_ACCOUNT);

[Severity: High]
Does this allocation leave the pointer array filled with uninitialized memory?
Using kvmalloc_objs() without GFP_ZERO or __GFP_ZERO (or instead of
kvzalloc_objs()) means the array might contain garbage pointers rather than
being safely zeroed out.

[ ... ]
> +		guard(write_lock)(&kvm->mmu_lock);
>  
> -		for (int i = 0; i < kvm->arch.nested_mmus_size; i++)
> -			tmp[i].pgt->mmu = &tmp[i];
> +		for (i = 0; i < S2_MMU_PER_VCPU; i++)
> +			kvm->arch.nested_mmus[i + kvm->arch.nested_mmus_size] = &tmp[i];
>  
> -		swap(kvm->arch.nested_mmus, tmp);
> -
> -		write_unlock(&kvm->mmu_lock);
> -
> -		kvfree(tmp);
> -	}
> +		kvm->arch.nested_mmus_size += S2_MMU_PER_VCPU;

[Severity: High]
Could a CPU or compiler reorder this size increment to be visible before
the pointer stores are complete? This block updates the array and size
without memory barriers to order them.

[ ... ]
>  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)))

[Severity: High]
Can this lockless read of the array encounter a garbage pointer?

If kvm_arch_flush_shadow_all() is called concurrently with
kvm_vcpu_init_nested() (for example, if userspace calls KVM_ARM_VCPU_INIT
while concurrently terminating the process and triggering
mmu_notifier_release()), it might read the newly incremented
nested_mmus_size before the actual pointers are stored.

Because the initial array allocation in kvm_init_nested() used
kvmalloc_objs() without zeroing, the unwritten slots contain uninitialized
memory. Reading a garbage pointer and dereferencing it here in
atomic_read() could allow an unprivileged userspace process to cause a
kernel panic. Pre-patch, the array elements were safely zero-initialized.

I notice a later patch in this series (Patch 2) removes the
kvm->arch.nested_mmus_size = 0 assignment but retains this same lockless
loop in kvm_arch_flush_shadow_all(), which appears to leave this garbage
pointer dereference risk unfixed.

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