Re: [PATCH v2 2/3] riscv: ptdump: Use per-level attribute bits for parsing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The refactoring is incomplete because `note_page()` and `dump_addr()` still hardcode `kernel_pg_levels` instead of using the caller-provided `st->pg_level` array.
- [Medium] The `dump_prot()` function retains hardcoded, architecture-specific logic for specific bitmasks, which is unsafe when processing generic, caller-provided bit arrays.
--

--- Patch [2]: [PATCH v2 2/3] riscv: ptdump: Use per-level attribute bits for parsing ---
commit 1ffa1d459c45da98aa9b8ff61631b9ce3dc228c8
Author: Dylan.Wu <fredwudi0305@gmail.com>

riscv: ptdump: Use per-level attribute bits for parsing

This commit updates the RISC-V page table dumper to use per-level attribute
bits and names passed via the ptdump_pg_state structure. This refactoring is
intended to allow future users, such as KVM, to provide their own protection
bits for parsing.

> diff --git a/arch/riscv/mm/ptdump.c b/arch/riscv/mm/ptdump.c
> --- a/arch/riscv/mm/ptdump.c
> +++ b/arch/riscv/mm/ptdump.c
> @@ -176,21 +188,21 @@ static void dump_prot(struct ptdump_pg_state *st)
>  
>  		val = st->current_prot & bits[i].mask;
>  		if (val) {
> -			if (pte_bits[i].mask == _PAGE_SOFT)
> -				snprintf(s, sizeof(s), pte_bits[i].set, val >> 8);
> +			if (bits[i].mask == _PAGE_SOFT)
> +				snprintf(s, sizeof(s), bits[i].set, val >> 8);

[Severity: Medium]
Since the goal of this patch is to support custom caller-provided bit arrays,
is it safe to retain these hardcoded checks for _PAGE_SOFT (and
_PAGE_MTMASK_SVPBMT below)?

If a future caller (like KVM) provides a custom bitmask that happens to match
the value of _PAGE_SOFT, but provides a format string in bits[i].set that
doesn't expect an integer, could this snprintf() cause a crash or output
garbage?

[ ... ]

> @@ -221,7 +233,7 @@ static void dump_addr(struct ptdump_pg_state *st, unsigned long addr)
>  	}
>  
>  	pt_dump_seq_printf(st->seq, "%9lu%c %s", delta, *unit,
> -			   pg_level[st->level].name);
> +			   kernel_pg_levels[st->level].name);
>  }

[Severity: Medium]
Will this ignore the caller's custom level names?

It looks like dump_addr() was updated to hardcode kernel_pg_levels instead
of using the caller-provided st->pg_level[st->level].name.

[ ... ]

> @@ -247,7 +259,7 @@ void note_page(struct ptdump_state *pt_st, unsigned long addr,
>  	u64 prot = 0;
>  
>  	if (level >= 0)
> -		prot = val & pg_level[level].mask;
> +		prot = val & kernel_pg_levels[level].mask;

[Severity: Medium]
Similar to the issue in dump_addr(), will this ignore the custom bitmasks
provided by the caller?

By hardcoding kernel_pg_levels[level].mask instead of using
st->pg_level[level].mask, it seems this would cause incorrect page table
attribute masking for custom callers.

Additionally, could this cause an out-of-bounds array read if a custom caller
uses more levels than the 5 defined in kernel_pg_levels?

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