> On 5 Jun 2026, at 3:32 AM, Namhyung Kim <[email protected]> wrote:
>
> On Mon, May 04, 2026 at 09:12:05PM +0530, Athira Rajeev wrote:
>> Replace snprintf with scnprintf in buffer offset calculations to
>> ensure the 'used' count will not exceed the "len".
>>
>> The current logic in perf_pmu__for_each_event uses an unconditional
>> + 1 increment to buf_used to account for null terminators. This can
>> cause a a stack buffer overflow in the subsequent scnprintf call.
>> When the local stack buffer buf (1024 bytes) is full, buf_used can
>> reach 1025. This causes the subsequent remaining space calculation
>> sizeof(buf) - buf_used to underflow.
>>
>> Use sub_non_neg() to see if space actually existed, and only
>> increment the offset if remaning space is present.
>>
>> Changes includes:
>> - Use sub_non_neg to check if space exists
>> - Replacing snprintf with scnprintf to ensure the return value
>> reflects the actual bytes written into the buffer.
>> - Only increment buf_used by 1 if space exists
>> - 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 <[email protected]>
>> ---
>> Changelog:
>> v2 -> v3:
>> - Split the scnprintf related changes in separate patch
>> - Handle the overflow issues and unconditional increment
>> wrapped around sub_non_neg addressing review comment from Sashiko
>>
>> tools/perf/util/pmu.c | 46 ++++++++++++++++++++++++++++++++-----------
>> 1 file changed, 35 insertions(+), 11 deletions(-)
>>
>> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
>> index 0b8d58543f17..4b9ade1a4cf9 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);
>> }
>> parse_events_terms__exit(&terms);
>> @@ -2201,6 +2205,7 @@ int perf_pmu__for_each_event(struct perf_pmu *pmu,
>> bool skip_duplicate_pmus,
>> int ret = 0;
>> struct hashmap_entry *entry;
>> size_t bkt;
>> + size_t size_rem, len;
>>
>> if (perf_pmu__is_tracepoint(pmu))
>> return tp_pmu__for_each_event(pmu, state, cb);
>> @@ -2234,17 +2239,36 @@ int perf_pmu__for_each_event(struct perf_pmu *pmu,
>> bool skip_duplicate_pmus,
>> }
>> buf_used = strlen(buf) + 1;
>> }
>> +
>> 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,
>> - "%G%s", event->scale, event->unit) + 1;
>> + /* Check the remaining space */
>> + size_rem = sub_non_neg(sizeof(buf), buf_used);
>> +
>> + if (size_rem > 0) {
>> + info.scale_unit = buf + buf_used;
>> + len = scnprintf(buf + buf_used, size_rem, "%G%s",
>> + event->scale, event->unit);
>> + /*
>> + * Increment buf_used by 1 only if
>> + * it fits remaining space
>> + */
>> + buf_used += min(len + 1, size_rem);
>
> Hmm.. it seems scnprintf() cannot return a number greater than or equal
> to size_rem. Can we just do like this?
>
> buf_used += scnprintf(...) + 1;
>
> Thanks,
> Namhyung
Sure
I will address this change in next version
Thanks
Athira
>
>> + }
>> }
>> 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;
>> + info.encoding_desc = NULL;
>> +
>> + /* Check the remaining space */
>> + size_rem = sub_non_neg(sizeof(buf), buf_used);
>> + if (size_rem > 0) {
>> + info.encoding_desc = buf + buf_used;
>> + len = scnprintf(buf + buf_used, size_rem, "%.*s/%s/",
>> + (int)pmu_name_len, info.pmu_name, event->terms);
>> + buf_used += min(len + 1, size_rem);
>> + }
>> +
>> info.str = event->terms;
>> info.topic = event->topic;
>> info.deprecated = perf_pmu_alias__check_deprecated(pmu, event);
>> @@ -2254,7 +2278,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
