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

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

New issues:
- [Medium] jd->debug_data and jd->unwinding_data are leaked on the new validation failure path.

Pre-existing issues:
- [High] Integer underflow when calculating unwinding_data_size in jit_repipe_unwinding_info leads to a massive heap allocation and out-of-bounds read.
- [High] Missing snprintf truncation clamping in jit_repipe_code_move and jit_repipe_code_load leads to an undersized event allocation calculation and out-of-bounds heap write.
- [High] jit_get_next_entry iterates over an unvalidated nr_entry array using fixed-size indexing for a variable-length structure, leading to out-of-bounds byte-swapping.
- [High] The parser lacks total_size boundary checks for JIT_CODE_MOVE, leading to uninitialized heap reads.
- [High] Lack of validation between buffer size and nr_entry leads to heap OOB read and write in debug info parsing.
- [Medium] The perf_event allocated via calloc is unconditionally leaked in jit_repipe_code_move.
- [Medium] Multiple memory leaks of dynamically allocated buffers in the jitdump parser, including missing cleanup in jit_close and overwrites in debug info.
- [Medium] The ALIGN_8 macro evaluates using signed arithmetic, causing integer wrap and massive length corruption for mmap2 events.
- [Medium] Truncation of 64-bit unwinding_mapped_size to 32-bit int can corrupt mmap2 event length.
--

--- Patch [2]: [PATCH 02/12] perf jitdump: Validate code_size against total_size in code load ---
commit 97b091976e049b90397f99d555b22a4934abb49d
Author: Arnaldo Carvalho de Melo <[email protected]>

perf jitdump: Validate code_size against total_size in code load

This commit introduces validation checks to ensure that the code_size
does not exceed the available size in the JIT load record. It adds bounds
checking to prevent out-of-bounds heap reads when calculating the code
blob pointer.

> diff --git a/tools/perf/util/jitdump.c b/tools/perf/util/jitdump.c
> index 4b7c7ba7cd95d..3195f94187164 100644
> --- a/tools/perf/util/jitdump.c
> +++ b/tools/perf/util/jitdump.c
> @@ -9,6 +9,7 @@
>  #include <fcntl.h>
>  #include <unistd.h>
>  #include <inttypes.h>
> +#include <limits.h>
>  #include <byteswap.h>
>  #include <sys/stat.h>
>  #include <sys/mman.h>
> @@ -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]
Does this early return skip freeing the buffered debug and unwinding data?

Later in the function, these dynamically allocated buffers are freed:

    if (jd->debug_data && jd->nr_debug_entries) {
            zfree(&jd->debug_data);
            jd->nr_debug_entries = 0;
    }

    if (jd->unwinding_data && jd->eh_frame_hdr_size) {
            zfree(&jd->unwinding_data);
            ...

If the code returns -1 here, do jd->debug_data and jd->unwinding_data remain
allocated, permanently leaking since jit_close() does not appear to clean them
up?

> +	}
> +
>  	sym   = (void *)((unsigned long)jr + sizeof(jr->load));
>  	code  = (unsigned long)jr + jr->load.p.total_size - csize;
>  	count = jr->load.code_index;

[ ... ]

Other findings in this file:

[Severity: High]
This is a pre-existing issue, but does integer underflow occur when calculating
unwinding_data_size in jit_repipe_unwinding_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);
    memcpy(unwinding_data, &jr->unwinding.unwinding_data, unwinding_data_size);
    ...
}

If total_size is smaller than sizeof(jr->unwinding), does the unsigned 32-bit
subtraction underflow to a massive value, causing malloc to allocate too much
memory and memcpy to perform an out-of-bounds read?


[Severity: Medium]
This is a pre-existing issue, but is the perf_event leaked in
jit_repipe_code_move()?

tools/perf/util/jitdump.c:jit_repipe_code_move() {
    ...
    event = calloc(1, sizeof(*event) + 16);
    if (!event)
        return -1;
    ...
    return ret;
}

The event pointer is allocated via calloc but is never freed on the success
or error return paths. Does this gradually exhaust memory when parsing large
dumps?


[Severity: High]
This is a pre-existing issue, but does missing snprintf truncation clamping
lead to out-of-bounds heap writes?

tools/perf/util/jitdump.c:jit_repipe_code_move() {
    ...
    size = snprintf(filename, PATH_MAX, "%s/jitted-%d-%" PRIu64 ".so", ...);
    size++; /* for \0 */
    event->mmap2.header.size = (sizeof(event->mmap2) - (sizeof(event->mmap2.filename) - size) + idr_size);
    ...
}

When generating the filename, snprintf returns the length that would have
been written. If this exceeds PATH_MAX, does the returned size wrap in the
unsigned arithmetic for header.size, underflowing it and pushing the trailing
id pointer out of the allocated event buffer bounds?


[Severity: Medium]
This is a pre-existing issue, but are dynamic buffers repeatedly leaked in
jit_repipe_debug_info() and jit_close()?

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

Repeated JIT_CODE_DEBUG_INFO records will malloc a new buffer and overwrite
jd->debug_data without freeing the old one. Additionally, jit_close() frees
the jd struct resources but does not seem to clean up internal heap buffers
like debug_data and unwinding_data. Does this leave them un-freed
indefinitely?


[Severity: High]
This is a pre-existing issue, but does standard array indexing on a variable
length struct cause memory corruption during bswap?

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

In cross-endian mode, this iterates over an unvalidated nr_entry array using
fixed-size indexing. Since debug_entry includes a variable-length name[]
string inline, doesn't fixed-size array indexing read from and byte-swap
completely incorrect offsets?


[Severity: High]
This is a pre-existing issue, but does jit_repipe_code_move() lack boundary
checks for the JIT_CODE_MOVE total_size?

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

Unlike the new validation added to JIT_CODE_LOAD in this patch,
jit_repipe_code_move() lacks a check confirming that total_size is greater
than or equal to sizeof(jr->move). Can malformed records cause the parser
to read uninitialized heap or stale data from jd->buf into perf.data?


[Severity: Medium]
This is a pre-existing issue, but does the ALIGN_8 macro silently overflow
into a negative value?

tools/perf/util/jitdump.c:jit_repipe_code_load() {
    ...
    int ret, csize, usize;
    /* ... */
    event->mmap2.len   = usize ? ALIGN_8(csize) + usize : csize;
    ...
}

Since csize is typed as a signed int, does calculating ALIGN_8(csize) with a
value near INT_MAX trigger signed integer overflow to a negative value, which
is then sign-extended into a corrupted uint64_t length?


[Severity: Medium]
This is a pre-existing issue, but can the truncation of unwinding_mapped_size
corrupt the mmap2 event length?

tools/perf/util/jitdump.c:jit_repipe_code_move() {
    ...
    int usize;
    /* ... */
    usize = jd->unwinding_mapped_size;
    /* ... */
    event->mmap2.len   = usize ? ALIGN_8(jr->move.code_size) + usize : jr->move.code_size;
    ...
}

The 64-bit jd->unwinding_mapped_size is truncated to a 32-bit signed integer
(usize). Could a maliciously crafted mapped_size result in a negative or
truncated value, corrupting the final event->mmap2.len?


[Severity: High]
This is a pre-existing issue, but does the lack of validation between buffer
size and nr_entry lead to heap out-of-bounds reads and writes?

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

The parser sets jd->nr_debug_entries from the unvalidated nr_entry file field
without confirming the allocated sz bytes can accommodate them. When
jit_process_debug_info() loops nr_entry times calling strlen on heap data,
does this result in out-of-bounds heap writes?

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