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

sashiko-bot
2026-04-11T07:19:17Z

Sashiko AI review found 1 potential issue(s):
- [Critical] Missing bounds check for `filename` length in `perf_event__synthesize_build_id` causes stack buffer overflow.
--

commit 2a4830e2b3a06525f60938587293cda249c94ea7
Author: Ian Rogers <irogers@google.com>

perf inject: Don't pass evsel with sample

This commit cleans up the handling of the sample and mmap_evsel structures
when synthesizing build id events. It refactors functions to avoid passing
evsel separately. It also adds a bounds check to the filename length in
perf_event__synthesize_mmap2_build_id() to prevent a potential buffer
overrun caught by Sashiko.

> diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
> index 85bee747f4cd2..86049b6991d1f 100644
> --- a/tools/perf/util/synthetic-events.c
> +++ b/tools/perf/util/synthetic-events.c
[ ... ]
> @@ -2252,7 +2267,6 @@ int perf_event__synthesize_build_id(const struct perf_tool *tool,
> struct perf_sample *sample,
> struct machine *machine,
> perf_event__handler_t process,
> - const struct evsel *evsel,
> __u16 misc,
> const struct build_id *bid,
> const char *filename)
> @@ -2275,12 +2289,13 @@ int perf_event__synthesize_build_id(const struct perf_tool *tool,
> ev.build_id.header.size = len;
> strcpy(ev.build_id.filename, filename);
>
> - if (evsel) {
> + if (sample->evsel) {
> void *array = &ev;
> int ret;

Should a bounds check for the filename length be added to this function as
well?

Like perf_event__synthesize_mmap2_build_id(), this function allocates a
union perf_event ev on the stack. The len variable is calculated using
strlen(filename) just prior to this block, and then used in a memset()
and strcpy() without bounds verification.

Since filename comes from untrusted profiling data, could a crafted
perf.data file with an unusually long filename overflow the ev stack
buffer here?

[ ... ]
> @@ -2308,13 +2322,24 @@ int perf_event__synthesize_mmap2_build_id(const struct perf_tool *tool,
> const char *filename)
> {
> union perf_event ev;
> + size_t filename_len = strlen(filename);
> size_t ev_len;
> void *array;
> int ret;
> + u64 sample_type = 0;
>
> - ev_len = sizeof(ev.mmap2) - sizeof(ev.mmap2.filename) + strlen(filename) + 1;
> + if (sample->evsel)
> + sample_type = sample->evsel->core.attr.sample_type;
> +
> + 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;
> +

This is the bounds check that was added for the mmap2 variant. Adding a
similar verification that len + MAX_ID_HDR_ENTRIES * sizeof(__u64) is within
sizeof(ev) to perf_event__synthesize_build_id() would prevent a similar
stack overflow.

--
Sashiko AI review · https://sashiko.dev/#/patchset/20260411065718.372240-1-irogers@google.com?part=14