Re: [PATCH 2/2] KVM: riscv: Register ptdump with debugfs on guest creation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Lockless reads of the guest PGD pointer (`kvm->arch.pgd`) outside `kvm->mmu_lock` lead to a Use-After-Free (UAF) and potential kernel stack overflow via undefined behavior.
- [High] Unbounded full page table walk under `mmu_lock` combined with `single_open` buffer restarts causes severe lock contention.
- [Medium] Concurrent reads of the debugfs file cause a data race by modifying the global static array `gstage_pg_levels` without synchronization.
- [Medium] The page table walk hardcodes the loop bound to `PTRS_PER_PTE`, silently truncating the guest address space dump for concatenated root tables.
--

commit efa8af5af3c60372dfe6205bc307cb96b4b1cf7a
Author: Dylan.Wu <fredwudi0305@gmail.com>
Subject: KVM: riscv: Register ptdump with debugfs on guest creation

Introduce KVM ptdump to show the guest gstage pagetables. This registers
a 'gstage_page_tables' file under the guest debugfs directory.
Userspace can now inspect the gstage layout and permissions, which
is useful for architectural debugging and memory management audits.

> diff --git a/arch/riscv/kvm/ptdump.c b/arch/riscv/kvm/ptdump.c
> new file mode 100644
> index 0000000000000..972d45d691b2b
> --- /dev/null
> +++ b/arch/riscv/kvm/ptdump.c

[ ... ]

> +static void kvm_ptdump_walk_level(struct ptdump_state *pt_st,
> +				  unsigned long *tbl, int level,
> +				  unsigned long start_addr)
> +{
> +	unsigned long addr = start_addr;
> +	unsigned long next, virt_addr;
> +	int i;
> +	unsigned long step = 1UL << (PAGE_SHIFT + (4 - level) * 9);

[Severity: High]
Can step calculation result in a negative bitwise shift? If level > 4 (which
could occur if pgd_levels is read as 0 due to concurrent VM destruction), this
triggers undefined behavior.

> +
> +	for (i = 0; i < PTRS_PER_PTE; i++, addr += step) {

[Severity: Medium]
Does this hardcoded loop bound truncate the guest address space dump? RISC-V 
KVM uses concatenated page tables for the top-level gstage directory (up to 
2048 entries for a 16KB root table), but this caps iteration at PTRS_PER_PTE.

> +		unsigned long val = tbl[i];

[ ... ]

> +static int kvm_ptdump_visitor(struct seq_file *m, void *v)
> +{
> +	struct kvm_ptdump_state *st = m->private;
> +	struct kvm *kvm = st->kvm;
> +	unsigned long *pgd = (unsigned long *)kvm->arch.pgd;
> +	int start_level = 5 - kvm->arch.pgd_levels;

[Severity: High]
Are these lockless reads of pgd and pgd_levels safe? If the VM is torn down 
concurrently, kvm_riscv_mmu_free_pgd() could clear these fields and free the 
pages. This read happens outside of mmu_lock, which might result in a dangling
pointer being passed to the page table walker.

> +	int i, j;
> +
> +	st->parser_state.level = -1;
> +	st->parser_state.start_address = 0;
> +	st->parser_state.seq = m;
> +
> +	for (i = 0; i < ARRAY_SIZE(gstage_pg_levels); i++) {
> +		gstage_pg_levels[i].bits = gstage_pte_bits;

[Severity: Medium]
Does this loop modify the global static array gstage_pg_levels without any 
synchronization? If multiple threads read the debugfs file concurrently, this
data race could cause corrupted or incomplete page table dumps.

> +		gstage_pg_levels[i].num = ARRAY_SIZE(gstage_pte_bits);
> +		gstage_pg_levels[i].mask = 0;
> +		for (j = 0; j < ARRAY_SIZE(gstage_pte_bits); j++)
> +			gstage_pg_levels[i].mask |= gstage_pte_bits[j].mask;
> +	}
> +
> +	read_lock(&kvm->mmu_lock);
> +	if (pgd) {
> +		kvm_ptdump_walk_level(&st->parser_state.ptdump, pgd,
> +			start_level, 0);
> +	}
> +	read_unlock(&kvm->mmu_lock);

[Severity: High]
Could walking the entire page table under mmu_lock cause severe lock contention
or a soft lockup for the guest VM, especially when combined with the buffer 
restarts from single_open() below?

> +
> +	note_page(&st->parser_state.ptdump, 0, -1, 0);
> +	return 0;
> +}
> +
> +static int kvm_ptdump_open(struct inode *inode, struct file *file)
> +{
> +	struct kvm *kvm = inode->i_private;
> +	struct kvm_ptdump_state *st;
> +	int ret;

[ ... ]

> +	st->parser_state.marker = st->marker;
> +	st->parser_state.pg_level = gstage_pg_levels;
> +	st->parser_state.ptdump.range = st->range;
> +
> +	ret = single_open(file, kvm_ptdump_visitor, st);

[Severity: High]
Does using single_open() here risk excessive retries? Since single_open() 
collects output into a contiguous buffer, if the output exceeds the initial 
buffer size, it discards it, doubles the allocation, and restarts the visitor. 
Each restart would repeat the entire recursive page table walk under the 
mmu_lock.

> +	if (ret) {
> +		kfree(st);
> +		kvm_put_kvm(kvm);
> +	}
> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701085030.124579-1-fredwudi0305@gmail.com?part=2