Re: [PATCH 03/12] perf jitdump: Prevent integer underflow in debug info size calculation

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

Pre-existing issues:
- [High] Missing validation of unwinding_size and eh_frame_hdr_size allows arbitrary heap memory extraction via ELF generation.
- [High] Broken byte-swapping logic for JIT_CODE_DEBUG_INFO records corrupts data and writes out of bounds, bypassing length validation.
- [Low] Lingering memory leak of `jd->debug_data` and `jd->unwinding_data` upon processing completion.
- [Low] Conditional cleanup in `jit_repipe_code_load()` leaves buffers un-freed on edge cases.
--

--- Patch [3]: [PATCH 03/12] perf jitdump: Prevent integer underflow in debug info size calculation ---
commit 4aaa391def5b9dab180e25ac7cc8112352a1ca2a
Author: Arnaldo Carvalho de Melo <[email protected]>

    perf jitdump: Prevent integer underflow in debug info size calculation
    
    jit_repipe_debug_info() and jit_repipe_unwinding_info() compute payload
    sizes by subtracting the fixed header size from total_size:
    
      sz = jr->prefix.total_size - sizeof(jr->info);
    
    When total_size is smaller than the header struct (from a truncated or
    corrupted jitdump record), the subtraction underflows to a massive
    value, causing an oversized allocation followed by an OOB memcpy.
    
    Validate that total_size covers at least the fixed header before the
    subtraction in both functions.

> diff --git a/tools/perf/util/jitdump.c b/tools/perf/util/jitdump.c
> index 3195f94187164..787f8a03dae87 100644
> --- a/tools/perf/util/jitdump.c
> +++ b/tools/perf/util/jitdump.c
> @@ -696,6 +700,10 @@ jit_repipe_unwinding_info(struct jit_buf_desc *jd, union jr_entry *jr)
>  	if (!(jd && jr))
>  		return -1;
>  
> +	/* total_size must cover at least the fixed header */
> +	if (jr->prefix.total_size < sizeof(jr->unwinding))
> +		return -1;
> +
>  	unwinding_data_size  = jr->prefix.total_size - sizeof(jr->unwinding);
>  	unwinding_data = malloc(unwinding_data_size);
>  	if (!unwinding_data)

[Severity: High]
This is a pre-existing issue, but is it possible for a maliciously crafted
JIT dump file to provide massive unwinding_size and eh_frame_hdr_size values?

In jit_repipe_unwinding_info(), untrusted sizes are directly assigned to the
descriptor without validating that they fit within the actual allocated
payload:

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

Later, in jit_add_eh_frame_info(), these unvalidated values could cause an
underflow or massive size calculation:

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

Could this force libelf to read massive amounts of out-of-bounds heap memory
and embed it into the output ELF file?

[Severity: High]
This isn't a bug introduced by this patch, but does the byte-swapping logic
for JIT_CODE_DEBUG_INFO records risk an out-of-bounds access before length
validation occurs?

In jit_get_next_entry(), a cross-endian file with a maliciously large nr_entry
could cause issues:

tools/perf/util/jitdump.c:jit_get_next_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);
			}
    ...
}

Since struct debug_entry contains a flexible array member (const char name[]),
the entries are variable-length.

Does using fixed-size array indexing (jr->info.entries[n]) read and byte-swap
memory beyond the allocated jd->buf boundaries?

[Severity: Low]
This is a pre-existing issue, but is there a conditional cleanup leak in
jit_repipe_code_load()?

tools/perf/util/jitdump.c:jit_repipe_code_load() {
    ...
	if (jd->debug_data && jd->nr_debug_entries) {
		zfree(&jd->debug_data);
		jd->nr_debug_entries = 0;
	}
    ...
}

If a JIT_CODE_DEBUG_INFO record allocates memory because total_size >
sizeof(header), but has 0 entries (jd->nr_debug_entries == 0), does
jd->debug_data remain allocated but fail the secondary check, causing a leak?

[Severity: Low]
This is a pre-existing issue, but is there a memory leak of jd->debug_data
and jd->unwinding_data upon processing completion?

In jit_process(), when the function tears down, it frees jd.buf but does not
appear to clean up jd->debug_data and jd->unwinding_data:

tools/perf/util/jitdump.c:jit_process() {
    ...
	nsinfo__put(jd.nsi);
	free(jd.buf);

	return ret;
}

Additionally, in jit_repipe_debug_info(), does the assignment to
jd->debug_data overwrite the previous pointer without properly freeing
it first?

tools/perf/util/jitdump.c:jit_repipe_debug_info() {
    ...
	jd->debug_data       = data;
    ...
}

Could these issues lead to lingering memory leaks on truncated or
malformed JIT dump files?

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