Re: [PATCH v4] KVM: arm64: nv: Keep the shadow S2 MMUs at fixed addresses

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

New issues:
- [Medium] The `kvm->arch.nested_mmus` array is leaked if VM creation fails after `kvm_arch_init_vm()` completes successfully.

Pre-existing issues:
- [High] If `kvm->arch.nested_mmus_size` is 0 or `nested_mmus` is freed, a nested stage-2 fault causes a host kernel panic in `get_s2_mmu_nested()` due to a race condition with `kvm_arch_flush_shadow_all()`.
--

--- Patch [1]: [PATCH v4] KVM: arm64: nv: Keep the shadow S2 MMUs at fixed addresses ---
commit 46aa9c715387bbae64560eea52655690ecd90bb6
Author: Karl Mehltretter <[email protected]>
Subject: KVM: arm64: nv: Keep the shadow S2 MMUs at fixed addresses

This commit turns nested_mmus into a pointer table allocated once during VM
creation. The MMUs are allocated separately as VCPUs are initialized to keep
their addresses stable, fixing a slab-use-after-free issue.

> diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
> index fb54f6dad995c..3c9b5c5b02d1f 100644
> --- a/arch/arm64/kvm/nested.c
> +++ b/arch/arm64/kvm/nested.c
> @@ -43,11 +43,18 @@ struct vncr_tlb {
>   */
>  #define S2_MMU_PER_VCPU		2
>  
> -void kvm_init_nested(struct kvm *kvm)
> +int kvm_init_nested(struct kvm *kvm)
>  {
> -	kvm->arch.nested_mmus = NULL;
> +	kvm->arch.nested_mmus = kvcalloc(KVM_MAX_VCPUS * S2_MMU_PER_VCPU,
> +					 sizeof(*kvm->arch.nested_mmus),
> +					 GFP_KERNEL_ACCOUNT);
> +	if (!kvm->arch.nested_mmus)
> +		return -ENOMEM;
> +

[Severity: Medium]
Does this allocation leak if VM creation fails later in the process?

During kvm_create_vm(), kvm_arch_init_vm() calls kvm_init_nested() which
allocates kvm->arch.nested_mmus. If a subsequent initialization step in
kvm_create_vm() fails (e.g., hardware_enable_all() or
kvm_init_mmu_notifier()), the error path bypasses mmu_notifier_unregister()
and directly calls kvm_arch_destroy_vm().

Since kvm_arch_destroy_vm() on ARM64 does not free kvm->arch.nested_mmus,
could the array be permanently leaked on those error paths?

[ ... ]

> @@ -1272,10 +1279,10 @@ void kvm_arch_flush_shadow_all(struct kvm *kvm)
>  	int i;
>  
>  	for (i = 0; i < kvm->arch.nested_mmus_size; i++) {
> -		struct kvm_s2_mmu *mmu = &kvm->arch.nested_mmus[i];
> +		struct kvm_s2_mmu *mmu = kvm->arch.nested_mmus[i];
>  
>  		if (!WARN_ON(atomic_read(&mmu->refcnt)))
> -			kvm_free_stage2_pgd(mmu);
> +			free_nested_s2_mmu(mmu);
>  	}
>  	kvfree(kvm->arch.nested_mmus);
>  	kvm->arch.nested_mmus = NULL;

[Severity: High]
This is a pre-existing issue, but does this function have a race
condition with VCPU threads?

When a KVM process is killed, exit_mm() invokes mmu_notifier_release(), which
calls kvm_arch_flush_shadow_all(). This function frees kvm->arch.nested_mmus
and sets nested_mmus_size = 0 without acquiring mmu_lock.

Since file descriptors are not closed until later, VCPU threads might still be
executing. If a VCPU thread calls vcpu_load() -> kvm_vcpu_load_hw_mmu() ->
get_s2_mmu_nested(), it acquires mmu_lock but not the SRCU lock. It can read
the concurrently modified nested_mmus_size as 0, skip the lookup loop, and
execute BUG_ON(atomic_read(&s2_mmu->refcnt)) on an uninitialized or NULL
pointer.

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