Re: [PATCH 05/12] perf jitdump: Check snprintf return before computing header size

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

Pre-existing issues:
- [High] In `jit_repipe_code_move`, an out-of-bounds heap read occurs when the required event size exceeds the allocated buffer, leaking heap memory into the `perf.data` file.
- [High] The dynamically allocated `event` buffer in `jit_repipe_code_move` is never freed, resulting in a memory leak.
- [Medium] The JIT event injection logic violates the perf ABI's dynamic packing rules for the `sample_id_all` suffix, leading to event data corruption.
- [Medium] The `uint64_t` variable `jd->unwinding_mapped_size` is silently truncated to a 32-bit signed `int`, potentially corrupting the event length.
--

--- Patch [5]: [PATCH 05/12] perf jitdump: Check snprintf return before computing header size ---
commit ec3758fe10c5b2ae1b52a849ea060531bfeec315
Author: Arnaldo Carvalho de Melo <[email protected]>

    perf jitdump: Check snprintf return before computing header size

This commit fixes a heap corruption in the jitdump mmap injection code.
It clamps the return value of snprintf to the filename buffer size in both
jit_repipe_code_load() and jit_repipe_code_move() to prevent oversized
header computations when the jitted filename exceeds PATH_MAX.

> diff --git a/tools/perf/util/jitdump.c b/tools/perf/util/jitdump.c
> index 078d3304d2b7e..d7e3dcfc63b78 100644
> --- a/tools/perf/util/jitdump.c
> +++ b/tools/perf/util/jitdump.c
> @@ -493,6 +493,9 @@ static int jit_repipe_code_load(struct jit_buf_desc *jd, union jr_entry *jr)
>  			jd->dir,
>  			nspid,
>  			count);
> +	/* snprintf returns would-be length on truncation, clamp to buffer */
> +	if (size >= sizeof(event->mmap2.filename))
> +		size = sizeof(event->mmap2.filename) - 1;
>  
>  	size++; /* for \0 */
>  

[Severity: Medium]
This is a pre-existing issue, but does the sample ID injection in
jit_repipe_code_load() violate the dynamic packing rules for the sample_id_all
suffix?

The code casts the suffix payload to a static struct and unconditionally writes
the timestamp at offset 8:

	struct {
		u32 pid, tid;
		u64 time;
	} *id;
...
	if (jd->sample_type & PERF_SAMPLE_TIME)
		id->time = convert_timestamp(jd, jr->load.p.timestamp);

If PERF_SAMPLE_TID is disabled but PERF_SAMPLE_TIME is present, shouldn't the
timestamp be placed at offset 0?

Leaving offset 0 uninitialized and forcing the timestamp into offset 8 could
corrupt the parsed timestamp and subsequent fields.

[Severity: Medium]
This is also a pre-existing issue, but does jit_repipe_code_load() silently
truncate the 64-bit unwinding mapped size?

The function assigns a uint64_t from the untrusted jitdump file to a signed
32-bit int:

	int ret, csize, usize;
...
	usize = jd->unwinding_mapped_size;

If the mapped_size is larger than INT_MAX, wouldn't this truncate into a
negative value?

This could then be added to the code size to compute event->mmap2.len, causing
unintended subtraction or massive wraparound.

> @@ -623,6 +626,9 @@ static int jit_repipe_code_move(struct jit_buf_desc *jd, union jr_entry *jr)
>  	         jd->dir,
>  		 nspid,
>  		 jr->move.code_index);
> +	/* snprintf returns would-be length on truncation, clamp to buffer */
> +	if (size >= sizeof(event->mmap2.filename))
> +		size = sizeof(event->mmap2.filename) - 1;
>  
>  	size++; /* for \0 */
>  

[Severity: High]
This isn't a bug introduced by this patch, but does jit_repipe_code_move()
have an out-of-bounds heap read when calculating the event size?

The event buffer is allocated with a hardcoded size:

	event = calloc(1, sizeof(*event) + 16);

But the header.size is dynamically computed using idr_size:

	event->mmap2.header.size = (sizeof(event->mmap2) - (sizeof(event->mmap2.filename) - size) + idr_size);

Since idr_size can be up to 64 bytes depending on the active sample_type, the
calculated header.size can exceed the allocated buffer by up to 48 bytes.

When perf_data__write() is later called in jit_inject_event(), couldn't this
read past the end of the allocation and leak heap memory into the perf.data
file?

[Severity: Medium]
This is a pre-existing issue, but does jit_repipe_code_move() also suffer from
the same dynamic packing violation as jit_repipe_code_load()?

The same static struct layout is used for the sample ID fields:

	struct {
		u32 pid, tid;
		u64 time;
	} *id;
...
	if (jd->sample_type & PERF_SAMPLE_TIME)
		id->time = convert_timestamp(jd, jr->load.p.timestamp);

This appears to corrupt the event data if PERF_SAMPLE_TID is disabled.

[Severity: High]
This is a pre-existing issue, but does jit_repipe_code_move() leak the event
buffer?

The buffer is dynamically allocated, but the function appears to return without
freeing it:

	event = calloc(1, sizeof(*event) + 16);
	if (!event)
		return -1;
...
out:
	perf_sample__exit(&sample);
	return ret;

Since a jitdump file can contain thousands of code move events, wouldn't this
consume significant memory and potentially lead to out-of-memory errors during
processing?

[Severity: Medium]
This is a pre-existing issue, but does jit_repipe_code_move() also truncate the
unwinding mapped size into a 32-bit integer?

Just like in jit_repipe_code_load(), the size is assigned to an int:

	int usize;
...
	usize = jd->unwinding_mapped_size;

Could this cause the same event length corruption if the mapped_size exceeds
INT_MAX?

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