[PATCH bpf v3 0/2] gen_loader fixes

[PATCH bpf v3 0/2] gen_loader fixes

[KP Singh]

  # v2 -> v3

  defer fixing 32 bit.

  Fix off-by-one in the signed loader's cleanup jump and reject
  non-exclusive metadata maps.

  Patch 1 fixes a pre-existing bug: the cleanup-label offset in
  emit_signature_match() does not account for the MOV insn emitted
  before the JMP.

  Patch 2 adds an exclusivity check so the loader bails if the
  metadata map lacks excl_prog_hash, preventing stale-hash attacks
  from concurrent map writers.

KP Singh (2):
  libbpf: fix off-by-one in emit_signature_match jump offset
  bpf, libbpf: reject non-exclusive metadata maps in the signed loader

 include/linux/bpf.h                           |  2 +-
 tools/lib/bpf/gen_loader.c                    | 20 +++++++++++++++++--
 .../selftests/bpf/progs/verifier_map_ptr.c    |  8 +++++---
 3 files changed, 24 insertions(+), 6 deletions(-)

-- 
2.53.0

[PATCH bpf v3 1/2] libbpf: fix off-by-one in emit_signature_match jump offset

[KP Singh]

The offset for the cleanup-label jump is computed before the MOV R7
instruction is emitted, but the JMP lands after it. Account for the
extra insn in the offset calculation (-2 instead of -1). Drop the
redundant self-loop in the else branch; gen->error = -ERANGE already
marks the generation as failed.

Fixes: fb2b0e290147 ("libbpf: Update light skeleton for signing")
Signed-off-by: KP Singh <kpsingh@kernel.org>
---
 tools/lib/bpf/gen_loader.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/lib/bpf/gen_loader.c b/tools/lib/bpf/gen_loader.c
index cd5c2543f54d..9478b8f78f26 100644
--- a/tools/lib/bpf/gen_loader.c
+++ b/tools/lib/bpf/gen_loader.c
@@ -592,13 +592,12 @@ static void emit_signature_match(struct bpf_gen *gen)
 		gen->hash_insn_offset[i] = gen->insn_cur - gen->insn_start;
 		emit2(gen, BPF_LD_IMM64_RAW_FULL(BPF_REG_3, 0, 0, 0, 0, 0));
 
-		off =  -(gen->insn_cur - gen->insn_start - gen->cleanup_label) / 8 - 1;
+		off = -(gen->insn_cur - gen->insn_start - gen->cleanup_label) / 8 - 2;
 		if (is_simm16(off)) {
 			emit(gen, BPF_MOV64_IMM(BPF_REG_7, -EINVAL));
 			emit(gen, BPF_JMP_REG(BPF_JNE, BPF_REG_2, BPF_REG_3, off));
 		} else {
 			gen->error = -ERANGE;
-			emit(gen, BPF_JMP_IMM(BPF_JA, 0, 0, -1));
 		}
 	}
 }
-- 
2.53.0

Re: [PATCH bpf v3 1/2] libbpf: fix off-by-one in emit_signature_match jump offset

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 1918 bytes --]

> Fixes: fb2b0e290147 ("libbpf: Update light skeleton for signing")

Daniel Borkmann reviewed v2 and pointed out that the Fixes tag
references the wrong commit. The commit fb2b0e290147 only modified
tools/lib/bpf/skel_internal.h and did not touch emit_signature_match.
He suggested using ea923080c145 ("libbpf: Embed and verify the
metadata hash in the loader") instead.

Reference: https://lore.kernel.org/bpf/a9345efb-b691-4fc1-9bad-d571bd324c25@iogearbox.net/

The search for the correct Fixes tag confirmed that fb2b0e290147 did
not introduce the emit_signature_match function or the buggy jump
offset calculation in tools/lib/bpf/gen_loader.c. The true origin
commit could not be determined in this repository's history.

Should the Fixes tag be updated to point to the commit that actually
introduced the emit_signature_match function with the buggy offset
calculation?

> diff --git a/tools/lib/bpf/gen_loader.c b/tools/lib/bpf/gen_loader.c
> index cd5c2543f54d..9478b8f78f26 100644
> --- a/tools/lib/bpf/gen_loader.c
> +++ b/tools/lib/bpf/gen_loader.c
> @@ -592,13 +592,12 @@ static void emit_signature_match(struct bpf_gen *gen)
>  		gen->hash_insn_offset[i] = gen->insn_cur - gen->insn_start;
>  		emit2(gen, BPF_LD_IMM64_RAW_FULL(BPF_REG_3, 0, 0, 0, 0, 0));
>
> -		off =  -(gen->insn_cur - gen->insn_start - gen->cleanup_label) / 8 - 1;
> +		off = -(gen->insn_cur - gen->insn_start - gen->cleanup_label) / 8 - 2;
>  		if (is_simm16(off)) {
>  			emit(gen, BPF_MOV64_IMM(BPF_REG_7, -EINVAL));
>  			emit(gen, BPF_JMP_REG(BPF_JNE, BPF_REG_2, BPF_REG_3, off));
>  		} else {
>  			gen->error = -ERANGE;
> -			emit(gen, BPF_JMP_IMM(BPF_JA, 0, 0, -1));
>  		}
>  	}
>  }


---
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/26314489328

[PATCH bpf v3 2/2] bpf, libbpf: reject non-exclusive metadata maps in the signed loader

[KP Singh]

The loader verifies map->sha against the metadata hash in its
instructions. map->sha is calculated when BPF_OBJ_GET_INFO_BY_FD is called
on the frozen map.

While the map is frozen, the loader must also ensure the map is
exclusive, as, without exclusivity, another BPF program with map access
can mutate the contents afterwards, so the check passes on stale data.

Place excl_prog_sha right after sha[] in struct bpf_map and have
gen_loader bail with -EINVAL when it is NULL, via BPF_PSEUDO_MAP_IDX at
fixed offset 32. The 8-byte read of the pointer field limits this to
64-bit kernels; gen_loader needs target pointer size tracking to emit
the right sized read on 32-bit (follow-up).

Fixes: fb2b0e290147 ("libbpf: Update light skeleton for signing")
Signed-off-by: KP Singh <kpsingh@kernel.org>
---
 include/linux/bpf.h                             |  2 +-
 tools/lib/bpf/gen_loader.c                      | 17 +++++++++++++++++
 .../selftests/bpf/progs/verifier_map_ptr.c      |  8 +++++---
 3 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index cd191c5fdb0a..11bec73db199 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -295,6 +295,7 @@ struct bpf_map_owner {
 
 struct bpf_map {
 	u8 sha[SHA256_DIGEST_SIZE];
+	char *excl_prog_sha;
 	const struct bpf_map_ops *ops;
 	struct bpf_map *inner_map_meta;
 #ifdef CONFIG_SECURITY
@@ -335,7 +336,6 @@ struct bpf_map {
 	atomic64_t sleepable_refcnt;
 	s64 __percpu *elem_count;
 	u64 cookie; /* write-once */
-	char *excl_prog_sha;
 };
 
 static inline const char *btf_field_type_name(enum btf_field_type type)
diff --git a/tools/lib/bpf/gen_loader.c b/tools/lib/bpf/gen_loader.c
index 9478b8f78f26..5077472b9fd2 100644
--- a/tools/lib/bpf/gen_loader.c
+++ b/tools/lib/bpf/gen_loader.c
@@ -600,6 +600,23 @@ static void emit_signature_match(struct bpf_gen *gen)
 			gen->error = -ERANGE;
 		}
 	}
+
+	/*
+	 * Reject if the metadata map is not exclusive. Without exclusivity
+	 * the cached map->sha[] verified above can be stale: another BPF
+	 * program with map access could have mutated the contents between
+	 * BPF_OBJ_GET_INFO_BY_FD and loader execution.
+	 */
+	emit2(gen, BPF_LD_IMM64_RAW_FULL(BPF_REG_1, BPF_PSEUDO_MAP_IDX,
+					 0, 0, 0, 0));
+	emit(gen, BPF_LDX_MEM(BPF_DW, BPF_REG_2, BPF_REG_1, SHA256_DWORD_SIZE * sizeof(__u64)));
+	off = -(gen->insn_cur - gen->insn_start - gen->cleanup_label) / 8 - 2;
+	if (is_simm16(off)) {
+		emit(gen, BPF_MOV64_IMM(BPF_REG_7, -EINVAL));
+		emit(gen, BPF_JMP_IMM(BPF_JEQ, BPF_REG_2, 0, off));
+	} else {
+		gen->error = -ERANGE;
+	}
 }
 
 void bpf_gen__record_attach_target(struct bpf_gen *gen, const char *attach_name,
diff --git a/tools/testing/selftests/bpf/progs/verifier_map_ptr.c b/tools/testing/selftests/bpf/progs/verifier_map_ptr.c
index e2767d27d8aa..a89d25ce4f91 100644
--- a/tools/testing/selftests/bpf/progs/verifier_map_ptr.c
+++ b/tools/testing/selftests/bpf/progs/verifier_map_ptr.c
@@ -70,13 +70,15 @@ __naked void bpf_map_ptr_write_rejected(void)
 	: __clobber_all);
 }
 
-/* The first element of struct bpf_map is a SHA256 hash of 32 bytes, accessing
- * into this array is valid. The opts field is now at offset 33.
+/*
+ * The first 32 bytes of struct bpf_map are the SHA256 hash; accessing into
+ * this array is valid. excl_prog_sha follows at offset 32, reading at
+ * offset 33 crosses into the pointer and must be rejected.
  */
 SEC("socket")
 __description("bpf_map_ptr: read non-existent field rejected")
 __failure
-__msg("cannot access ptr member ops with moff 32 in struct bpf_map with off 33 size 4")
+__msg("cannot access ptr member excl_prog_sha with moff 32 in struct bpf_map with off 33 size 4")
 __failure_unpriv
 __msg_unpriv("access is allowed only to CAP_PERFMON and CAP_SYS_ADMIN")
 __flag(BPF_F_ANY_ALIGNMENT)
-- 
2.53.0

Re: [PATCH bpf v3 2/2] bpf, libbpf: reject non-exclusive metadata maps in the signed loader

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 3151 bytes --]

> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index cd191c5fdb0a..11bec73db199 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -295,6 +295,7 @@ struct bpf_map_owner {
>
>  struct bpf_map {
>  	u8 sha[SHA256_DIGEST_SIZE];
> +	char *excl_prog_sha;

Daniel Borkmann suggested adding BUILD_BUG_ON checks to validate the
struct layout assumptions that gen_loader relies on. Specifically:

  BUILD_BUG_ON(offsetof(struct bpf_map, sha) != 0);
  BUILD_BUG_ON(offsetof(struct bpf_map, excl_prog_sha) != SHA256_DIGEST_SIZE);
  BUILD_BUG_ON(sizeof_field(struct bpf_map, excl_prog_sha) != sizeof(__u64));

Would this make the code more robust against future struct layout changes?

>  	const struct bpf_map_ops *ops;
>  	struct bpf_map *inner_map_meta;
>  #ifdef CONFIG_SECURITY

[ ... ]

> diff --git a/tools/lib/bpf/gen_loader.c b/tools/lib/bpf/gen_loader.c
> index 9478b8f78f26..5077472b9fd2 100644
> --- a/tools/lib/bpf/gen_loader.c
> +++ b/tools/lib/bpf/gen_loader.c
> @@ -600,6 +600,23 @@ static void emit_signature_match(struct bpf_gen *gen)
>  			gen->error = -ERANGE;
>  		}
>  	}
> +
> +	/*
> +	 * Reject if the metadata map is not exclusive. Without exclusivity
> +	 * the cached map->sha[] verified above can be stale: another BPF
> +	 * program with map access could have mutated the contents between
> +	 * BPF_OBJ_GET_INFO_BY_FD and loader execution.
> +	 */
> +	emit2(gen, BPF_LD_IMM64_RAW_FULL(BPF_REG_1, BPF_PSEUDO_MAP_IDX,
> +					 0, 0, 0, 0));
> +	emit(gen, BPF_LDX_MEM(BPF_DW, BPF_REG_2, BPF_REG_1, SHA256_DWORD_SIZE * sizeof(__u64)));

Will this BPF_DW (8-byte) read pass the BPF verifier on 32-bit
architectures?

Sashiko AI flagged this in v2, and you confirmed through testing on a
32-bit kernel that the verifier rejects this with 'cannot access ptr
member excl_prog_sha with moff 0 in struct (anon) with off 0 size 8'.
The issue is that __bpf_md_ptr doesn't emit padding in BTF; the kernel's
BTF represents the pointer member with a size of 4 bytes on 32-bit
systems, while btf_struct_walk() enforces exact size matching.

The commit message acknowledges this: 'The 8-byte read of the pointer
field limits this to 64-bit kernels; gen_loader needs target pointer
size tracking to emit the right sized read on 32-bit (follow-up).'

This means the security check will be silently bypassed on 32-bit
systems until the follow-up is implemented.

Could SHA256_DIGEST_SIZE be used here instead of 'SHA256_DWORD_SIZE *
sizeof(__u64)'? Daniel Borkmann pointed out that SHA256_DWORD_SIZE is
already SHA256_DIGEST_SIZE / sizeof(__u64), so the multiplication
essentially reconstructs SHA256_DIGEST_SIZE.

> +	off = -(gen->insn_cur - gen->insn_start - gen->cleanup_label) / 8 - 2;
> +	if (is_simm16(off)) {
> +		emit(gen, BPF_MOV64_IMM(BPF_REG_7, -EINVAL));
> +		emit(gen, BPF_JMP_IMM(BPF_JEQ, BPF_REG_2, 0, off));
> +	} else {
> +		gen->error = -ERANGE;
> +	}
>  }

[ ... ]

---
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/26314489328