Re: [PATCH 08/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:
- [High] Missing NULL check for the result of `btf__type_by_id()` in `synthesize_bpf_prog_name` leading to a potential NULL pointer dereference.
- [High] The `synthesize_bpf_prog_name` function dereferences `func_infos` unconditionally if a BTF object is present, even if the BPF program does not have function info.
- [High] Unsafe use of snprintf in length accumulation sequences leads to size_t underflow and stack buffer overflows in `perf_pmu__for_each_event`.
- [High] The `bpil_offs_to_addr` function translates offsets to addresses without bounds checking, allowing out-of-bounds reads from malicious perf.data files.
- [Medium] Leaked `info_node->metadata` on the error path where `perf_env__insert_bpf_prog_info` fails.
- [Medium] Leaked BTF and rodata objects in `bpf_metadata_create` when `bpf_metadata_alloc` fails.
--

commit 7fa567954fb3de5f92865c25d5e803bee0ef956a
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

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

Both functions accumulate formatted output via snprintf. This patch switches
them to scnprintf() to prevent buffer size underflows and write overruns
when accumulating the output length.

> 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) {
> @@ -153,9 +153,10 @@ static int synthesize_bpf_prog_name(char *buf, int size,
>  			short_name = info->name;
>  	} else
>  		short_name = "F";
> -	if (short_name)
> -		name_len += snprintf(buf + name_len, size - name_len,
> -				     "_%s", short_name);
> +	if (short_name) {
> +		name_len += scnprintf(buf + name_len, size - name_len,
> +				      "_%s", short_name);
> +	}
>  	return name_len;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but could the surrounding code in
synthesize_bpf_prog_name() segfault if a BPF program does not have func_info?

In synthesize_bpf_prog_name(), if the program lacks function info, func_infos
becomes NULL. However, bpf_metadata_create() can pass the .rodata MAP's BTF,
satisfying the if (btf) check:

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);
    ...
}

Is it possible that finfo evaluates to an offset from NULL, leading to a
segfault when dereferencing finfo->type_id?

[Severity: High]
This is a pre-existing issue, but does btf__type_by_id() guarantee returning
a non-NULL pointer here?

If the provided type_id is out of bounds, t becomes NULL. Would the
immediate dereference of t->name_off then cause a segfault?

While reviewing these formatting string fixes, I noticed a few other
pre-existing regressions in the perf bpf and pmu utilities.

[Severity: High]
This isn't a bug introduced by this patch, but does the length accumulation
in perf_pmu__for_each_event() risk a stack buffer overflow?

tools/perf/util/pmu.c:perf_pmu__for_each_event() {
    ...
    buf_used += snprintf(buf + buf_used, sizeof(buf) - buf_used,
                         "%G%s", event->scale, event->unit) + 1;
    ...
}

If the snprintf output is truncated, buf_used exceeds the buffer size. The
subsequent call uses the remaining size, which would underflow to a large
positive value. Can this cause the next snprintf call to write out of
bounds?

[Severity: Medium]
This is a pre-existing issue, but does the error path in
perf_env__add_bpf_info() leak the metadata object?

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;
    }
    ...
}

If perf_env__insert_bpf_prog_info() fails, info_linear and info_node are
freed, but there is no call to bpf_metadata_free(info_node->metadata).
Should the allocated metadata be freed before jumping to the out label?

[Severity: Medium]
This is a pre-existing issue, but does the failure path for
bpf_metadata_alloc() in bpf_metadata_create() leak the map resources?

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

If bpf_metadata_read_map_data() succeeds earlier in the loop, map.btf and
map.rodata are allocated. If bpf_metadata_alloc() then fails, the loop
continues without calling bpf_metadata_free_map_data(). Should these
resources be cleaned up before continuing the loop?

[Severity: High]
This is a pre-existing issue, but does bpil_offs_to_addr() translate offsets
to addresses without bounds checking?

tools/perf/util/bpf-utils.c:bpil_offs_to_addr() {
    ...
    offs = bpf_prog_info_read_offset_u64(&info_linear->info,
                                         desc->array_offset);
    addr = offs + ptr_to_u64(info_linear->data);
    bpf_prog_info_set_offset_u64(&info_linear->info,
                                 desc->array_offset, addr);
    ...
}

The untrusted offset from info_linear->info is added to
ptr_to_u64(info_linear->data) to compute addr. If a crafted perf.data file
provides an offset exceeding data_len, could this allow arbitrary
out-of-bounds reads when the perf tool dereferences it?

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