Re: [PATCH bpf-next] bpf: Refuse the prologue for an extension if callers keep packet pointers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] bpf: bypass of called_with_pkt_ptrs check in bpf_check_ext_prologue()
--

commit 83bf0cef185c53983f34422c2173a9a0850a623f
Author: Alexei Starovoitov <ast@kernel.org>

bpf: Refuse the prologue for an extension if callers keep packet pointers

This commit tracks whether global subprograms that do not change packet data
are called while packet pointers or packet dynptr slices are held. It
refuses loading or attaching an extension program with a prologue to such a
function unless the target program already has a prologue of its own.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 63fff641d9858..c31287d2231c0 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -22209,6 +22230,30 @@ static int check_unreliable_func_args(struct bpf_verifier_log *log, struct btf *
>  	return -EINVAL;
>  }
>  
> +/*
> + * The prologue of an extension gives a cloned skb a new head, unless the
> + * prologue of the target program has done that. The callers of the function
> + * that it replaces must not hold on to the old head.
> + */
> +int bpf_check_ext_prologue(struct bpf_verifier_log *log,
> +			   const struct bpf_prog *tgt_prog, u32 btf_id)
> +{
> +	const struct bpf_prog_aux *aux = tgt_prog->aux;
> +	int i;
> +
> +	if (aux->has_prologue)
> +		return 0;
> +
> +	for (i = 0; i < aux->func_info_cnt; i++)
> +		if (aux->func_info[i].type_id == btf_id &&
> +		    !aux->func_info_aux[i].called_with_pkt_ptrs)
> +			return 0;

[Severity: Critical]
Can this loop in bpf_check_ext_prologue() bypass the called_with_pkt_ptrs
check if two subprograms in aux->func_info[] share the same type_id?

During program load, prepare_btf_func() and check_btf_func() in
kernel/bpf/check_btf.c verify that each krecord[i].type_id references a
BTF_KIND_FUNC, but do not require type_id to be unique across subprograms:

kernel/bpf/check_btf.c:prepare_btf_func() {
	...
		/* check type_id */
		type = btf_type_by_id(btf, krecord[i].type_id);
		if (!type || !btf_type_is_func(type)) {
			verbose(env, "invalid type id %d in func info",
				krecord[i].type_id);
			goto err_free;
		}
	...
}

When resolving the attach target in bpf_check_attach_target(), the loop
stops at the first subprogram matching btf_id and attaches the extension
trampoline to tgt_prog->aux->func[subprog]->bpf_func:

kernel/bpf/verifier.c:bpf_check_attach_target() {
	...
		for (i = 0; i < aux->func_info_cnt; i++)
			if (aux->func_info[i].type_id == btf_id) {
				subprog = i;
				break;
			}
	...
}

However, in bpf_check_ext_prologue(), if the first subprogram matching
btf_id has called_with_pkt_ptrs set to true, the loop does not break and
continues scanning aux->func_info[]. If a subsequent subprogram shares the
same btf_id and has called_with_pkt_ptrs set to false,
bpf_check_ext_prologue() returns 0 even though the extension replaces the
first matching subprogram, allowing bpf_unclone_prologue() to reallocate
skb->head while callers of that first subprogram still hold packet pointers.

Should bpf_check_ext_prologue() stop at the first entry matching btf_id (or
take the resolved subprog index directly)?

> +
> +	bpf_log(log,
> +		"Extension writing packet data cannot replace a function whose callers keep packet pointers\n");
> +	return -EINVAL;
> +}

[ ... ]

>

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