Re: [PATCH v7 2/2] perf evsel: Find process with busy PMUs for EBUSY

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Use-After-Free and state corruption due to strtok() on a stack-allocated buffer.
- [Medium] Terminal injection vulnerability via unsanitized process command lines.
- [Low] Improper string comparison bound can lead to false positive process identification.
--

commit a4a94ff69c8d665f6d09eaea15fdbbbd89954b18
Author: Chun-Tse Shao <ctshao@google.com>

perf evsel: Find process with busy PMUs for EBUSY

This commit parses fdinfo to match the PMU type of a perf event that
failed to open with EBUSY, and reports the conflicting processes.

> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -3969,29 +3970,64 @@ static int dump_perf_event_processes(char *msg, size_t size)
>  				continue;
>  			/* Take care as readlink doesn't null terminate the string. */
>  			if (!strncmp(buf, "anon_inode:[perf_event]", link_size)) {

Is link_size the correct bound for this comparison?

If the symlink target is shorter than "anon_inode:[perf_event]" but exactly
matches the prefix (for example, if link_size is 4 and buf is "anon"), this
strncmp will return 0 and falsely identify the process. Should this also
check that link_size equals the exact length of the expected string?

[ ... ]

> +				buf[fdinfo_size] = '\0';
> +
> +				line = strtok(buf, "\n");
> +				while (line != NULL) {
> +					if (sscanf(line,
> +						   "perf_event_attr.type:\t%u",
> +						   &perf_event_type) == 1)
> +						break;
> +					line = strtok(NULL, "\n");
>  				}

buf is a stack-allocated array. Since strtok stores a pointer to the parsed
string in its internal static state, does breaking out of this loop early
leave the internal pointer pointing to popped stack memory?

If another part of the tool later calls strtok(NULL, ...) in the same thread,
could this cause a use-after-free or memory corruption as strtok writes null
bytes? Would it be safer to use strtok_r here to avoid clobbering the shared
parsing state?

[ ... ]

> +					cmdline_size = read(cmdline_fd, buf, sizeof(buf) - 1);
> +					close(cmdline_fd);
> +					if (cmdline_size < 0)
> +						continue;
> +					buf[cmdline_size] = '\0';
> +					for (ssize_t i = 0; i < cmdline_size; i++) {
> +						if (buf[i] == '\0')
> +							buf[i] = ' ';
> +					}
> +
> +					if (printed == 0)
> +						printed += scnprintf(
> +							msg, size,
> +							"Possible processes:\n");
> +
> +					printed += scnprintf(msg + printed, size - printed,
> +							"%s %s\n", proc_entry->d_name, buf);
> +					break;

Does this safely handle unprivileged process command lines?

While null bytes are replaced with spaces, other non-printable characters and
ANSI escape sequences are preserved. If a malicious user intentionally holds a
PMU and crafts a command line with escape sequences, could those sequences
execute in the terminal of the administrator running perf stat, potentially
allowing terminal output spoofing?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518224819.1247382-1-ctshao@google.com?part=2