Re: [PATCH v3 1/2] KVM: arm64: Fix spurious warning for benign stage 2 teardown race

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Use-After-Free of nested_mmus array in stage2_apply_range due to concurrent kvfree
--

commit 2cabe3fbde6089df082f059c9d4b617266797a8f
Author: Lorenzo Stoakes (ARM) <ljs@kernel.org>

KVM: arm64: Fix spurious warning for benign stage 2 teardown race

This commit addresses a spurious warning observed during stage 2 page table
teardown by preventing concurrent PGT teardown from being treated as an error
when the mmu_lock has been dropped and re-acquired.

> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index 9ba86450fe4af..2d44cd6a5aed9 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
[ ... ]
> @@ -59,27 +59,36 @@ static phys_addr_t stage2_range_addr_end(phys_addr_t addr, phys_addr_t end)
> -static int stage2_apply_range(struct kvm_s2_mmu *mmu, phys_addr_t addr,
> +static int stage2_apply_range(struct kvm_s2_mmu *mmu, phys_addr_t start,
>  			      phys_addr_t end,
>  			      int (*fn)(struct kvm_pgtable *, u64, u64),
>  			      bool resched)
>  {
>  	struct kvm *kvm = kvm_s2_mmu_to_kvm(mmu);
> +	bool lock_dropped = false;
> +	phys_addr_t addr = start;
>  	int ret;
>  	u64 next;
>  
>  	do {
>  		struct kvm_pgtable *pgt = mmu->pgt;

[Severity: Critical]
This isn't a bug introduced by this patch, but while this change correctly
avoids the warning when pgt is NULL, does the lock-dropping behavior here
expose a Use-After-Free on the mmu pointer itself?

Looking at the teardown paths, it appears stage2_apply_range() can race with
concurrent freeing of the kvm->arch.nested_mmus array.

If Thread A (e.g., kcompactd) is iterating over the array in
kvm_nested_s2_unmap() and yields kvm->mmu_lock inside this loop via
cond_resched_rwlock_write(), Thread B (such as exit_mmap ->
mmu_notifier_release) could concurrently call kvm_arch_flush_shadow_all().
Thread B acquires mmu_lock to clear the PGDs, and then locklessly calls
kvfree(kvm->arch.nested_mmus) after dropping the lock.

When Thread A wakes up and re-acquires mmu_lock to resume the loop, its
local mmu pointer would point into the freed nested_mmus array, leading to a
Use-After-Free right here when it accesses mmu->pgt.

A similar concurrent kvfree() appears to exist in kvm_vcpu_init_nested(),
which reallocates and frees the array (kvfree(tmp)) while readers might be
yielding here.

Is there a need to defer these kvfree() calls or better protect the yielding
readers in stage2_apply_range() to prevent this race?

> +		/*
> +		 * We may be raced on PGT teardown when we release the
> +		 * kvm->mmu_lock. That's fine as the PGT is legitimately no
> +		 * longer present.
> +		 */
>  		if (!pgt)
> -			return -EINVAL;
> +			return lock_dropped ? 0 : -EINVAL;
>  
>  		next = stage2_range_addr_end(addr, end);
>  		ret = fn(pgt, addr, next - addr);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901-kvm-arm-nested-virt-fix-v3-0-b154676f7e4c@kernel.org?part=1