Re: [PATCH v23 09/14] KVM: arm64: Consolidate stage2 unmap range into kvm_stage2_unmap_range

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] kvm: arm64: UAF in stage2_apply_range() when nested_mmus is reallocated
--

commit f49216e04631aee4e96534b9604d42f899f085d6
Author: Suzuki K Poulose <suzuki.poulose@arm.com>

KVM: arm64: Consolidate stage2 unmap range into kvm_stage2_unmap_range

This patch simplifies the stage-2 unmap operations by removing
__unmap_stage2_range() and folding its logic directly into
kvm_stage2_unmap_range(). It also updates callers to use the consolidated
function.

> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index 9ba86450fe4af..3fef16393f568 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
> @@ -314,36 +314,30 @@ static void invalidate_icache_guest_page(void *va, size_t size)
[ ... ]
> -static void __unmap_stage2_range(struct kvm_s2_mmu *mmu, phys_addr_t start, u64 size,
> -				 bool may_block)
> +void kvm_stage2_unmap_range(struct kvm_s2_mmu *mmu, phys_addr_t start,
> +			    u64 size, bool may_block)
>  {
>  	struct kvm *kvm = kvm_s2_mmu_to_kvm(mmu);
> -	phys_addr_t end = start + size;
> +
> +	if (kvm_vm_is_protected(kvm))
> +		return;
>  
>  	lockdep_assert_held_write(&kvm->mmu_lock);
>  	WARN_ON(size & ~PAGE_MASK);
> -	WARN_ON(stage2_apply_range(mmu, start, end, KVM_PGT_FN(kvm_pgtable_stage2_unmap),
> -				   may_block));
> -}
>  
> -void kvm_stage2_unmap_range(struct kvm_s2_mmu *mmu, phys_addr_t start,
> -			    u64 size, bool may_block)
> -{
> -	if (kvm_vm_is_protected(kvm_s2_mmu_to_kvm(mmu)))
> -		return;
> -
> -	__unmap_stage2_range(mmu, start, size, may_block);
> +	WARN_ON(stage2_apply_range(mmu, start, start + size,
> +				   KVM_PGT_FN(kvm_pgtable_stage2_unmap),
> +				   may_block));

[Severity: High]
Does this allow a use-after-free of the kvm->arch.nested_mmus array if
may_block is true?

If kvm_nested_s2_unmap() is called during a memory reclaim event via
kvm_mmu_notifier_invalidate_range_start(), it caches a pointer to an
element of kvm->arch.nested_mmus (e.g., mmu = &kvm->arch.nested_mmus[i])
and passes it here with may_block=true.

When stage2_apply_range() iterates, it can drop kvm->mmu_lock via
cond_resched_rwlock_write():

stage2_apply_range() {
	do {
		struct kvm_pgtable *pgt = mmu->pgt;
		...
		if (resched && next != end)
			cond_resched_rwlock_write(&kvm->mmu_lock);
	} while (addr = next, addr != end);
}

Concurrently, could userspace initializing a new vCPU via KVM_ARM_VCPU_INIT
invoke kvm_vcpu_init_nested(), grab kvm->mmu_lock, reallocate the
kvm->arch.nested_mmus array, and free the old one?

kvm_vcpu_init_nested() {
	...
	swap(kvm->arch.nested_mmus, tmp);
	write_unlock(&kvm->mmu_lock);
	kvfree(tmp);
}

If stage2_apply_range() then reacquires the lock, it would dereference
mmu->pgt using the mmu pointer that still points into the now-freed
nested_mmus array.

Can we ensure the array is not freed while we iterate, or refresh the mmu
pointer after reacquiring the lock?

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261007073537.2454351-1-suzuki.poulose@arm.com?part=9