KVM: arm64: nv: Shadow S2 life-cycle fixes

PATCH Applied Marc Zyngier 4 participants 7 messages Sep 11 – Sep 15

Subsystems (3)

Stable branch arm64 KVM arm64

lore

Share & tools

lore

Marc Zyngier

author maintainer

2026-09-11 16:22 UTC #0 lore raw

v4 cover KVM: arm64: nv: Shadow S2 life-cycle fixes

7.3-rc2 has introduced annoying conflicts with this, so in the spirit of making things a bit easier for Oliver, here's the version rebased on -rc2!

* From v3 [1]

- Fixed spacing (Wei-Lin)

- Rebased on 7.3-rc2

- Collected RB from Lorenzo, with thanks.

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

Marc Zyngier (2):
KVM: arm64: nv: Fix life cycle of the nested_mmus array
KVM: arm64: nv: Delay freeing of shadow S2 structures until VM
destruction

 arch/arm64/include/asm/kvm_host.h   |   2 +-
 arch/arm64/include/asm/kvm_nested.h |   3 +-
 arch/arm64/kvm/arm.c                |   8 ++-
 arch/arm64/kvm/nested.c             | 100 ++++++++++++++--------------
 4 files changed, 58 insertions(+), 55 deletions(-)

Signature · 1 line

2.47.3

Marc Zyngier

author maintainer

2026-09-11 16:22 UTC #1 lore raw

PATCH v4 1/2 KVM: arm64: nv: Fix life cycle of the nested_mmus array

The nested_mmus array holds the shadow page tables that are used when a guest is running a nested context. These structures are allocated on VCPU_INIT for whole guest, which implies that they may have to be relocated as the array grows.

Should a VCPU_INIT occur whilst a vcpu is actively running an L2 and that the allocation requires relocation, that vcpu will still be running with a pointer to the previous structure, which will have been freed.

Fix this by turning the array of structures to an array of pointers, which is now allocated at VM creation, sized to the absolute maximum that KVM can handle.

In turn, each VCPU_INIT contributes S2_MMU_PER_VCPU to the pool. No reallocation is ever performed, and the life cycle of each object is much clearer:

- the nested_mmus array is allocated in kvm_init_nested(), and freed in kvm_arch_destroy_vm()

- s2_mmu structures are allocated in kvm_vcpu_init_nested(), and freed on kvm_arch_flush_shadow_all()

Finally, the freeing of vcpu->arch.vncr_array is made consistent rather than being done on some failure paths, but not others.

Fixes 4f128f8e1aaa ("KVM: arm64: nv: Support multiple nested Stage-2 mmu structures")

Reported-by Shen Yongchao <[email protected]>

Reported-by Karl Mehltretter <[email protected]>

Acked-by Lorenzo Stoakes (ARM) <[email protected]>

Link https://lore.kernel.org/r/[email protected] open here

Signed-off-by Marc Zyngier <[email protected]>

Cc [email protected]

---
 arch/arm64/include/asm/kvm_host.h   |  2 +-
 arch/arm64/include/asm/kvm_nested.h |  2 +-
 arch/arm64/kvm/arm.c                |  8 ++-
 arch/arm64/kvm/nested.c             | 93 +++++++++++++----------------
 4 files changed, 51 insertions(+), 54 deletions(-)

arch/arm64/include/asm/kvm_host.h +1 -1

index 27fe0cd5b2d7a..cd9b9d2462f96 100644
@@ -322,7 +322,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;

arch/arm64/include/asm/kvm_nested.h +1 -1

index 1ed7083358096..5b8edb2e8a87d 100644
@@ -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);

arch/arm64/kvm/arm.c +6 -2

index 8b080804bc90b..b53219e048bdf 100644
@@ -236,8 +236,6 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
 	mutex_unlock(&kvm->lock);
 #endif
 
-	kvm_init_nested(kvm);
-
 	ret = kvm_share_hyp(kvm, kvm + 1);
 	if (ret)
 		return ret;

@@ -252,6 +250,10 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
 	if (ret)
 		goto err_free_cpumask;
 
+	ret = kvm_init_nested(kvm);
+	if (ret)
+		goto err_uninit_mmu;
+
 	if (is_protected_kvm_enabled()) {
 		/*
 		 * If any failures occur after this is successful, make sure to

@@ -280,6 +282,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
 
 err_uninit_mmu:
 	kvm_uninit_stage2_mmu(kvm);
+	kvfree(kvm->arch.nested_mmus);
 err_free_cpumask:
 	free_cpumask_var(kvm->arch.supported_cpus);
 err_unshare_kvm:

@@ -337,6 +340,7 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
 
 	kvm_unshare_hyp(kvm, kvm + 1);
 
+	kvfree(kvm->arch.nested_mmus);
 	kvm_arm_teardown_hypercalls(kvm);
 }

arch/arm64/kvm/nested.c +43 ~~-50~~

171 lines not on this page · raw message

Signature · 1 line

2.47.3

[email protected]

AI review

2026-09-11 16:44 UTC #2 lore raw

Re: [PATCH v4 1/2] KVM: arm64: nv: Fix life cycle of the nested_mmus array

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] Data race in `kvm_arch_flush_shadow_all()` leads to garbage pointer dereference and kernel panic --

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Data race in kvm_arch_flush_shadow_all() leads to garbage pointer dereference and kernel panic --

commit 87dd1db0613c9f0d7e10bf7593d998a853fa1c02
Author: Marc Zyngier <[email protected]>

KVM: arm64: nv: Fix life cycle of the nested_mmus array

This commit alters the life cycle of the nested_mmus array to allocate an array of pointers at VM creation time. It avoids reallocation during VCPU_INIT by sizing the array to the absolute maximum KVM can handle.

Link https://lore.kernel.org/r/[email protected] open here

Quoted 4 lines

arch/arm64/kvm/nested.c +0 -0
index 3c4fc566eafc6..0a32d6bd3f6c6 100644

[ ... ]

arch/arm64/kvm/nested.c struct vncr_tlb { · line 52

Quoted 11 lines

arch/arm64/kvm/nested.c +4 -2

@@ -45,11 +45,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_objs(struct kvm_s2_mmu *,
+					      KVM_MAX_VCPUS * S2_MMU_PER_VCPU,
+					      GFP_KERNEL_ACCOUNT);

[Severity: High]
Does this allocation leave the pointer array filled with uninitialized memory? Using kvmalloc_objs() without GFP_ZERO or __GFP_ZERO (or instead of kvzalloc_objs()) means the array might contain garbage pointers rather than being safely zeroed out.

[ ... ]

Quoted 4 lines

arch/arm64/kvm/nested.c +1 -2

+		guard(write_lock)(&kvm->mmu_lock);
 
-		for (int i = 0; i < kvm->arch.nested_mmus_size; i++)
-			tmp[i].pgt->mmu = &tmp[i];

arch/arm64/kvm/nested.c int kvm_vcpu_init_nested(struct kvm_vcpu *vcpu)

Quoted 10 lines

arch/arm64/kvm/nested.c +3 -6

+		for (i = 0; i < S2_MMU_PER_VCPU; i++)
+			kvm->arch.nested_mmus[i + kvm->arch.nested_mmus_size] = &tmp[i];
 
-		swap(kvm->arch.nested_mmus, tmp);
-
-		write_unlock(&kvm->mmu_lock);
-
-		kvfree(tmp);
-	}
+		kvm->arch.nested_mmus_size += S2_MMU_PER_VCPU;

[Severity: High]
Could a CPU or compiler reorder this size increment to be visible before the pointer stores are complete? This block updates the array and size without memory barriers to order them.

[ ... ]

arch/arm64/kvm/nested.c void kvm_nested_s2_flush(struct kvm *kvm) · line 1317

Quoted 10 lines

void kvm_arch_flush_shadow_all(struct kvm *kvm)
{

arch/arm64/kvm/nested.c +2 -4

-	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)))

[Severity: High]
Can this lockless read of the array encounter a garbage pointer?

If kvm_arch_flush_shadow_all() is called concurrently with kvm_vcpu_init_nested() (for example, if userspace calls KVM_ARM_VCPU_INIT while concurrently terminating the process and triggering mmu_notifier_release()), it might read the newly incremented nested_mmus_size before the actual pointers are stored.

Because the initial array allocation in kvm_init_nested() used kvmalloc_objs() without zeroing, the unwritten slots contain uninitialized memory. Reading a garbage pointer and dereferencing it here in atomic_read() could allow an unprivileged userspace process to cause a kernel panic. Pre-patch, the array elements were safely zero-initialized.

I notice a later patch in this series (Patch 2) removes the kvm->arch.nested_mmus_size = 0 assignment but retains this same lockless loop in kvm_arch_flush_shadow_all(), which appears to leave this garbage pointer dereference risk unfixed.

Signature · 1 line

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

Wei-Lin Chang

Reviewed-by on v4 1/2

2026-09-11 18:57 UTC #3 lore raw

Re: [PATCH v4 1/2] KVM: arm64: nv: Fix life cycle of the nested_mmus array

In reply to Marc Zyngier

Quoted 27 lines · Marc Zyngier

The nested_mmus array holds the shadow page tables that are used when a guest is running a nested context. These structures are allocated on VCPU_INIT for whole guest, which implies that they may have to be relocated as the array grows.
Should a VCPU_INIT occur whilst a vcpu is actively running an L2 and that the allocation requires relocation, that vcpu will still be running with a pointer to the previous structure, which will have been freed.
Fix this by turning the array of structures to an array of pointers, which is now allocated at VM creation, sized to the absolute maximum that KVM can handle.
In turn, each VCPU_INIT contributes S2_MMU_PER_VCPU to the pool. No reallocation is ever performed, and the life cycle of each object is much clearer:
- the nested_mmus array is allocated in kvm_init_nested(), and freed in kvm_arch_destroy_vm()
- s2_mmu structures are allocated in kvm_vcpu_init_nested(), and freed on kvm_arch_flush_shadow_all()
Finally, the freeing of vcpu->arch.vncr_array is made consistent rather than being done on some failure paths, but not others.

Quoted 8 lines

Fixes 4f128f8e1aaa ("KVM: arm64: nv: Support multiple nested Stage-2 mmu structures")
Reported-by Shen Yongchao <[email protected]>
Reported-by Karl Mehltretter <[email protected]>
Suggested-by Karl Mehltretter <[email protected]>
Acked-by Lorenzo Stoakes (ARM) <[email protected]>
Link https://lore.kernel.org/r/[email protected] open here
Signed-off-by Marc Zyngier <[email protected]>
Cc [email protected]

Reviewed-by Wei-Lin Chang <[email protected]>

Quoted 1 line

---

[...]

Marc Zyngier

author maintainer

2026-09-11 16:22 UTC #4 lore raw

PATCH v4 2/2 KVM: arm64: nv: Delay freeing of shadow S2 structures until VM destruction

In reply to Marc Zyngier

We free the shadow S2 structures from kvm_arch_flush_shadow_all(), which is a Bad Idea(tm). Freeing the page tables is fair game (this is what this callback is for), but freeing the container that could still be referenced by another part of the system is not great.

Instead, grow separate destructors that gets called when we tear the VM down for good. From there, we can nuke both the individual MMUs as well as the global array that points to them, safe in the knowledge that the vcpus themselves have been destroyed already.

Fixes 4f128f8e1aaac ("KVM: arm64: nv: Support multiple nested Stage-2 mmu structures")

Reviewed-by Lorenzo Stoakes (ARM) <[email protected]>

Signed-off-by Marc Zyngier <[email protected]>

Cc [email protected]

---
 arch/arm64/include/asm/kvm_nested.h |  1 +
 arch/arm64/kvm/arm.c                |  4 ++--
 arch/arm64/kvm/nested.c             | 15 ++++++++++-----
 3 files changed, 13 insertions(+), 7 deletions(-)

arch/arm64/include/asm/kvm_nested.h +1 -0

index 5b8edb2e8a87d..586026e859030 100644
@@ -67,6 +67,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 int kvm_init_nested(struct kvm *kvm);
+extern void kvm_destroy_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);

arch/arm64/kvm/arm.c +2 -2

index b53219e048bdf..eaf583b771931 100644
@@ -282,7 +282,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
 
 err_uninit_mmu:
 	kvm_uninit_stage2_mmu(kvm);
-	kvfree(kvm->arch.nested_mmus);
+	kvm_destroy_nested(kvm);
 err_free_cpumask:
 	free_cpumask_var(kvm->arch.supported_cpus);
 err_unshare_kvm:

@@ -340,7 +340,7 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
 
 	kvm_unshare_hyp(kvm, kvm + 1);
 
-	kvfree(kvm->arch.nested_mmus);
+	kvm_destroy_nested(kvm);
 	kvm_arm_teardown_hypercalls(kvm);
 }

arch/arm64/kvm/nested.c +10 -5

index 0a32d6bd3f6c6..d60f6f69e293d 100644
@@ -56,6 +56,15 @@ int kvm_init_nested(struct kvm *kvm)
 	return kvm->arch.nested_mmus ? 0 : -ENOMEM;
 }
 
+void kvm_destroy_nested(struct kvm *kvm)
+{
+	for (int i = 0; i < kvm->arch.nested_mmus_size; i+= S2_MMU_PER_VCPU)
+		kvfree(kvm->arch.nested_mmus[i]);
+
+	kvm->arch.nested_mmus_size = 0;
+	kvfree(kvm->arch.nested_mmus);
+}
+
 static int init_nested_s2_mmu(struct kvm *kvm, struct kvm_s2_mmu *mmu)
 {
 	/*

@@ -1311,16 +1320,12 @@ void kvm_nested_s2_flush(struct kvm *kvm)
 
 void kvm_arch_flush_shadow_all(struct kvm *kvm)
 {
-	for (int i = kvm->arch.nested_mmus_size - 1; i >= 0; i--) {
+	for (int i = 0; i < kvm->arch.nested_mmus_size; 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);
 	}
-	kvm->arch.nested_mmus_size = 0;
 	kvm_uninit_stage2_mmu(kvm);
 }

Signature · 1 line

2.47.3

Wei-Lin Chang

Reviewed-by on v4 2/2

2026-09-11 18:59 UTC #5 lore raw

Re: [PATCH v4 2/2] KVM: arm64: nv: Delay freeing of shadow S2 structures until VM destruction

Quoted 10 lines · Marc Zyngier

We free the shadow S2 structures from kvm_arch_flush_shadow_all(), which is a Bad Idea(tm). Freeing the page tables is fair game (this is what this callback is for), but freeing the container that could still be referenced by another part of the system is not great.
Instead, grow separate destructors that gets called when we tear the VM down for good. From there, we can nuke both the individual MMUs as well as the global array that points to them, safe in the knowledge that the vcpus themselves have been destroyed already.

Quoted 4 lines

Fixes 4f128f8e1aaac ("KVM: arm64: nv: Support multiple nested Stage-2 mmu structures")
Reviewed-by Lorenzo Stoakes (ARM) <[email protected]>
Signed-off-by Marc Zyngier <[email protected]>
Cc [email protected]

Reviewed-by Wei-Lin Chang <[email protected]>

Quoted 1 line

---

[...]

Oliver Upton

maintainer Applied

2026-09-15 22:25 UTC #6 lore raw

In reply to Marc Zyngier

Quoted 9 lines · Marc Zyngier

7.3-rc2 has introduced annoying conflicts with this, so in the spirit of making things a bit easier for Oliver, here's the version rebased on -rc2!
* From v3 [1]
- Fixed spacing (Wei-Lin)
[...]

Applied to fixes, thanks!

[1/2] KVM: arm64: nv: Fix life cycle of the nested_mmus array

      https://git.kernel.org/kvmarm/kvmarm/c/33346f8960c7
[2/2] KVM: arm64: nv: Delay freeing of shadow S2 structures until VM destruction
      https://git.kernel.org/kvmarm/kvmarm/c/e5843f4effaa

--
Best,
Oliver