Re: [PATCH v5] 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:
- [High] `kvm_arch_flush_shadow_all` incorrectly frees the fixed `nested_mmus` pointer table, causing a guaranteed NULL pointer dereference on subsequent VCPU initialization.
- [Low] Inconsistent error path label ordering in `kvm_vcpu_init_nested` bypasses the cleanup of `vncr_array` if MMU object allocation fails.
--

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

KVM: arm64: nv: Keep the shadow S2 MMUs at fixed addresses

This commit refactors nested S2 MMUs to use a fixed-size pointer table
allocated during VM creation. The MMU objects are allocated separately so
their addresses remain stable, fixing a use-after-free bug.

> diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
> --- a/arch/arm64/kvm/nested.c
> +++ b/arch/arm64/kvm/nested.c
[ ... ]
> @@ -82,52 +95,46 @@ int kvm_vcpu_init_nested(struct kvm_vcpu *vcpu)
[ ... ]
> +	for (i = 0; i < S2_MMU_PER_VCPU; i++) {
> +		mmu = kzalloc_obj(*mmu, GFP_KERNEL_ACCOUNT);
> +		if (!mmu) {
> +			ret = -ENOMEM;
> +			goto err_free_mmus;
>  		}
[ ... ]
> +err_free_vncr:
> +	free_page((unsigned long)vcpu->arch.ctxt.vncr_array);
> +	vcpu->arch.ctxt.vncr_array = NULL;
>  
> -	kvm->arch.nested_mmus_size = num_mmus;
> +err_free_mmus:
> +	while (i--)
> +		free_nested_s2_mmu(kvm->arch.nested_mmus[kvm->arch.nested_mmus_size + i]);
>  
> -	return 0;
> +	return ret;
>  }

[Severity: Low]
Does this error path unwind in the correct order?

If kzalloc_obj() fails during initialization, the code jumps to the
err_free_mmus label, which completely bypasses the cleanup in err_free_vncr.

This leaves the newly allocated vncr_array anchored to
vcpu->arch.ctxt.vncr_array instead of freeing it immediately. While it will
eventually be freed when the VCPU is destroyed, this seems to create an
inconsistent state compared to the other error paths in this function.

[ ... ]
> @@ -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]
Can this lead to a NULL pointer dereference on subsequent VCPU
initialization?

The patch redesigns nested_mmus to be a fixed-size pointer table allocated
once at VM creation. However, it appears this legacy cleanup remains, freeing
and nulling kvm->arch.nested_mmus when kvm_arch_flush_shadow_all() runs.

If a VM's file descriptor is kept alive by another process, and a
subsequent KVM_ARM_VCPU_INIT ioctl is issued, kvm_vcpu_init_nested() will
attempt to append to the array:

    kvm->arch.nested_mmus[kvm->arch.nested_mmus_size + i] = mmu;

Since the dynamic reallocation logic was removed in this patch, nested_mmus
will still be NULL here, triggering a kernel panic. Should this teardown
path avoid freeing the array itself now?

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