libbpf: Drop redundant self-loop in emit_check_err

PATCH Applied Daniel Borkmann 4 participants 8 messages May 29 – Jun 1

Subsystems (2)

BPF libbpf

lore

Share & tools

lore

Daniel Borkmann

author maintainer

2026-05-29 09:41 UTC #0 lore raw

PATCH 1/3 libbpf: Drop redundant self-loop in emit_check_err

When the cleanup-label jump offset does not fit in s16, emit_check_err() sets gen->error = -ERANGE and then emits a BPF_JMP_IMM(BPF_JA, 0, 0, -1) self-loop.

The latter emit() is dead: gen->error is assigned on the preceding line, and emit() then bails out early in realloc_insn_buf() the moment gen->error is set, so the jump is never written into the instruction stream.

gen->error alone already marks the generation as failed. This is a follow-up to 7dd62566e0d1 ("libbpf: fix off-by-one in emit_signature_match jump offset") which removed the jump in emit_signature_match() but not in other locations.

Signed-off-by Daniel Borkmann <[email protected]>

---
 tools/lib/bpf/gen_loader.c | 1 -
 1 file changed, 1 deletion(-)

tools/lib/bpf/gen_loader.c +0 -1

index 9478b8f78f26..7b95ced7bcba 100644
@@ -293,7 +293,6 @@ static void emit_check_err(struct bpf_gen *gen)
 		emit(gen, BPF_JMP_IMM(BPF_JSLT, BPF_REG_7, 0, off));
 	} else {
 		gen->error = -ERANGE;
-		emit(gen, BPF_JMP_IMM(BPF_JA, 0, 0, -1));
 	}
 }

Signature · 1 line

2.43.0

Daniel Borkmann

author maintainer

2026-05-29 09:41 UTC #1 lore raw

PATCH 2/3 libbpf: Skip hash computation when loader generation failed

bpf_gen__finish() calls compute_sha_update_offsets() gated only on the gen_hash option, without first consulting gen->error. On a failed generation this is buggy: a failed realloc_data_buf() sets gen->data_start to NULL (leaving gen->data_cur dangling), so compute_sha_update_offsets() runs libbpf_sha256() over a NULL buffer with a bogus length; a failed realloc_insn_buf() likewise sets gen->insn_start to NULL and the hash immediates get patched through that NULL base.

The computed program is discarded in either case, since the following "if (!gen->error)" block does not publish opts->insns once an error is set. Thus, skip the hash pass when generation has already failed.

Fixes ea923080c145 ("libbpf: Embed and verify the metadata hash in the loader")

Reported-by sashiko <[email protected]>

Signed-off-by Daniel Borkmann <[email protected]>

---
 tools/lib/bpf/gen_loader.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

tools/lib/bpf/gen_loader.c +4 -4

index 7b95ced7bcba..3a6e1d53f287 100644
@@ -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;
 		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;
 }

Signature · 1 line

2.43.0

[email protected]

AI review

2026-05-29 10:21 UTC #2 lore raw

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

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 <[email protected]>

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.

Quoted 9 lines

tools/lib/bpf/gen_loader.c +0 -1

index 7b95ced7bcbaf..3a6e1d53f287a 100644
@@ -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))

tools/lib/bpf/gen_loader.c int bpf_gen__finish(struct bpf_gen *gen, int nr_progs, int nr_maps) · line 406

Quoted 10 lines

tools/lib/bpf/gen_loader.c +3 -3

-		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?

Quoted 9 lines

tools/lib/bpf/gen_loader.c +1 -0

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

Signature · 1 line

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2

Daniel Borkmann

author maintainer

2026-05-29 13:49 UTC #3 lore raw

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

On 5/29/26 12:21 PM, [email protected] wrote:
[...]

Quoted 32 lines

[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.

Quoted 6 lines

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?

Yeap, for the endianess we would need something like the below ... will cook a patch for it to address :

tools/lib/bpf/gen_loader.c +2 -2

index 19f9c05f8388..501508e5affa 100644
@@ -1071,7 +1071,7 @@ void bpf_gen__prog_load(struct bpf_gen *gen,
                 prog_idx, prog_type, insns_off, insn_cnt, license_off);
 
        /* convert blob insns to target endianness */
-       if (gen->swapped_endian) {
+       if (gen->swapped_endian && !gen->error) {
                struct bpf_insn *insn = gen->data_start + insns_off;
                int i;

@@ -1109,7 +1109,7 @@ void bpf_gen__prog_load(struct bpf_gen *gen,
                 sizeof(struct bpf_core_relo));
 
        /* convert all info blobs to target endianness */
-       if (gen->swapped_endian)
+       if (gen->swapped_endian && !gen->error)
                info_blob_bswap(gen, func_info, line_info, core_relos, load_attr);
 
        libbpf_strlcpy(attr.prog_name, prog_name, sizeof(attr.prog_name));

Daniel Borkmann

author maintainer

2026-05-29 09:41 UTC #4 lore raw

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

In reply to Daniel Borkmann

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.

Signed-off-by Daniel Borkmann <[email protected]>

---
 tools/lib/bpf/gen_loader.c | 2 ++
 1 file changed, 2 insertions(+)

tools/lib/bpf/gen_loader.c +2 -0

index 3a6e1d53f287..492360ca07ea 100644
@@ -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;
 	}
 	gen->insn_start = insn_start;

@@ -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;
 	}
 	gen->data_start = data_start;

Signature · 1 line

2.43.0

[email protected]

AI review

2026-05-29 10:20 UTC #5 lore raw

Re: [PATCH bpf-next 3/3] libbpf: Also reset {insn,data}_cur on realloc failure

This looks like a fix for a real bug where gen->insn_cur and gen->data_cur are left pointing into a freed buffer on -ENOMEM. Should it carry a Fixes: tag pointing at the commit that introduced the…

Quoted 9 lines

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.
Signed-off-by Daniel Borkmann <[email protected]>

This looks like a fix for a real bug where gen->insn_cur and gen->data_cur are left pointing into a freed buffer on -ENOMEM. Should it carry a Fixes: tag pointing at the commit that introduced the free()+NULL handling in realloc_insn_buf() and realloc_data_buf()?

The exact SHA could not be determined here, so it would need to be looked up from a full bpf-next/torvalds tree.

---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/26630703706

Daniel Borkmann

author maintainer

2026-05-29 11:22 UTC #6 lore raw

Re: [PATCH bpf-next 3/3] libbpf: Also reset {insn,data}_cur on realloc failure

Quoted 10 lines · On 5/29/26 12:20 PM, [email protected] wrote:

Quoted 9 lines
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.
Signed-off-by Daniel Borkmann <[email protected]>

Quoted 7 lines

This looks like a fix for a real bug where gen->insn_cur and gen->data_cur are left pointing into a freed buffer on -ENOMEM. Should it carry a Fixes: tag pointing at the commit that introduced the free()+NULL handling in realloc_insn_buf() and realloc_data_buf()?
The exact SHA could not be determined here, so it would need to be looked up from a full bpf-next/torvalds tree.

(I've added this given patch 2 in this series where stale data was used.)

[email protected]

CI Applied

2026-06-01 00:50 UTC #7 lore raw

In reply to Daniel Borkmann

This series was applied to bpf/bpf-next.git (master) by Alexei Starovoitov <[email protected]>:

Hello:

This series was applied to bpf/bpf-next.git (master) by Alexei Starovoitov <[email protected]>:

Quoted 9 lines · On Fri, 29 May 2026 11:41:16 +0200 you wrote:

When the cleanup-label jump offset does not fit in s16, emit_check_err() sets gen->error = -ERANGE and then emits a BPF_JMP_IMM(BPF_JA, 0, 0, -1) self-loop.
The latter emit() is dead: gen->error is assigned on the preceding line, and emit() then bails out early in realloc_insn_buf() the moment gen->error is set, so the jump is never written into the instruction stream.
[...]

Here is the summary with links:
- [bpf-next,1/3] libbpf: Drop redundant self-loop in emit_check_err

    https://git.kernel.org/bpf/bpf-next/c/e2c88266147f
  - [bpf-next,2/3] libbpf: Skip hash computation when loader generation failed
    https://git.kernel.org/bpf/bpf-next/c/3c5e2f1a8584
  - [bpf-next,3/3] libbpf: Also reset {insn,data}_cur on realloc failure
    https://git.kernel.org/bpf/bpf-next/c/d2f7bd066ed4

You are awesome, thank you!

Signature · 2 lines

Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html