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

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

[Athira Rajeev]

The format_alias() function in util/pmu.c has a check to
detect whether the event has parameterized field ( =? ).
The string alias->terms contains the event and if the event
has user configurable parameter, there will be presence of
sub string "=?" in the alias->terms.

Snippet of code:

 /* Paramemterized events have the parameters shown. */
       if (strstr(alias->terms, "=?")) {
               /* No parameters. */
               snprintf(buf, len, "%.*s/%s/", (int)pmu_name_len, pmu->name, alias->name);

if "strstr" contains the substring, it returns a pointer
and hence enters the above check which is not the expected
check. And hence "perf list" doesn't have the parameterized
fields in the result.

Fix this check to use:

if (!strstr(alias->terms, "=?")) {

With this change, perf list shows the events correctly with
the strings showing parameters.

Other changes in this patch:
- Replace snprintf with scnprintf in buffer offset calculations to
ensure the 'used' count will not exceed the "len"
- If a parameterized event uses a built-in perf keyword for its
parameter name (eg, config=?), the lexer parses it as a predefined
term token, which sets term->config to NULL. Add check to use
parse_events__term_type_str() if term->config is NULL

Signed-off-by: Athira Rajeev <atrajeev@linux.ibm.com>
---
Changelog:
v2 -> v3:
After review from Sashiko, added below changes:
- Corrected commit message to say "strstr" which was previously
added as "str_ends_with".
- Updated snprintf with scnprintf in perf_pmu__for_each_event()
as well.
v1 -> v2:
After review from Sashiko, added below changes:
- Replace snprintf with scnprintf in buffer offset calculations to
ensure the 'used' count will not exceed the "len"
- If a parameterized event uses a built-in perf keyword for its
parameter name (eg, config=?), the lexer parses it as a predefined
term token, which sets term->config to NULL. Add check to use
parse_events__term_type_str() if term->config is NULL

 tools/perf/util/pmu.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 23337d2fa281..ca0ceb0623ca 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -2117,7 +2117,7 @@ static char *format_alias(char *buf, int len, const struct perf_pmu *pmu,
 						   skip_duplicate_pmus);
 
 	/* Paramemterized events have the parameters shown. */
-	if (strstr(alias->terms, "=?")) {
+	if (!strstr(alias->terms, "=?")) {
 		/* No parameters. */
 		snprintf(buf, len, "%.*s/%s/", (int)pmu_name_len, pmu->name, alias->name);
 		return buf;
@@ -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);
 	}
 	parse_events_terms__exit(&terms);
@@ -2237,13 +2241,13 @@ int perf_pmu__for_each_event(struct perf_pmu *pmu, bool skip_duplicate_pmus,
 		info.scale_unit = NULL;
 		if (strlen(event->unit) || event->scale != 1.0) {
 			info.scale_unit = buf + buf_used;
-			buf_used += snprintf(buf + buf_used, sizeof(buf) - buf_used,
+			buf_used += scnprintf(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,
+		buf_used += scnprintf(buf + buf_used, sizeof(buf) - buf_used,
 				"%.*s/%s/", (int)pmu_name_len, info.pmu_name, event->terms) + 1;
 		info.str = event->terms;
 		info.topic = event->topic;
@@ -2254,7 +2258,7 @@ int perf_pmu__for_each_event(struct perf_pmu *pmu, bool skip_duplicate_pmus,
 	}
 	if (pmu->selectable) {
 		info.name = buf;
-		snprintf(buf, sizeof(buf), "%s//", pmu->name);
+		scnprintf(buf, sizeof(buf), "%s//", pmu->name);
 		info.alias = NULL;
 		info.scale_unit = NULL;
 		info.desc = NULL;
-- 
2.47.3

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

[sashiko-bot]

Sashiko AI review found 1 potential issue(s):
- [High] Unconditional increment of `buf_used` causes unsigned integer underflow leading to a stack buffer overflow.
--

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

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

This commit fixes the event term checking by changing strstr(alias->terms, "=?");
to !strstr(alias->terms, "=?"). It also replaces some snprintf calls with
scnprintf in buffer offset calculations to prevent out-of-bounds counting.

> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index 23337d2fa281..ca0ceb0623ca 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c

[ ... ]

> @@ -2237,13 +2241,13 @@ int perf_pmu__for_each_event(struct perf_pmu *pmu, bool skip_duplicate_pmus,
>  		info.scale_unit = NULL;
>  		if (strlen(event->unit) || event->scale != 1.0) {
>  			info.scale_unit = buf + buf_used;
> -			buf_used += snprintf(buf + buf_used, sizeof(buf) - buf_used,
> +			buf_used += scnprintf(buf + buf_used, sizeof(buf) - buf_used,
>  					"%G%s", event->scale, event->unit) + 1;
>  		}

Can this unconditional + 1 addition to buf_used cause a regression by
triggering a stack buffer overflow in the subsequent scnprintf call?

buf is a local 1024-byte stack array, and buf_used tracks the offset as an
unsigned size_t. If format_alias() fills buf to its limit, strlen(buf) will
be 1023, causing buf_used to be initialized to 1024.

If the event has a scale or unit, the size parameter to scnprintf becomes
sizeof(buf) - buf_used, which evaluates to 1024 - 1024 = 0. scnprintf safely
writes nothing and returns 0.

However, buf_used += 0 + 1 unconditionally increments buf_used to 1025.

>  		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,
> +		buf_used += scnprintf(buf + buf_used, sizeof(buf) - buf_used,
>  				"%.*s/%s/", (int)pmu_name_len, info.pmu_name, event->terms) + 1;

Does the size calculation sizeof(buf) - buf_used underflow here?

Since buf_used is now 1025, 1024 - 1025 underflows the size_t type to
SIZE_MAX. scnprintf would then be invoked with an effectively unbounded size
and an out-of-bounds pointer (buf + 1025), which could overwrite adjacent
memory on the stack.

Would it be better to bound the remaining space calculation and only increment
the offset if space actually existed, perhaps using sub_non_neg() like the
patch does earlier in format_alias()?

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