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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The generated BPF loader program uses a hardcoded 8-byte read (BPF_DW) to access the `excl_prog_sha` pointer field, which causes the BPF verifier to reject the loader on 32-bit architectures and silently bypasses the exclusivity check.
- [High] Branch target offset calculation is off-by-one, causing the jump target to land past the beginning of the cleanup block.
--

commit 51df41105aded76b418cec093cbbaaad09c03147
Author: KP Singh <kpsingh@kernel.org>

bpf, libbpf: reject non-exclusive metadata maps in the signed loader

This patch updates the BPF loader to verify that metadata maps are exclusive
while they are frozen. It places excl_prog_sha in struct bpf_map and emits a
check in gen_loader to reject the map with -EINVAL if excl_prog_sha is NULL.

> diff --git a/tools/lib/bpf/gen_loader.c b/tools/lib/bpf/gen_loader.c
> index cd5c2543f54d8..becfda5b90253 100644
> --- a/tools/lib/bpf/gen_loader.c
> +++ b/tools/lib/bpf/gen_loader.c
[ ... ]
> @@ -601,6 +601,23 @@ static void emit_signature_match(struct bpf_gen *gen)
>  			emit(gen, BPF_JMP_IMM(BPF_JA, 0, 0, -1));
>  		}
>  	}
> +
> +	/* 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)));

[Severity: High]
Does this 8-byte read (BPF_DW) cause issues on 32-bit architectures?

Since pointers are 4 bytes long on 32-bit systems, the verifier will reject
the loader program with an access size mismatch error (-EACCES) because it
expects the memory access size to exactly match the BTF-defined pointer
size.

Also, if the verifier check were bypassed, wouldn't an 8-byte read straddle
both the 4-byte excl_prog_sha and the adjacent 4-byte ops pointer? Since
ops is always non-zero, the resulting 64-bit value would never be 0,
causing the BPF_JEQ check below to silently fall through and bypass the
exclusivity check.

> +	off = -(gen->insn_cur - gen->insn_start - gen->cleanup_label) / 8 - 1;
> +	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));

[Severity: High]
Is the branch target offset calculation off by one?

The offset off is computed based on insn_cur before emitting the
BPF_MOV64_IMM instruction. Since emitting the BPF_MOV64_IMM instruction
advances insn_cur by 8 bytes, and BPF branch offsets are evaluated relative
to the instruction immediately following the branch itself, the actual jump
target evaluates to cleanup_label + 8 bytes (1 instruction) instead of the
cleanup_label itself.

This skips the first instruction of the cleanup block. If nr_progs > 0, it
skips loading the first FD into BPF_REG_1 and calls sys_close() with an
uninitialized or stale register (e.g., a map pointer), potentially closing a
random/incorrect file descriptor in the system. If nr_progs == 0 and
nr_maps > 0, it may jump into the middle of a 16-byte BPF_LD_IMM64
instruction, causing verifier rejection.

Does this need to be adjusted, even though it appears to be a pattern copied
from the loop above it?

> +	} else {
> +		gen->error = -ERANGE;
> +		emit(gen, BPF_JMP_IMM(BPF_JA, 0, 0, -1));
> +	}
>  }

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