Re: [PATCH bpf-next v2 39/43] bpf: Refuse the prologue for an extension that cannot take it

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] bpf: missing changes_pkt_data check and mark for extension prologue
- [Medium] bpf: bpf_convert_ctx_accesses() rejects extensions with no-op gen_prologue()
--

commit 2e35aa5750da9b2ad248d0380937d26b001912e7
Author: Alexei Starovoitov <ast@kernel.org>

bpf: Refuse the prologue for an extension that cannot take it

This commit adds ext_prog_takes_prologue() to reject BPF_PROG_TYPE_EXT
programs that write to packet data unless their first argument is
ARG_PTR_TO_CTX and any remaining register arguments are scalars.

> diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
> index fe59e3114d828..32911d7f494cb 100644
> --- a/kernel/bpf/fixups.c
> +++ b/kernel/bpf/fixups.c
> @@ -858,6 +858,25 @@ int bpf_opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env,
>  	return 0;
>  }
>  
> +/*
> + * The prologue takes the context from R1, and it can clobber R2-R5. An
> + * extension gets the arguments of the function that it replaces.
> + */
> +static bool ext_prog_takes_prologue(const struct bpf_verifier_env *env)
> +{
> +	const struct bpf_subprog_info *sub = &env->subprog_info[0];
> +	int i, cnt = min_t(int, sub->arg_slot_cnt, MAX_BPF_FUNC_REG_ARGS);
> +
> +	if (!cnt || sub->args[0].arg_type != ARG_PTR_TO_CTX)
> +		return false;
> +
> +	for (i = 1; i < cnt; i++)
> +		if (sub->args[i].arg_type != ARG_SCALAR)
> +			return false;
> +
> +	return true;
> +}
> +
>  /* convert load instructions that access fields of a context type into a
>   * sequence of instructions that access fields of the underlying structure:
>   *     struct __sk_buff    -> struct sk_buff
> @@ -909,6 +928,13 @@ int bpf_convert_ctx_accesses(struct bpf_verifier_env *env)
>  			verifier_bug(env, "gen_prologue is null");
>  			return -EFAULT;
>  		}
> +		if (env->seen_direct_write &&
> +		    env->prog->type == BPF_PROG_TYPE_EXT &&
> +		    !ext_prog_takes_prologue(env)) {
> +			verbose(env,
> +				"Extension writing packet data must take ctx and scalars only\n");
> +			return -EINVAL;
> +		}
>  		cnt = ops->gen_prologue(insn_buf, env->seen_direct_write,
>  					env->prog);

[Severity: Critical]
Can allowing an extension program to receive bpf_unclone_prologue() here
cause a use-after-free of skb->head in the target program when the
replaced global subprogram has changes_pkt_data == false?

When bpf_unclone_prologue() is prepended to an extension program, it emits
a call to BPF_FUNC_skb_pull_data:

net/core/filter.c:bpf_unclone_prologue() {
	...
	/* ret = bpf_skb_pull_data(skb, 0); */
	*insn++ = BPF_MOV64_REG(BPF_REG_6, BPF_REG_1);
	*insn++ = BPF_ALU64_REG(BPF_XOR, BPF_REG_2, BPF_REG_2);
	*insn++ = BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
			       BPF_FUNC_skb_pull_data);
	...
}

At runtime, if skb->cloned is set (for instance, when the target program
did not perform direct packet writes itself and thus received no
bpf_unclone_prologue() at entry, or cloned the skb via
bpf_clone_redirect() before the call), bpf_skb_pull_data() calls
bpf_try_make_writable() -> skb_ensure_writable() -> pskb_expand_head(),
which reallocates skb->head and frees the old buffer.

In the target program, check_func_call() only invalidates packet pointers
and skb dynptr slices in callee-saved registers (R6-R9) if the called
subprogram has changes_pkt_data set:

kernel/bpf/verifier.c:check_func_call() {
	...
	if (env->subprog_info[subprog].changes_pkt_data)
		clear_all_pkt_pointers(env);
	...
}

However, direct packet stores (such as *(u8 *)(long)skb->data = val) and
bpf_dynptr_slice_rdwr() set env->seen_direct_write during do_check_main(),
after bpf_check_cfg() and check_attach_btf_id() ->
bpf_check_attach_target() have already run. Neither sets
prog->aux->changes_pkt_data during bpf_check_cfg(), and
bpf_convert_ctx_accesses() does not set prog->aux->changes_pkt_data or
verify that the target subprogram has tgt_changes_pkt_data set (both for
initial attach in bpf_check_attach_target() and subsequent re-attach via
bpf_tracing_prog_attach()).

If the target program holds a packet pointer (PTR_TO_PACKET,
PTR_TO_PACKET_META, or a BPF_DYNPTR_TYPE_SKB slice) in R6-R9 across a call
to a global function sub(struct __sk_buff *skb) with changes_pkt_data ==
false, and an extension replaces sub() and writes to packet data, wouldn't
the caller dereference the stale pointer in R6-R9 into the freed skb->head
after the extension's prologue runs pskb_expand_head()?

[Severity: Medium]
Also, should the ext_prog_takes_prologue() check be performed only after
ops->gen_prologue() returns cnt > 0?

In kernel/bpf/verifier.c, may_access_direct_pkt_data() sets
env->seen_direct_write = true on BPF_WRITE for BPF_PROG_TYPE_XDP,
BPF_PROG_TYPE_SK_MSG, and BPF_PROG_TYPE_CGROUP_SOCKOPT in addition to the
skb program types:

kernel/bpf/verifier.c:may_access_direct_pkt_data() {
	switch (env->prog->type) {
	...
	case BPF_PROG_TYPE_SCHED_CLS:
	case BPF_PROG_TYPE_SCHED_ACT:
	case BPF_PROG_TYPE_XDP:
	case BPF_PROG_TYPE_LWT_XMIT:
	case BPF_PROG_TYPE_SK_SKB:
	case BPF_PROG_TYPE_SK_MSG:
		if (meta && !meta->btf && meta->func_id)
			return meta->fn->pkt_access;

		if (t == BPF_WRITE)
			env->seen_direct_write = true;
		return true;

	case BPF_PROG_TYPE_CGROUP_SOCKOPT:
		if (t == BPF_WRITE)
			env->seen_direct_write = true;

		return true;
	...
}

For XDP, SK_MSG, and CGROUP_SOCKOPT targets, ops->gen_prologue() is
bpf_noop_prologue() or cg_sockopt_get_prologue(), which return 0 when
env->seen_direct_write is set. Checking ext_prog_takes_prologue() before
checking cnt > 0 rejects extensions attached to those program types (for
example, an XDP extension taking a struct bpf_dynptr * and calling
bpf_dynptr_slice_rdwr(), or taking ctx in R2) even though no prologue is
generated for them.

>  		if (cnt >= INSN_BUF_SIZE) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261017163745.3341787-1-alexei.starovoitov@gmail.com?part=39