Re: [PATCH v11 27/33] perf synthetic-events: Bound check when synthesizing mmap2 and build_id events

sashiko-bot
2026-04-13T01:55:57Z

Sashiko AI review found 3 potential issue(s):
- [Critical] Pre-existing buffer overflow and struct corruption due to copy-paste error in build ID truncation.
- [High] The new bounds check silently drops events for valid long paths by returning `-E2BIG`.
- [Low] Abstraction break in bounds checking logic.
--

commit 7fd80916fb020cf9f0223c8f3c7b78eaf05a550e
Author: Ian Rogers <irogers@google.com>

perf synthetic-events: Bound check when synthesizing mmap2 and build_id events

This commit adds bound checks when synthesizing mmap2 and build_id events
to make sure the filename doesn't overflow the event and lead to stack
corruption.

> diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
> index de812a2befbc7..a7fef7ac3da68 100644
> --- a/tools/perf/util/synthetic-events.c
> +++ b/tools/perf/util/synthetic-events.c
> @@ -2257,14 +2257,20 @@ int perf_event__synthesize_build_id(const struct perf_tool *tool,
> const char *filename)
> {
> union perf_event ev;
> - size_t len;
> + size_t len, filename_len = strlen(filename);
[ ... ]
> - len = sizeof(ev.build_id) + strlen(filename) + 1;
> + if (filename_len >= sizeof(ev.mmap2.filename))
> + return -EINVAL;

This isn't a bug, but does it make sense to check the filename length against
ev.mmap2.filename in a build_id event? It might be cleaner to use PATH_MAX
directly to avoid coupling the bounds check to the mmap2 struct definition.

> +
> + 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;

Will this check silently drop events for valid but long paths?

Since union perf_event is statically sized to fit its largest member, paths
that approach the maximum length might leave less than 48 bytes of headroom
for the sample ID headers.

This would cause the function to return -E2BIG for valid files instead of
safely truncating the filename to leave room for the sample IDs, potentially
dropping the event entirely.

> @@ -2303,14 +2309,21 @@ 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);

[ ... ]

While reviewing this function, there appears to be a pre-existing issue just
below the diff context regarding how the build_id size is truncated:

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);

Because ev is a union, modifying ev.build_id.size does not actually truncate
ev.mmap2.build_id_size.

Could this allow the memcpy() to overflow the 20-byte mmap2.build_id array and
corrupt adjacent fields if bid->size is too large?

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