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

[sashiko-bot@kernel.org]

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