Re: [PATCH 15/23] perf pmu: Use scnprintf() in format_alias()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Pre-existing stack buffer overflow in `perf_pmu__for_each_event` due to unchecked `snprintf` return values.
--

commit 7b55c41b9ee2781220a71c8d96e4e7a17ebb8cf5
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf pmu: Use scnprintf() in format_alias()

format_alias() accumulates strings into a caller-provided buffer using
snprintf(). On truncation, snprintf() returns the would-have-written
count, which can exceed the buffer size. Switch to scnprintf() which
returns the actual number of bytes written, keeping used within bounds.

> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index 4ddccc863727d..2fce38140eb8a 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -2155,11 +2155,11 @@ static char *format_alias(char *buf, int len, const struct perf_pmu *pmu,
[ ... ]
> -	used = snprintf(buf, len, "%.*s/%s", (int)pmu_name_len, pmu->name, alias->name);
> +	used = scnprintf(buf, len, "%.*s/%s", (int)pmu_name_len, pmu->name, alias->name);
>  
>  	list_for_each_entry(term, &terms.terms, list) {
>  		if (term->type_val == PARSE_EVENTS__TERM_TYPE_STR)
> -			used += snprintf(buf + used, sub_non_neg(len, used),
> +			used += scnprintf(buf + used, sub_non_neg(len, used),
>  					",%s=%s", term->config,
>  					term->val.str);
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a similar issue in
perf_pmu__for_each_event() in the same file:

tools/perf/util/pmu.c:perf_pmu__for_each_event()
		if (strlen(event->unit) || event->scale != 1.0) {
			info.scale_unit = buf + buf_used;
			buf_used += snprintf(buf + buf_used, sizeof(buf) - buf_used,
					"%G%s", event->scale, event->unit) + 1;
		}
		info.desc = event->desc;
		info.long_desc = event->long_desc;
		info.encoding_desc = buf + buf_used;
		buf_used += snprintf(buf + buf_used, sizeof(buf) - buf_used,
				"%.*s/%s/", (int)pmu_name_len, info.pmu_name, event->terms) + 1;

Does this code also need to use scnprintf() instead of snprintf() for the
event unit, scale, and terms?

Since buf_used accumulates the return values of snprintf(), can it exceed
sizeof(buf) if the event properties are sufficiently long?

If that happens, the unsigned subtraction sizeof(buf) - buf_used would underflow
to a very large value. 

Could this disable bounds checking on subsequent calls and allow snprintf() to
write past the end of the buf[] stack buffer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260610195157.2091137-1-acme@kernel.org?part=15