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

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

New issues:
- [Medium] Replacing array indexing with sequential pointer arithmetic (`debug_entry_next`) on unpadded, variable-length structures results in unaligned struct pointers, causing undefined behavior and SIGBUS crashes on strict-alignment architectures.

Pre-existing issues:
- [Critical] The newly added bounds checking and `nr_entry` clamping for `JIT_CODE_DEBUG_INFO` records are scoped exclusively inside the `if (jd->needs_bswap)` block, leaving native-endian files completely vulnerable to a massive heap buffer overflow.
- [High] Missing validation of `unwinding_size` against `unwinding_data_size` in `JIT_CODE_UNWINDING_INFO` leads to an out-of-bounds read during ELF generation.
- [Medium] The `event` buffer is leaked on all return paths in `jit_repipe_code_move()`.
- [Medium] `jit_repipe_code_move` fails to validate that the record's `total_size` is large enough to contain the `jr_code_move` payload, leading to the processing of uninitialized memory.
--

--- Patch [5]: [PATCH 05/12] perf jitdump: Bounds-check debug entry byte-swap loop ---
commit 14beafbf2609e41d04f4020f064d84fdbb15c5a7
Author: Arnaldo Carvalho de Melo <[email protected]>

perf jitdump: Bounds-check debug entry byte-swap loop
    
This commit replaces array indexing with pointer arithmetic and adds bounds
checking for JIT_CODE_DEBUG_INFO records during byte-swapping to prevent
out-of-bounds reads.

> diff --git a/tools/perf/util/jitdump.c b/tools/perf/util/jitdump.c
> index f79e9420c6bd7..7efbaa07f1ba7 100644
> --- a/tools/perf/util/jitdump.c
> +++ b/tools/perf/util/jitdump.c
> @@ -320,14 +320,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;

[Severity: Critical]
This is a pre-existing issue, but by placing the bounds checking and
nr_entry clamping exclusively inside the if (jd->needs_bswap) block,
native-endian files bypass this validation completely.

If a maliciously crafted JIT dump file with a native endianness matching the
host provides an arbitrarily large untrusted nr_entry count for a
JIT_CODE_DEBUG_INFO record, this path is skipped.

Later in the processing, this unvalidated value is assigned directly:

tools/perf/util/jitdump.c:jit_repipe_debug_info()
    jd->nr_debug_entries = jr->info.nr_entry;

And then blindly iterated over on the heap:

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);
    }

Could we move the bounds validation outside of the byte-swapping block so all
inputs are checked?

> +				/* 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);

[Severity: Medium]
Does this unpadded sequential pointer arithmetic lead to unaligned memory
accesses?

struct debug_entry contains 64-bit and 32-bit fields, requiring 8-byte
alignment, but it is not __packed. The debug_entry_next() helper calculates
the next entry's address by adding the length of the string:

tools/perf/util/jitdump.h:debug_entry_next()
    void *a = ent + 1;
    size_t l = strlen(ent->name) + 1;
    return a + l;

This leaves the pointer unaligned. When the byte-swapping loop uses
ent = debug_entry_next(ent) and accesses ent->addr = bswap_64(ent->addr),
the compiler generates aligned load/store instructions for an unaligned
address, leading to SIGBUS crashes on strict-alignment architectures.

>  			}
> +			/* clamp so downstream consumers don't overrun */
> +			jr->info.nr_entry = n;
>  		}
>  		break;
>  	case JIT_CODE_UNWINDING_INFO:
>  		if (jd->needs_bswap) {
>  			jr->unwinding.unwinding_size = bswap_64(jr->unwinding.unwinding_size);

[Severity: High]
This is a pre-existing issue, but is the unwinding_size validated against the
actual payload size?

In jit_repipe_unwinding_info(), the unwinding size is saved directly from the
payload without validation against the allocated heap buffer size:

tools/perf/util/jitdump.c:jit_repipe_unwinding_info()
    jd->unwinding_size = jr->unwinding.unwinding_size;

This unvalidated size is later passed to libelf:

tools/perf/util/genelf.c:jit_add_eh_frame_info()
    d->d_size = unwinding_table_size;

A crafted record with a small total_size but a huge unwinding_size could
cause libelf to read out-of-bounds heap memory and embed it into the output
ELF, leading to an information leak.

>  			jr->unwinding.eh_frame_hdr_size = bswap_64(jr->unwinding.eh_frame_hdr_size);
>  			jr->unwinding.mapped_size = bswap_64(jr->unwinding.mapped_size);
>  		}
>  		break;

While reviewing this patch, I also noticed a couple of pre-existing issues
in the handling of JIT_CODE_MOVE records:

tools/perf/util/jitdump.c:jit_repipe_code_move()
[ ... ]
> 	nspid = jr->load.pid;
> 	pid   = jr_entry_pid(jd, jr);

[Severity: Medium]
This isn't a bug introduced by this patch, but does jit_repipe_code_move()
validate that the record's total_size is large enough to contain the
jr_code_move payload?

If a malformed record is provided with a truncated total_size, fread() will
read only a partial record into jd->buf. The remaining fields will be
uninitialized or contain leftover data, which are then used to construct
the perf event.

[ ... ]
> 	event = calloc(1, sizeof(*event) + 16);
> 	if (!event)
> 		return -1;
[ ... ]
> 	ret = perf_event__process_mmap2(tool, event, &sample, jd->machine);
> 	if (ret)
> 		goto out;
> 
> 	ret = jit_inject_event(jd, event);
> 	if (!ret)
> 		build_id__mark_dso_hit(tool, event, &sample, jd->machine);
> out:
> 	perf_sample__exit(&sample);
> 	return ret;

[Severity: Medium]
This is a pre-existing issue, but does this leak the event buffer?

The memory for event is allocated via calloc() and the function returns at
the out: label without calling free(event). Neither of the processing
functions take ownership of the memory, so the buffer appears to be leaked on
every JIT_CODE_MOVE record.

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