Re: [PATCH 1/2] KVM: arm64: nv: Allocate the shadow S2 MMUs individually

[Marc Zyngier <maz@kernel.org>]

On Mon, 03 Aug 2026 23:44:04 +0100,
Karl Mehltretter <kmehltretter@gmail.com> wrote:
> 
> A vCPU in L2 caches its shadow S2 MMU in vcpu->arch.hw_mmu.
> 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
> the first vCPU with a pointer to freed memory.  hw_mmu cannot be fixed up
> the same way: a running vCPU reads it without holding mmu_lock.  Copying
> also duplicates the MMU's refcount, leaving the live copy permanently
> elevated.
> 
> KASAN reports this as a slab-use-after-free in kvm_handle_guest_abort().
> 
> Make nested_mmus a pointer table and allocate each MMU separately.
> Growing the table now moves only pointer entries, preserving cached
> hw_mmu pointers, pgt->mmu back-pointers, and each MMU's refcount.  Fully
> initialise new MMUs before publishing the table and its size under
> mmu_lock.
> 
> The old failure path passed uninitialised entries to
> kvm_free_stage2_pgd(), which needs mmu->arch.  Use an allocation helper
> that returns only fully initialised MMUs, so error cleanup frees only
> completed objects; kvm_init_stage2_mmu() unwinds a failed initialisation.

I like the approach, but this is making things a bit more complicated
than they should really be IMO.

>
> Fixes: 4f128f8e1aaa ("KVM: arm64: nv: Support multiple nested Stage-2 mmu structures")
> Cc: stable@vger.kernel.org
> Assisted-by: Claude:claude-fable-5
> Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>
> ---
> 
> Tested on an arm64 KASAN kernel under QEMU TCG with EL2 emulation
> (-machine virt,virtualization=on -cpu max, kvm-arm.mode=nested): the
> selftest in patch 2 reports the slab-use-after-free without this patch
> and passes with it.
> 
>  arch/arm64/include/asm/kvm_host.h |  6 +-
>  arch/arm64/kvm/nested.c           | 92 +++++++++++++++++++++----------
>  2 files changed, 67 insertions(+), 31 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index bae2c4f92ef5..f587b01039f9 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -319,10 +319,10 @@ struct kvm_arch {
>  	u64 fgu[__NR_FGT_GROUP_IDS__];
>  
>  	/*
> -	 * Stage 2 paging state for VMs with nested S2 using a virtual
> -	 * VMID.
> +	 * Stage 2 paging state for VMs with nested S2 using a virtual VMID.
> +	 * MMUs are individually allocated to keep their addresses stable.
>  	 */
> -	struct kvm_s2_mmu *nested_mmus;
> +	struct kvm_s2_mmu **nested_mmus;
>  	size_t nested_mmus_size;
>  	int nested_mmus_next;
>  
> diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
> index dfb96edbdc43..af804a5ddca7 100644
> --- a/arch/arm64/kvm/nested.c
> +++ b/arch/arm64/kvm/nested.c
> @@ -5,6 +5,7 @@
>   */
>  
>  #include <linux/bitfield.h>
> +#include <linux/err.h>
>  #include <linux/kvm.h>
>  #include <linux/kvm_host.h>
>  
> @@ -66,11 +67,36 @@ static int init_nested_s2_mmu(struct kvm *kvm, struct kvm_s2_mmu *mmu)
>  	return kvm_init_stage2_mmu(kvm, mmu, kvm_get_pa_bits(kvm));
>  }
>  
> +static struct kvm_s2_mmu *alloc_nested_s2_mmu(struct kvm *kvm)
> +{
> +	struct kvm_s2_mmu *mmu;
> +	int ret;
> +
> +	mmu = kzalloc_obj(*mmu, GFP_KERNEL_ACCOUNT);
> +	if (!mmu)
> +		return ERR_PTR(-ENOMEM);
> +
> +	ret = init_nested_s2_mmu(kvm, mmu);
> +	if (ret) {
> +		/* kvm_init_stage2_mmu() frees its internal allocations on error */
> +		kfree(mmu);
> +		return ERR_PTR(ret);
> +	}
> +
> +	return mmu;
> +}
> +
> +static void free_nested_s2_mmu(struct kvm_s2_mmu *mmu)
> +{
> +	kvm_free_stage2_pgd(mmu);
> +	kfree(mmu);
> +}
> +
>  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 **tmp;
> +	int i, num_mmus, ret = 0;
>  
>  	if (test_bit(KVM_ARM_VCPU_HAS_EL2_E2H0, kvm->arch.vcpu_features) &&
>  	    !cpus_have_final_cap(ARM64_HAS_HCR_NV1))
> @@ -96,38 +122,48 @@ int kvm_vcpu_init_nested(struct kvm_vcpu *vcpu)
>  		if (!tmp)
>  			return -ENOMEM;
>  
> +		/*
> +		 * Populate new slots before publishing: table walkers hold
> +		 * mmu_lock and iterate up to nested_mmus_size.
> +		 */
> +		for (i = kvm->arch.nested_mmus_size; i < num_mmus; i++) {
> +			struct kvm_s2_mmu *mmu = alloc_nested_s2_mmu(kvm);
> +
> +			if (IS_ERR(mmu)) {
> +				ret = PTR_ERR(mmu);
> +				break;
> +			}
> +
> +			tmp[i] = mmu;
> +		}

You now are allocating each s2_mmu individually. But you know you want
at most S2_MMU_PER_VCPU structures, and not any extra ones. So you
could allocate one block for the current vcpu, and let the next guy
allocate its own quota.

The other thing is that reallocating the pointer array isn't great. It
adds complexity, and makes everything more fragile than it should be.

See the hack below that seems to work OK.

	M.

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 81d359ac7af14..65a1305c4a749 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -321,7 +321,7 @@ struct kvm_arch {
 	 * Stage 2 paging state for VMs with nested S2 using a virtual
 	 * VMID.
 	 */
-	struct kvm_s2_mmu *nested_mmus;
+	struct kvm_s2_mmu **nested_mmus;
 	size_t nested_mmus_size;
 	int nested_mmus_next;
 
diff --git a/arch/arm64/include/asm/kvm_nested.h b/arch/arm64/include/asm/kvm_nested.h
index 012d711034d17..d21be647ac571 100644
--- a/arch/arm64/include/asm/kvm_nested.h
+++ b/arch/arm64/include/asm/kvm_nested.h
@@ -66,7 +66,7 @@ static inline u64 translate_ttbr0_el2_to_ttbr0_el1(u64 ttbr0)
 
 extern bool forward_smc_trap(struct kvm_vcpu *vcpu);
 extern bool forward_debug_exception(struct kvm_vcpu *vcpu);
-extern void kvm_init_nested(struct kvm *kvm);
+extern int kvm_init_nested(struct kvm *kvm);
 extern int kvm_vcpu_init_nested(struct kvm_vcpu *vcpu);
 extern void kvm_init_nested_s2_mmu(struct kvm_s2_mmu *mmu);
 extern struct kvm_s2_mmu *lookup_s2_mmu(struct kvm_vcpu *vcpu);
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 9a6c72a186727..ae27ccc37b330 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -236,7 +236,9 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
 	mutex_unlock(&kvm->lock);
 #endif
 
-	kvm_init_nested(kvm);
+	ret = kvm_init_nested(kvm);
+	if (ret)
+		return ret;
 
 	ret = kvm_share_hyp(kvm, kvm + 1);
 	if (ret)
diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
index dfb96edbdc43c..e7a066a41b552 100644
--- a/arch/arm64/kvm/nested.c
+++ b/arch/arm64/kvm/nested.c
@@ -44,11 +44,15 @@ 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 = kvmalloc_array(KVM_MAX_VCPUS * S2_MMU_PER_VCPU,
+					       sizeof(struct s2_mmu *),
+					       GFP_KERNEL_ACCOUNT);
 	kvm->arch.nested_mmus_size = 0;
 	atomic_set(&kvm->arch.vncr_map_count, 0);
+
+	return kvm->arch.nested_mmus ? 0 : -ENOMEM;
 }
 
 static int init_nested_s2_mmu(struct kvm *kvm, struct kvm_s2_mmu *mmu)
@@ -69,8 +73,7 @@ static int init_nested_s2_mmu(struct kvm *kvm, struct kvm_s2_mmu *mmu)
 int kvm_vcpu_init_nested(struct kvm_vcpu *vcpu)
 {
 	struct kvm *kvm = vcpu->kvm;
-	struct kvm_s2_mmu *tmp;
-	int num_mmus, ret = 0;
+	int num_mmus;
 
 	if (test_bit(KVM_ARM_VCPU_HAS_EL2_E2H0, kvm->arch.vcpu_features) &&
 	    !cpus_have_final_cap(ARM64_HAS_HCR_NV1))
@@ -92,42 +95,34 @@ 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);
+		struct kvm_s2_mmu *tmp;
+		int ret = 0;
+
+		tmp = kvcalloc(S2_MMU_PER_VCPU, sizeof(*tmp), GFP_KERNEL_ACCOUNT);
 		if (!tmp)
 			return -ENOMEM;
 
-		write_lock(&kvm->mmu_lock);
+		for (int i = 0; !ret && i < S2_MMU_PER_VCPU; i++)
+			ret = init_nested_s2_mmu(kvm, &tmp[i]);
 
-		if (kvm->arch.nested_mmus_size) {
-			memcpy(tmp, kvm->arch.nested_mmus,
-			       size_mul(sizeof(*tmp), kvm->arch.nested_mmus_size));
+		if (ret) {
+			for (int i = 0; i < S2_MMU_PER_VCPU; i++)
+				kvm_free_stage2_pgd(&tmp[i]);
 
-			for (int i = 0; i < kvm->arch.nested_mmus_size; i++)
-				tmp[i].pgt->mmu = &tmp[i];
+			kvfree(tmp);
+			free_page((unsigned long)vcpu->arch.ctxt.vncr_array);
+			vcpu->arch.ctxt.vncr_array = NULL;
+			return ret;
 		}
+		
+		guard(write_lock)(&kvm->mmu_lock);
 
-		swap(kvm->arch.nested_mmus, tmp);
+		for (int i = 0; i < S2_MMU_PER_VCPU; i++)
+			kvm->arch.nested_mmus[i + kvm->arch.nested_mmus_size] = &tmp[i];
 
-		write_unlock(&kvm->mmu_lock);
-
-		kvfree(tmp);
+		kvm->arch.nested_mmus_size += S2_MMU_PER_VCPU;
 	}
 
-	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;
-
-		return ret;
-	}
-
-	kvm->arch.nested_mmus_size = num_mmus;
-
 	return 0;
 }
 
@@ -725,7 +720,7 @@ void kvm_s2_mmu_iterate_by_vmid(struct kvm *kvm, u16 vmid,
 	write_lock(&kvm->mmu_lock);
 
 	for (int 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 (!kvm_s2_mmu_valid(mmu))
 			continue;
@@ -767,7 +762,7 @@ struct kvm_s2_mmu *lookup_s2_mmu(struct kvm_vcpu *vcpu)
 	 *   if S2 translation is disabled.
 	 */
 	for (int 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 (!kvm_s2_mmu_valid(mmu))
 			continue;
@@ -806,7 +801,7 @@ static struct kvm_s2_mmu *get_s2_mmu_nested(struct kvm_vcpu *vcpu)
 	for (i = kvm->arch.nested_mmus_next;
 	     i < (kvm->arch.nested_mmus_size + kvm->arch.nested_mmus_next);
 	     i++) {
-		s2_mmu = &kvm->arch.nested_mmus[i % kvm->arch.nested_mmus_size];
+		s2_mmu = kvm->arch.nested_mmus[i % kvm->arch.nested_mmus_size];
 
 		if (atomic_read(&s2_mmu->refcnt) == 0)
 			break;
@@ -1223,7 +1218,7 @@ void kvm_nested_s2_wp(struct kvm *kvm)
 		return;
 
 	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 (kvm_s2_mmu_valid(mmu))
 			kvm_stage2_wp_range(mmu, 0, kvm_phys_size(mmu));
@@ -1242,7 +1237,7 @@ void kvm_nested_s2_unmap(struct kvm *kvm, bool may_block)
 		return;
 
 	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 (kvm_s2_mmu_valid(mmu))
 			kvm_stage2_unmap_range(mmu, 0, kvm_phys_size(mmu), may_block);
@@ -1261,7 +1256,7 @@ void kvm_nested_s2_flush(struct kvm *kvm)
 		return;
 
 	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 (kvm_s2_mmu_valid(mmu))
 			kvm_stage2_flush_range(mmu, 0, kvm_phys_size(mmu));
@@ -1270,13 +1265,14 @@ void kvm_nested_s2_flush(struct kvm *kvm)
 
 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];
+	for (int i = kvm->arch.nested_mmus_size - 1; i >= 0; i--) {
+		struct kvm_s2_mmu *mmu = kvm->arch.nested_mmus[i];
 
 		if (!WARN_ON(atomic_read(&mmu->refcnt)))
 			kvm_free_stage2_pgd(mmu);
+
+		if ((i % S2_MMU_PER_VCPU) == 0)
+			kvfree(mmu);
 	}
 	kvfree(kvm->arch.nested_mmus);
 	kvm->arch.nested_mmus = NULL;

-- 
Without deviation from the norm, progress is not possible.