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
> + }
> }
> 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
>
