Re: [PATCH 09/11] perf bpf: Use scnprintf() in snprintf_hex() and synthesize_bpf_prog_name()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Buffer overflow in `hwmon_pmu__for_each_event` due to unsafe `snprintf` accumulation.
- [Critical] Buffer overflow in `perf_pmu__for_each_event` due to unsafe `snprintf` accumulation.
- [Medium] Missing validation for `btf__type_by_id()` return value in `synthesize_bpf_prog_name` leads to a NULL pointer dereference.
- [Medium] Missing bounds checking when accessing BPF function info and program tags allows out-of-bounds memory reads.
- [Medium] Leaked `map.btf` and `map.rodata` in `bpf_metadata_create()` on error path.
- [Medium] Leaked `info_node->metadata` when `perf_env__insert_bpf_prog_info` fails in `perf_env__add_bpf_info()`.
--

commit 36b9c4933bfd61cb63ed28852ac600bdb93eebb7
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf bpf: Use scnprintf() in snprintf_hex() and synthesize_bpf_prog_name()

This commit fixes buffer overflows by switching from snprintf() to scnprintf()
when accumulating formatted strings. This prevents writing past the end of
the buffer if truncation previously occurred.

> diff --git a/tools/perf/util/bpf-event.c b/tools/perf/util/bpf-event.c
> index a27945c279efb..2c09842469f1f 100644
> --- a/tools/perf/util/bpf-event.c
> +++ b/tools/perf/util/bpf-event.c
[ ... ]
> @@ -140,7 +140,7 @@ static int synthesize_bpf_prog_name(char *buf, int size,
>  	const struct btf_type *t;
>  	int name_len;
>  
> -	name_len = snprintf(buf, size, "bpf_prog_");
> +	name_len = scnprintf(buf, size, "bpf_prog_");
>  	name_len += snprintf_hex(buf + name_len, size - name_len,
>  				 prog_tags[sub_id], BPF_TAG_SIZE);
>  	if (btf) {

While reviewing this function, I noticed a couple of pre-existing issues
just below this diff hunk:

tools/perf/util/bpf-event.c:synthesize_bpf_prog_name() {
        if (btf) {
                finfo = func_infos + sub_id * info->func_info_rec_size;
                t = btf__type_by_id(btf, finfo->type_id);
                short_name = btf__name_by_offset(btf, t->name_off);
...

[Severity: Medium]
This isn't a bug introduced by this patch, but does calculating the finfo
pointer using the unvalidated info->func_info_rec_size from the untrusted
perf.data file allow out-of-bounds memory reads?

[Severity: Medium]
This is a pre-existing issue, but btf__type_by_id() can return NULL if
finfo->type_id is invalid or out of bounds. Will accessing t->name_off
immediately after cause a NULL pointer dereference here?


Additionally, I noticed a few other pre-existing issues in the perf tools
codebase during this review:

tools/perf/util/bpf-event.c:bpf_metadata_create() {
...
        metadata = bpf_metadata_alloc(info->nr_prog_tags, map.num_vars);
        if (!metadata)
                continue;
...

[Severity: Medium]
This isn't a bug introduced by this patch, but does this error path
permanently leak the memory allocated for map.btf and map.rodata by
skipping the bpf_metadata_free_map_data() cleanup?


tools/perf/util/bpf-event.c:perf_env__add_bpf_info() {
...
        if (!perf_env__insert_bpf_prog_info(env, info_node)) {
                pr_debug("%s: duplicate add bpf info request for id %u\n",
                         __func__, btf_id);
                free(info_linear);
                free(info_node);
                goto out;
        }
...

[Severity: Medium]
This is a pre-existing issue, but does freeing info_node here without freeing
info_node->metadata leak the dynamically allocated metadata?


tools/perf/util/hwmon_pmu.c:hwmon_pmu__for_each_event() {
...
        len += hwmon_pmu__describe_items(hwm, desc_buf + len, sizeof(desc_buf) - len,
                                         key, value->items, /*is_alarm=*/false);
...

[Severity: Critical]
This isn't a bug introduced by this patch, but it is a similar issue to the
ones being fixed.

Does accumulating the length from snprintf() without checking for truncation
cause sizeof(desc_buf) - len to wrap to a huge value, leading to a stack
buffer overflow in subsequent calls? 

Note that this issue was not fixed by the later patches in the series.


tools/perf/util/pmu.c:perf_pmu__for_each_event() {
...
        buf_used += snprintf(buf + buf_used, sizeof(buf) - buf_used,
                        "%.*s/%s/", (int)pmu_name_len, info.pmu_name, event->terms) + 1;
...

[Severity: Critical]
This is a pre-existing issue, but does accumulating the return value of
snprintf() into buf_used risk truncation wrapping? 

If buf_used exceeds sizeof(buf), sizeof(buf) - buf_used underflows, leading
to a stack buffer overflow on the next write. 

Note that this issue was also not fixed by the later patches in the series.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260607232925.1935819-1-acme@kernel.org?part=9