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

On Thu, Aug 06, 2026 at 09:24:51PM +0200, Karl Mehltretter wrote:
> kvm_vcpu_init_nested() can grow kvm->arch.nested_mmus while initialising
> another vCPU: it copies the MMUs, publishes the new allocation, and frees
> the old one. It updates pgt->mmu back-pointers, but not hw_mmu, leaving
> already-running vCPUs with pointers to freed memory. hw_mmu cannot be
> fixed up the same way: a running vCPU reads it without holding mmu_lock.
> The nested S2 ptdump file's debugfs private data is also affected, as it
> points into the freed array.
> 
> KASAN reports an access through the stale hw_mmu pointer as a
> slab-use-after-free in kvm_handle_guest_abort().
> 
> Turn nested_mmus into a pointer table allocated once for the maximum
> number of vCPUs during VM creation. Allocate the MMUs separately as
> vCPUs are initialised and append their pointers to that table. The MMU
> objects never move, so cached hw_mmu pointers, pgt->mmu back-pointers,
> and ptdump private data remain valid.
> 
> Two issues in the old implementation are also fixed:
> 
> - The old failure path passed uninitialised MMUs to
>   kvm_free_stage2_pgd(), which derives kvm from mmu->arch and can
>   therefore dereference an invalid pointer. Only call
>   kvm_free_stage2_pgd() for initialised MMUs.
> - Previously, initialisation of the new MMUs was not ordered before
>   publication of nested_mmus_size. Fix this by taking mmu_lock when
>   increasing nested_mmus_size.
> 
> Fixes: 4f128f8e1aaa ("KVM: arm64: nv: Support multiple nested Stage-2 mmu structures")
> Cc: [email protected]
> Suggested-by: Marc Zyngier <[email protected]>
> Assisted-by: Claude:claude-fable-5
> Signed-off-by: Karl Mehltretter <[email protected]>

[...]

>  int kvm_vcpu_init_nested(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm *kvm = vcpu->kvm;
> -	struct kvm_s2_mmu *tmp;
> -	int num_mmus, ret = 0;
> +	struct kvm_s2_mmu *mmu;
> +	int num_mmus, ret = 0, i;
>  
>  	if (test_bit(KVM_ARM_VCPU_HAS_EL2_E2H0, kvm->arch.vcpu_features) &&
>  	    !cpus_have_final_cap(ARM64_HAS_HCR_NV1))
> @@ -91,42 +104,38 @@ int kvm_vcpu_init_nested(struct kvm_vcpu *vcpu)
>  	 */
>  	num_mmus = atomic_read(&kvm->online_vcpus) * S2_MMU_PER_VCPU;
>  
> -	if (num_mmus > kvm->arch.nested_mmus_size) {
> -		tmp = kvcalloc(num_mmus, sizeof(*tmp), GFP_KERNEL_ACCOUNT);
> -		if (!tmp)
> -			return -ENOMEM;
> -
> -		write_lock(&kvm->mmu_lock);
> +	if (num_mmus <= kvm->arch.nested_mmus_size)
> +		return 0;
>  
> -		if (kvm->arch.nested_mmus_size) {
> -			memcpy(tmp, kvm->arch.nested_mmus,
> -			       size_mul(sizeof(*tmp), kvm->arch.nested_mmus_size));
> +	lockdep_assert_held(&kvm->arch.config_lock);
>  
> -			for (int i = 0; i < kvm->arch.nested_mmus_size; i++)
> -				tmp[i].pgt->mmu = &tmp[i];
> +	for (i = 0; i < S2_MMU_PER_VCPU; i++) {
> +		mmu = kzalloc_obj(*mmu, GFP_KERNEL_ACCOUNT);
> +		if (!mmu) {
> +			ret = -ENOMEM;
> +			break;
>  		}
>  
> -		swap(kvm->arch.nested_mmus, tmp);
> -
> -		write_unlock(&kvm->mmu_lock);
> +		ret = init_nested_s2_mmu(kvm, mmu);
> +		if (ret) {
> +			kfree(mmu);
> +			free_page((unsigned long)vcpu->arch.ctxt.vncr_array);
> +			vcpu->arch.ctxt.vncr_array = NULL;
> +			break;
> +		}
>  
> -		kvfree(tmp);
> +		kvm->arch.nested_mmus[kvm->arch.nested_mmus_size + i] = mmu;
>  	}
>  
> -	for (int i = kvm->arch.nested_mmus_size; !ret && i < num_mmus; i++)
> -		ret = init_nested_s2_mmu(kvm, &kvm->arch.nested_mmus[i]);
> -
>  	if (ret) {
> -		for (int i = kvm->arch.nested_mmus_size; i < num_mmus; i++)
> -			kvm_free_stage2_pgd(&kvm->arch.nested_mmus[i]);
> -
> -		free_page((unsigned long)vcpu->arch.ctxt.vncr_array);
> -		vcpu->arch.ctxt.vncr_array = NULL;
> +		while (i--)
> +			free_nested_s2_mmu(kvm->arch.nested_mmus[kvm->arch.nested_mmus_size + i]);
>  
>  		return ret;
>  	}
>  
> -	kvm->arch.nested_mmus_size = num_mmus;
> +	guard(write_lock)(&kvm->mmu_lock);
> +	kvm->arch.nested_mmus_size += S2_MMU_PER_VCPU;
>  
>  	return 0;
>  }

Hey sorry for nitpicking once more, I actually like v2 more, as this
version splits the error handling to two places. Marc's implementation
[1] is also good, but that requires a twist in kvm_arch_flush_shadow_all().
So I'd personally go with v2 (+my suggestions for it :)), but also fine if
others have other opinions.

I also think the comment in kvm_vcpu_init_nested() is worth adjusting
slightly, too.

Thanks,
Wei-Lin Chang

[1]: https://lore.kernel.org/kvmarm/[email protected]/

[...]

> @@ -1273,10 +1282,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;
> -- 
> 2.39.5 (Apple Git-154)
>