Re: [PATCH 04/12] perf jitdump: Bounds-check debug entry byte-swap loop

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

Pre-existing issues:
- [High] The newly added bounds-checking and string NUL-termination validation for debug entries are isolated within the `if (jd->needs_bswap)` block. For native-endian files, validation is bypassed, leaving out-of-bounds heap read/write vulnerabilities unmitigated.
--

--- Patch [4]: [PATCH 04/12] perf jitdump: Bounds-check debug entry byte-swap loop ---
commit b0b40e7a47c677d680f97fb1312ed129f617d651
Author: Arnaldo Carvalho de Melo <[email protected]>

perf jitdump: Bounds-check debug entry byte-swap loop

This commit replaces array indexing with pointer arithmetic for debug_entry
structures in the JIT_CODE_DEBUG_INFO byte-swap loop to handle
variable-length names. It also adds bounds checking against total_size to
prevent out-of-bounds reads.

> diff --git a/tools/perf/util/jitdump.c b/tools/perf/util/jitdump.c
> index 787f8a03dae87..078d3304d2b7e 100644
> --- a/tools/perf/util/jitdump.c
> +++ b/tools/perf/util/jitdump.c

[ ... ]

> @@ -318,14 +318,32 @@ jit_get_next_entry(struct jit_buf_desc *jd)
>  	switch(id) {
>  	case JIT_CODE_DEBUG_INFO:
>  		if (jd->needs_bswap) {
> +			void *end = (void *)jr + jr->prefix.total_size;
> +			struct debug_entry *ent;
>  			uint64_t n;
> +
>  			jr->info.code_addr = bswap_64(jr->info.code_addr);
>  			jr->info.nr_entry  = bswap_64(jr->info.nr_entry);
> -			for (n = 0 ; n < jr->info.nr_entry; n++) {
> -				jr->info.entries[n].addr    = bswap_64(jr->info.entries[n].addr);
> -				jr->info.entries[n].lineno  = bswap_32(jr->info.entries[n].lineno);
> -				jr->info.entries[n].discrim = bswap_32(jr->info.entries[n].discrim);
> +
> +			/*
> +			 * debug_entry has a variable-length name[], so array
> +			 * indexing would compute wrong offsets — use
> +			 * debug_entry_next() and bounds-check each entry.
> +			 */
> +			ent = &jr->info.entries[0];
> +			for (n = 0; n < jr->info.nr_entry; n++) {
> +				if ((void *)ent + sizeof(*ent) > end)
> +					break;
> +				/* name must be NUL-terminated within the record */
> +				if (!memchr(ent->name, '\0', (char *)end - ent->name))
> +					break;
> +				ent->addr    = bswap_64(ent->addr);
> +				ent->lineno  = bswap_32(ent->lineno);
> +				ent->discrim = bswap_32(ent->discrim);
> +				ent = debug_entry_next(ent);
>  			}
> +			/* clamp so downstream consumers don't overrun */
> +			jr->info.nr_entry = n;
>  		}
>  		break;

[Severity: High]
This is a pre-existing issue, but by keeping the new bounds-checking and
clamping logic exclusively inside the if (jd->needs_bswap) block, doesn't
this leave native-endian files unvalidated?

For a native-endian file, jd->needs_bswap is false, so jr->info.nr_entry
bypasses this clamping in jit_get_next_entry(). Later in
tools/perf/util/jitdump.c, it is assigned directly:

jit_repipe_debug_info() {
    ...
    jd->nr_debug_entries = jr->info.nr_entry;
    ...
}

This unvalidated count is then used in tools/perf/util/genelf_debug.c:

jit_process_debug_info() {
    ...
    for (i = 0; i < nr_debug_entries; i++) {
        ent->addr = ent->addr - code_addr;
        ent = debug_entry_next(ent);
    }
    ...
}

If the number of entries is artificially inflated, or if NUL terminators are
missing, could this lead to out-of-bounds reads and writes in
jit_process_debug_info() when processing a native-endian jitdump file?

Should the entry validation and clamping logic be pulled out of the
jd->needs_bswap block so it applies to all files regardless of endianness?

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