Re: [PATCH 2/2] KVM: arm64: nv: Fix null ptr deref in kvm_nested_s2_unmap() on S2 teardown

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

Pre-existing issues:
- [Critical] Use-After-Free on `kvm->arch.nested_mmus` array due to concurrent teardown.
- [Critical] Incomplete Fix: `kvm_nested_s2_wp()` is vulnerable to the same NULL pointer dereference and UAF.
- [High] Data Race: `kvm->arch.nested_mmus_size` updated locklessly causing uninitialized memory reads.
--

--- Patch [2]: [PATCH 2/2] KVM: arm64: nv: Fix null ptr deref in kvm_nested_s2_unmap() on S2 teardown ---
commit 04c9d62ec51b0000a28c8099341f6a2d31ce7848
Author: Lorenzo Stoakes (ARM) <[email protected]>

KVM: arm64: nv: Fix null ptr deref in kvm_nested_s2_unmap() on S2 teardown

This commit adds a NULL check for kvm->arch.mmu.pgt in kvm_nested_s2_unmap()
to prevent a NULL pointer dereference. This happens when the MMU lock is
dropped during stage 2 teardown, allowing concurrent progress that clears the
page table.

> diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
> index 17123f0b6daba..9fc0b1696dcb6 100644
> --- a/arch/arm64/kvm/nested.c
> +++ b/arch/arm64/kvm/nested.c
> @@ -1295,7 +1295,11 @@ void kvm_nested_s2_unmap(struct kvm *kvm, bool may_block)
>  			kvm_stage2_unmap_range(mmu, 0, kvm_phys_size(mmu), may_block);

[Severity: Critical]
This isn't a bug introduced by this patch, but does this loop contain a
Use-After-Free vulnerability?

While iterating over the nested_mmus array in kvm_nested_s2_unmap(),
kvm_stage2_unmap_range() can drop kvm->mmu_lock inside stage2_apply_range().

Concurrently, if kvm_mmu_notifier_release() calls kvm_arch_flush_shadow_all(),
it frees the kvm->arch.nested_mmus array unconditionally and without taking
kvm->mmu_lock:

kvm_arch_flush_shadow_all() {
    ...
    kvfree(kvm->arch.nested_mmus);
    ...
}

If the array is freed during the lock-drop window, the local mmu pointer
becomes dangling, and accessing it when the lock is reacquired could result
in a Use-After-Free.

>  	}
>  
> -	kvm_invalidate_vncr_ipa(kvm, 0, BIT(kvm->arch.mmu.pgt->ia_bits));
> +	/* NULL pgt should only be possible if raced when mmu_lock dropped. */
> +	if (kvm->arch.mmu.pgt)
> +		kvm_invalidate_vncr_ipa(kvm, 0, BIT(kvm->arch.mmu.pgt->ia_bits));
> +	else
> +		WARN_ON(!may_block);
>  }

[Severity: Critical]
This is a pre-existing issue, but does kvm_nested_s2_wp() suffer from the exact
same NULL pointer dereference and Use-After-Free vulnerability?

Like kvm_nested_s2_unmap(), kvm_nested_s2_wp() iterates over the nested_mmus
array and drops mmu_lock via stage2_apply_range_resched(). When it resumes
and exits the loop, it also unconditionally executes
BIT(kvm->arch.mmu.pgt->ia_bits).

Should the same fix be applied to kvm_nested_s2_wp() to prevent a guaranteed
NULL pointer dereference when kvm_arch_flush_shadow_all() clears the page
table in the window where the lock is dropped?

[Severity: High]
This isn't a bug introduced by this patch, but is there a data race on
kvm->arch.nested_mmus_size?

In kvm_vcpu_init_nested(), nested_mmus_size is updated outside of
kvm->mmu_lock and without memory barriers:

kvm_vcpu_init_nested() {
    ...
    kvm->arch.nested_mmus_size = num_mmus;
    ...
}

A concurrent reader like kvm_nested_s2_unmap() can see the updated size
before the stores initializing the new array elements become visible. Because
the new array is allocated with kvcalloc(), an uninitialized/zeroed
mmu->tlb_vttbr causes kvm_s2_mmu_valid() to incorrectly evaluate to TRUE.

Will this cause the reader to process the zeroed mmu structure, passing it
to kvm_stage2_unmap_range(), which would dereference a NULL mmu->pgt or
access a partially initialized pgt?

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