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

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

Pre-existing issues:
- [High] Incomplete fix for out-of-bounds read/write in JIT_CODE_DEBUG_INFO parsing
- [High] Missing bounds validation for JIT_CODE_UNWINDING_INFO sizes leads to OOB heap read and information leak
- [High] Missing null-terminator validation for JIT_CODE_LOAD symbol string causes OOB read and information leak
--

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

This commit replaces array indexing with debug_entry_next() pointer arithmetic
for JIT_CODE_DEBUG_INFO records, and adds bounds-checking against the record's
total_size. This prevents out-of-bounds reads during byte-swapping of untrusted
jitdump input.

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

[Severity: High]
This is a pre-existing issue, but does this skip validation for native-endian
files?

The bounds checking and clamping are only performed when jd->needs_bswap is true.
If the file is native-endian, the untrusted nr_entry is passed directly to the
downstream loop without validation:

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 this lead to out-of-bounds heap reads and writes when processing a crafted
JIT_CODE_DEBUG_INFO record with native endianness?

[ ... ]

> +			/* clamp so downstream consumers don't overrun */
> +			jr->info.nr_entry = n;
>  		}
>  		break;
>  	case JIT_CODE_UNWINDING_INFO:

[Severity: High]
This is a pre-existing issue, but is the unwinding_size bounded by the actual
record size?

In jit_repipe_unwinding_info(), jd->unwinding_size is set directly from the
untrusted jr->unwinding.unwinding_size. Later, this unvalidated size is passed
to libelf:

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

Could this cause an out-of-bounds heap read and embed adjacent heap memory into
the generated ELF file?

>  		if (jd->needs_bswap) {
>  			jr->unwinding.unwinding_size = bswap_64(jr->unwinding.unwinding_size);
>  			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;
>  	case JIT_CODE_CLOSE:
>  		break;
>  	case JIT_CODE_LOAD:

[Severity: High]
This is a pre-existing issue, but is the symbol string in JIT_CODE_LOAD checked
for null-termination within the record bounds?

In jit_repipe_code_load(), the string pointer is derived from the record
without validation:

tools/perf/util/jitdump.c:jit_repipe_code_load() {
    ...
    sym = (void *)((unsigned long)jr + sizeof(jr->load));
    ...
}

This unvalidated string is later passed to strlen() and strcpy() in
jit_write_elf(), which could scan past the record into adjacent heap memory and
leak data into the generated ELF string table.

>  		if (jd->needs_bswap) {
>  			jr->load.pid       = bswap_32(jr->load.pid);
>  			jr->load.tid       = bswap_32(jr->load.tid);

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