Re: [PATCH 03/12] perf jitdump: Validate code_size against total_size in code load

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

Pre-existing issues:
- [Critical] Missing total_size validation leads to integer underflow and massive memory corruption or crashes in debug/unwinding info parsing.
- [Critical] `jit_get_next_entry` fails to bound `nr_entry` and improperly uses array indexing on a variable-length struct, causing heap buffer overflows.
- [Critical] Unvalidated `nr_entry` in debug info leads to out-of-bounds heap accesses during ELF generation.
- [Critical] Truncation in `snprintf` without clamping causes integer underflow when calculating event size in `jit_repipe_code_move`, leading to out-of-bounds heap writes.
- [High] Missing `free(event)` causes a memory leak in `jit_repipe_code_move`.
- [High] Heap information leak into generated ELF file via unvalidated `unwinding_size`.
--

--- Patch [3]: [PATCH 03/12] perf jitdump: Validate code_size against total_size in code load ---
commit 1f1a1ff66272152cc1c578445365c0049a6fa38d
Author: Arnaldo Carvalho de Melo <[email protected]>

perf jitdump: Validate code_size against total_size in code load

This commit adds bounds checking for code_size in jit_repipe_code_load()
to prevent out-of-bounds heap reads during pointer arithmetic. It ensures
that code_size fits within the jitdump record's total_size and prevents
integer wrapping when cast to a signed int.

> diff --git a/tools/perf/util/jitdump.c b/tools/perf/util/jitdump.c
> index dcb26d9c6c8fc..14bd23c8d1963 100644
> --- a/tools/perf/util/jitdump.c
> +++ b/tools/perf/util/jitdump.c
[ ... ]
> @@ -452,6 +453,16 @@ static int jit_repipe_code_load(struct jit_buf_desc *jd, union jr_entry *jr)
>  	csize = jr->load.code_size;
>  	usize = jd->unwinding_mapped_size;
>  	addr  = jr->load.code_addr;
> +
> +	/* code blob lives at the end of the record, validate it fits */
> +	if (jr->load.p.total_size < sizeof(jr->load) ||
> +	    jr->load.code_size > jr->load.p.total_size - sizeof(jr->load) ||
> +	    jr->load.code_size > INT_MAX) {
> +		pr_warning("jitdump: invalid code_size %" PRIu64 " (total_size=%u) in code_load record\n",
> +			   (uint64_t)jr->load.code_size, jr->load.p.total_size);
> +		return -1;
> +	}
> +
>  	sym   = (void *)((unsigned long)jr + sizeof(jr->load));
>  	code  = (unsigned long)jr + jr->load.p.total_size - csize;
>  	count = jr->load.code_index;

[Severity: High]
This is a pre-existing issue, but I noticed a memory leak in
jit_repipe_code_move() when handling an untrusted file:

tools/perf/util/jitdump.c:jit_repipe_code_move() {
    ...
	out:
		perf_sample__exit(&sample);
		return ret;
}

Does this code leak the dynamically allocated event buffer on this return
path?

[Severity: Critical]
This is a pre-existing issue, but there appears to be an integer underflow
risk when calculating allocation sizes in jit_repipe_unwinding_info() and
jit_repipe_debug_info():

tools/perf/util/jitdump.c:jit_repipe_unwinding_info() {
    ...
	unwinding_data_size  = jr->prefix.total_size - sizeof(jr->unwinding);
	unwinding_data = malloc(unwinding_data_size);
    ...
}

And similarly:

tools/perf/util/jitdump.c:jit_repipe_debug_info() {
    ...
	sz  = jr->prefix.total_size - sizeof(jr->info);
	data = malloc(sz);
    ...
}

If a maliciously crafted record has a total_size smaller than the fixed header
size, could this subtraction underflow and cause a massive allocation and
subsequent out-of-bounds copy?

[Severity: Critical]
This is a pre-existing issue, but there might be a heap buffer overflow in
jit_get_next_entry() when byte-swapping entries:

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

Since nr_entry is not bounds-checked against total_size, and struct debug_entry
contains a flexible array member, does standard array indexing here compute
misaligned pointers and write out-of-bounds of the allocated buffer?

[Severity: Critical]
This is a pre-existing issue, but the unvalidated nr_entry from debug info
seems to cause out-of-bounds access during ELF generation:

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 jd->nr_debug_entries is set from an attacker-controlled nr_entry without
verification, could this loop read out-of-bounds via strlen() in
debug_entry_next() and write out-of-bounds via ent->addr?

[Severity: Critical]
This is a pre-existing issue, but I see a risk of underflow in event size
calculation in jit_repipe_code_move() due to un-clamped snprintf length:

tools/perf/util/jitdump.c:jit_repipe_code_move() {
    ...
	event->mmap2.header.size = (sizeof(event->mmap2) -
			(sizeof(event->mmap2.filename) - size) + idr_size);
    ...
	id = (void *)((unsigned long)event + event->mmap.header.size - idr_size);
	if (jd->sample_type & PERF_SAMPLE_TID) {
		id->pid  = pid;
		id->tid  = tid;
    ...
}

Since snprintf returns the length that would have been written, if size exceeds
the filename buffer size, might the subtraction underflow and corrupt the event
header size? 

Would this result in out-of-bounds pointer arithmetic for id and lead to
arbitrary heap writes?

[Severity: High]
This is a pre-existing issue, but there appears to be a heap information leak
into generated ELF files due to an unvalidated unwinding_size:

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

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

If jd->unwinding_size is populated directly from an untrusted payload without
verification against the actual available size, can libelf read out-of-bounds
heap memory and leak it into the generated file?

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