Re: [PATCH 04/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] Unclamped `snprintf` return values when formatting filenames cause integer underflow, inflating event size and leading to out-of-bounds heap writes.
- [High] Cross-endian byte-swapping of debug info entries uses incorrect fixed-size array indexing on a variable-length structure, causing severe heap corruption.
- [High] Unvalidated structural sizes for unwinding info lead to massive out-of-bounds heap reads and information disclosure during ELF generation.
- [High] Missing validation of `nr_entry` against the payload size in `jit_repipe_debug_info()` leads to heap out-of-bounds accesses during debug info processing.
--

--- Patch [4]: [PATCH 04/12] perf jitdump: Prevent integer underflow in debug info size calculation ---
commit 279da6780ef70ba142d7c4f7db07000972f0e035
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. This prevents
an underflow when total_size is smaller than the header struct.

> diff --git a/tools/perf/util/jitdump.c b/tools/perf/util/jitdump.c
> index 14bd23c8d1963..f79e9420c6bd7 100644
> --- a/tools/perf/util/jitdump.c
> +++ b/tools/perf/util/jitdump.c
> @@ -671,6 +671,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)

[Severity: High]
This is a pre-existing issue, but does this function also need to validate
jr->info.nr_entry against the payload size?

It appears the unvalidated count is saved to jd->nr_debug_entries. Later,
in jit_process_debug_info(), a loop iterates nr_debug_entries times:

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

Since debug_entry_next() computes pointer offsets by reading variable-length
strings, could an artificially inflated nr_entry force the loop to iterate
past the allocated debug_data buffer and cause an out-of-bounds heap write?

[ ... ]

> @@ -699,6 +703,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 isn't a bug introduced by this patch, but could the structural sizes
extracted from the untrusted record cause out-of-bounds reads later?

In jit_repipe_unwinding_info(), these sizes are blindly extracted:

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;
[ ... ]
}

Then they are used to configure libelf data sections:

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

If jr->unwinding.unwinding_size is inflated beyond the actual payload size,
could libelf read past the end of the unwinding_data allocation and leak
arbitrary heap memory to the generated ELF file?


[Severity: High]
This is a pre-existing issue, but I also noticed a potential integer underflow
when formatting filenames in jit_repipe_code_move() and
jit_repipe_code_load().

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

If an untrusted jitdump file causes snprintf to truncate the filename (for
example by providing a long nspid or code_index), size will exceed PATH_MAX.
Would the subsequent calculation for event->mmap2.header.size undergo a double
integer underflow since both operands are unsigned size_t?

This mathematically reduces to sizeof(event->mmap2) - PATH_MAX + size + idr_size.
Because size > PATH_MAX, header.size strictly exceeds the size of the event
structure.

The id pointer is then calculated as event + header.size - idr_size, placing
it past the end of the dynamically allocated event buffer, which could lead to
an out-of-bounds heap write on id->pid and id->tid.


[Severity: High]
This isn't a bug introduced by this patch, but does cross-endian byte-swapping
of debug info entries incorrectly use fixed-size array indexing?

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 for the name, each
entry is variable in length. Will using standard array indexing ignore the
interleaved variable-length strings and cause the byte-swap operations to
stride incorrectly across the buffer, potentially writing out-of-bounds on
the heap?

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