Re: [PATCH bpf-next v3 11/16] bpf: Refuse the prologue for an extension that cannot take it

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] bpf: unclone prologue in ext_prog_takes_prologue() misses changes_pkt_data
--

commit 5782cfd239c6517a4947d82c921ca266a19c1bde
Author: Alexei Starovoitov <ast@kernel.org>

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

This commit rejects BPF extension programs that require an unclone prologue
unless their arguments consist of a context pointer in R1 followed only by
scalars.

> diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
> index fe59e3114d828..a8739fbd03495 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
> @@ -915,6 +934,12 @@ int bpf_convert_ctx_accesses(struct bpf_verifier_env *env)
>  			verifier_bug(env, "prologue is too long");
>  			return -EFAULT;
>  		} else if (cnt) {
> +			if (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;
> +			}
>  			new_prog = __bpf_patch_insn_data(env, 0, insn_buf, cnt,
>  							 BPF_PATCH_MOVE_TARGET);
>  			if (!new_prog)

[Severity: Critical]
When an extension program receives bpf_unclone_prologue() in
bpf_convert_ctx_accesses(), that prologue 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);
	...
}

Because this call is only synthesized during bpf_convert_ctx_accesses(),
after bpf_check_cfg() and check_attach_btf_id() -> bpf_check_attach_target()
have already run, an extension that writes to packet data via direct packet
access or bpf_dynptr_slice_rdwr() without calling a packet-changing helper
will have prog->aux->changes_pkt_data == false during target validation:

kernel/bpf/verifier.c:bpf_check_attach_target() {
	...
	tgt_changes_pkt_data = aux->func
			       ? aux->func[subprog]->aux->changes_pkt_data
			       : aux->changes_pkt_data;
	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;
	}
	...
}

If the target tc, lwt_xmit, or sk_skb program only reads packet data (or
clones the skb via bpf_clone_redirect() and reloads skb->data) and calls a
global subprogram taking (struct __sk_buff *skb, scalars...) with
changes_pkt_data == false, check_func_call() does not call
clear_all_pkt_pointers(env), allowing the caller to hold packet pointers or
read-only skb dynptr slices in callee-saved registers across the call.

When the extension replaces that global subprogram and runs on a cloned skb,
its bpf_unclone_prologue() calls bpf_skb_pull_data() ->
skb_ensure_writable() -> pskb_expand_head(), which reallocates and frees
skb->head while the caller still holds pointers into the old skb->head.

Could this cause a use-after-free in the caller after the extension returns,
and should an extension receiving a prologue also require the target function
to have changes_pkt_data set (and set prog->aux->changes_pkt_data for
subsequent BPF_LINK_CREATE attachments)?

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