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

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

[Alexei Starovoitov]

From: Alexei Starovoitov <ast@kernel.org>

An extension that writes to the packet of a tc, lwt_xmit or sk_skb program
starts with bpf_unclone_prologue().  If the skb is cloned, that gives it
a new head.

It does not count as changing packet data.  changes_pkt_data goes by the
helpers and kfuncs that a program calls, and whether it gets a prologue
is known only after it has been compared with its target.  So the extension
can replace a global function that leaves the packet alone, and the callers
of that function keep their packet pointers across the call.  These point
into the old head, which is left to the other clone.  Once that is freed,
by the reader of a packet socket for example:

  BUG: KASAN: slab-use-after-free in bpf_prog_53bc21b9810cadbc_t_read+0x2e8/0x36e
  Read of size 1 at addr ffff888107bfe73c by task tt11/102
  Freed by task 112:
   kfree+0x161/0x500
   skb_release_data+0x2dc/0x340
   consume_skb+0xa6/0x1c0
   packet_recvmsg+0x4b3/0xa20

Nothing is wrong if the target program writes to the packet too: its own
prologue has uncloned the skb, and that of the extension does nothing.
Nor if the callers hold no packet pointers, like a dispatcher, or if the
main function is replaced.

Note for every global function that leaves the packet alone if it is
called while packet pointers, or slices of the dynptr of a packet, are
held.  What the callers of a global function hold is not known while it
is verified, so the functions that it calls are taken for such as well.
Refuse an extension with a prologue for such a function, unless the target
program has a prologue, both when the extension is loaded and when it
is attached to another target.

Fixes: 7e40781cc8b7 ("bpf: verifier: Use target program's type for access verifications")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/bpf/sashiko-outbox-175849@kernel.org/
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 include/linux/bpf.h          |  2 ++
 include/linux/bpf_verifier.h |  2 ++
 kernel/bpf/fixups.c          |  6 +++++
 kernel/bpf/verifier.c        | 48 ++++++++++++++++++++++++++++++++++++
 4 files changed, 58 insertions(+)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index feb7de6483e3..54ae73e211f7 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1757,6 +1757,7 @@ struct bpf_func_info_aux {
 	bool unreliable;
 	bool called[BPF_FUNC_CTX_CNT];
 	bool verified[BPF_FUNC_CTX_CNT];
+	bool called_with_pkt_ptrs;
 };
 
 enum bpf_jit_poke_reason {
@@ -1894,6 +1895,7 @@ struct bpf_prog_aux {
 	bool jits_use_priv_stack;
 	bool priv_stack_requested;
 	bool changes_pkt_data;
+	bool has_prologue;
 	bool might_sleep;
 	bool kprobe_write_ctx;
 	struct {
diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index 7a72bb06fe56..951a90458d18 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -1381,6 +1381,8 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
 			    const struct bpf_prog *tgt_prog,
 			    u32 btf_id,
 			    struct bpf_attach_target_info *tgt_info);
+int bpf_check_ext_prologue(struct bpf_verifier_log *log,
+			   const struct bpf_prog *tgt_prog, u32 btf_id);
 void bpf_free_kfunc_btf_tab(struct bpf_kfunc_btf_tab *tab);
 
 int mark_chain_precision(struct bpf_verifier_env *env, int regno);
diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
index a8739fbd0349..673751b86aee 100644
--- a/kernel/bpf/fixups.c
+++ b/kernel/bpf/fixups.c
@@ -940,6 +940,12 @@ int bpf_convert_ctx_accesses(struct bpf_verifier_env *env)
 					"Extension writing packet data must take ctx and scalars only\n");
 				return -EINVAL;
 			}
+			if (env->prog->type == BPF_PROG_TYPE_EXT &&
+			    bpf_check_ext_prologue(&env->log,
+						   env->prog->aux->dst_prog,
+						   env->prog->aux->attach_btf_id))
+				return -EINVAL;
+			env->prog->aux->has_prologue = true;
 			new_prog = __bpf_patch_insn_data(env, 0, insn_buf, cnt,
 							 BPF_PATCH_MOVE_TARGET);
 			if (!new_prog)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 63fff641d985..c31287d2231c 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -10867,6 +10867,20 @@ static void clear_all_pkt_pointers(struct bpf_verifier_env *env)
 	}));
 }
 
+static bool holds_pkt_pointers(struct bpf_verifier_env *env)
+{
+	struct bpf_func_state *state;
+	struct bpf_reg_state *reg;
+	bool ret = false;
+
+	bpf_for_each_reg_in_vstate(env->cur_state, state, reg, ({
+		if (reg_is_pkt_pointer_any(reg) || reg_is_dynptr_slice_pkt(reg))
+			ret = true;
+	}));
+
+	return ret;
+}
+
 enum {
 	AT_PKT_END = -1,
 	BEYOND_PKT_END = -2,
@@ -11472,6 +11486,13 @@ static int check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
 			bpf_diag_mod_begin(env, &caller->regs[BPF_REG_0], NULL, BPF_DIAG_MOD_WRITE);
 		clear_caller_saved_regs(env, caller->regs);
 		invalidate_outgoing_stack_args(env, cur_func(env));
+		/*
+		 * What the callers of a global function hold is not known
+		 * while it is verified.
+		 */
+		if (!env->subprog_info[subprog].changes_pkt_data &&
+		    (state->frame[0]->subprogno || holds_pkt_pointers(env)))
+			sub_aux->called_with_pkt_ptrs = true;
 
 		/*
 		 * A non-void global function returns a 64-bit SCALAR_VALUE in
@@ -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;
+
+	bpf_log(log,
+		"Extension writing packet data cannot replace a function whose callers keep packet pointers\n");
+	return -EINVAL;
+}
+
 /*
  * An extension is verified as a program that runs under the RCU read lock. Does
  * the function that it is to replace?
@@ -22333,6 +22378,9 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
 					"Extension program changes packet data, while original does not\n");
 				return -EINVAL;
 			}
+			if (prog->aux->has_prologue &&
+			    bpf_check_ext_prologue(log, tgt_prog, btf_id))
+				return -EINVAL;
 
 			tgt_might_sleep = aux->func
 					  ? aux->func[subprog]->aux->might_sleep

base-commit: a6303535c2ad9d49598d52556183911f02287791
-- 
2.55.0

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

[sashiko-bot]

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

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

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 4919 bytes --]

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

TL;DR: bpf_check_ext_prologue() may check a different subprog than the one
attached when func_info type ids are duplicated, so a packet-writing freplace
could replace a function whose callers hold packet pointers, risking
use-after-free.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -22206,12 +22227,36 @@ static int check_unreliable_func_args(struct bpf_verifier_log *log, struct btf *
>
>  	bpf_log(log, "Cannot attach to static function %s: unreliable BTF and stack arguments\n",
>  		tname);
>  	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;
> +
> +	bpf_log(log,
> +		"Extension writing packet data cannot replace a function whose callers keep packet pointers\n");
> +	return -EINVAL;
> +}
> +

Can this loop pick a different subprog than the one that
bpf_check_attach_target() actually replaces?

The loop accepts the extension if any func_info entry with a matching
type_id has called_with_pkt_ptrs clear.  bpf_check_attach_target() instead
resolves the target to the first entry with that type_id:

    for (i = 0; i < aux->func_info_cnt; i++)
        if (aux->func_info[i].type_id == btf_id) {
            subprog = i;
            break;
        }
    ...
    addr = (long) tgt_prog->aux->func[subprog]->bpf_func;

The kernel does not require func_info type ids to be unique.
prepare_btf_func() only checks that each record's type is BTF_KIND_FUNC and
that insn_off increases, and check_btf_func() only checks that insn_off
matches the start of each subprog.  find_exception_callback_insn_off() also
tolerates duplicates.

So a target loaded with hand-made BTF could do this:

    - global subprogs A (func_info[1]) and B (func_info[2]) share the same
      FUNC type id X
    - main holds a packet pointer in R6 when it calls A, so A gets
      called_with_pkt_ptrs = true
    - main calls B while holding no packet pointers, so B stays false
    - an extension that writes the packet is loaded with attach_btf_id = X
      against this tc target, which has no prologue of its own

bpf_check_attach_target() resolves to subprog 1 (A).  bpf_check_ext_prologue()
skips i=1 and returns 0 at i=2 (B).  The extension then replaces A, and its
bpf_unclone_prologue() gives a cloned skb a new head while main still holds
R6 pointing into the old head, which is the use-after-free described in the
commit message.

The same mismatch exists on the attach path through
bpf_tracing_prog_attach() -> bpf_check_attach_target().

The other checks in bpf_check_attach_target() (func_has_rcu_read_lock(),
tgt_changes_pkt_data, tgt_might_sleep) all index by the resolved subprog.

Would it work to pass the subprog index that bpf_check_attach_target() has
already resolved into bpf_check_ext_prologue()?  Alternatively, deciding on
the first matching entry would also agree with the attach target lookup:

    if (aux->func_info[i].type_id == btf_id)
        return aux->func_info_aux[i].called_with_pkt_ptrs ? -EINVAL : 0;

Triggering this requires the privileges to load tc and freplace programs
(CAP_BPF plus CAP_NET_ADMIN, since BPF_PROG_TYPE_EXT is a net-admin program
type), but the verifier is still expected to enforce memory safety for those
loaders.

[ ... ]

> @@ -22330,12 +22375,15 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
>  					       : 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 (prog->aux->has_prologue &&
> +			    bpf_check_ext_prologue(log, tgt_prog, btf_id))
> +				return -EINVAL;
>
>  			tgt_might_sleep = aux->func
>  					  ? aux->func[subprog]->aux->might_sleep
>  					  : aux->might_sleep;
>  			if (prog->aux->might_sleep && !tgt_might_sleep) {
>  				bpf_log(log,

Here subprog is already resolved, but only btf_id is passed down to
bpf_check_ext_prologue(), which repeats the lookup.


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/39087416318