Re: [PATCH 12/12] perf jitdump: Validate unwinding sizes against record payload

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

Pre-existing issues:
- [High] State leakage of unwinding data due to flawed conditional cleanup, corrupting subsequent JIT loads.
- [High] Heap buffer overflow in `jit_repipe_code_move` due to undersized padding for the `event` buffer.
--

--- Patch [12]: [PATCH 12/12] perf jitdump: Validate unwinding sizes against record payload ---
commit 6a41da3e449813b67858843724cc900be799f924
Author: Arnaldo Carvalho de Melo <[email protected]>

perf jitdump: Validate unwinding sizes against record payload

This commit validates unwinding sizes against the record payload to
prevent an out-of-bounds heap read in libelf. It also changes the type
of mapped_size to uint64_t to prevent truncation.

> diff --git a/tools/perf/util/jitdump.c b/tools/perf/util/jitdump.c
> index 5898a7d8eb962..d25a9fe9b020c 100644
> --- a/tools/perf/util/jitdump.c
> +++ b/tools/perf/util/jitdump.c
> @@ -462,7 +462,8 @@ static int jit_repipe_code_load(struct jit_buf_desc *jd, union jr_entry *jr)
>  	u16 idr_size;
>  	const char *sym;
>  	uint64_t count;
> -	int ret, csize, usize;
> +	int ret, csize;
> +	uint64_t usize;
>  	pid_t nspid, pid, tid;
>  	struct {
>  		u32 pid, tid;
> @@ -543,7 +544,7 @@ static int jit_repipe_code_load(struct jit_buf_desc *jd, union jr_entry *jr)
>  
>  	event->mmap2.pgoff = GEN_ELF_TEXT_OFFSET;
>  	event->mmap2.start = addr;
> -	event->mmap2.len   = usize ? ALIGN_8(csize) + usize : csize;
> +	event->mmap2.len   = usize ? ALIGN_8((uint64_t)csize) + usize : (uint64_t)csize;

[Severity: High]
This is a pre-existing issue, but is it possible for stale unwinding state to
persist and corrupt subsequent JIT code loads?

Looking earlier in jit_repipe_code_load(), the cleanup logic requires
eh_frame_hdr_size to be non-zero:

	if (jd->unwinding_data && jd->eh_frame_hdr_size) {
		zfree(&jd->unwinding_data);
		jd->eh_frame_hdr_size = 0;
		jd->unwinding_mapped_size = 0;
		jd->unwinding_size = 0;
	}

If a JIT_CODE_UNWINDING_INFO record provides unwinding data without an EH
frame header (where eh_frame_hdr_size == 0), this cleanup is bypassed.
The stale unwinding_data and unwinding_mapped_size would then remain in the
context descriptor (jd) and incorrectly apply to subsequent JIT_CODE_LOAD
events.

>  	event->mmap2.pid   = pid;
>  	event->mmap2.tid   = tid;
>  	event->mmap2.ino   = st.st_ino;

[ ... ]

> @@ -612,7 +613,7 @@ static int jit_repipe_code_move(struct jit_buf_desc *jd, union jr_entry *jr)
>  	char *filename;
>  	size_t size;
>  	struct stat st;
> -	int usize;
> +	uint64_t usize;

[Severity: High]
This is also a pre-existing issue, but could this undersized padding cause a
heap buffer overflow when copying sample IDs?

Looking further down in jit_repipe_code_move(), the event buffer is allocated
with a hardcoded 16-byte padding:

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

However, the mmap2 event size is calculated using the machine's idr_size:

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

And the sample ID data is written based on that dynamically sized offset:

	id = (void *)((unsigned long)event + event->mmap.header.size - idr_size);

If idr_size exceeds 16 bytes and the output filename is near PATH_MAX, the
calculated size could exceed the allocated space, causing the program to
write the pid, tid, and time out of bounds, followed by an out-of-bounds read
during output.

>  	u16 idr_size;
>  	int ret;
>  	pid_t nspid, pid, tid;

[ ... ]

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