Re: [PATCH bpf-next 2/3] libbpf: Skip hash computation when loader generation failed

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Unchecked `add_data()` failures lead to NULL pointer dereferences or heap buffer overflows in `bpf_gen__prog_load()` and `info_blob_bswap()` during endianness swapping.
--

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

libbpf: Skip hash computation when loader generation failed

This patch fixes a bug in bpf_gen__finish() where compute_sha_update_offsets()
was called on failed loader generation. It moves the hash computation inside
the !gen->error block to avoid running libbpf_sha256() over NULL buffers.

> diff --git a/tools/lib/bpf/gen_loader.c b/tools/lib/bpf/gen_loader.c
> index 7b95ced7bcbaf..3a6e1d53f287a 100644
> --- a/tools/lib/bpf/gen_loader.c
> +++ b/tools/lib/bpf/gen_loader.c
> @@ -397,13 +397,12 @@ int bpf_gen__finish(struct bpf_gen *gen, int nr_progs, int nr_maps)
>  			      blob_fd_array_off(gen, i));
>  	emit(gen, BPF_MOV64_IMM(BPF_REG_0, 0));
>  	emit(gen, BPF_EXIT_INSN());
> -	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) {
>  		struct gen_loader_opts *opts = gen->opts;
>  
> +		if (OPTS_GET(opts, gen_hash, false))
> +			compute_sha_update_offsets(gen);
> +
>  		opts->insns = gen->insn_start;

[Severity: Critical]
This isn't a regression introduced by this patch, but while reviewing this
file I noticed a pre-existing issue with how generation errors are handled in
bpf_gen__prog_load() and info_blob_bswap().

If realloc_data_buf() fails inside add_data(), it frees gen->data_start,
sets it to NULL, sets gen->error, and add_data() returns 0.

However, bpf_gen__prog_load() doesn't check gen->error before swapping
endianness:

tools/lib/bpf/gen_loader.c:bpf_gen__prog_load() {
    ...
    insns_off = add_data(gen, insns, insn_cnt * sizeof(struct bpf_insn));
    ...
    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);
    }
    ...
}

If add_data() failed, this evaluates to insn = NULL + 0. Can this cause a
NULL pointer dereference when accessing insn inside bpf_insn_bswap()?

Furthermore, if gen->error was previously set (e.g., from a failed
realloc_insn_buf) while gen->data_start is non-NULL, add_data() still returns
0. 

Since the loop evaluates insn = gen->data_start + 0, can this overwrite the
beginning of gen->data_start and write past the end of the buffer if it is
smaller than insn_cnt * sizeof(struct bpf_insn)?

Could we add a check for gen->error before proceeding with the endianness
swapping?

>  		opts->insns_sz = gen->insn_cur - gen->insn_start;
>  		opts->data = gen->data_start;
> @@ -418,6 +417,7 @@ int bpf_gen__finish(struct bpf_gen *gen, int nr_progs, int nr_maps)
>  				bpf_insn_bswap(insn++);
>  		}
>  	}
> +	pr_debug("gen: finish %s\n", errstr(gen->error));
>  	return gen->error;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260529094119.307264-1-daniel@iogearbox.net?part=2