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

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

Pre-existing issues:
- [Critical] Use-After-Free of the dynamically allocated `kvm->arch.nested_mmus` array when the MMU lock is dropped during stage-2 unmap operations.
- [High] NULL Pointer Dereference in `kvm_vcpu_init_nested()` during concurrent VM teardown.
--

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

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

This patch suppresses a warning in stage2_apply_range() when the page table
pointer is found to be NULL after reacquiring the MMU lock. It assumes that
the NULL page table is a benign result of a 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)
>   * long will also starve other vCPUs. We have to also make sure that the page
>   * tables are not freed while we released the lock.
>   */
> -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;
> +		/*
> +		 * 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;

[Severity: Critical]
This is a pre-existing issue, but does suppressing this warning mask a
regression where the mmu pointer itself is used after being freed?

When kvm_nested_s2_unmap() passes an mmu pointer to stage2_apply_range(),
that pointer references a dynamically allocated array:

arch/arm64/kvm/nested.c:kvm_nested_s2_unmap() {
    ...
    struct kvm_s2_mmu *mmu = &kvm->arch.nested_mmus[i];
    if (kvm_s2_mmu_valid(mmu))
        kvm_stage2_unmap_range(mmu, ...);
}

If cond_resched_rwlock_write() drops the lock inside stage2_apply_range(),
a concurrent VM teardown or VCPU initialization can completely free this
array:

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

When the lock is reacquired, the loop continues and accesses mmu->pgt
from the now-freed memory. Is it possible that the warning was actually
firing because we were reading a freed mmu structure?


[Severity: High]
This is also a pre-existing issue, but looking at the concurrent VM teardown
path, does kvm_vcpu_init_nested() have a NULL pointer dereference?

When kvm_vcpu_init_nested() reallocates the array, it unconditionally
updates the mmu pointer for each entry:

arch/arm64/kvm/nested.c:kvm_vcpu_init_nested() {
    ...
    for (int i = 0; i < kvm->arch.nested_mmus_size; i++)
        tmp[i].pgt->mmu = &tmp[i];
    ...
}

If kvm_arch_flush_shadow_all() concurrently calls kvm_free_stage2_pgd()
and sets pgt to NULL before the lock is acquired here, wouldn't this crash
when dereferencing tmp[i].pgt?

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