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

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

Pre-existing issues:
- [High] Cross-endian byte-swapping in `jit_get_next_entry()` lacks bounds checks for specific record types and incorrectly indexes variable-sized structures, causing out-of-bounds memory corruption.
- [High] Missing size validation for `JIT_CODE_MOVE` records allows uninitialized heap memory to be read and injected into synthesized perf events.
- [High] Missing validation of attacker-controlled sizes in `jit_repipe_unwinding_info` allows a massive out-of-bounds heap read when generating ELF files.
- [High] Missing NUL-termination check for the 'sym' string in `jit_repipe_code_load` allows an out-of-bounds read and heap info leak.
- [Medium] `jit_process_dump()` silently ignores parser errors and continues processing, which can hide corruption and cause downstream semantic bugs.
- [Medium] Buffers `jd->debug_data` and `jd->unwinding_data` are leaked during `jit_process` teardown and when record metadata contains zero-counts.
- [Medium] Calling `funlockfile()` on `jd->in` in the `jit_open()` error path before successfully locking it invokes undefined behavior.
--

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

perf jitdump: Prevent integer underflow in debug info size calculation

Prevent integer underflow by checking if total_size covers the fixed header
size in jit_repipe_debug_info and jit_repipe_unwinding_info.

> 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
> @@ -669,6 +669,10 @@ static int jit_repipe_debug_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->info))
> +		return -1;
> +
>  	sz  = jr->prefix.total_size - sizeof(jr->info);
>  	data = malloc(sz);
>  	if (!data)
> @@ -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 could we also validate the unwinding sizes
extracted here?

In jit_repipe_unwinding_info():

    jd->eh_frame_hdr_size = jr->unwinding.eh_frame_hdr_size;
    jd->unwinding_size = jr->unwinding.unwinding_size;

These sizes are extracted without validation against the allocated payload size.

Later, in jit_add_eh_frame_info() in tools/perf/util/genelf.c:

    uint64_t unwinding_table_size = unwinding_size - unwinding_header_size;
    ...
    d->d_size = unwinding_table_size;

Could this unvalidated unwinding_size allow libelf to read out-of-bounds heap
memory and embed it in the generated ELF file?


[Severity: High]
This is a pre-existing issue, but does cross-endian byte-swapping in
jit_get_next_entry() lack bounds checks for specific record types?

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

Because struct debug_entry contains a flexible array member (const char name[]),
doesn't standard array[n] indexing calculate the wrong offsets (skipping exactly
16 bytes and ignoring the string payload)?

Could this cause bswap_64 and bswap_32 to read from and write to unintended
out-of-bounds memory locations, especially since nr_entry itself is never
validated against the payload size?


[Severity: High]
This isn't a bug introduced by this patch, but does jit_repipe_code_move()
miss size validation for JIT_CODE_MOVE records?

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;
    event->mmap2.pid   = pid;
    ...
}

If a truncated JIT_CODE_MOVE record is processed where total_size is smaller
than sizeof(struct jr_code_move), jit_get_next_entry() only reads total_size
bytes.

Does this unconditionally access uninitialized heap memory and inject it into
synthesized perf events?


[Severity: Medium]
This is a pre-existing issue, but does jit_process_dump() silently ignore
parser errors?

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

If a handler like jit_repipe_debug_info returns -1, the return value is
assigned to ret, but the while loop does not break.

Could this continue processing subsequent records, overwriting ret, and
silently hide the corruption?


[Severity: Medium]
This isn't a bug introduced by this patch, but does jit_process() leak
jd->debug_data and jd->unwinding_data during teardown?

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

These buffers are dynamically allocated during parsing. If the JIT dump trace
ends immediately after a debug or unwinding record, or if parsing aborts, the
stack-allocated jd context is destroyed without freeing these buffers.

Additionally, in jit_repipe_code_load():

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

Is it possible to skip zfree if jd->nr_debug_entries == 0, leaking the active
allocation?


[Severity: Medium]
This is a pre-existing issue, but does jit_open() invoke undefined behavior on
the error path?

tools/perf/util/jitdump.c:jit_open() {
    ...
error:
    free(buf);
    funlockfile(jd->in);
    fclose(jd->in);
}

If the initial metadata buffer allocation fails, the code jumps to the error
label and calls funlockfile(jd->in).

Since the lock was never acquired with flockfile() prior to this point, doesn't
unlocking a file descriptor that is not locked by the thread invoke undefined
behavior?


[Severity: High]
This isn't a bug introduced by this patch, but is there a missing
NUL-termination check for the sym string in jit_repipe_code_load()?

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

A JIT_CODE_LOAD record sets code_size == total_size - sizeof(jr->load). The
sym string pointer is calculated to point to the end of the struct, which
directly overlaps with the raw code blob.

Later, in jit_write_elf() in tools/perf/util/genelf.c:

    symlen = 2 + strlen(sym);
    strsym = calloc(1, symlen);
    ...
    strcpy(strsym + 1, sym);

If the string is not validated to be NUL-terminated within bounds, could
strlen(sym) read past the jd->buf allocation and copy out-of-bounds heap
memory into the ELF string table?

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