Re: [PATCH 11/12] perf jitdump: Validate sym string NUL-termination in code load

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

Pre-existing issues:
- [High] Pre-existing heap out-of-bounds read in `jit_repipe_code_move` due to a hardcoded `+ 16` allocation size while using a dynamically sized header length.
--

--- Patch [11]: [PATCH 11/12] perf jitdump: Validate sym string NUL-termination in code load ---
commit acba84089a50bc7dce59fc1ccce1335a41bbb013
Author: Arnaldo Carvalho de Melo <[email protected]>

perf jitdump: Validate sym string NUL-termination in code load

This commit adds a memchr() check to verify that the symbol name is
NUL-terminated within the region between the load header and the code blob.
This prevents an out-of-bounds read that could occur if code_size equals
total_size - sizeof(jr->load).

> diff --git a/tools/perf/util/jitdump.c b/tools/perf/util/jitdump.c
> index 5a3ea2681fb37..5898a7d8eb962 100644
> --- a/tools/perf/util/jitdump.c
> +++ b/tools/perf/util/jitdump.c
> @@ -487,6 +487,13 @@ static int jit_repipe_code_load(struct jit_buf_desc *jd, union jr_entry *jr)
>  
>  	sym   = (void *)((unsigned long)jr + sizeof(jr->load));
>  	code  = (unsigned long)jr + jr->load.p.total_size - csize;
> +
> +	/* sym string lives between the load header and the code blob */
> +	if (!memchr(sym, '\0', code - (unsigned long)sym)) {
> +		pr_warning("jitdump: unterminated symbol name in code_load record\n");
> +		return -1;
> +	}
> +
>  	count = jr->load.code_index;
>  	idr_size = jd->machine->id_hdr_size;

[Severity: High]
This is a pre-existing issue, but I noticed a potential out-of-bounds read in
the related jit_repipe_code_move() function when calculating header sizes.

In tools/perf/util/jitdump.c:jit_repipe_code_move(), the event buffer is
allocated with a hardcoded +16 byte extra capacity:

	/*
	 * +16 to account for sample_id_all (hack)
	 */
	event = calloc(1, sizeof(*event) + 16);

However, when setting the header size, it adds the dynamic idr_size:

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

Could this result in an out-of-bounds read when perf_data__write() processes
this event? 

If idr_size exceeds 16 bytes (which can happen depending on the sample_type)
and the dynamically sized filename string takes up the remaining slack space,
the calculated event->mmap2.header.size might exceed the allocated buffer size.
Could this leak uninitialized heap memory into the output stream?

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