Re: [PATCH V2] tools/perf: Fix the check for parameterized field in event term

[sashiko-bot@kernel.org]

Sashiko AI review found 2 potential issue(s):
- [High] Stack-based buffer overflow in `perf_pmu__for_each_event` due to unchecked `snprintf` truncation underflow.
- [Medium] Meaningful code and commit message mismatch regarding the substring check.
--

commit d8e4b8dc1bd869d8438cc91880ce9791dc581a82
Author: Athira Rajeev <atrajeev@linux.ibm.com>

tools/perf: Fix the check for parameterized field in event term

This commit fixes an issue where the check for parameterized fields in
event terms was incorrect, causing them not to show in "perf list". It
also updates buffer sizing logic to use scnprintf instead of snprintf to
prevent out-of-bounds writes.

> Fix this check to use:
>
> if (str_ends_with(alias->terms, "=?")) {

This isn't a bug, but the commit message states it is using
str_ends_with(), while the implemented code actually uses
!strstr(alias->terms, "=?").

The code implementation appears correct since parameterized terms might not
be at the very end of the string, but the commit message does not match
the code.

> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index 23337d2fa281..fad7ef4c8e5d 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
[ ... ]
> @@ -2129,15 +2129,19 @@ static char *format_alias(char *buf, int len, const struct perf_pmu *pmu,
>  		pr_err("Failure to parse '%s' terms '%s': %d\n",
>  			alias->name, alias->terms, ret);
>  		parse_events_terms__exit(&terms);
> -		snprintf(buf, len, "%.*s/%s/", (int)pmu_name_len, pmu->name, alias->name);
> +		scnprintf(buf, len, "%.*s/%s/", (int)pmu_name_len, pmu->name, alias->name);
>  		return buf;
>  	}
> -	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) {
> +		const char *name = term->config;
> +
> +		if (!name)
> +			name = parse_events__term_type_str(term->type_term);
>  		if (term->type_val == PARSE_EVENTS__TERM_TYPE_STR)
> -			used += snprintf(buf + used, sub_non_neg(len, used),
> -					",%s=%s", term->config,
> +			used += scnprintf(buf + used, sub_non_neg(len, used),
> +					",%s=%s", name,
>  					term->val.str);
>  	}

The switch to scnprintf() here prevents buffer overflow within
format_alias(), but does the immediate caller perf_pmu__for_each_event()
also need this fix?

Looking at tools/perf/util/pmu.c:perf_pmu__for_each_event():

        char buf[1024];
        ...
        info.name = format_alias(buf, sizeof(buf), pmu, event, ...);
        buf_used = strlen(buf) + 1;
        ...
        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;
        }

If snprintf truncates the string, it returns the number of characters it
would have written. This causes buf_used to exceed sizeof(buf).

The subsequent size calculation sizeof(buf) - buf_used would then underflow
to a massive unsigned value, leading to a stack buffer overflow on the
next write.

Should the snprintf calls in perf_pmu__for_each_event() also be converted
to scnprintf to prevent stack corruption?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260426043006.48113-1-atrajeev@linux.ibm.com?part=1