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

[Athira Rajeev <atrajeev@linux.ibm.com>]

> On 17 Mar 2026, at 9:39 PM, Ian Rogers <irogers@google.com> wrote:
> 
> On Tue, Mar 17, 2026 at 1:56 AM Venkat <venkat88@linux.ibm.com> wrote:
>> 
>>> On 14 Mar 2026, at 2:03 PM, Athira Rajeev <atrajeev@linux.ibm.com> wrote:
>>> 
>>> 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.
>>> 
>>> Signed-off-by: Athira Rajeev <atrajeev@linux.ibm.com>
> 
> Thanks Athira, Sashiko is noting in its review:
> https://sashiko.dev/#/patchset/20260314083304.75321-1-atrajeev%40linux.ibm.com

Thanks Ian for pointing this. Its interesting to see this review.
I will check through the review.

Thanks
Athira
> 
> By inverting this check, parameterized events now proceed to
> parse_events_terms() and the rest of format_alias().
> 
> If a parameterized event uses a built-in perf keyword for its parameter name
> (e.g., config=?), the lexer parses it as a predefined term token, which sets
> term->config to NULL.
> 
> Does this cause a NULL pointer dereference in the term iteration loop?
> 
> 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),
>      ",%s=%s", term->config,
>      term->val.str);
> }
> 
> Passing a NULL pointer to the %s format specifier for term->config might
> cause a segmentation fault or formatting corruption.
> 
> Can this also lead to a stack buffer overflow due to how snprintf() return
> values are accumulated?
> 
> snprintf() returns the number of characters that would have been written.
> If the event names and terms are long enough, the "used" variable can exceed
> "len" (which is typically 1024 bytes).
> 
> When evaluating "buf + used" in the same loop:
> 
>  used += snprintf(buf + used, sub_non_neg(len, used), ...);
> 
> If "used" exceeds 1024, this creates an out-of-bounds pointer. Modern
> compilers can use this undefined behavior to infer that "used" <= 1024
> must be true, which optimizes out the safety check inside sub_non_neg():
> 
> static int sub_non_neg(int a, int b)
> {
>  if (b > a)
>    return 0;
>  return a - b;
> }
> 
> The compiler can reduce this to simply "a - b" (or "len - used").
> If "used" is actually greater than 1024, "len - used" evaluates to a
> negative integer. This negative value is implicitly cast to size_t for
> snprintf(), resulting in a massive size limit.
> 
> Would this cause snprintf() to write past the end of the stack buffer
> without bounds checking? Using scnprintf() might prevent "used" from
> exceeding "len".
> 
> Thanks,
> Ian
> 
>>> ---
>>> tools/perf/util/pmu.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>> 
>>> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
>>> index 23337d2fa281..0b8d58543f17 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;
>>> --
>>> 2.47.3
>>> 
>> 
>> Tested this patch, and its working as expected.
>> 
>> Before Patch:
>> 
>> ./perf list hv_24x7 | grep -i CPM_EXT_INT_OS
>>  hv_24x7/CPM_EXT_INT_OS/                            [Kernel PMU event]
>> 
>> After Patch:
>> 
>> ./perf list hv_24x7 | grep -i CPM_EXT_INT_OS
>> hv_24x7/CPM_EXT_INT_OS,domain=?,core=?/ [Kernel PMU event]
>> 
>> 
>> ./perf stat -e hv_24x7/PM_PAU_CYC,chip=0/
>> 
>> 
>> Performance counter stats for 'system wide':
>> 
>>        2018866563      hv_24x7/PM_PAU_CYC,chip=0/
>> 
>>     229.938231314 seconds time elapsed
>> 
>> Tested-by: Venkat Rao Bagalkote <venkat88@linux.ibm.com>
>> 
>> Regards,
>> Venkat.