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

[sashiko-bot@kernel.org]

Sashiko AI review found 3 potential issue(s):
- [High] Incomplete fix for stack buffer overflow during event synthesis; `perf_event__synthesize_build_id` remains vulnerable.
- [High] `perf_event__synthesize_mmap2_build_id` unconditionally aborts if `evsel` is NULL, which silently drops valid events during DSO hit marking.
- [Medium] The bounds check in `perf_event__synthesize_mmap2_build_id` drops valid events with long paths instead of properly allocating the promised space.
--

commit 9127a57d3b6c56a5eb8812d5d12e6bf3a1c023e6
Author: Ian Rogers <irogers@google.com>

perf inject: Don't pass evsel with sample

This commit simplifies the handling of evsel by using the one already
contained in the sample instead of passing it as a separate argument.
It also adds checks to avoid potential buffer overruns when appending
sample identifiers.

> Add space for sample identifiers in
> perf_event__synthesize_mmap2_build_id to avoid Sashiko caught
> potential buffer overrun.

The commit message states "Add space for sample identifiers in
perf_event__synthesize_mmap2_build_id". However, looking at the code changes,
it appears we only added a bounds check rather than allocating additional
space.

Could this wording be updated if the intent was just to add bounds checking?
Since sizeof(ev) is bounded by the union size, does returning -E2BIG here
risk dropping valid events with long paths?

> diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
> index 85bee747f4cd2..502b1f847c91c 100644
> --- a/tools/perf/util/synthetic-events.c
> +++ b/tools/perf/util/synthetic-events.c

[ ... ]

> @@ -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;
>  
>  		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->evsel->core.attr.sample_type,
> +						       sample);

Does this function also need the capacity check that was added to
perf_event__synthesize_mmap2_build_id()?

If filename approaches PATH_MAX, len can reach ~4136 bytes. Appending up to
48 bytes of sample identifiers could exceed sizeof(ev) and cause a stack
buffer overflow.

[ ... ]

> @@ -2299,7 +2314,6 @@ int perf_event__synthesize_mmap2_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,
>  					  __u32 pid, __u32 tid,
>  					  __u64 start, __u64 len, __u64 pgoff,
> @@ -2308,13 +2322,23 @@ 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;
>  
> -	ev_len = sizeof(ev.mmap2) - sizeof(ev.mmap2.filename) + strlen(filename) + 1;
> +	if (!sample->evsel)
> +		return -EINVAL; /* Evsel is needed for sample_type data. */

Previously, if evsel was NULL, this function skipped appending the sample ID
trailer but still called process() to successfully repipe the synthesized
mmap2 event.

Will this early return silently drop valid events during DSO hit marking,
where mmap_evsel might be NULL and fail to resolve?

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

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