Re: [PATCH 10/12] perf jitdump: Validate debug entries on native (non-swap) path

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

Pre-existing issues:
- [High] Iterating over `struct debug_entry` with `debug_entry_next()` produces unaligned pointers, causing undefined behavior and hardware exceptions (SIGBUS) on strict-alignment architectures.
- [Medium] Parsing errors in JIT dump records are silently ignored in `jit_process_dump()`, leaving stale debug and unwinding data active and overwriting error return codes.
- [Medium] `jit_repipe_code_move()` processes `JIT_CODE_MOVE` records without verifying that the record's `total_size` is large enough to contain the struct, leading to uninitialized heap memory reads.
--

--- Patch [10]: [PATCH 10/12] perf jitdump: Validate debug entries on native (non-swap) path ---
commit f31dd5de27938e11279a2fce6356aeba4e9918b4
Author: Arnaldo Carvalho de Melo <[email protected]>

perf jitdump: Validate debug entries on native (non-swap) path

The bounds-checking and nr_entry clamping added for the byte-swap path
only runs when jd->needs_bswap is true. This patch adds bounds-checked
iteration in jit_repipe_debug_info() to validate each debug_entry fits in
the payload.

> diff --git a/tools/perf/util/jitdump.c b/tools/perf/util/jitdump.c
> index d3de307532d55..5a3ea2681fb37 100644
> --- a/tools/perf/util/jitdump.c
> +++ b/tools/perf/util/jitdump.c
> @@ -694,8 +694,10 @@ static int jit_repipe_code_move(struct jit_buf_desc *jd, union jr_entry *jr)

[Severity: Medium]
This is a pre-existing issue, but does jit_repipe_code_move() process
JIT_CODE_MOVE records without verifying that the record's total_size is
large enough to contain the struct?

If a malformed JIT_CODE_MOVE record specifies a small total_size, fread()
might read 0 bytes, leaving the jr->move fields mapped to uninitialized
bytes in jd->buf. Could this cause uninitialized heap memory to be read,
byte-swapped in jit_get_next_entry(), and written to the output perf.data
file?

>  
>  static int jit_repipe_debug_info(struct jit_buf_desc *jd, union jr_entry *jr)
>  {
> -	void *data;
> -	size_t sz;
> +	struct debug_entry *ent;
> +	void *data, *end;
> +	size_t sz, valid;
> +	uint64_t i;

[ ... ]

>  	/*
> -	 * we must use nr_entry instead of size here because
> -	 * we cannot distinguish actual entry from padding otherwise
> +	 * Clamp nr_debug_entries to entries that actually fit in the
> +	 * payload.  The byte-swap path already does this for cross-endian
> +	 * files; validate on the native path too, since downstream
> +	 * jit_process_debug_info() iterates via debug_entry_next() which
> +	 * calls strlen() on each entry's name field.
>  	 */
> -	jd->nr_debug_entries = jr->info.nr_entry;
> +	end = data + sz;
> +	ent = data;
> +	valid = 0;
> +	for (i = 0; i < jr->info.nr_entry; i++) {
> +		if ((void *)ent + sizeof(*ent) > end)
> +			break;
> +		/* name must be NUL-terminated within the payload */
> +		if (!memchr(ent->name, '\0', (char *)end - ent->name))
> +			break;
> +		ent = debug_entry_next(ent);

[Severity: High]
This isn't a bug introduced by this patch, but does debug_entry_next()
produce unaligned pointers here?

Because struct debug_entry contains 64-bit and 32-bit fields but lacks
__attribute__((packed)), the compiler emits 8-byte aligned memory accesses.
Since the variable-length string does not pad to 8-byte boundaries,
subsequent entries appear to become naturally unaligned in memory.

When downstream code dereferences these fields, such as:

tools/perf/util/genelf_debug.c:jit_process_debug_info() {
    ...
    ent->addr = ent->addr - code_addr;
    ...
}

could this perform unaligned 64-bit loads and stores, triggering hardware
alignment faults (SIGBUS) on strict-alignment architectures?

> +		valid++;
> +	}
> +	jd->nr_debug_entries = valid;
>  
>  	return 0;
>  }

[Severity: Medium]
This is a pre-existing issue, but are the error codes returned by this
function silently ignored by its caller?

In jit_process_dump():

tools/perf/util/jitdump.c:jit_process_dump() {
    ...
    ret = jit_repipe_debug_info(jd, jr);
    ...
    default:
        ret = 0;
        continue;
}

If jit_repipe_debug_info() returns -1 (e.g., due to OOM), the while loop
doesn't seem to break. If a subsequent unrecognized record or JIT_CODE_CLOSE
triggers the default case, ret is overwritten to 0. Does this successfully
hide the parse failure and leave stale debug data active?

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