[PATCH bpf-next v3 00/16] bpf: Memory safety, lifetime and locking fixes (followups)

[PATCH bpf-next v3 00/16] bpf: Memory safety, lifetime and locking fixes (followups)

[Alexei Starovoitov]

From: Alexei Starovoitov <ast@kernel.org>

The first 30 patches of v2 are applied. This is the rest of the series,
with what the reviews of v2 have found, and two fixes on top of patches
that are applied already. Most of the patches are from Josef.

Every bug was reproduced in a VM on top of bpf-next, on the kernel right
before and right after the patch that fixes it. The splats are in the
commit logs. That goes for what the reviews have found as well.

Programs that are attached to other programs:
  01 a tracing program with a target program passes for an iterator program
  02 extensions reach further into the context than their target
  04 extensions of functions that run sleepable
  11 extensions get the unclone prologue without an skb in R1

Lifetime of local storage, programs, sockets and devices:
  03 values of a dead owner are freed under sleepable programs
  05 the exception table of a program that still runs is not found
  06 BPF_PROG_TEST_RUN of sk_lookup uses the socket outside of RCU
  07 cpumap uses the device of a queued frame after it is gone

Dynptrs of packets, where a write can reallocate the head of the skb:
  08 kfuncs that write through a dynptr
  09 bpf_dynptr_write() in a global function
  10 a dynptr passed to a global function
  12 kfuncs that write through a dynptr do not unclone the skb
  13 the source of the write is a slice of the same skb
  14 overlapping source and destination in an skb
  15 overlapping source and destination in an xdp_buff
  16 slices as arguments of helpers that change the packet

Some of the fixes make the kernel refuse what it has accepted so far:

 - 01: a tracing program with BPF_TRACE_ITER cannot have a target program.
 - 02: an extension of a raw tracepoint, tracepoint or syscall program
   cannot be attached if it reaches further into the context than that
   program. A program that leaves the context to its extensions has to
   touch as much of it as they do.
 - 04: the main function of a sleepable program, and a global function that
   is called without the RCU read lock, cannot be extended. The
   summarization selftest is adjusted.
 - 08-10, 16: packet pointers and slices are invalidated in more places.
   That hits some programs which are harmless at run time. A program that
   keeps a packet pointer across a global function which only writes to a
   ringbuf dynptr is one example.
 - 11: an extension of a tc, lwt_xmit or sk_skb program that writes to
   packet data has to get the context as its first argument, and scalars
   beside it.

test_progs in its three flavors, test_maps and test_verifier show no new
failures. cpumask/test_insert_leave is flaky when the tests run in
parallel, with and without the series: its program does not check
is_test_task(), so any other fork in the system makes it fail.

veristat sees no difference for the 5691 programs of the selftests, nor for
the 466 programs of the sched_ext schedulers that BPF CI builds: same
verdicts, insns, states and stack depth.

v2 -> v3, after the reviews by sashiko and the BPF CI bot. The numbers in
brackets are those of v2:
 - rebased on top of patches 1-30
 - 01: new. A tracing program with attach_prog_fd got around [26].
 - 02: new. What [28] says of max_tp_access holds for max_ctx_offset too.
 - 03 [31]: map_free() took smap away from an element whose fields
   destroy() had left to the RCU Tasks Trace callback: they were leaked
 - 09 [37]: Fixes tag
 - 10 [38]: the dynptr of the bpf_user_ringbuf_drain() callback was taken
   for an argument of a global function
 - 11 [39]: look at the arguments only if there is a prologue. Extensions
   of XDP, sk_msg and sockopt programs were refused for no reason.
 - 14 [42], 16 [43]: commit logs
 - 15: new. XDP was missed in [42].
 - 05 [33], 07 [35]: no change. The symbols have to outlive the grace
   period, and an order 3 allocation per cpumap entry is fine.

Not in this version: the prologue of an extension can move the head of the
skb under the packet pointers of its caller, as sashiko has pointed out for
[39]. To count such an extension as one that changes packet data would
refuse what commit 7e40781cc8b7 ("bpf: verifier: Use target program's type
for access verifications") has made possible. That takes more thought.

v2: https://lore.kernel.org/bpf/20261017163745.3341787-1-alexei.starovoitov@gmail.com/
v1: https://lore.kernel.org/bpf/20261017012058.355787-1-alexei.starovoitov@gmail.com/

Alexei Starovoitov (6):
  bpf: Don't take a program with a target program for an iterator
  bpf: Don't let an extension reach further into the context than its
    target
  bpf: Don't take an extension for a function that runs sleepable
  bpf: Refuse the prologue for an extension that cannot take it
  bpf: Unclone the skb for kfuncs that write through a dynptr
  bpf: Use memmove() to store bytes in an xdp_buff

Josef Bacik (10):
  bpf: Keep values of a dead owner for sleepable programs
  bpf: Keep the symbols of a program until it has ended
  bpf: Use the socket of a sk_lookup test run under RCU
  bpf, cpumap: Hold the device of frames in the queue
  bpf: Drop packet pointers behind kfuncs writing an skb
  bpf: Let bpf_dynptr_write() mark a global function
  bpf: Don't take a dynptr argument for a local dynptr
  bpf: Follow the head if an skb is written from a slice
  bpf: Use memmove() to store bytes in an skb
  bpf: Refuse packet slices for packet-changing helpers

 include/linux/bpf.h                           |  2 +
 include/linux/bpf_verifier.h                  |  2 +
 kernel/bpf/bpf_iter.c                         |  6 +-
 kernel/bpf/bpf_local_storage.c                | 31 +++++--
 kernel/bpf/cfg.c                              |  4 +-
 kernel/bpf/core.c                             | 50 +++++++----
 kernel/bpf/cpumap.c                           | 35 ++++++--
 kernel/bpf/fixups.c                           | 25 ++++++
 kernel/bpf/map_in_map.c                       |  7 +-
 kernel/bpf/syscall.c                          | 18 +++-
 kernel/bpf/verifier.c                         | 90 +++++++++++++++++--
 net/bpf/test_run.c                            | 19 ++--
 net/core/filter.c                             | 28 +++++-
 .../selftests/bpf/prog_tests/summarization.c  | 21 ++---
 14 files changed, 265 insertions(+), 73 deletions(-)


base-commit: c058636f788f8751e419b407206516280226e568
-- 
2.55.0

[PATCH bpf-next v3 01/16] bpf: Don't take a program with a target program for an iterator

[Alexei Starovoitov]

From: Alexei Starovoitov <ast@kernel.org>

bpf_iter_prog_supported() goes by the name of the attach function.  Commit
7cc639dc45c5 ("bpf: Don't take an extension for an iterator program")
keeps out extensions, for which that is the name of a function of another
program.  A tracing program with BPF_TRACE_ITER that is loaded with
attach_prog_fd gets there as well: unlike for BPF_TRACE_RAW_TP,
bpf_check_attach_target() does not mind the target program.

If the function is called like that of an iterator target, bpf_iter_bpf_map
for example, its id in the BTF of the program is cached as the id of the
target, even if the load fails later.  Programs of that target which are
loaded already cannot be attached any more.  And the program itself can be
attached as an iterator of that target, with a context that the verifier
has taken from the prototype of that function:

  BUG: KASAN: stack-out-of-bounds in bpf_prog_66d4234e2e50f5b6_ts26_forged+0x12a/0x20b
  Read of size 8 at addr ffffc90000d9fc48 by task ts26/104

Refuse every program that has a target program.  That covers extensions.

Fixes: 15d83c4d7cef ("bpf: Allow loading of a bpf_iter program")
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/bpf_iter.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/bpf/bpf_iter.c b/kernel/bpf/bpf_iter.c
index 5fd376bd210b..de4334bfee5d 100644
--- a/kernel/bpf/bpf_iter.c
+++ b/kernel/bpf/bpf_iter.c
@@ -352,10 +352,10 @@ int bpf_iter_prog_supported(struct bpf_prog *prog)
 	int prefix_len = strlen(prefix);
 
 	/*
-	 * The attach function of an extension is a subprogram of the iterator
-	 * program that it extends, whatever its name is.
+	 * The attach function of a program that has a target program is a
+	 * function of that program, whatever its name is.
 	 */
-	if (prog->type == BPF_PROG_TYPE_EXT)
+	if (prog->aux->dst_prog)
 		return -EINVAL;
 
 	if (strncmp(attach_fname, prefix, prefix_len))
-- 
2.55.0

[PATCH bpf-next v3 02/16] bpf: Don't let an extension reach further into the context than its target

[Alexei Starovoitov]

From: Alexei Starovoitov <ast@kernel.org>

How large the context of a raw tracepoint, tracepoint or syscall program
is depends on what the program is attached to, or is run with.
bpf_probe_register(), perf_event_set_bpf_prog() and the test runs compare
that with max_ctx_offset of the program.

An extension of the program, or of one of its global functions, gets the
same context.  How far it reaches is noted in its own max_ctx_offset, which
nobody looks at.  On a tracepoint with one argument it reads the stack
behind the arguments, in a test run the memory behind the context, or
from NULL if there is none:

  BUG: KASAN: stack-out-of-bounds in bpf_prog_954b5d2051843629_ts28_ext_far+0x7e/0x113
  Read of size 8 at addr ffffc90000def7b8 by task ts28/104

  BUG: KASAN: slab-out-of-bounds in bpf_prog_954b5d2051843629_ts28_ext_far+0x7e/0x113
  Read of size 8 at addr ffff88810712c3e8 by task ts28/104

Refuse to attach an extension that reaches further than the program which
it extends.  A program that leaves the context to its extensions has to
touch as much of it as they do.

Fixes: be8704ff07d2 ("bpf: Introduce dynamic program extensions")
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/syscall.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index c412bcd1903f..d026ab779ce8 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -3851,6 +3851,19 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog,
 		err = -EINVAL;
 		goto out_unlock;
 	}
+	/*
+	 * The size of these contexts depends on what the program is attached
+	 * to, or run with. That is compared with max_ctx_offset of the program,
+	 * and nobody looks at the one of its extensions.
+	 */
+	if (prog->type == BPF_PROG_TYPE_EXT &&
+	    (tgt_prog->type == BPF_PROG_TYPE_RAW_TRACEPOINT ||
+	     tgt_prog->type == BPF_PROG_TYPE_TRACEPOINT ||
+	     tgt_prog->type == BPF_PROG_TYPE_SYSCALL) &&
+	    prog->aux->max_ctx_offset > tgt_prog->aux->max_ctx_offset) {
+		err = -EINVAL;
+		goto out_unlock;
+	}
 
 	err = bpf_link_prime(&link->link.link, &link_primer);
 	if (err)
-- 
2.55.0

[PATCH bpf-next v3 03/16] bpf: Keep values of a dead owner for sleepable programs

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

bpf_local_storage_destroy() frees the elements of an owner behind a plain
RCU grace period, on the ground that no program can refer to an owner that
goes away.  But the pointer that bpf_task_storage_get() and its like
return stays valid for the verifier after the program has released the
owner, or has left the bpf_rcu_read_lock() section it had the owner from.
A sleepable program is in no RCU read side critical section, so it can
write to the value after the owner has died and the element was freed:

  BUG: KASAN: slab-use-after-free in _copy_from_user+0x42/0xa0
  Write of size 8 at addr ffff8880074404a8 by task lsuaf/55

Free the element behind an RCU Tasks Trace grace period if sleepable code
may use the map.  A program that a map is bound to was loaded, and counted
in map->sleepable_refcnt, before it got a value from the live owner, and
stays counted until a grace period behind its last run.

A program can also get at a storage map by a lookup in an outer map.  Then
only the outer map is counted, its programs can come after the inner map
was put into it, and can hold on to the inner map after it was taken out.
So note in a map that it has been an inner map, and treat it like one with
sleepable users from then on.

The same goes for what the special fields of the value hold.  The page of
a uptr has to stay pinned, and a kptr which the program stores after the
owner has died has to be released.  Leave the fields, and the smap that it
takes to free them, to bpf_selem_free_trace_rcu(), as bpf_selem_unlink()
does.  bpf_local_storage_map_free() waits for that callback, and leaves
smap alone if it comes across such an element.

Elements of such maps now wait for sleepable programs that are running
when their owner dies.  Other maps are freed as before.

Fixes: dcf456c9a095 ("bpf: Fix usage of trace RCU in local storage.")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 include/linux/bpf.h            |  1 +
 kernel/bpf/bpf_local_storage.c | 31 +++++++++++++++++++++++++------
 kernel/bpf/map_in_map.c        |  7 +++++--
 3 files changed, 31 insertions(+), 8 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 22947fa192e4..a10af47978a4 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -341,6 +341,7 @@ struct bpf_map {
 	bool frozen; /* write-once; write-protected by freeze_mutex */
 	bool free_after_mult_rcu_gp;
 	bool free_after_rcu_gp;
+	bool was_inner_map;
 	atomic64_t sleepable_refcnt;
 	s64 __percpu *elem_count;
 	u64 cookie; /* write-once */
diff --git a/kernel/bpf/bpf_local_storage.c b/kernel/bpf/bpf_local_storage.c
index 49ea00c99390..b12fcec47ffa 100644
--- a/kernel/bpf/bpf_local_storage.c
+++ b/kernel/bpf/bpf_local_storage.c
@@ -352,7 +352,8 @@ int bpf_selem_unlink(struct bpf_local_storage_elem *selem)
 static void bpf_selem_unlink_nofail(struct bpf_local_storage_elem *selem,
 				    struct bpf_local_storage_map_bucket *b)
 {
-	bool in_map_free = !!b, free_storage = false;
+	bool in_map_free = !!b, free_storage = false, sleepable_users = false;
+	bool keep_fields = false;
 	struct bpf_local_storage *local_storage;
 	struct bpf_local_storage_map *smap;
 	unsigned long flags;
@@ -362,17 +363,26 @@ static void bpf_selem_unlink_nofail(struct bpf_local_storage_elem *selem,
 	smap = rcu_dereference_check(SDATA(selem)->smap, bpf_rcu_lock_held());
 
 	if (smap) {
+		/* A map that is freed, or NULL here, has no program left. */
+		sleepable_users = !in_map_free &&
+				  (atomic64_read(&smap->map.sleepable_refcnt) ||
+				   READ_ONCE(smap->map.was_inner_map));
 		b = b ? : select_bucket(smap, local_storage);
 		err = raw_res_spin_lock_irqsave(&b->lock, flags);
 		if (!err) {
 			/*
 			 * Call bpf_obj_free_fields() under b->lock to make sure it is done
 			 * exactly once for an selem. Safe to free special fields immediately
-			 * as no BPF program should be referencing the selem.
+			 * as no BPF program should be referencing the selem, unless it
+			 * is sleepable: see below.
 			 */
 			if (likely(selem_linked_to_map(selem))) {
 				hlist_del_init_rcu(&selem->map_node);
-				bpf_obj_free_fields(smap->map.record, SDATA(selem)->data);
+				if (sleepable_users)
+					keep_fields = true;
+				else
+					bpf_obj_free_fields(smap->map.record,
+							    SDATA(selem)->data);
 				unlink++;
 			}
 			raw_res_spin_unlock_irqrestore(&b->lock, flags);
@@ -387,7 +397,15 @@ static void bpf_selem_unlink_nofail(struct bpf_local_storage_elem *selem,
 		 * with selem1 as raw_res_spin_lock_irqsave() returns -ETIMEDOUT.
 		 */
 		WARN_ON_ONCE(err && in_map_free);
-		if (!err || in_map_free)
+		/*
+		 * A sleepable program can still use, and fill, the fields of a value
+		 * that it got before it let go of the owner. As for bpf_selem_unlink(),
+		 * bpf_selem_free_trace_rcu() frees them, and takes smap for that:
+		 * map_free() waits for this RCU read side critical section and then
+		 * for the callback. It must not take smap away from an selem that
+		 * destroy() has unlinked from the map.
+		 */
+		if (in_map_free ? err || unlink : !err && !keep_fields)
 			RCU_INIT_POINTER(SDATA(selem)->smap, NULL);
 	}
 
@@ -434,11 +452,12 @@ static void bpf_selem_unlink_nofail(struct bpf_local_storage_elem *selem,
 	 *
 	 * The owner that map_free() takes an selem from is alive: sleepable code, which
 	 * is in no RCU read side critical section, may be walking its list for another
-	 * map.
+	 * map.  And it can still use a value that it got before it let go of an owner
+	 * that destroy() is called for.
 	 */
 	if (unlink == 2 ||
 	    atomic_cmpxchg(&selem->state, SELEM_UNLINKED, SELEM_TOFREE) == SELEM_UNLINKED)
-		bpf_selem_free(selem, !in_map_free);
+		bpf_selem_free(selem, !in_map_free && !sleepable_users);
 
 	if (free_storage)
 		bpf_local_storage_free(local_storage, !in_map_free);
diff --git a/kernel/bpf/map_in_map.c b/kernel/bpf/map_in_map.c
index 5ee8aae7435b..0fa3f2ac0f99 100644
--- a/kernel/bpf/map_in_map.c
+++ b/kernel/bpf/map_in_map.c
@@ -110,10 +110,13 @@ void *bpf_map_fd_get_ptr(struct bpf_map *map,
 		return ERR_PTR(-ENOTSUPP);
 
 	inner_map_meta = map->inner_map_meta;
-	if (inner_map_meta->ops->map_meta_equal(inner_map_meta, inner_map))
+	if (inner_map_meta->ops->map_meta_equal(inner_map_meta, inner_map)) {
 		bpf_map_inc(inner_map);
-	else
+		/* Its users are those of the outer maps from now on */
+		WRITE_ONCE(inner_map->was_inner_map, true);
+	} else {
 		inner_map = ERR_PTR(-EINVAL);
+	}
 
 	return inner_map;
 }
-- 
2.55.0

[PATCH bpf-next v3 04/16] bpf: Don't take an extension for a function that runs sleepable

[Alexei Starovoitov]

From: Alexei Starovoitov <ast@kernel.org>

An extension cannot be sleepable, and it is verified as a program that
runs under the RCU read lock.  But it runs in the context of the function
that it replaces.  For a sleepable program, or a function that is called
from a bpf_wq or bpf_task_work callback, that is rcu_read_lock_trace().

The verifier lets such an extension use maps and pointers that rely on
RCU.  Its maps are not counted as maps with sleepable users either, so
that what it got from them is freed behind a plain RCU grace period:

  WARNING: net/core/sock_map.c:903 at __sock_hash_lookup_elem+0xe8/0xf0

  BUG: KASAN: slab-use-after-free in bpf_prog_b3bad285227d855b_ext_storage+0x245/0x297
  Write of size 8 at addr ffff888108717a50 by task fr/955

The verifier knows the contexts that a global function is called in.
Refuse to replace one that is called without the RCU read lock, and the
main function of a sleepable program.  A function that sleepable code only
calls under bpf_rcu_read_lock() can still be replaced.

The summarization test has replaced functions of sleepable programs by
ones that do not sleep.  Expect that to fail.

Fixes: 1e6c62a88215 ("bpf: Introduce sleepable BPF programs")
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/verifier.c                         | 20 ++++++++++++++++++
 .../selftests/bpf/prog_tests/summarization.c  | 21 +++++--------------
 2 files changed, 25 insertions(+), 16 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 2c812a5f1da8..d738ce6e7428 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -22151,6 +22151,21 @@ static int check_unreliable_func_args(struct bpf_verifier_log *log, struct btf *
 	return -EINVAL;
 }
 
+/*
+ * An extension is verified as a program that runs under the RCU read lock. Does
+ * the function that it is to replace?
+ */
+static bool func_has_rcu_read_lock(const struct bpf_prog *prog, int subprog)
+{
+	const struct bpf_func_info_aux *sub_aux = &prog->aux->func_info_aux[subprog];
+
+	if (!subprog)
+		return !prog->sleepable;
+
+	return !sub_aux->called[BPF_FUNC_CTX_SLEEPABLE] &&
+	       !sub_aux->called[BPF_FUNC_CTX_NO_RCU_LOCK];
+}
+
 int bpf_check_attach_target(struct bpf_verifier_log *log,
 			    const struct bpf_prog *prog,
 			    const struct bpf_prog *tgt_prog,
@@ -22247,6 +22262,11 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
 					"Cannot extend a writable raw tracepoint program\n");
 				return -EINVAL;
 			}
+			if (!func_has_rcu_read_lock(tgt_prog, subprog)) {
+				bpf_log(log,
+					"Cannot extend a function that runs without RCU read lock\n");
+				return -EINVAL;
+			}
 			tgt_changes_pkt_data = aux->func
 					       ? aux->func[subprog]->aux->changes_pkt_data
 					       : aux->changes_pkt_data;
diff --git a/tools/testing/selftests/bpf/prog_tests/summarization.c b/tools/testing/selftests/bpf/prog_tests/summarization.c
index 6951786044ca..3e4bcf071735 100644
--- a/tools/testing/selftests/bpf/prog_tests/summarization.c
+++ b/tools/testing/selftests/bpf/prog_tests/summarization.c
@@ -53,17 +53,6 @@ static void test_aux(const char *main_prog_name,
 	err = summarization_freplace__load(freplace);
 	print_verifier_log(log);
 
-	/* The might_sleep extension doesn't work yet as sleepable calls are not
-	 * allowed, but preserve the check in case it's supported later and then
-	 * this particular combination can be enabled.
-	 */
-	if (!strcmp("might_sleep", replacement) && err) {
-		ASSERT_HAS_SUBSTR(log, "sleepable helper bpf_copy_from_user#", "error log");
-		ASSERT_EQ(err, -EINVAL, "err");
-		test__skip();
-		goto out;
-	}
-
 	if (expect_load) {
 		ASSERT_OK(err, "summarization_freplace__load");
 	} else {
@@ -86,9 +75,8 @@ static void test_aux(const char *main_prog_name,
  * Also test a case when main subprogram itself is replaced and is a single
  * subprogram in a program.
  *
- * This holds for might_sleep programs. It is ok to replace might_sleep with
- * might_sleep and with does_not_sleep, but does_not_sleep cannot be replaced
- * with might_sleep.
+ * An extension is verified as a non-sleepable program, hence nothing that runs
+ * sleepable can be replaced.
  */
 void test_summarization_freplace(void)
 {
@@ -111,7 +99,7 @@ void test_summarization_freplace(void)
 		},
 	};
 	const char *pkt_err = "Extension program changes packet data";
-	const char *slp_err = "Extension program may sleep";
+	const char *slp_err = "Cannot extend a function that runs without RCU read lock";
 	struct {
 		const char *func;
 		bool has_side_effect;
@@ -136,7 +124,8 @@ void test_summarization_freplace(void)
 				if (!test__start_subtest(buf))
 					continue;
 				test_aux(mains[t][i].main, mains[t][i].to_be_replaced, replacements[t][j].func,
-					 mains[t][i].has_side_effect || !replacements[t][j].has_side_effect,
+					 t == 0 &&
+					 (mains[t][i].has_side_effect || !replacements[t][j].has_side_effect),
 					 replacements[t][j].err_msg);
 			}
 		}
-- 
2.55.0

[PATCH bpf-next v3 05/16] bpf: Keep the symbols of a program until it has ended

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

When the last reference to a program goes, __bpf_prog_put_noref() takes
its symbols out of the kallsyms tree and frees its line info at once, and
waits for the grace periods only with the rest.  But the program may still
be running: the link or the array it was called from is gone, the call is
not over.

search_bpf_extables() finds the exception table of a program by its
symbol.  A load that faults, and that the table is there to fix up, is
fatal from then on:

  Oops: general protection fault, probably for non-canonical address
  0xdead0000000006f8: 0000 [#1] SMP NOPTI
  RIP: 0010:0xffffffffc06008f6

It takes a program that is detached and closed on one CPU while it runs on
another.  Unwinders and bpf_prog_find_from_stack() do not know the program
any more either, and the stack dump of a BPF stream reads the freed line
info.

Free the line info behind the grace periods the program needs, as it is
done with the BTF that its text comes from.  Remove the symbols in
bpf_prog_free_deferred(), which runs behind them as well.  The symbols are
looked up under rcu_read_lock(), so the images and the program itself are
freed one more grace period later.  All that the program refers to is
released when it was before.

Fixes: 3dec541b2e63 ("bpf: Add support for BTF pointers to x86 JIT")
Fixes: d7c431cafcb4 ("bpf: Add dump_stack() analogue to print to BPF stderr")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 include/linux/bpf.h  |  1 +
 kernel/bpf/core.c    | 50 +++++++++++++++++++++++++++++++-------------
 kernel/bpf/syscall.c |  5 ++---
 3 files changed, 38 insertions(+), 18 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index a10af47978a4..feb7de6483e3 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1971,6 +1971,7 @@ struct bpf_prog_aux {
 	union {
 		struct work_struct work;
 		struct rcu_head	rcu;
+		struct rcu_work rwork;
 	};
 	struct bpf_stream *stream[2];
 	struct mutex st_ops_assoc_mutex;
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index e1216927373a..6e32db9777c2 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -3085,12 +3085,40 @@ static void bpf_free_used_btfs(struct bpf_prog_aux *aux)
 	kfree(aux->used_btfs);
 }
 
-static void bpf_prog_free_deferred(struct work_struct *work)
+static void bpf_prog_free_images(struct work_struct *work)
 {
 	struct bpf_prog_aux *aux;
 	int i;
 
+	aux = container_of(to_rcu_work(work), struct bpf_prog_aux, rwork);
+	for (i = 0; i < aux->real_func_cnt; i++) {
+		/* We can just unlink the subprog poke descriptor table as
+		 * it was originally linked to the main program and is also
+		 * released along with it.
+		 */
+		aux->func[i]->aux->poke_tab = NULL;
+		bpf_jit_free(aux->func[i]);
+	}
+	if (aux->real_func_cnt) {
+		kfree(aux->func);
+		bpf_prog_unlock_free(aux->prog);
+	} else {
+		bpf_jit_free(aux->prog);
+	}
+}
+
+static void bpf_prog_free_deferred(struct work_struct *work)
+{
+	struct bpf_prog_aux *aux;
+	bool ksyms;
+
 	aux = container_of(work, struct bpf_prog_aux, work);
+	/*
+	 * A prog that still runs when its last reference goes needs its symbols,
+	 * by which its exception table is found.  It has ended by now.
+	 */
+	ksyms = aux->real_func_cnt || !list_empty(&aux->ksym.lnode);
+	bpf_prog_kallsyms_del_all(aux->prog);
 #ifdef CONFIG_BPF_SYSCALL
 	bpf_free_kfunc_btf_tab(aux->kfunc_btf_tab);
 #endif
@@ -3109,20 +3137,12 @@ static void bpf_prog_free_deferred(struct work_struct *work)
 #endif
 	if (aux->dst_trampoline)
 		bpf_trampoline_put(aux->dst_trampoline);
-	for (i = 0; i < aux->real_func_cnt; i++) {
-		/* We can just unlink the subprog poke descriptor table as
-		 * it was originally linked to the main program and is also
-		 * released along with it.
-		 */
-		aux->func[i]->aux->poke_tab = NULL;
-		bpf_jit_free(aux->func[i]);
-	}
-	if (aux->real_func_cnt) {
-		kfree(aux->func);
-		bpf_prog_unlock_free(aux->prog);
-	} else {
-		bpf_jit_free(aux->prog);
-	}
+	/* The symbols are looked up under rcu_read_lock() */
+	INIT_RCU_WORK(&aux->rwork, bpf_prog_free_images);
+	if (ksyms)
+		queue_rcu_work(system_percpu_wq, &aux->rwork);
+	else
+		bpf_prog_free_images(&aux->rwork.work);
 }
 
 void bpf_prog_free(struct bpf_prog *fp)
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index d026ab779ce8..849bc9136299 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2461,6 +2461,8 @@ static void __bpf_prog_put_rcu(struct rcu_head *rcu)
 	struct bpf_prog_aux *aux = container_of(rcu, struct bpf_prog_aux, rcu);
 
 	btf_put(aux->btf);
+	kvfree(aux->jited_linfo);
+	kvfree(aux->linfo);
 	kvfree(aux->func_info);
 	kfree(aux->func_info_aux);
 	free_uid(aux->user);
@@ -2485,10 +2487,7 @@ static void __bpf_prog_put_rcu_tasks(struct rcu_head *rcu)
 
 static void __bpf_prog_put_noref(struct bpf_prog *prog, bool deferred)
 {
-	bpf_prog_kallsyms_del_all(prog);
 	module_put(prog->aux->mod);
-	kvfree(prog->aux->jited_linfo);
-	kvfree(prog->aux->linfo);
 	kfree(prog->aux->kfunc_tab);
 	kfree(prog->aux->ctx_arg_info);
 	if (prog->aux->attach_btf)
-- 
2.55.0

[PATCH bpf-next v3 06/16] bpf: Use the socket of a sk_lookup test run under RCU

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

bpf_sk_lookup_assign() accepts only sockets which are freed by RCU, and
takes no reference. bpf_prog_test_run_sk_lookup() looks at sk_reuseport of
the selected socket and generates its cookie, which is a write, behind
bpf_test_timer_leave(), that is outside of the RCU read side critical
section in which the program ran. If the socket is closed in the meantime
it may be gone.

Do both before the section ends.

Fixes: 7c32e8f8bc33 ("bpf: Add PROG_TEST_RUN support for sk_lookup programs")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 net/bpf/test_run.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index d73cd247f8cd..3043a7393c29 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -1656,20 +1656,19 @@ int bpf_prog_test_run_sk_lookup(struct bpf_prog *prog, const union bpf_attr *kat
 		ctx.selected_sk = NULL;
 		retval = BPF_PROG_SK_LOOKUP_RUN_ARRAY(progs, ctx, bpf_prog_run);
 	} while (bpf_test_timer_continue(&t, 1, repeat, &ret, &duration));
-	bpf_test_timer_leave(&t);
-
-	if (ret < 0)
-		goto out;
 
+	/* Nothing but RCU keeps the selected socket */
 	user_ctx->cookie = 0;
-	if (ctx.selected_sk) {
-		if (ctx.selected_sk->sk_reuseport && !ctx.no_reuseport) {
+	if (!ret && ctx.selected_sk) {
+		if (ctx.selected_sk->sk_reuseport && !ctx.no_reuseport)
 			ret = -EOPNOTSUPP;
-			goto out;
-		}
-
-		user_ctx->cookie = sock_gen_cookie(ctx.selected_sk);
+		else
+			user_ctx->cookie = sock_gen_cookie(ctx.selected_sk);
 	}
+	bpf_test_timer_leave(&t);
+
+	if (ret < 0)
+		goto out;
 
 	ret = bpf_test_finish(kattr, uattr, NULL, NULL, 0, 0, retval, duration);
 	if (!ret)
-- 
2.55.0

[PATCH bpf-next v3 07/16] bpf, cpumap: Hold the device of frames in the queue

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

A frame that is redirected to another CPU waits in the queue of the cpumap
entry with a plain pointer to the device it came from, and an skb from
generic XDP with skb->dev.  Nothing keeps that device.  Closing and
unregistering it waits for its NAPI and for RCU, but not for the kthread
of the entry.  If the kthread is late by more than that, since its CPU is
busy with something of a higher priority, it builds skbs for a device that
has been freed and hands them to the stack:

  BUG: KASAN: use-after-free in dev_gro_receive+0x1f51/0x2b30
  Read of size 8 at addr ffff88800a7340b0 by task cpumap/1/map:1/54

Take a reference to the device for each entry of the queue.  The kthread
cannot drop it at the end of the batch, since its GRO node may keep the skb
for longer.  Let it note the devices and drop the references behind a full
GRO flush, which it does whenever the queue is empty, and from now on also
when it has noted about 2048 of them.

A device whose frames are stuck in a queue is now unregistered only when
the kthread has got to them.  RTNL is not held meanwhile, but if the device
goes with its network namespace, the cleanup of all namespaces waits.

Fixes: 1c601d829ab0 ("bpf: cpumap xdp_buff to skb conversion and allocation")
Fixes: 11941f8a8536 ("bpf: cpumap: Implement generic cpumap")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/cpumap.c | 35 ++++++++++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 7 deletions(-)

diff --git a/kernel/bpf/cpumap.c b/kernel/bpf/cpumap.c
index 5e59ab896f05..eb87131b1bef 100644
--- a/kernel/bpf/cpumap.c
+++ b/kernel/bpf/cpumap.c
@@ -45,6 +45,7 @@
  */
 
 #define CPU_MAP_BULK_SIZE 8  /* 8 == one cacheline on 64-bit archs */
+#define CPU_MAP_HELD_DEVS 2048
 struct bpf_cpu_map_entry;
 struct bpf_cpu_map;
 
@@ -74,6 +75,13 @@ struct bpf_cpu_map_entry {
 
 	struct completion kthread_running;
 	struct rcu_work free_work;
+
+	/*
+	 * Every frame and skb in the queue holds a reference to its device.
+	 * The kthread keeps it until nothing is left in GRO.
+	 */
+	struct net_device *held[CPU_MAP_HELD_DEVS];
+	u32 nr_held;
 };
 
 struct bpf_cpu_map {
@@ -129,9 +137,11 @@ static void __cpu_map_ring_cleanup(struct ptr_ring *ring)
 		WARN_ON_ONCE(1);
 		if (unlikely(__ptr_test_bit(0, &ptr))) {
 			__ptr_clear_bit(0, &ptr);
+			dev_put(((struct sk_buff *)ptr)->dev);
 			kfree_skb(ptr);
 			continue;
 		}
+		dev_put(((struct xdp_frame *)ptr)->dev_rx);
 		xdp_return_frame(ptr);
 	}
 }
@@ -311,7 +321,7 @@ static int cpu_map_kthread_run(void *data)
 		void *frames[CPUMAP_BATCH];
 		void *skbs[CPUMAP_BATCH];
 		u32 i, n, m;
-		bool empty;
+		bool empty, full;
 
 		/* Release CPU reschedule checks */
 		if (__ptr_ring_empty(rcpu->queue)) {
@@ -344,10 +354,12 @@ static int cpu_map_kthread_run(void *data)
 				struct sk_buff *skb = f;
 
 				__ptr_clear_bit(0, &skb);
+				rcpu->held[rcpu->nr_held++] = skb->dev;
 				skbs[ret.skb_n++] = skb;
 				continue;
 			}
 
+			rcpu->held[rcpu->nr_held++] = ((struct xdp_frame *)f)->dev_rx;
 			frames[ret.xdp_n++] = f;
 			page = virt_to_page(f);
 
@@ -398,10 +410,13 @@ static int cpu_map_kthread_run(void *data)
 		/* Flush either every 64 packets or in case of empty ring */
 		packets += n;
 		empty = __ptr_ring_empty(rcpu->queue);
-		if (packets >= NAPI_POLL_WEIGHT || empty) {
-			cpu_map_gro_flush(rcpu, empty);
+		full = empty || rcpu->nr_held > CPU_MAP_HELD_DEVS - CPUMAP_BATCH;
+		if (packets >= NAPI_POLL_WEIGHT || full) {
+			cpu_map_gro_flush(rcpu, full);
 			packets = 0;
 		}
+		while (full && rcpu->nr_held)
+			dev_put(rcpu->held[--rcpu->nr_held]);
 
 		local_bh_enable(); /* resched point, may call do_softirq() */
 	}
@@ -738,12 +753,14 @@ static void bq_flush_to_queue(struct xdp_bulk_queue *bq)
 
 	for (i = 0; i < bq->count; i++) {
 		struct xdp_frame *xdpf = bq->q[i];
-		int err;
 
-		err = __ptr_ring_produce(q, xdpf);
-		if (err) {
+		if (__ptr_ring_full(q)) {
 			drops++;
 			xdp_return_frame_rx_napi(xdpf);
+		} else {
+			/* The device cannot go before the flush, from now on it can */
+			dev_hold(xdpf->dev_rx);
+			__ptr_ring_produce(q, xdpf);
 		}
 		processed++;
 	}
@@ -807,11 +824,15 @@ int cpu_map_generic_redirect(struct bpf_cpu_map_entry *rcpu,
 
 	__skb_pull(skb, skb->mac_len);
 	skb_set_redirected(skb, false);
+	dev_hold(skb->dev);
 	__ptr_set_bit(0, &skb);
 
 	ret = ptr_ring_produce(rcpu->queue, skb);
-	if (ret < 0)
+	if (ret < 0) {
+		__ptr_clear_bit(0, &skb);
+		dev_put(skb->dev);
 		goto trace;
+	}
 
 	wake_up_process(rcpu->kthread);
 trace:
-- 
2.55.0

[PATCH bpf-next v3 08/16] bpf: Drop packet pointers behind kfuncs writing an skb

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

bpf_dynptr_write() to the dynptr of an skb ends in
__bpf_skb_store_bytes(), which makes the range writable first. That pulls
data into the head or unclones it, and either can reallocate the head. So
check_helper_call() invalidates all packet pointers and all slices of the
skb behind the helper.

Several kfuncs take the same road: bpf_dynptr_copy() and
bpf_dynptr_memset(), if they get no writable slice of the destination
because it is not linear, and bpf_probe_read_{user,kernel}[_str]_dynptr()
and bpf_copy_from_user[_task][_str]_dynptr() likewise. The verifier does
not know. A program can keep a packet pointer or a slice across such a
call and read and write the old head, which has been freed.

Treat a kfunc of that kind as bpf_dynptr_write() is treated, by the type
of the dynptr it writes to, which is the first argument of all of them.
Where a global function is only scanned for what it may do, the type is
not known yet. There any call of these kfuncs counts, as a tail call does.

Fixes: daec295a7094 ("bpf/helpers: Introduce bpf_dynptr_copy kfunc")
Fixes: a498ee7576de ("bpf: Implement dynptr copy kfuncs")
Fixes: 5fc5d8fded57 ("bpf: Add bpf_dynptr_memset() kfunc")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 include/linux/bpf_verifier.h |  2 ++
 kernel/bpf/verifier.c        | 44 ++++++++++++++++++++++++++++++++----
 2 files changed, 42 insertions(+), 4 deletions(-)

diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index e420d211f423..7a72bb06fe56 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -1803,6 +1803,8 @@ struct bpf_call_arg_meta {
 	u32 kfunc_flags;
 	const char *func_name;
 	struct arg_constant_desc arg_constant;
+	/* The type of the dynptr that the kfunc writes through */
+	enum bpf_dynptr_type dynptr_write_type;
 
 	/* arg_{btf,btf_id,owning_ref} are used by kfunc-specific handling,
 	 * generally to pass info about user-defined local kptr types to later
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index d738ce6e7428..597302cf7086 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -1002,6 +1002,11 @@ static enum bpf_dynptr_type dynptr_reg_type(struct bpf_verifier_env *env, struct
 	return bpf_stack_slot(state, spi)->spilled_ptr.dynptr.type;
 }
 
+static bool dynptr_write_changes_pkt_data(enum bpf_dynptr_type type)
+{
+	return type == BPF_DYNPTR_TYPE_SKB || type == BPF_DYNPTR_TYPE_SKB_META;
+}
+
 static bool is_dynptr_type_expected(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
 				    enum bpf_arg_type arg_type)
 {
@@ -8976,6 +8981,7 @@ static bool is_kfunc_arg_scalar_with_name(const struct btf *btf,
 static bool is_bpf_cast_to_kern_ctx_kfunc(const struct bpf_call_arg_meta *meta);
 static bool is_bpf_dynptr_clone_kfunc(const struct bpf_call_arg_meta *meta);
 static bool is_bpf_iter_css_task_new_kfunc(const struct bpf_call_arg_meta *meta);
+static bool is_kfunc_dynptr_write(struct bpf_call_arg_meta *meta);
 static bool is_bpf_obj_drop_kfunc(u32 func_id);
 static bool is_bpf_percpu_obj_drop_kfunc(u32 func_id);
 static bool is_bpf_rbtree_add_kfunc(u32 func_id);
@@ -10040,6 +10046,8 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, u32 slot, u32 p
 		err = process_dynptr_func(env, reg, argno, insn_idx, arg_type, meta);
 		if (err)
 			return err;
+		if (!arg && is_kfunc_dynptr_write(meta))
+			meta->dynptr_write_type = meta->dynptr.type;
 		break;
 	}
 	case ARG_PTR_TO_ITER:
@@ -12654,8 +12662,7 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
 		if (dynptr_type == BPF_DYNPTR_TYPE_INVALID)
 			return -EFAULT;
 
-		if (dynptr_type == BPF_DYNPTR_TYPE_SKB ||
-		    dynptr_type == BPF_DYNPTR_TYPE_SKB_META)
+		if (dynptr_write_changes_pkt_data(dynptr_type))
 			/* this will trigger clear_all_pkt_pointers(), which will
 			 * invalidate all dynptr slices associated with the skb
 			 */
@@ -13704,9 +13711,36 @@ static bool is_kfunc_bpf_preempt_enable(struct bpf_call_arg_meta *meta)
 	return is_kfunc_call(meta, special_kfunc_list[KF_bpf_preempt_enable]);
 }
 
+/* These write to what their first argument, a dynptr, points to */
+BTF_SET_START(dynptr_write_kfuncs)
+BTF_ID(func, bpf_dynptr_copy)
+BTF_ID(func, bpf_dynptr_memset)
+#ifdef CONFIG_BPF_EVENTS
+BTF_ID(func, bpf_probe_read_user_dynptr)
+BTF_ID(func, bpf_probe_read_kernel_dynptr)
+BTF_ID(func, bpf_probe_read_user_str_dynptr)
+BTF_ID(func, bpf_probe_read_kernel_str_dynptr)
+BTF_ID(func, bpf_copy_from_user_dynptr)
+BTF_ID(func, bpf_copy_from_user_str_dynptr)
+BTF_ID(func, bpf_copy_from_user_task_dynptr)
+BTF_ID(func, bpf_copy_from_user_task_str_dynptr)
+#endif
+BTF_SET_END(dynptr_write_kfuncs)
+
+static bool is_kfunc_dynptr_write(struct bpf_call_arg_meta *meta)
+{
+	return meta->btf == btf_vmlinux &&
+	       btf_id_set_contains(&dynptr_write_kfuncs, meta->func_id);
+}
+
+/*
+ * A write through the dynptr of an skb may reallocate its head. Before the
+ * arguments are checked nobody knows what kind of dynptr it is.
+ */
 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]);
+	return is_kfunc_call(meta, special_kfunc_list[KF_bpf_xdp_pull_data]) ||
+	       is_kfunc_dynptr_write(meta);
 }
 
 static u32 kfunc_abi_slots(const struct btf_func_model *fm)
@@ -15644,7 +15678,9 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
 		}
 	}
 
-	if (bpf_is_kfunc_pkt_changing(&meta))
+	if (is_kfunc_dynptr_write(&meta) ?
+	    dynptr_write_changes_pkt_data(meta.dynptr_write_type) :
+	    bpf_is_kfunc_pkt_changing(&meta))
 		clear_all_pkt_pointers(env);
 
 	proto_slots = kfunc_abi_slots(&desc->func_model);
-- 
2.55.0

[PATCH bpf-next v3 09/16] bpf: Let bpf_dynptr_write() mark a global function

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

The caller of a global function loses its packet pointers if the function
may change packet data. That is found out by a scan for calls of the
helpers which bpf_helper_changes_pkt_data() lists.

bpf_dynptr_write() is not on the list, because it depends on the type of
the dynptr: check_helper_call() looks at it. The scan cannot do that, and
so a global function which makes a dynptr from the skb, or gets one as an
argument, and writes through it counts as harmless. The write can
reallocate the head of the skb, and the caller goes on with pointers into
the old one, which has been freed.

Let every call of bpf_dynptr_write() mark the function, as a tail call
does. The same mark decides whether a program may replace a function.

Fixes: 51081a3f25c7 ("bpf: track changes_pkt_data property for global functions")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/cfg.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/kernel/bpf/cfg.c b/kernel/bpf/cfg.c
index bce7b5abcfda..2ee950e9ac82 100644
--- a/kernel/bpf/cfg.c
+++ b/kernel/bpf/cfg.c
@@ -598,7 +598,9 @@ 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))
+			/* nobody knows yet whether the dynptr is that of an skb */
+			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;
-- 
2.55.0

[PATCH bpf-next v3 10/16] bpf: Don't take a dynptr argument for a local dynptr

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

A global function is verified on its own. A dynptr which it gets as an
argument is assumed to be of type BPF_DYNPTR_TYPE_LOCAL, but the callers
may pass one of any type, that of an skb for example.

The type decides whether a write through the dynptr invalidates packet
pointers, and whether a slice of it is invalidated together with them.
Inside of the function neither happens. It can take a packet pointer from
its context argument, or a slice from the dynptr, write through the dynptr
and go on using the pointer, although the write may have reallocated the
head of the skb.

Assume the type of an skb dynptr instead. Nothing becomes forbidden for the
dynptr itself, and a clone inherits the caution. The dynptr which the
callback of bpf_user_ringbuf_drain() gets is known to be local, and stays
so.

Fixes: a64bfe618665 ("bpf: add support for passing dynptr pointer to global subprog")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/verifier.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 597302cf7086..338376492647 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -21589,8 +21589,12 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog,
 				reg->type = SCALAR_VALUE;
 				mark_reg_unknown(env, regs, i);
 			} else if (arg->arg_type == ARG_PTR_TO_DYNPTR) {
-				/* assume unspecial LOCAL dynptr type */
-				__mark_dynptr_reg(reg, BPF_DYNPTR_TYPE_LOCAL, true, ++env->id_gen, 0);
+				/*
+				 * The callers may hand in a dynptr of any type.
+				 * Take it for that of an skb, so that a write
+				 * through it and its slices are treated with care.
+				 */
+				__mark_dynptr_reg(reg, BPF_DYNPTR_TYPE_SKB, true, ++env->id_gen, 0);
 			} else if (base_type(arg->arg_type) == ARG_PTR_TO_MEM) {
 				reg->type = PTR_TO_MEM;
 				reg->type |= arg->arg_type &
-- 
2.55.0

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

[Alexei Starovoitov]

From: Alexei Starovoitov <ast@kernel.org>

A tc, lwt_xmit or sk_skb program that writes to packet data starts with
bpf_unclone_prologue().  It takes the skb from R1, and if the skb is cloned
it calls bpf_skb_pull_data() and restores R1 only.

That suits the main function of a program.  An extension gets the prologue
too, but it starts with the arguments of the function that it replaces.
If the first one is not the context, something else is taken for an skb:

  BUG: KASAN: stack-out-of-bounds in skb_ensure_writable+0x84/0x1a0
  Read of size 8 at addr ffffc90000270ad0 by task uncl/105
   skb_ensure_writable+0x84/0x1a0
   bpf_skb_pull_data+0x32/0xf0
   bpf_prog_853eb1991d35e093_ext_slice+0x4f/0x321
   bpf_prog_9eb18752b5d0959a_t_glob+0x2ac/0x2de
  BUG: unable to handle page fault for address: 0000000082298821

And whatever R2-R5 held is lost, while the verifier goes on trusting
pointers in them.

Refuse an extension that needs a prologue unless it gets the context as
its first argument and scalars beside it.  A scalar is still clobbered if
the skb is cloned, which the verifier can cope with.  Extensions of XDP,
sk_msg and sockopt programs get no prologue and load as before.

Fixes: be8704ff07d2 ("bpf: Introduce dynamic program extensions")
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/fixups.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
index fe59e3114d82..a8739fbd0349 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)
-- 
2.55.0

[PATCH bpf-next v3 12/16] bpf: Unclone the skb for kfuncs that write through a dynptr

[Alexei Starovoitov]

From: Alexei Starovoitov <ast@kernel.org>

bpf_dynptr_copy(), bpf_dynptr_memset() and the kfuncs that read user or
kernel memory into a dynptr ask bpf_dynptr_slice_rdwr() for a slice and
write to it.  For the linear part of an skb that is a pointer into its
head.  bpf_dynptr_slice_rdwr() counts on the verifier to have uncloned the
skb, see bpf_unclone_prologue().

The verifier does so if the program itself calls bpf_dynptr_slice_rdwr()
or writes to the packet.  A program that only uses these kfuncs gets no
prologue, and changes the data of all the clones of the skb.  A packet
socket, whose clone is taken before tc ingress runs, sees what
bpf_dynptr_memset() has written there.

Treat a call that may write to an skb like a direct write.

Fixes: daec295a7094 ("bpf/helpers: Introduce bpf_dynptr_copy kfunc")
Fixes: a498ee7576de ("bpf: Implement dynptr copy kfuncs")
Fixes: 5fc5d8fded57 ("bpf: Add bpf_dynptr_memset() kfunc")
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/verifier.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 338376492647..3bc1df7aabe2 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -15683,6 +15683,14 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
 	    bpf_is_kfunc_pkt_changing(&meta))
 		clear_all_pkt_pointers(env);
 
+	/*
+	 * These kfuncs write to the head of the skb in place if what they write
+	 * to is linear. Like a direct write that takes bpf_unclone_prologue().
+	 */
+	if (is_kfunc_dynptr_write(&meta) &&
+	    dynptr_write_changes_pkt_data(meta.dynptr_write_type))
+		may_access_direct_pkt_data(env, NULL, BPF_WRITE);
+
 	proto_slots = kfunc_abi_slots(&desc->func_model);
 	if (proto_slots > MAX_BPF_FUNC_REG_ARGS) {
 		struct bpf_func_state *caller = cur_func(env);
-- 
2.55.0

[PATCH bpf-next v3 13/16] bpf: Follow the head if an skb is written from a slice

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

The source of bpf_dynptr_write() may be a slice of the skb which is
written to, that is a pointer into its head. bpf_dynptr_copy() does it by
itself for a copy inside of one skb, if the source is linear and the
destination is not.

__bpf_skb_store_bytes() and __bpf_skb_meta_store_bytes() make the skb
writable first. If that reallocates the head, the source points to the old
one, and freed memory is copied into the packet.

The offsets in the head do not change when it is reallocated here. Move
the source along with the head.

Fixes: 2012c867c800 ("bpf: verifier: Accept dynptr mem as mem in helpers")
Fixes: f38499ff45f5 ("bpf: Unclone skb head on bpf_dynptr_write to skb metadata")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 net/core/filter.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index caa9c3ef0a78..0abedbb04ab7 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -1715,6 +1715,24 @@ static inline void bpf_pull_mac_rcsum(struct sk_buff *skb)
 		skb_postpull_rcsum(skb, skb_mac_header(skb), skb->mac_len);
 }
 
+/*
+ * The source of a write through a dynptr may be a slice of the same skb.
+ * Making the skb writable can move its head: follow the move.
+ */
+static int bpf_try_make_writable_from(struct sk_buff *skb, u32 write_len,
+				      const void **from)
+{
+	const void *head = skb->head;
+	bool in_head = *from >= head &&
+		       *from < (const void *)skb_end_pointer(skb);
+	int err;
+
+	err = bpf_try_make_writable(skb, write_len);
+	if (!err && in_head)
+		*from = skb->head + (*from - head);
+	return err;
+}
+
 BPF_CALL_5(bpf_skb_store_bytes, struct sk_buff *, skb, u32, offset,
 	   const void *, from, u32, len, u64, flags)
 {
@@ -1724,7 +1742,7 @@ BPF_CALL_5(bpf_skb_store_bytes, struct sk_buff *, skb, u32, offset,
 		return -EINVAL;
 	if (unlikely(offset > INT_MAX))
 		return -EFAULT;
-	if (unlikely(bpf_try_make_writable(skb, offset + len)))
+	if (unlikely(bpf_try_make_writable_from(skb, offset + len, &from)))
 		return -EFAULT;
 
 	ptr = skb->data + offset;
@@ -12427,7 +12445,7 @@ int __bpf_skb_meta_store_bytes(struct sk_buff *skb, u32 offset,
 {
 	if (unlikely(flags))
 		return -EINVAL;
-	if (unlikely(bpf_try_make_writable(skb, 0)))
+	if (unlikely(bpf_try_make_writable_from(skb, 0, &from)))
 		return -EFAULT;
 
 	memmove(bpf_skb_meta_pointer(skb, offset), from, len);
-- 
2.55.0

[PATCH bpf-next v3 14/16] bpf: Use memmove() to store bytes in an skb

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

Through bpf_dynptr_write() and bpf_dynptr_copy() the source of
bpf_skb_store_bytes() can be a slice of the same skb which overlaps the
destination. memcpy() then reads bytes which it has overwritten already. On
x86 with FSRM it repeats the first bytes of the source instead of copying
it. Local, ringbuf and skb metadata dynptrs are written with memmove() for
that reason.

Fixes: 2012c867c800 ("bpf: verifier: Accept dynptr mem as mem in helpers")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 net/core/filter.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index 0abedbb04ab7..7fbee7570d0c 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -1749,7 +1749,8 @@ BPF_CALL_5(bpf_skb_store_bytes, struct sk_buff *, skb, u32, offset,
 	if (flags & BPF_F_RECOMPUTE_CSUM)
 		__skb_postpull_rcsum(skb, ptr, len, offset);
 
-	memcpy(ptr, from, len);
+	/* through a dynptr the source may be an overlapping slice of the skb */
+	memmove(ptr, from, len);
 
 	if (flags & BPF_F_RECOMPUTE_CSUM)
 		__skb_postpush_rcsum(skb, ptr, len, offset);
-- 
2.55.0

[PATCH bpf-next v3 15/16] bpf: Use memmove() to store bytes in an xdp_buff

[Alexei Starovoitov]

From: Alexei Starovoitov <ast@kernel.org>

bpf_dynptr_write() to the dynptr of an xdp_buff ends in
bpf_xdp_store_bytes(). The source can be a slice of the same xdp_buff which
overlaps the destination, and the helper itself takes such a slice as well.
memcpy() then reads bytes which it has overwritten already. On x86 with
FSRM

  p = bpf_dynptr_slice_rdwr(&ptr, 0, NULL, 64);
  bpf_dynptr_write(&ptr, 8, p, 64, 0);

repeats the first 8 bytes of the packet instead of moving 64.

A write that crosses from the linear part into a fragment is copied piece
by piece, where the first piece can overwrite what the second one has yet
to read. That is not dealt with here.

Fixes: 2012c867c800 ("bpf: verifier: Accept dynptr mem as mem in helpers")
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 net/core/filter.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index 7fbee7570d0c..b7ac145b68b1 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -4330,7 +4330,8 @@ BPF_CALL_4(bpf_xdp_store_bytes, struct xdp_buff *, xdp, u32, offset,
 	if (!ptr)
 		bpf_xdp_copy_buf(xdp, offset, buf, len, true);
 	else
-		memcpy(ptr, buf, len);
+		/* through a dynptr the source may be an overlapping slice */
+		memmove(ptr, buf, len);
 
 	return 0;
 }
-- 
2.55.0

[PATCH bpf-next v3 16/16] bpf: Refuse packet slices for packet-changing helpers

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

The helpers which may change packet data do not take a packet pointer as a
memory argument: they have no pkt_access. They make the skb writable, or
make room in it, before they read what the argument points to, and that
can reallocate the head which it points into.

A slice from bpf_dynptr_slice() of the dynptr of the skb points into the
same head, but it is a PTR_TO_MEM, and the verifier lets it pass.
bpf_lwt_push_encap(), bpf_lwt_seg6_store_bytes() and bpf_lwt_seg6_action()
then read the old head, which has been freed. bpf_skb_store_bytes() follows
the head by now, but a slice is no better than a packet pointer there.

Refuse a slice of a packet where a packet pointer is refused for that
reason. All other helpers take slices as before. bpf_dynptr_write() and
bpf_dynptr_copy() can be used to copy inside of a packet.

Fixes: 2012c867c800 ("bpf: verifier: Accept dynptr mem as mem in helpers")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/verifier.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 3bc1df7aabe2..63fff641d985 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -9687,6 +9687,16 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, u32 slot, u32 p
 	if (err)
 		return err;
 
+	/*
+	 * A slice of a packet is packet data under another name. A helper which
+	 * may reallocate the packet must not go on reading from the old one.
+	 */
+	if (is_helper(meta) && reg_is_dynptr_slice_pkt(reg) &&
+	    bpf_helper_changes_pkt_data(meta->func_id)) {
+		verbose(env, "function access to the packet is not allowed\n");
+		return -EACCES;
+	}
+
 	if (arg_type_is_raw_mem(arg_type))
 		meta->arg_raw_mem.mask |= BIT(slot);
 
-- 
2.55.0

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

[sashiko-bot]

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

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

[bot+bpf-ci]

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

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

TL;DR: Commit message only: the Fixes: tag may point at the wrong commit,
since extensions could not write packet data (or get the prologue) until
7e40781cc8b7, though be8704ff07d2 may be defensible.

> bpf: Refuse the prologue for an extension that cannot take it
>
> A tc, lwt_xmit or sk_skb program that writes to packet data starts with
> bpf_unclone_prologue().  It takes the skb from R1, and if the skb is cloned
> it calls bpf_skb_pull_data() and restores R1 only.
>
> That suits the main function of a program.  An extension gets the prologue
> too, but it starts with the arguments of the function that it replaces.
> If the first one is not the context, something else is taken for an skb:
>
>   BUG: KASAN: stack-out-of-bounds in skb_ensure_writable+0x84/0x1a0
>   Read of size 8 at addr ffffc90000270ad0 by task uncl/105
>    skb_ensure_writable+0x84/0x1a0
>    bpf_skb_pull_data+0x32/0xf0
>    bpf_prog_853eb1991d35e093_ext_slice+0x4f/0x321
>    bpf_prog_9eb18752b5d0959a_t_glob+0x2ac/0x2de
>   BUG: unable to handle page fault for address: 0000000082298821
>
> And whatever R2-R5 held is lost, while the verifier goes on trusting
> pointers in them.
>
> Refuse an extension that needs a prologue unless it gets the context as
> its first argument and scalars beside it.  A scalar is still clobbered if
> the skb is cloned, which the verifier can cope with.  Extensions of XDP,
> sk_msg and sockopt programs get no prologue and load as before.
>
> Fixes: be8704ff07d2 ("bpf: Introduce dynamic program extensions")
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>

Is the Fixes: tag pointing at the right commit?

At be8704ff07d2, the prologue could not be reached from an extension.  The
prologue is only generated when env->seen_direct_write is set, and at that
commit the flag was only set via may_access_direct_pkt_data(), which
switched on env->prog->type:

    switch (env->prog->type) {
    ...
    default:
        return false;
    }

BPF_PROG_TYPE_EXT fell into the default case, so an extension could not
write packet data directly.  bpf_unclone_prologue() returns 0 when
!direct_write, so no prologue was patched in and R1-R5 of the extension
were never clobbered.

Direct packet writes from an extension, and therefore the prologue, first
became possible with 7e40781cc8b7 ("bpf: verifier: Use target program's
type for access verifications").  That commit introduced
resolve_prog_type() and made may_access_direct_pkt_data() use the target
program's type, so an extension replacing a tc, lwt_xmit or sk_skb global
function could set seen_direct_write.

That said, the tag may still be defensible, since be8704ff07d2 is what
copied the target's ops into env->ops, which supplies gen_prologue.

Should this include:

  Fixes: 7e40781cc8b7 ("bpf: verifier: Use target program's type for access verifications")


---
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/39078926044

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

[Alexei Starovoitov]

On Sat, Oct 17, 2026 at 04:29 PM sashiko-bot@kernel.org <sashiko-bot@kernel.org> wrote:
> Could this cause a use-after-free in the caller after the extension returns,

Yes, for a target that only reads the packet. It's an old bug, not from
this patch. Reproduced on bpf-next without the series:

  BUG: KASAN: slab-use-after-free in bpf_prog_769e8c14b3a52c7a_t_read+0x386/0x404
  Read of size 1 at addr ffff888104c61e3c by task tt11/102
  Freed by task 113:
   kfree+0x161/0x500
   skb_release_data+0x2dc/0x340
   consume_skb+0xa6/0x1c0
   packet_recvmsg+0x4b3/0xa20

Two corrections though:

> skb_ensure_writable() -> pskb_expand_head(), which reallocates and frees
> skb->head while the caller still holds pointers into the old skb->head.

For a cloned skb it only drops a reference. The old head goes when the
other clone is freed, so another cpu has to do that while the caller is
still running. And the caller can only read: a target that writes
to the packet has its own prologue.

> 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

bpf_clone_redirect() unclones the skb before it 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)?

That alone is too strict. It would refuse
freplace/test_pkt_write_access_subprog in fexit_bpf2bpf, which is fine:
the target writes to the packet, so the skb is uncloned at entry and
the prologue of the extension does nothing.

Will send a follow-up: refuse such an extension unless the target function
changes packet data or the target prog has the prologue, at load
and at BPF_LINK_CREATE.

Re: [PATCH bpf-next v3 00/16] bpf: Memory safety, lifetime and locking fixes (followups)

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <ast@kernel.org>:

On Sat, 17 Oct 2026 23:09:34 +0000 you wrote:
> From: Alexei Starovoitov <ast@kernel.org>
> 
> The first 30 patches of v2 are applied. This is the rest of the series,
> with what the reviews of v2 have found, and two fixes on top of patches
> that are applied already. Most of the patches are from Josef.
> 
> Every bug was reproduced in a VM on top of bpf-next, on the kernel right
> before and right after the patch that fixes it. The splats are in the
> commit logs. That goes for what the reviews have found as well.
> 
> [...]

Here is the summary with links:
  - [bpf-next,v3,01/16] bpf: Don't take a program with a target program for an iterator
    https://git.kernel.org/bpf/bpf-next/c/b83bacf2161d
  - [bpf-next,v3,02/16] bpf: Don't let an extension reach further into the context than its target
    https://git.kernel.org/bpf/bpf-next/c/fcbfa99aa657
  - [bpf-next,v3,03/16] bpf: Keep values of a dead owner for sleepable programs
    https://git.kernel.org/bpf/bpf-next/c/f693030a187b
  - [bpf-next,v3,04/16] bpf: Don't take an extension for a function that runs sleepable
    https://git.kernel.org/bpf/bpf-next/c/b6458abbafe4
  - [bpf-next,v3,05/16] bpf: Keep the symbols of a program until it has ended
    https://git.kernel.org/bpf/bpf-next/c/4eaf4d71b91a
  - [bpf-next,v3,06/16] bpf: Use the socket of a sk_lookup test run under RCU
    https://git.kernel.org/bpf/bpf-next/c/6aff0129f327
  - [bpf-next,v3,07/16] bpf, cpumap: Hold the device of frames in the queue
    https://git.kernel.org/bpf/bpf-next/c/f99351272933
  - [bpf-next,v3,08/16] bpf: Drop packet pointers behind kfuncs writing an skb
    https://git.kernel.org/bpf/bpf-next/c/19e6ca6b4a22
  - [bpf-next,v3,09/16] bpf: Let bpf_dynptr_write() mark a global function
    https://git.kernel.org/bpf/bpf-next/c/589998616e0f
  - [bpf-next,v3,10/16] bpf: Don't take a dynptr argument for a local dynptr
    https://git.kernel.org/bpf/bpf-next/c/b7f97db88948
  - [bpf-next,v3,11/16] bpf: Refuse the prologue for an extension that cannot take it
    https://git.kernel.org/bpf/bpf-next/c/aa486cff4964
  - [bpf-next,v3,12/16] bpf: Unclone the skb for kfuncs that write through a dynptr
    https://git.kernel.org/bpf/bpf-next/c/bb72c2866831
  - [bpf-next,v3,13/16] bpf: Follow the head if an skb is written from a slice
    https://git.kernel.org/bpf/bpf-next/c/1c19f2b3b2f3
  - [bpf-next,v3,14/16] bpf: Use memmove() to store bytes in an skb
    https://git.kernel.org/bpf/bpf-next/c/1db084f42884
  - [bpf-next,v3,15/16] bpf: Use memmove() to store bytes in an xdp_buff
    https://git.kernel.org/bpf/bpf-next/c/ab08863c58b1
  - [bpf-next,v3,16/16] bpf: Refuse packet slices for packet-changing helpers
    https://git.kernel.org/bpf/bpf-next/c/a6f8a67c13dd

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html