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

sashiko-bot
2026-04-12T02:40:12Z

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 <irogers@google.com>

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
[ ... ]
> @@ -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/20260412020833.641177-1-irogers@google.com?part=14