Re: [PATCH v10 14/29] perf inject: Don't pass evsel with sample

On Sun, Apr 12, 2026 at 02:40:11AM +0000, [email protected] wrote:
> Sashiko AI review found 3 potential issue(s):
> - [High] Typo in bounds check for `build_id_size` in `perf_event__synthesize_mmap2_build_id` causes memory corruption and out-of-bounds read/write.
> - [High] The bounds check for `build_id.size` in `perf_event__synthesize_build_id` limits the size to the destination buffer's capacity instead of the source buffer's capacity, allowing an out-of-bounds read.
> - [Medium] The patch alters the PID and TID used in the synthesized ID sample of `mmap2` events, corrupting the event's sampling context.
> --
> 
> commit 86c6e453885cee2a25c1c4a4cc9e00545f4ba854
> Author: Ian Rogers <[email protected]>
> 
> perf inject: Don't pass evsel with sample
> 
> This commit cleans up the handling of sample and mmap_evsel when
> synthesizing build id events. It also attempts to bound check the filename
> length to avoid potential buffer overruns.
> 
> > diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
> > index 85bee747f4cd2..9a6941cfaad34 100644
> > --- a/tools/perf/util/synthetic-events.c
> > +++ b/tools/perf/util/synthetic-events.c

I've removed all new bound checkings in this file for now.

Thanks,
Namhyung

> [ ... ]
> > @@ -2252,17 +2267,22 @@ int perf_event__synthesize_build_id(const struct perf_tool *tool,
> [ ... ]
> > +	if (filename_len >= sizeof(ev.mmap2.filename))
> > +		return -EINVAL;
> > +
> > +	len = sizeof(ev.build_id) + filename_len + 1;
> >  	len = PERF_ALIGN(len, sizeof(u64));
> >  
> > +	if (len + MAX_ID_HDR_ENTRIES * sizeof(__u64) > sizeof(ev))
> > +		return -E2BIG;
> > +
> >  	memset(&ev, 0, len);
> 
> Since the commit message explicitly mentions fixing potential buffer overruns
> in this function, is this out-of-bounds read another regression that needs
> addressing?
> 
> Looking at the rest of perf_event__synthesize_build_id():
> 
>     ev.build_id.size = bid->size;
>     if (ev.build_id.size > sizeof(ev.build_id.build_id))
>         ev.build_id.size = sizeof(ev.build_id.build_id);
>     memcpy(ev.build_id.build_id, bid->data, ev.build_id.size);
> 
> The check limits ev.build_id.size to the destination buffer's capacity
> (24 bytes). However, bid->data is only 20 bytes. If a malformed file
> provides a bid->size between 21 and 24, could memcpy() read out-of-bounds
> from bid->data?
> 
> [ ... ]
> > @@ -2308,13 +2328,24 @@ int perf_event__synthesize_mmap2_build_id(const struct perf_tool *tool,
> [ ... ]
> > +	if (filename_len >= sizeof(ev.mmap2.filename))
> > +		return -EINVAL;
> > +
> > +	ev_len = sizeof(ev.mmap2) - sizeof(ev.mmap2.filename) + filename_len + 1;
> >  	ev_len = PERF_ALIGN(ev_len, sizeof(u64));
> >  
> > +	if (ev_len + MAX_ID_HDR_ENTRIES * sizeof(__u64) > sizeof(ev))
> > +		return -E2BIG;
> > +
> >  	memset(&ev, 0, ev_len);
> 
> Similar to the bounds check above, this function is mentioned in the commit
> message as receiving bounds checks, but does this existing typo cause a
> regression here?
> 
> Looking further down in perf_event__synthesize_mmap2_build_id():
> 
>     ev.mmap2.build_id_size = bid->size;
>     if (ev.mmap2.build_id_size > sizeof(ev.mmap2.build_id))
>         ev.build_id.size = sizeof(ev.mmap2.build_id);
>     memcpy(ev.mmap2.build_id, bid->data, ev.mmap2.build_id_size);
> 
> If ev.mmap2.build_id_size exceeds the bounds, it modifies ev.build_id.size
> instead of truncating ev.mmap2.build_id_size. Could this result in an
> out-of-bounds read and write in the subsequent memcpy(), potentially
> corrupting adjacent fields like prot, flags, and filename?
> 
> [ ... ]
> > @@ -2339,7 +2370,12 @@ int perf_event__synthesize_mmap2_build_id(const struct perf_tool *tool,
> >  
> >  	array = &ev;
> >  	array += ev.header.size;
> > -	ret = perf_event__synthesize_id_sample(array, evsel->core.attr.sample_type, sample);
> > +	ret = __perf_event__synthesize_id_sample(array, sample_type,
> > +						pid, tid,
> > +						sample->time,
> > +						sample->id,
> > +						sample->stream_id,
> > +						sample->cpu);
> >  	if (ret < 0)
> >  		return ret;
> 
> Before this patch, the sample's PID and TID were extracted from the sampling
> context (sample->pid and sample->tid). By switching to
> __perf_event__synthesize_id_sample(), this now passes the mmap2 target
> pid and tid.
> 
> If a thread maps memory for a different process, the target mmap.pid differs
> from the sampling context sample->pid. Could this regression break event
> correlation for tools relying on the sample ID to identify the thread that
> generated the event?
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=14