Commit 37cce22d authored by Daniel Xu's avatar Daniel Xu Committed by Alexei Starovoitov
Browse files

bpf: verifier: Refactor helper access type tracking 



Previously, the verifier was treating all PTR_TO_STACK registers passed
to a helper call as potentially written to by the helper. However, all
calls to check_stack_range_initialized() already have precise access type
information available.

Rather than treat ACCESS_HELPER as a proxy for BPF_WRITE, pass
enum bpf_access_type to check_stack_range_initialized() to more
precisely track helper arguments.

One benefit from this precision is that registers tracked as valid
spills and passed as a read-only helper argument remain tracked after
the call.  Rather than being marked STACK_MISC afterwards.

An additional benefit is the verifier logs are also more precise. For
this particular error, users will enjoy a slightly clearer message. See
included selftest updates for examples.

Acked-by: default avatarEduard Zingerman <eddyz87@gmail.com>
Signed-off-by: default avatarDaniel Xu <dxu@dxuuu.xyz>
Link: https://lore.kernel.org/r/ff885c0e5859e0cd12077c3148ff0754cad4f7ed.1736886479.git.dxu@dxuuu.xyz


Signed-off-by: default avatarAlexei Starovoitov <ast@kernel.org>
parent 8ac412a3

kernel/bpf/verifier.c

+16 −29
Original line number Diff line number Diff line
@@ -5303,7 +5303,7 @@ enum bpf_access_src { 
static int check_stack_range_initialized(struct bpf_verifier_env *env,

					 int regno, int off, int access_size,

					 bool zero_size_allowed,

					 enum bpf_access_src type,

					 enum bpf_access_type type,

					 struct bpf_call_arg_meta *meta);









static struct bpf_reg_state *reg_state(struct bpf_verifier_env *env, int regno)








@@ -5336,7 +5336,7 @@ static int check_stack_read_var_off(struct bpf_verifier_env *env,







	/* Note that we pass a NULL meta, so raw access will not be permitted.









	 */









	err = check_stack_range_initialized(env, ptr_regno, off, size,









					    false, ACCESS_DIRECT, NULL);









					    false, BPF_READ, NULL);









	if (err)









		return err;
















@@ -7190,7 +7190,7 @@ static int check_stack_slot_within_bounds(struct bpf_verifier_env *env,







static int check_stack_access_within_bounds(









		struct bpf_verifier_env *env,









		int regno, int off, int access_size,









		enum bpf_access_src src, enum bpf_access_type type)









		enum bpf_access_type type)









{









	struct bpf_reg_state *regs = cur_regs(env);









	struct bpf_reg_state *reg = regs + regno;








@@ -7199,10 +7199,7 @@ static int check_stack_access_within_bounds(







	int err;









	char *err_extra;

















	if (src == ACCESS_HELPER)









		/* We don't know if helpers are reading or writing (or both). */









		err_extra = " indirect access to";









	else if (type == BPF_READ)









	if (type == BPF_READ)









		err_extra = " read from";









	else









		err_extra = " write to";








@@ -7420,7 +7417,7 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regn















	} else if (reg->type == PTR_TO_STACK) {









		/* Basic bounds checks. */









		err = check_stack_access_within_bounds(env, regno, off, size, ACCESS_DIRECT, t);









		err = check_stack_access_within_bounds(env, regno, off, size, t);









		if (err)









			return err;
















@@ -7640,13 +7637,11 @@ static int check_atomic(struct bpf_verifier_env *env, int insn_idx, struct bpf_i







static int check_stack_range_initialized(









		struct bpf_verifier_env *env, int regno, int off,









		int access_size, bool zero_size_allowed,









		enum bpf_access_src type, struct bpf_call_arg_meta *meta)









		enum bpf_access_type type, struct bpf_call_arg_meta *meta)









{









	struct bpf_reg_state *reg = reg_state(env, regno);









	struct bpf_func_state *state = func(env, reg);









	int err, min_off, max_off, i, j, slot, spi;









	char *err_extra = type == ACCESS_HELPER ? " indirect" : "";









	enum bpf_access_type bounds_check_type;









	/* Some accesses can write anything into the stack, others are









	 * read-only.









	 */








@@ -7657,18 +7652,10 @@ static int check_stack_range_initialized(







		return -EACCES;









	}

















	if (type == ACCESS_HELPER) {









		/* The bounds checks for writes are more permissive than for









		 * reads. However, if raw_mode is not set, we'll do extra









		 * checks below.









		 */









		bounds_check_type = BPF_WRITE;









	if (type == BPF_WRITE)









		clobber = true;









	} else {









		bounds_check_type = BPF_READ;









	}









	err = check_stack_access_within_bounds(env, regno, off, access_size,









					       type, bounds_check_type);

















	err = check_stack_access_within_bounds(env, regno, off, access_size, type);









	if (err)









		return err;
















@@ -7685,8 +7672,8 @@ static int check_stack_range_initialized(







			char tn_buf[48];

















			tnum_strn(tn_buf, sizeof(tn_buf), reg->var_off);









			verbose(env, "R%d%s variable offset stack access prohibited for !root, var_off=%s\n",









				regno, err_extra, tn_buf);









			verbose(env, "R%d variable offset stack access prohibited for !root, var_off=%s\n",









				regno, tn_buf);









			return -EACCES;









		}









		/* Only initialized buffer on stack is allowed to be accessed








@@ -7767,14 +7754,14 @@ static int check_stack_range_initialized(







		}

















		if (tnum_is_const(reg->var_off)) {









			verbose(env, "invalid%s read from stack R%d off %d+%d size %d\n",









				err_extra, regno, min_off, i - min_off, access_size);









			verbose(env, "invalid read from stack R%d off %d+%d size %d\n",









				regno, min_off, i - min_off, access_size);









		} else {









			char tn_buf[48];

















			tnum_strn(tn_buf, sizeof(tn_buf), reg->var_off);









			verbose(env, "invalid%s read from stack R%d var_off %s+%d size %d\n",









				err_extra, regno, tn_buf, i - min_off, access_size);









			verbose(env, "invalid read from stack R%d var_off %s+%d size %d\n",









				regno, tn_buf, i - min_off, access_size);









		}









		return -EACCES;









mark:








@@ -7849,7 +7836,7 @@ static int check_helper_mem_access(struct bpf_verifier_env *env, int regno,







		return check_stack_range_initialized(









				env,









				regno, reg->off, access_size,









				zero_size_allowed, ACCESS_HELPER, meta);









				zero_size_allowed, access_type, meta);









	case PTR_TO_BTF_ID:









		return check_ptr_to_btf_access(env, regs, regno, reg->off,









					       access_size, BPF_READ, -1);
































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



+3
−3






























Original line number
Diff line number
Diff line







@@ -192,7 +192,7 @@ int ringbuf_invalid_api(void *ctx)

















/* Can't add a dynptr to a map */









SEC("?raw_tp")









__failure __msg("invalid indirect read from stack")









__failure __msg("invalid read from stack")









int add_dynptr_to_map1(void *ctx)









{









	struct bpf_dynptr ptr;








@@ -210,7 +210,7 @@ int add_dynptr_to_map1(void *ctx)

















/* Can't add a struct with an embedded dynptr to a map */









SEC("?raw_tp")









__failure __msg("invalid indirect read from stack")









__failure __msg("invalid read from stack")









int add_dynptr_to_map2(void *ctx)









{









	struct test_info x;








@@ -398,7 +398,7 @@ int data_slice_missing_null_check2(void *ctx)







 * dynptr argument









 */









SEC("?raw_tp")









__failure __msg("invalid indirect read from stack")









__failure __msg("invalid read from stack")









int invalid_helper1(void *ctx)









{









	struct bpf_dynptr ptr;
































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



+1
−1






























Original line number
Diff line number
Diff line







@@ -26,7 +26,7 @@ __noinline int foo(const struct Big *big)







}



















SEC("cgroup_skb/ingress")









__failure __msg("invalid indirect access to stack")









__failure __msg("invalid read from stack")









int global_func10(struct __sk_buff *skb)









{









	const struct Small small = {.x = skb->len };
































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



+3
−2






























Original line number
Diff line number
Diff line







@@ -70,7 +70,8 @@ __naked int helper_uninit_to_misc(void *ctx)







		r1 = r10;				\









		r1 += -128;				\









		r2 = 32;				\









		call %[bpf_trace_printk];		\









		r3 = 0;					\









		call %[bpf_probe_read_user];		\









		/* Call to dummy() forces print_verifier_state(..., true),	\









		 * thus showing the stack state, matched by __msg().		\









		 */					\








@@ -79,7 +80,7 @@ __naked int helper_uninit_to_misc(void *ctx)







		exit;					\









"









		      :









		      : __imm(bpf_trace_printk),









		      : __imm(bpf_probe_read_user),









			__imm(dummy)









		      : __clobber_all);









}
































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



+1
−1






























Original line number
Diff line number
Diff line







@@ -28,7 +28,7 @@ __naked void stack_out_of_bounds(void)







SEC("socket")









__description("uninitialized stack1")









__success __log_level(4) __msg("stack depth 8")









__failure_unpriv __msg_unpriv("invalid indirect read from stack")









__failure_unpriv __msg_unpriv("invalid read from stack")









__naked void uninitialized_stack1(void)









{









	asm volatile ("					\