Re: [PATCH AUTOSEL 6.18-5.15] libbpf: Also reset {insn,data}_cur on realloc failure

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing `gen->error` check in `bpf_gen__finish()` leads to a NULL pointer dereference in `compute_sha_update_offsets()` if a previous allocation failed.
- [High] Missing `gen->error` check in `bpf_gen__prog_load()` leads to a NULL pointer dereference during endianness swapping after an allocation failure.
--

commit d36df279d8dfc0fe65a516d12685c54faba8d9da
Author: Daniel Borkmann <daniel@iogearbox.net>

libbpf: Also reset {insn,data}_cur on realloc failure

realloc_insn_buf() as well as realloc_data_buf() free and NULL
gen->insn_start / gen->data_start on -ENOMEM but leave gen->insn_cur /
gen->data_cur pointing into the old, freed buffer. Just reset the
cursors to NULL alongside the base pointers so the freed state is
coherent.

Link: https://lore.kernel.org/r/20260529094119.307264-3-daniel@iogearbox.net

> diff --git a/tools/lib/bpf/gen_loader.c b/tools/lib/bpf/gen_loader.c
> index cd5c2543f54d8..8722ecc023819 100644
> --- a/tools/lib/bpf/gen_loader.c
> +++ b/tools/lib/bpf/gen_loader.c
> @@ -63,6 +63,7 @@ static int realloc_insn_buf(struct bpf_gen *gen, __u32 size)
>  		gen->error = -ENOMEM;
>  		free(gen->insn_start);
>  		gen->insn_start = NULL;
> +		gen->insn_cur = NULL;
>  		return -ENOMEM;
>  	}

[Severity: High]
This is a pre-existing issue, but missing a gen->error check in
bpf_gen__finish() leads to a NULL pointer dereference in
compute_sha_update_offsets() if this allocation fails.

If a buffer allocation fails here, gen->error is set and gen->insn_start is
freed and set to NULL. The libbpf loader relies on bpf_gen__finish() to return
the error at the end of the operation, but it unconditionally calls
compute_sha_update_offsets() before checking gen->error:

tools/lib/bpf/gen_loader.c:bpf_gen__finish() {
    ...
	if (OPTS_GET(gen->opts, gen_hash, false))
		compute_sha_update_offsets(gen);

	pr_debug("gen: finish %s\n", errstr(gen->error));
	if (!gen->error) {
    ...
}

Which then directly dereferences the NULL gen->insn_start:

tools/lib/bpf/gen_loader.c:compute_sha_update_offsets() {
    ...
	for (i = 0; i < SHA256_DWORD_SIZE; i++) {
		struct bpf_insn *insn =
			(struct bpf_insn *)(gen->insn_start + gen->hash_insn_offset[i]);
		sha_dw = tgt_endian(sha[i]);
		insn[0].imm = (__u32)sha_dw;
    ...
}

Should bpf_gen__finish() check gen->error before calling
compute_sha_update_offsets()?

[ ... ]

> @@ -86,6 +87,7 @@ static int realloc_data_buf(struct bpf_gen *gen, __u32 size)
>  		gen->error = -ENOMEM;
>  		free(gen->data_start);
>  		gen->data_start = NULL;
> +		gen->data_cur = NULL;
>  		return -ENOMEM;
>  	}

[Severity: High]
This is a pre-existing issue, but missing a gen->error check in
bpf_gen__prog_load() leads to a NULL pointer dereference during endianness
swapping if this allocation fails.

bpf_gen__prog_load() dynamically allocates buffers by calling add_data(), which
eventually calls realloc_data_buf(). If this allocation fails, gen->error is
set to -ENOMEM, gen->data_start is set to NULL, and add_data() returns 0.

However, bpf_gen__prog_load() ignores gen->error and continues execution.
If gen->swapped_endian is true, it attempts to iterate over instructions
stored in gen->data_start, directly dereferencing the NULL pointer:

tools/lib/bpf/gen_loader.c:bpf_gen__prog_load() {
    ...
	insns_off = add_data(gen, insns, insn_cnt * sizeof(struct bpf_insn));
    ...
	/* convert blob insns to target endianness */
	if (gen->swapped_endian) {
		struct bpf_insn *insn = gen->data_start + insns_off;
		int i;

		for (i = 0; i < insn_cnt; i++, insn++)
			bpf_insn_bswap(insn);
	}
    ...
}

Similarly, the info_blob_bswap() helper function also dereferences the
NULL gen->data_start buffer:

tools/lib/bpf/gen_loader.c:info_blob_bswap() {
	struct bpf_func_info *fi = gen->data_start + func_info;
    ...
	for (i = 0; i < load_attr->func_info_cnt; i++)
		bpf_func_info_bswap(fi++);
    ...
}

Should bpf_gen__prog_load() check gen->error and abort early?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831133314.4125787-44-sashal@kernel.org?part=1