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

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

Pre-existing issues:
- [Medium] Out-of-bounds heap read leaking information into `perf.data` via `jit_repipe_code_move()`.
--

--- Patch [6]: [PATCH 06/12] perf jitdump: Check snprintf return before computing header size ---
commit 8f494719852b9efdd2ae99317e82686b5aa7c596
Author: Arnaldo Carvalho de Melo <[email protected]>

perf jitdump: Check snprintf return before computing header size

This commit fixes a heap corruption bug caused by an underflow when
calculating the header size if a jitted filename exceeds PATH_MAX. It
clamps the length returned by snprintf() to fit within the bounds of
the event's filename buffer in both jit_repipe_code_load() and
jit_repipe_code_move().

> diff --git a/tools/perf/util/jitdump.c b/tools/perf/util/jitdump.c
> index 7efbaa07f1ba7..45f0e21b0e780 100644
> --- a/tools/perf/util/jitdump.c
> +++ b/tools/perf/util/jitdump.c
[ ... ]
> @@ -625,6 +628,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: Medium]
This is a pre-existing issue, but does this function allocate enough memory
to handle large dynamic idr_size values?

Earlier in jit_repipe_code_move(), the event is allocated with a hardcoded 
+ 16 byte extension:

tools/perf/util/jitdump.c:jit_repipe_code_move() {
    ...
	/*
	 * +16 to account for sample_id_all (hack)
	 */
	event = calloc(1, sizeof(*event) + 16);
    ...
}

However, the event size calculation uses the dynamic idr_size:

tools/perf/util/jitdump.c:jit_repipe_code_move() {
    ...
	event->mmap2.header.size = (sizeof(event->mmap2) -
			(sizeof(event->mmap2.filename) - size) + idr_size);
    ...
}

If the idr_size is greater than 16 (for example 24 or 32 bytes depending on 
sample types like TID, TIME, and CPU), could this lead to an out-of-bounds 
heap read when the event is written out? It appears the calculation for the 
header size expects the allocation to match the actual idr_size.

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