Re: [PATCH bpf-next 15/20] libbpf: Collect .bpf_cleanup records and pass them to the kernel

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] bpf: undefined behavior when calling qsort with a NULL pointer
- [Low] bpf: array bounds violation via type-punned pointer arithmetic
--

commit 1e591245d7ec663b872eae44c8fa7f02cfd956a4
Author: Yonghong Song <yonghong.song@linux.dev>

libbpf: Collect .bpf_cleanup records and pass them to the kernel

This commit parses the compiler-emitted .bpf_cleanup section and hands
the resulting table to BPF_PROG_LOAD so the kernel can resolve exceptions.

> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index ea1c09fa37939..d7cb93a70f58d 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c

[ ... ]

> @@ -4847,6 +4885,214 @@ static struct bpf_program *find_prog_by_sec_insn(const struct bpf_object *obj,

[ ... ]

> +static int bpf_prog_collect_cleanup_info(struct bpf_object *obj,
> +					 struct bpf_program *prog)
> +{
> +	size_t i;
> +	int j;
> +
> +	for (i = 0; i < obj->cleanup_rec_cnt; i++) {
> +		struct cleanup_raw_rec *raw = &obj->cleanup_recs[i];
> +		struct bpf_program *owner = NULL;
> +		struct bpf_cleanup_info ci = {};
> +		__u32 *fields = (__u32 *)&ci;
> +		void *tmp;
> +
> +		for (j = 0; j < CLEANUP_REC_FIELDS; j++) {
> +			size_t idx = raw->insn_idx[j], final;
> +			struct bpf_program *p;

[ ... ]

> +			if (owner == prog) {
> +				final = raw->insn_idx[j] - prog->sec_insn_off;
> +			} else if (prog_is_subprog(obj, owner) && owner->sub_insn_off) {
> +				/* sub_insn_off is where this subprogram was
> +				 * appended to the main program being relocated;
> +				 * zero means it is not part of it.
> +				 */
> +				final = owner->sub_insn_off +
> +					raw->insn_idx[j] - owner->sec_insn_off;
> +			} else {
> +				owner = NULL;
> +				break;
> +			}
> +			fields[j] = final;

[Severity: Low]
Is it safe to cast a pointer to a struct field into an array pointer and access
subsequent members via pointer arithmetic in this loop?

The struct bpf_cleanup_info fields are populated by casting a pointer to the
struct to a __u32 * array and indexing into it in a loop.

According to C11 6.5.6, pointer arithmetic on a pointer to a scalar (the first
struct field) only allows accessing up to one element past it, making fields[1]
and fields[2] undefined behavior. 

This can trigger compiler warnings (-Warray-bounds) and potentially cause
aggressive optimizers to generate incorrect code.

> +		}
> +		if (!owner)
> +			continue;
> +
> +		tmp = libbpf_reallocarray(prog->cleanup_info, prog->cleanup_info_cnt + 1,
> +					  sizeof(*prog->cleanup_info));
> +		if (!tmp)
> +			return -ENOMEM;
> +		prog->cleanup_info = tmp;
> +		prog->cleanup_info_rec_size = sizeof(struct bpf_cleanup_info);
> +		prog->cleanup_info[prog->cleanup_info_cnt++] = ci;
> +
> +		pr_debug("prog '%s': cleanup region [%u,%u) -> landing pad %u\n",
> +			 prog->name, ci.begin_off, ci.end_off, ci.landing_pad_off);
> +	}
> +
> +	qsort(prog->cleanup_info, prog->cleanup_info_cnt,
> +	      sizeof(*prog->cleanup_info), cmp_cleanup_info);

[Severity: Medium]
Can this unconditionally call qsort() with a NULL base pointer?

If obj->cleanup_rec_cnt is 0, or if none of the records match the program,
prog->cleanup_info_cnt remains 0 and prog->cleanup_info will be NULL. 

Calling qsort() with a NULL pointer invokes undefined behavior under C11 7.1.4,
even if the element count is 0. This triggers UBSAN errors (nonnull attribute
violation in libc headers) and can fail runtime checks.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260917055645.3926444-1-yonghong.song@linux.dev?part=15