Re: [PATCH v2 6/6] KVM: arm64: ptdump: Introduce the shadow ptdump file

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] Concurrent reallocation of nested_mmus array leads to Use-After-Free in kvm_ptdump_guest_nested_show().
- [High] If kvm_ptdump_parser_init() fails, st->kvm is left as NULL, causing a kernel panic on subsequent debugfs reads.
- [Medium] Missing note_page_flush() causes the last contiguous block of page table entries to be silently dropped.

Pre-existing issues:
- [Critical] kvm_arch_flush_shadow_all() frees the nested_mmus array even if active VCPUs hold references to its elements.
- [High] Unprotected iteration over nested_mmus in kvm_arch_flush_shadow_all() races with array reallocation.
--

commit 4dce12b467bf2d3540834dccee1f3e623af43d42
Author: Wei-Lin Chang <weilin.chang@arm.com>

KVM: arm64: ptdump: Introduce the shadow ptdump file

This commit creates a ptdump file for shadow page tables used in nested
virtualization. It dumps all valid shadow page tables, their mmu index,
VTCR_EL2, VTTBR_EL2, and stage-2 enablement status. It also modifies
kvm_arch_flush_shadow_all() to detach the nested mmu array under mmu_lock.

> diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
> index efd5d21c7ac75..cbf7143819669 100644
> --- a/arch/arm64/kvm/nested.c
> +++ b/arch/arm64/kvm/nested.c
> @@ -1184,6 +1184,7 @@ void kvm_nested_s2_flush(struct kvm *kvm)
>  
>  void kvm_arch_flush_shadow_all(struct kvm *kvm)
>  {
> +	struct kvm_s2_mmu *mmus;
>  	int i;
>  
>  	for (i = 0; i < kvm->arch.nested_mmus_size; i++) {

[Severity: High]
This is a pre-existing issue, but can this loop race with array reallocation?

If a concurrent thread calls KVM_ARM_VCPU_INIT to initialize a new nested
vCPU, kvm_vcpu_init_nested() expands the array via kvrealloc() holding
only kvm->arch.config_lock. Since this loop doesn't hold config_lock or
mmu_lock, could it dereference the array while or after it is freed?

>  		struct kvm_s2_mmu *mmu = &kvm->arch.nested_mmus[i];
>  
>  		if (!WARN_ON(atomic_read(&mmu->refcnt)))
>  			kvm_free_stage2_pgd(mmu);
>  	}
> -	kvfree(kvm->arch.nested_mmus);
> -	kvm->arch.nested_mmus = NULL;
> -	kvm->arch.nested_mmus_size = 0;
> +
> +	scoped_guard(write_lock, &kvm->mmu_lock) {
> +		mmus = kvm->arch.nested_mmus;
> +		kvm->arch.nested_mmus = NULL;
> +		kvm->arch.nested_mmus_size = 0;
> +	}
> +
> +	kvfree(mmus);

[Severity: Critical]
This is a pre-existing issue, but does this prematurely free the array if 
active VCPUs hold references?

If a running vCPU holds a reference to its nested MMU, where atomic_read
shows refcnt > 0, the loop above skips freeing the page table, but the
backing array is freed anyway. 

When the vCPU exits and calls kvm_vcpu_put_hw_mmu(), it will dereference the
freed pointer. Should the array lifetime be tied to the active references?

>  	kvm_uninit_stage2_mmu(kvm);
>  }

[ ... ]

> diff --git a/arch/arm64/kvm/ptdump.c b/arch/arm64/kvm/ptdump.c
> index 40f93b7c7ad98..1649eaa75798c 100644
> --- a/arch/arm64/kvm/ptdump.c
> +++ b/arch/arm64/kvm/ptdump.c
> @@ -181,6 +181,50 @@ static int kvm_ptdump_guest_canonical_show(struct seq_file *m, void *unused)
>  	return 0;
>  }
>  
> +static int kvm_ptdump_guest_nested_show(struct seq_file *m, void *unused)
> +{
> +	int ret = 0, i;
> +	struct kvm_ptdump_guest_state *st = m->private;
> +	struct kvm *kvm = st->kvm;
> +	struct kvm_pgtable_walker walker = (struct kvm_pgtable_walker) {
> +		.cb	= kvm_ptdump_visitor,
> +		.arg	= &st->parser_state,
> +		.flags	= KVM_PGTABLE_WALK_LEAF,
> +	};
> +
> +	guard(write_lock)(&kvm->mmu_lock);
> +
> +	if (!kvm->arch.nested_mmus)
> +		return 0;
> +
> +	for (i = 0; i < kvm->arch.nested_mmus_size; i++) {

[Severity: High]
Is it safe to iterate over the nested_mmus array holding only mmu_lock?

If userspace reads the debugfs file while a concurrent thread issues a
KVM_ARM_VCPU_INIT ioctl to initialize a new nested vCPU, kvm_vcpu_init_nested()
expands the array via kvrealloc() holding only kvm->arch.config_lock:

kvm_vcpu_init_nested()
    tmp = kvrealloc(kvm->arch.nested_mmus, ...);
    swap(kvm->arch.nested_mmus, tmp);

Could the debugfs reader dereference the array while or after it is freed by
kvrealloc()?

> +		struct kvm_s2_mmu *mmu = &kvm->arch.nested_mmus[i];
> +
> +		if (!mmu->pgt)
> +			continue;
> +
> +		if (kvm_s2_mmu_valid(mmu)) {
> +			memset(st, 0, sizeof(*st));
> +			ret = kvm_ptdump_parser_init(st, kvm, mmu->pgt);
> +			if (ret)
> +				return ret;

[Severity: High]
Does an early return here leave st->kvm as NULL?

The memset() clears st->kvm. If kvm_ptdump_parser_init() returns an error,
the function propagates it, leaving st->kvm NULL. 

If userspace issues another read() on the same file descriptor, seq_read will
call the show() callback again, which will dereference the NULL st->kvm
at the mmu_lock acquisition:

guard(write_lock)(&kvm->mmu_lock);

Could we restore st->kvm on the error path?

> +			st->parser_state = (struct ptdump_pg_state) {
> +				.marker		= &st->ipa_marker[0],
> +				.level		= -1,
> +				.pg_level	= &st->level[0],
> +				.seq		= m,
> +			};
> +			seq_printf(m, "nested mmu %d VTCR: 0x%016llx VTTBR: 0x%016llx s2: %s\n",
> +				   i, mmu->tlb_vtcr, mmu->tlb_vttbr,
> +				   mmu->nested_stage2_enabled ? "enabled" : "disabled");
> +			ret = kvm_pgtable_walk(mmu->pgt, 0, BIT(mmu->pgt->ia_bits), &walker);
> +			if (ret)
> +				return ret;

[Severity: Medium]
Does the page table walk lack a subsequent call to note_page_flush()?

The ptdump API requires a final call to note_page_flush() to emit the final
buffered contiguous block of page table attributes. Because this is missing
after the kvm_pgtable_walk() completion, does the last block get silently
dropped from the debugfs output?

> +		}
> +	}
> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260630121005.1130996-1-weilin.chang@arm.com?part=6