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

[+ Shen, who privately reported this with a 7 hour lead...]

On Tue, 04 Aug 2026 15:31:13 +0100,
Marc Zyngier <[email protected]> wrote:
> 
> On Mon, 03 Aug 2026 23:44:04 +0100,
> Karl Mehltretter <[email protected]> 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: [email protected]
> > Assisted-by: Claude:claude-fable-5
> > Signed-off-by: Karl Mehltretter <[email protected]>
> > ---
> > 
> > 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.

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