Re: [PATCH v7 3/5] perf synthetic-events: Fix stack buffer overflow and bounds in cgroup synthesis

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Pre-existing 1-byte stack buffer overflow when appending a slash to the cgroup mount point.
- [High] Calling pthread_join() on uninitialized thread IDs during error cleanup.
--

--- Patch [3]: [PATCH v7 3/5] perf synthetic-events: Fix stack buffer overflow and bounds in cgroup synthesis ---
commit 65d6809cf0ab7cea41d6812171193757a21f8d99
Author: Ian Rogers <irogers@google.com>

perf synthetic-events: Fix stack buffer overflow and bounds in cgroup synthesis

The commit fixes a stack buffer overflow bug in perf_event__synthesize_cgroup()
by eliminating in-place path mutation. It uses PERF_ALIGN for path_len, clamps
raw_path_len, and uses strlcpy to prevent overruns.

> diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
> index e9952c27b3bc0..7b8095e013b98 100644
> --- a/tools/perf/util/synthetic-events.c
> +++ b/tools/perf/util/synthetic-events.c
> @@ -635,15 +635,22 @@ static int perf_event__synthesize_cgroup(const struct perf_tool *tool,
>  					 struct machine *machine)
>  {
>  	size_t event_size = sizeof(event->cgroup) - sizeof(event->cgroup.path);

[ ... ]

This is a pre-existing issue, but I noticed a potential problem in
perf_event__synthesize_cgroups():

tools/perf/util/synthetic-events.c:perf_event__synthesize_cgroups() {
	...
	char cgrp_root[PATH_MAX];
	...
	if (cgroupfs_find_mountpoint(cgrp_root, PATH_MAX, "perf_event") < 0) {
	...
	/* make sure the path starts with a slash (after mount point) */
	strcat(cgrp_root, "/");
	...
}

[Severity: High]
Can this strcat() cause a 1-byte stack buffer overflow if the mount point
path returned by cgroupfs_find_mountpoint() is PATH_MAX - 1 bytes long?

cgroupfs_find_mountpoint() can return paths up to PATH_MAX - 1 characters
(4095 bytes) into the cgrp_root buffer of size PATH_MAX.

When strcat() appends the "/", the '/' would be written at index 4095 and the
NUL terminator at index 4096.

Since cgrp_root is sized PATH_MAX (valid indices 0 to 4095), writing the NUL
terminator at index 4096 causes an overflow. While a 4095-byte mount path is
uncommon, it seems structurally possible.


This is also a pre-existing issue, but I noticed a potential problem in the
error handling path of perf_event__synthesize_threads():

tools/perf/util/synthetic-events.c:perf_event__synthesize_threads() {
	...
	synthesize_threads = calloc(thread_nr, sizeof(pthread_t));
	...
	for (i = 0; i < thread_nr; i++) {
		if (pthread_create(&synthesize_threads[i], NULL,
				   synthesize_threads_worker, &args[i]))
			goto out_join;
	}
	err = 0;
out_join:
	for (i = 0; i < thread_nr; i++)
		pthread_join(synthesize_threads[i], NULL);
	...
}

[Severity: High]
Is it safe to join uninitialized threads in this error cleanup loop?

If pthread_create() fails, the code jumps to out_join. The cleanup loop then
unconditionally iterates through all thread_nr elements, calling
pthread_join().

Because synthesize_threads was allocated with calloc, the uninitialized or
failed thread IDs are 0.

On libc implementations like musl where pthread_t is a pointer, passing 0
results in a NULL pointer dereference inside pthread_join(), which could crash
the application during the error recovery path.

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