Re: [PATCH bpf-next v2 1/2] bpf: Reject packet dynptr slices in mutating helpers and subprograms

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] bpf: check_func_arg() rejects packet dynptrs on static subprograms
- [Critical] bpf: bpf_is_kfunc_pkt_changing() omits dynptr probe/user copy kfuncs
- [High] bpf: local dynptrs falsely treated as packet-changing in verifier
- [High] bpf: visit_insn() unconditionally marks dynptr writes as pkt-changing
- [Medium] bpf: check_func_arg() calls dynptr_reg_type() before slot validation
- [Medium] bpf: check_func_arg() changes error message in packet access selftest
--

commit f9f5c54032b2d6a161d1ff99ce27d3be9660ce7c
Author: Luxiao Xu <rakukuip@gmail.com>

bpf: Reject packet dynptr slices in mutating helpers and subprograms

This commit rejects passing packet pointers, packet dynptr slices, and
packet dynptrs to helpers, kfuncs, and subprograms that may change packet
data. It also models bpf_dynptr_copy() and bpf_dynptr_memset() as
packet-changing and uses a stack buffer fallback in bpf_dynptr_copy() for
non-linear skb destinations.

> diff --git a/kernel/bpf/cfg.c b/kernel/bpf/cfg.c
> index bce7b5abcfda3..9382d1d821291 100644
> --- a/kernel/bpf/cfg.c
> +++ b/kernel/bpf/cfg.c
> @@ -598,7 +598,8 @@ static int visit_insn(int t, struct bpf_verifier_env *env)
>  			 */
>  			if (ret == 0 && fp->might_sleep)
>  				mark_subprog_might_sleep(env, t);
> -			if (bpf_helper_changes_pkt_data(insn->imm))
> +			if (bpf_helper_changes_pkt_data(insn->imm) ||
> +			    insn->imm == BPF_FUNC_dynptr_write)
>  				mark_subprog_changes_pkt_data(env, t);
>  		} else if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) {
>  			struct bpf_call_arg_meta meta;

[Severity: High]
Won't marking any subprogram that calls bpf_dynptr_write(),
bpf_dynptr_copy(), or bpf_dynptr_memset() as changes_pkt_data during CFG
analysis cause regressions for programs that only write to ringbuf or
map-backed local dynptrs?

In visit_insn(), BPF_FUNC_dynptr_write is unconditionally marked via
mark_subprog_changes_pkt_data(). Similarly, for kfuncs in visit_insn():

	ret = bpf_fetch_kfunc_arg_meta(env, insn->imm, insn->off, &meta);
	...
	if (ret == 0 && bpf_is_kfunc_pkt_changing(&meta))
		mark_subprog_changes_pkt_data(env, t);

meta.dynptr_dst_type and meta.dynptr.type are zero at CFG analysis time, so
bpf_is_kfunc_pkt_changing() returns true via !type for every
bpf_dynptr_copy() and bpf_dynptr_memset() call regardless of the target
dynptr type.

At the end of bpf_check_cfg(), subprog_info[0].changes_pkt_data is copied
to env->prog->aux->changes_pkt_data:

	env->prog->aux->changes_pkt_data = env->subprog_info[0].changes_pkt_data;

This causes several issues when a program only writes to local
(bpf_dynptr_from_mem()) or ringbuf (bpf_ringbuf_reserve_dynptr()) dynptrs:

1. In sock_map_prog_attach_check(), attaching a BPF_SK_SKB_STREAM_PARSER
   program fails if prog->aux->changes_pkt_data is set:

	if (prog && attach_type == BPF_SK_SKB_STREAM_PARSER &&
	    prog->aux->changes_pkt_data)
		return -EINVAL;

   Stream parser programs cannot create writable skb dynptrs, yet any
   stream parser using bpf_dynptr_write(), bpf_dynptr_copy(), or
   bpf_dynptr_memset() on a local or ringbuf dynptr will now be rejected on
   attach.

2. In check_attach_btf_id(), a BPF_PROG_TYPE_EXT (freplace) extension
   program is rejected if prog->aux->changes_pkt_data is set while
   tgt_changes_pkt_data is false:

	if (prog->aux->changes_pkt_data && !tgt_changes_pkt_data) {
		bpf_log(log,
			"Extension program changes packet data, while original does not\n");
		return -EINVAL;
	}

3. A global subprogram that takes a packet pointer, packet slice, or
   read-only packet dynptr and copies data into a ringbuf or map dynptr via
   bpf_dynptr_write() or bpf_dynptr_copy() is marked changes_pkt_data, so
   check_func_arg() rejects passing the packet argument to it and
   check_func_call() invalidates caller packet pointers upon return.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index ec136cf377c13..d1a1a37e1f479 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
[ ... ]
> @@ -9616,6 +9619,24 @@ static enum bpf_access_type func_arg_access_type(enum bpf_arg_type arg_type)
>  	return BPF_READ | BPF_WRITE;
>  }
>  
> +static bool helper_changes_pkt_data(struct bpf_verifier_env *env,
> +				    enum bpf_func_id func_id,
> +				    const struct bpf_call_arg_meta *meta)
> +{
> +	if (bpf_helper_changes_pkt_data(func_id))
> +		return true;
> +
> +	if (func_id == BPF_FUNC_dynptr_write) {
> +		enum bpf_dynptr_type type = meta->dynptr_dst_type ?: meta->dynptr.type;
> +
> +		return type == BPF_DYNPTR_TYPE_SKB ||
> +		       type == BPF_DYNPTR_TYPE_SKB_META ||
> +		       (type == BPF_DYNPTR_TYPE_LOCAL && env->cur_state->curframe);
> +	}
> +
> +	return false;
> +}
> +
>  static int check_func_arg(struct bpf_verifier_env *env, u32 arg, u32 slot, u32 prev_slot,
>  			  struct bpf_call_arg_meta *meta,
>  			  int insn_idx)
> @@ -9903,6 +9924,22 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, u32 slot, u32 p
>  		enum bpf_access_type access_type;
>  		bool known_memory;
>  
> +		/*
> +		 * PTR_TO_PACKET gets passed as PTR_TO_MEM, preventing us from adjusting
> +		 * bounds tracking information.
> +		 */
> +		if (((is_subprog(meta) && meta->subprog_may_change_pkt) ||
> +		     (is_kfunc(meta) ? bpf_is_kfunc_pkt_changing(meta) :
> +				       helper_changes_pkt_data(env, meta->func_id, meta))) &&
> +		    (reg_is_pkt_pointer_any(reg) || reg_is_dynptr_slice_pkt(reg))) {
> +			verbose(env,
> +				"cannot pass packet pointer %s to %s(): function may change packet data\n",
> +				reg_arg_name(env, argno),
> +				is_subprog(meta) ? meta->func_name :
> +				(is_kfunc(meta) ? meta->func_name : func_id_name(meta->func_id)));
> +			return -EINVAL;
> +		}

[Severity: Medium]
Does intercepting reg_is_pkt_pointer_any(reg) here break the
test11_cls_unsuitable_helper_1 selftest in
tools/testing/selftests/bpf/progs/verifier_helper_packet_access.c?

That test passes PTR_TO_PACKET in R3 to bpf_skb_store_bytes() and asserts
on the error message from check_helper_mem_access():

	SEC("tc")
	__description("helper access to packet: test11, cls unsuitable helper 1")
	__failure __msg("function access to the packet")
	__naked void test11_cls_unsuitable_helper_1(void)

Because check_func_arg() now rejects reg_is_pkt_pointer_any(reg) before
check_helper_mem_access() runs, the verifier emits "cannot pass packet
pointer R3 to bpf_skb_store_bytes(): function may change packet data"
instead of "function access to the packet is not allowed".

> +
>  		/* The access to this pointer is only checked when we hit the
>  		 * next is_mem_size argument below.
>  		 */
[ ... ]
> @@ -10005,6 +10032,17 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, u32 slot, u32 p
>  		break;
>  	}
>  	case ARG_PTR_TO_DYNPTR: {
> +		enum bpf_dynptr_type dynptr_type = dynptr_reg_type(env, reg);
> +
> +		if (is_subprog(meta) && meta->subprog_may_change_pkt &&
> +		    (dynptr_type == BPF_DYNPTR_TYPE_SKB ||
> +		     dynptr_type == BPF_DYNPTR_TYPE_SKB_META ||
> +		     dynptr_type == BPF_DYNPTR_TYPE_XDP)) {
> +			verbose(env,
> +				"cannot pass packet dynptr %s to %s(): function may change packet data\n",
> +				reg_arg_name(env, argno), meta->func_name);
> +			return -EINVAL;
> +		}

[Severity: Critical]
Should this check be restricted to global subprograms rather than all
is_subprog(meta) calls?

In btf_prepare_func_args(), struct bpf_dynptr * parameters are translated
to ARG_PTR_TO_DYNPTR for both global and static subprograms. When
check_func_call() verifies a call to a static subprogram, it first invokes
btf_check_subprog_call() -> btf_check_func_arg_match() -> check_func_arg()
with meta->btf set and meta->subprog_may_change_pkt = sub->changes_pkt_data:

	err = btf_check_func_arg_match(env, subprog, btf);
	/* Compiler optimizations can remove arguments from static functions
	 * or mismatched type can be passed into a global function.
	 * In such cases mark the function as unreliable from BTF point of view.
	 */
	if (err)
		prog->aux->func_info_aux[subprog].unreliable = true;
	return err;

Because static subprograms are verified inline by check_static_func_call()
with the caller's exact stack state, passing a packet dynptr to a static
subprogram is safe. However, returning -EINVAL here marks the static
subprogram as BTF-unreliable.

Worse, if the static subprogram takes stack arguments (more than 5 argument
slots, so bpf_in_stack_arg_cnt(subprog) > 0), setting unreliable = true on
the first call causes btf_check_subprog_call() on any subsequent call to
return -EINVAL immediately at the top of the function before
btf_check_func_arg_match() can run check_outgoing_stack_args():

	if (prog->aux->func_info_aux[subprog].unreliable)
		return -EINVAL;

When check_static_func_call() then steps into the static subprogram and
executes an r11 stack argument read, check_stack_arg_read() dereferences
caller->stack_arg_regs[spi] without check_outgoing_stack_args() having
validated caller->out_stack_arg_cnt or allocated caller->stack_arg_regs:

	if (spi + 1 > bpf_in_stack_arg_cnt(subprog)) {
		verbose(env, "invalid read from stack arg off %d depth %d\n",
			off, bpf_in_stack_arg_cnt(subprog) * BPF_REG_SIZE);
		return -EACCES;
	}

	caller = vstate->frame[vstate->curframe - 1];
	arg = &caller->stack_arg_regs[spi];
	cur = vstate->frame[vstate->curframe];
	bpf_diag_mod_begin(env, &cur->regs[dst_regno], arg, BPF_DIAG_MOD_WRITE);
	cur->regs[dst_regno] = *arg;

If a second caller did not write outgoing stack arguments,
caller->stack_arg_regs is NULL (causing a NULL pointer dereference) or
smaller than spi (causing an out-of-bounds read).

---

[Severity: Medium]
Also in check_func_arg() under case ARG_PTR_TO_DYNPTR, is it safe to call
dynptr_reg_type(env, reg) before process_dynptr_func() validates the stack
slot?

dynptr_reg_type() looks up the stack slot and reads
spilled_ptr.dynptr.type directly from the anonymous union in struct
bpf_reg_state:

	spi = dynptr_get_spi(env, reg);
	if (spi < 0)
		return BPF_DYNPTR_TYPE_INVALID;
	state = bpf_func(env, reg);
	return bpf_stack_slot(state, spi)->spilled_ptr.dynptr.type;

If reg is not an initialized dynptr (for example, MEM_UNINIT or a stack
slot holding a non-dynptr spilled register where mem_size or another union
member happens to equal 3, 4, or 5), dynptr_reg_type() will read that union
field as BPF_DYNPTR_TYPE_SKB, BPF_DYNPTR_TYPE_XDP, or
BPF_DYNPTR_TYPE_SKB_META before process_dynptr_func() checks
is_dynptr_reg_valid_init(). In addition, if dynptr_get_spi() fails on a bad
stack offset, calling it here and then again inside process_dynptr_func()
will log duplicate error messages.

>  		if (is_bpf_dynptr_clone_kfunc(meta) &&
>  		    (arg_type & MEM_UNINIT)) {
>  			enum bpf_dynptr_type parent_type = meta->dynptr.type;
> @@ -12541,7 +12579,8 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
>  			return -EFAULT;
>  
>  		if (dynptr_type == BPF_DYNPTR_TYPE_SKB ||
> -		    dynptr_type == BPF_DYNPTR_TYPE_SKB_META)
> +		    dynptr_type == BPF_DYNPTR_TYPE_SKB_META ||
> +		    (dynptr_type == BPF_DYNPTR_TYPE_LOCAL && env->cur_state->curframe))
>  			/* this will trigger clear_all_pkt_pointers(), which will
>  			 * invalidate all dynptr slices associated with the skb
>  			 */
[ ... ]
> @@ -13592,7 +13635,19 @@ static bool is_kfunc_bpf_preempt_enable(struct bpf_call_arg_meta *meta)
>  
>  bool bpf_is_kfunc_pkt_changing(struct bpf_call_arg_meta *meta)
>  {
> -	return is_kfunc_call(meta, special_kfunc_list[KF_bpf_xdp_pull_data]);
> +	if (is_kfunc_call(meta, special_kfunc_list[KF_bpf_xdp_pull_data]))
> +		return true;
> +
> +	if (is_kfunc_call(meta, special_kfunc_list[KF_bpf_dynptr_copy]) ||
> +	    is_kfunc_call(meta, special_kfunc_list[KF_bpf_dynptr_memset])) {
> +		enum bpf_dynptr_type type = meta->dynptr_dst_type ?: meta->dynptr.type;
> +
> +		return !type || type == BPF_DYNPTR_TYPE_SKB ||
> +		       type == BPF_DYNPTR_TYPE_SKB_META ||
> +		       type == BPF_DYNPTR_TYPE_LOCAL;
> +	}
> +
> +	return false;
>  }

[Severity: High]
Why is BPF_DYNPTR_TYPE_LOCAL treated as packet-changing in
bpf_is_kfunc_pkt_changing(), helper_changes_pkt_data(), and
check_helper_call()?

1. In bpf_is_kfunc_pkt_changing(), returning true when type is
   BPF_DYNPTR_TYPE_LOCAL means that during check_kfunc_call():

	if (bpf_is_kfunc_pkt_changing(&meta)) {
		bpf_clear_RET_pkt_reg(env, &regs[BPF_REG_0]);
		clear_all_pkt_pointers(env);
	}

   any call to bpf_dynptr_copy() or bpf_dynptr_memset() writing into a
   map-backed local dynptr (created via bpf_dynptr_from_mem()) will clear
   all active PTR_TO_PACKET pointers and packet dynptr slices, even in the
   main program at curframe == 0.

2. In helper_changes_pkt_data() and check_helper_call() for
   BPF_FUNC_dynptr_write, the condition checks:

	(type == BPF_DYNPTR_TYPE_LOCAL && env->cur_state->curframe)

   However, global subprograms are verified via do_check_subprogs() ->
   do_check_common() with state->curframe == 0:

	state->curframe = 0;
	...
	init_func_state(env, state->frame[0],
			BPF_MAIN_FUNC /* callsite */,
			0 /* frameno */,
			subprog);

   Meanwhile, env->cur_state->curframe > 0 holds when verifying static
   subprograms and synchronous callbacks (such as bpf_loop() or
   bpf_for_each_map_elem()) inline via setup_func_entry(), where dynptrs
   keep their actual backing type and BPF_DYNPTR_TYPE_LOCAL is a genuine
   map-backed dynptr. Inside any static subprogram or callback, this check
   falsely rejects bpf_dynptr_write(&map_dynptr, 0, pkt_ptr_or_slice, len, 0)
   and falsely invalidates all packet pointers across all frames when
   writing to a map-backed dynptr.

   Furthermore, if check_func_arg() already prevents passing packet dynptrs
   to global subprograms that may change packet data, can a
   BPF_DYNPTR_TYPE_LOCAL dynptr in a mutating global subprogram ever be
   backed by a packet?

---

[Severity: Critical]
Are the dynptr probe and user copy kfuncs in kernel/trace/bpf_trace.c also
missing from bpf_is_kfunc_pkt_changing() and __bpf_dynptr_copy()?

Specifically, these eight kfuncs are registered in common_btf_ids for
BPF_PROG_TYPE_UNSPEC (and therefore reachable from networking programs):

- bpf_probe_read_user_dynptr()
- bpf_probe_read_kernel_dynptr()
- bpf_probe_read_user_str_dynptr()
- bpf_probe_read_kernel_str_dynptr()
- bpf_copy_from_user_dynptr()
- bpf_copy_from_user_str_dynptr()
- bpf_copy_from_user_task_dynptr()
- bpf_copy_from_user_task_str_dynptr()

They call __bpf_dynptr_copy() and __bpf_dynptr_copy_str() in
kernel/trace/bpf_trace.c, which fall back to calling __bpf_dynptr_write()
in a loop when bpf_dynptr_slice_rdwr() returns NULL (e.g., on a non-linear
skb dynptr):

	for (off = 0; off < size; off += chunk_sz) {
		chunk_sz = min_t(u64, sizeof(buf), size - off);
		err = copy_fn(buf, unsafe_src + off, chunk_sz, tsk);
		if (err)
			return err;
		err = __bpf_dynptr_write(dst, doff + off, buf, chunk_sz, 0);
		if (err)
			return err;
	}

When dst is a non-linear BPF_DYNPTR_TYPE_SKB dynptr, __bpf_dynptr_write()
calls __bpf_skb_store_bytes() -> bpf_try_make_writable(), which can
reallocate skb->head via pskb_expand_head(). Because
bpf_is_kfunc_pkt_changing() does not include these kfuncs,
check_kfunc_call() will not call clear_all_pkt_pointers(), leaving existing
PTR_TO_PACKET pointers and packet dynptr slices pointing to freed skb->head
memory, and allowing bpf_probe_read_kernel_dynptr() or
bpf_probe_read_kernel_str_dynptr() with size > 256 to read from a freed
packet slice on subsequent iterations of the loop.

>  
>  static u32 kfunc_abi_slots(const struct btf_func_model *fm)
>  {

[ ... ]

>
>
>

完美 (Wait—do not output anything after the blank line!)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792222148.git.rakukuip@gmail.com?part=1