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] Out-of-bounds heap read and information leak in `jit_repipe_code_move()` due to undersized event allocation.
- [High] Memory/state leak and mapping corruption when `eh_frame_hdr_size` is 0.
--

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

perf jitdump: Validate unwinding sizes against record payload

Validate that unwinding_size fits within the record payload and that
eh_frame_hdr_size does not exceed unwinding_size before allocating or
storing the values, so a bogus record cannot force a large allocation
that is then discarded. mapped_size is likewise taken from the record
and was narrowed into an int. Use uint64_t for usize so the value
cannot truncate.

> 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
[ ... ]
> @@ -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 the unwinding state
to be leaked and corrupt subsequent event lengths?

Looking at jit_repipe_code_load() just above this assignment, we only
clean up the unwinding state if eh_frame_hdr_size is 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 runtime emits a record with eh_frame_hdr_size == 0, this check
fails. As a result, unwinding_mapped_size is not reset.

When processing the next JIT code load record, the stale unwinding_mapped_size
is pulled into the usize variable and added to event->mmap2.len here.

Could this inflate the mapping size and cause collisions in the perf mapping
tree, corrupting the stack trace unwinding process?

>  	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;
>  	u16 idr_size;

[Severity: High]
This is a pre-existing issue, but does the event allocation in
jit_repipe_code_move() leave room for a buffer over-read?

The event buffer is allocated with a hardcoded +16 padding:

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

However, later in the same function, the header size incorporates idr_size:

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

If idr_size (derived from the PERF_SAMPLE_* format) is greater than 16 bytes,
header.size could exceed the allocated chunk size.

If perf_data__write() later reads header.size bytes, could this cause an
out-of-bounds heap read and leak uninitialized heap memory into the output
perf.data file?

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