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

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

Pre-existing issues:
- [Critical] The patch masks a severe Use-After-Free (UAF) vulnerability by assuming the NULL pointer read is a benign race, while failing to realize the `mmu` pointer itself points into freed memory (`kvm->arch.nested_mmus`).
--

--- Patch [1]: [PATCH v2 1/2] KVM: arm64: Fix spurious warning for benign stage 2 teardown race ---
commit 720eac2d4e746b3335443e77837000d086115449
Author: Lorenzo Stoakes (ARM) <[email protected]>

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

This commit attempts to fix a warning triggered during nested VM teardown
by ignoring cases where mmu->pgt is NULL after reacquiring the MMU lock.
It changes stage2_apply_range() to return 0 instead of -EINVAL when the
page table is no longer present due to concurrent teardown.

> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index 74e7e7f7564c8..31e049ded0932 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
> @@ -59,19 +59,25 @@ 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);
> +	phys_addr_t addr = start;
>  	int ret;
>  	u64 next;
>  
>  	do {
>  		struct kvm_pgtable *pgt = mmu->pgt;

[Severity: Critical]
This is a pre-existing issue, but does this dereference of mmu->pgt result
in a use-after-free if the nested_mmus array is reallocated or freed while
the lock is dropped?

Looking at stage2_apply_range(), the kvm->mmu_lock can be dropped via
cond_resched_rwlock_write() at the end of this loop.

During this window, the mmu pointer (which points into the
kvm->arch.nested_mmus array) can be invalidated. For example,
kvm_arch_flush_shadow_all() can free the array locklessly:

arch/arm64/kvm/nested.c:kvm_arch_flush_shadow_all() {
    ...
    kvfree(kvm->arch.nested_mmus);
    ...
}

Or it can be reallocated in kvm_vcpu_init_nested():

arch/arm64/kvm/nested.c:kvm_vcpu_init_nested() {
    ...
    tmp = kvm->arch.nested_mmus;
    ...
    kvfree(tmp);
    ...
}

When the loop in stage2_apply_range() resumes after reacquiring the lock,
accessing mmu->pgt would read from freed memory. By treating a NULL pgt as
a benign race, does this patch mask the underlying object lifecycle bug and
leave the use-after-free unaddressed if the freed memory is modified?

> +		/*
> +		 * 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 resched && addr > start ? 0 : -EINVAL;
>  
>  		next = stage2_range_addr_end(addr, end);
>  		ret = fn(pgt, addr, next - addr);

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