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

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

Pre-existing issues:
- [High] `jit_repipe_code_move()` contains a memory leak and a heap buffer overflow due to `snprintf` truncation.
- [Medium] Missing cleanup of `jd->debug_data` and `jd->unwinding_data` causes memory leaks.
- [Medium] Missing bounds check in `jit_repipe_code_move()` allows reading uninitialized heap memory and injecting it into the output `perf.data`.
- [Medium] Integer underflow when parsing `JIT_CODE_UNWINDING_INFO` size causes a massive out-of-bounds heap read and SIGSEGV.
- [Medium] Callback errors are silently swallowed in the `jit_process_dump()` loop, masking data validation failures.
--

--- Patch [2]: [PATCH 02/12] perf jitdump: Validate code_size against total_size in code load ---
commit 386282de6b8b07ac74f2456d302053e3738822ce
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 the jitdump record to
prevent out-of-bounds heap reads when processing code load entries.

> --- tools/perf/util/jitdump.c
> +++ tools/perf/util/jitdump.c
[ ... ]
> @@ -450,6 +451,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;
> +	}
> +

[Severity: Medium]
This isn't a bug introduced by this patch, but does this early return skip
the cleanup for jd->debug_data and jd->unwinding_data?

When parsing consecutive debug info records, the previous state allocations
are unconditionally overwritten without being freed. If the bounds check
fails here or the stream ends, the buffers appear to leak since neither
the processing loop nor jit_close() cleans them up.

[Severity: Medium]
This is a pre-existing issue, but will the caller actually abort when we
return -1 here?

Looking at jit_process_dump(), the loop assigns the return value but doesn't
seem to check it before continuing:

jit_process_dump() {
    ...
    while ((jr = jit_get_next_entry(jd))) {
        switch(jr->prefix.id) {
        case JIT_CODE_LOAD:
            ret = jit_repipe_code_load(jd, jr);
            break;
    ...
}

Does this mean errors are silently swallowed and parsing proceeds on
corrupted state, masking the data validation failures?

>  	sym   = (void *)((unsigned long)jr + sizeof(jr->load));

[Severity: Medium]
This is a pre-existing issue, but does jit_repipe_code_move() lack a
similar bounds check?

The generic parser only guarantees a minimum size of 16 bytes for a record,
but the move function accesses fields unconditionally:

jit_repipe_code_move() {
    ...
    event->mmap2.start = jr->move.new_code_addr;
    event->mmap2.len   = usize ? ALIGN_8(jr->move.code_size) + usize
                               : jr->move.code_size;
    ...
}

Could this allow reading uninitialized heap memory and injecting it into
the output perf.data if the record is truncated?

[Severity: High]
This is a pre-existing issue, but is there a memory leak and potential
heap buffer overflow in jit_repipe_code_move()?

Unlike jit_repipe_code_load(), the move function fails to clamp the snprintf
return value for the size:

jit_repipe_code_move() {
    ...
    size = snprintf(filename, PATH_MAX, "%s/jitted-%d-%" PRIu64 ".so",
                    jd->dir,
                    nspid,
                    jr->move.code_index);
    size++; /* for \0 */
    ...
}

If the parsed path exceeds PATH_MAX, snprintf returns the would-be length,
which inflates event->mmap2.header.size. Does this corrupt the subsequent
id pointer offset and lead to an out-of-bounds write?

Also, the dynamically allocated event buffer appears to be leaked entirely
on both success and error paths before the function returns:

jit_repipe_code_move() {
    ...
out:
    perf_sample__exit(&sample);
    return ret;
}

[Severity: Medium]
This is a pre-existing issue, but could jit_repipe_unwinding_info()
suffer from integer underflow?

If a malformed file supplies a total_size less than sizeof(jr->unwinding),
the unwinding_data_size calculation underflows into a very large positive
integer:

jit_repipe_unwinding_info() {
    ...
    unwinding_data_size  = jr->prefix.total_size - sizeof(jr->unwinding);
    unwinding_data = malloc(unwinding_data_size);
    ...
    memcpy(unwinding_data, &jr->unwinding.unwinding_data,
           unwinding_data_size);
    ...
}

Could this lead to an immediate out-of-bounds read crash if the subsequent
malloc succeeds overcommitting?

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