Commit 06880982 authored by Alexei Starovoitov's avatar Alexei Starovoitov
Browse files

Merge branch 'bpf-fix-unsound-scalar-forking-for-bpf_or' 

Daniel Wade says:

====================
bpf: Fix unsound scalar forking for BPF_OR

maybe_fork_scalars() unconditionally sets the pushed path dst register
to 0 for both BPF_AND and BPF_OR.  For AND this is correct (0 & K == 0),
but for OR it is wrong (0 | K == K, not 0).  This causes the verifier to
track an incorrect value on the pushed path, leading to a verifier/runtime
divergence that allows out-of-bounds map value access.

v4: Use block comment style for multi-line comments in selftests (Amery Hung)
    Add Reviewed-by/Acked-by tags
v3: Use single-line comment style in selftests (Alexei Starovoitov)
v2: Use push_stack(env, env->insn_idx, ...) to re-execute the insn
    on the pushed path (Eduard Zingerman)
====================

Link: https://patch.msgid.link/20260314021521.128361-1-danjwade95@gmail.com


Signed-off-by: default avatarAlexei Starovoitov <ast@kernel.org>
parents 1abd3feb 0ad1734c

kernel/bpf/verifier.c

+1 −1
Original line number Diff line number Diff line
@@ -15999,7 +15999,7 @@ static int maybe_fork_scalars(struct bpf_verifier_env *env, struct bpf_insn *ins 
	else

		return 0;









	branch = push_stack(env, env->insn_idx + 1, env->insn_idx, false);









	branch = push_stack(env, env->insn_idx, env->insn_idx, false);









	if (IS_ERR(branch))









		return PTR_ERR(branch);








































tools/testing/selftests/bpf/progs/verifier_bounds.c



+94
−0






























Original line number
Diff line number
Diff line







@@ -2037,4 +2037,98 @@ __naked void signed_unsigned_intersection32_case2(void *ctx)







	: __clobber_all);









}



















SEC("socket")









__description("maybe_fork_scalars: OR with constant rejects OOB")









__failure __msg("invalid access to map value")









__naked void or_scalar_fork_rejects_oob(void)









{









	asm volatile ("					\









	r1 = 0;						\









	*(u64*)(r10 - 8) = r1;				\









	r2 = r10;					\









	r2 += -8;					\









	r1 = %[map_hash_8b] ll;				\









	call %[bpf_map_lookup_elem];			\









	if r0 == 0 goto l0_%=;				\









	r9 = r0;					\









	r6 = *(u64*)(r9 + 0);				\









	r6 s>>= 63;					\









	r6 |= 8;					\









	/* r6 is -1 (current) or 8 (pushed) */		\









	if r6 s< 0 goto l0_%=;				\









	/* pushed path: r6 = 8, OOB for value_size=8 */	\









	r9 += r6;					\









	r0 = *(u8*)(r9 + 0);				\









l0_%=:	r0 = 0;						\









	exit;						\









"	:









	: __imm(bpf_map_lookup_elem),









	  __imm_addr(map_hash_8b)









	: __clobber_all);









}



















SEC("socket")









__description("maybe_fork_scalars: AND with constant still works")









__success __retval(0)









__naked void and_scalar_fork_still_works(void)









{









	asm volatile ("					\









	r1 = 0;						\









	*(u64*)(r10 - 8) = r1;				\









	r2 = r10;					\









	r2 += -8;					\









	r1 = %[map_hash_8b] ll;				\









	call %[bpf_map_lookup_elem];			\









	if r0 == 0 goto l0_%=;				\









	r9 = r0;					\









	r6 = *(u64*)(r9 + 0);				\









	r6 s>>= 63;					\









	r6 &= 4;					\









	/*						\









	 * r6 is 0 (pushed, 0&4==0) or 4 (current)	\









	 * both within value_size=8			\









	 */						\









	if r6 s< 0 goto l0_%=;				\









	r9 += r6;					\









	r0 = *(u8*)(r9 + 0);				\









l0_%=:	r0 = 0;						\









	exit;						\









"	:









	: __imm(bpf_map_lookup_elem),









	  __imm_addr(map_hash_8b)









	: __clobber_all);









}



















SEC("socket")









__description("maybe_fork_scalars: OR with constant allows in-bounds")









__success __retval(0)









__naked void or_scalar_fork_allows_inbounds(void)









{









	asm volatile ("					\









	r1 = 0;						\









	*(u64*)(r10 - 8) = r1;				\









	r2 = r10;					\









	r2 += -8;					\









	r1 = %[map_hash_8b] ll;				\









	call %[bpf_map_lookup_elem];			\









	if r0 == 0 goto l0_%=;				\









	r9 = r0;					\









	r6 = *(u64*)(r9 + 0);				\









	r6 s>>= 63;					\









	r6 |= 4;					\









	/*						\









	 * r6 is -1 (current) or 4 (pushed)		\









	 * pushed path: r6 = 4, within value_size=8	\









	 */						\









	if r6 s< 0 goto l0_%=;				\









	r9 += r6;					\









	r0 = *(u8*)(r9 + 0);				\









l0_%=:	r0 = 0;						\









	exit;						\









"	:









	: __imm(bpf_map_lookup_elem),









	  __imm_addr(map_hash_8b)









	: __clobber_all);









}



















char _license[] SEC("license") = "GPL";