[PATCH bpf-next 00/38] bpf: Memory safety, lifetime and locking fixes

[PATCH bpf-next 00/38] bpf: Memory safety, lifetime and locking fixes

[Alexei Starovoitov]

From: Alexei Starovoitov <ast@kernel.org>

This series collects fixes for bugs in the BPF core, the verifier and the
BPF side of the networking stack. All but patch 14 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.

Sleeping, or doing without RCU, where that is not allowed:
  02 hash map for_each callbacks of sleepable programs
  05 sleepable programs on the task_vma iterator
  06 bpf_find_vma() callbacks of sleepable programs
  23 maps that rely on RCU in bpf_wq and bpf_task_work callbacks

Buffers that iterators and writable tracepoints hand to programs:
  18 BPF_LINK_UPDATE skips the size check of the attach
  24 tail calls from an iterator program
  25 extensions of an iterator program
  26 tail calls from a writable raw tracepoint program
  27 writes over bpf_timer, bpf_spin_lock, kptr in a live map value

The sk_storage map iterator:
  14 a socket that sk_clone() is still building can be found and used
  15 NULL dereference if the socket goes away
  16 the socket is not held while the program runs

Lifetime of local storage, programs and maps:
  10 owner_refcnt saturates on the last unpin
  28 map_free() frees elements under sleepable readers
  29 values of a dead owner are freed under sleepable programs
  21 a program is freed under its bpf_wq callback
  22 an inner map is freed under a sleepable callback
  30 the exception table of a program that still runs is not found

XDP and devices:
  01 devmap egress programs free page_pool fragments as plain pages
  19 stale offload->prog after bpf_prog_realloc()
  20 a program is bound to a netdev that is being unregistered
  32 cpumap uses the device of a queued frame after it is gone

seg6local End.BPF and bpf_skb_adjust_room():
  11 the SRH of the inner packet is not validated after End.DT6
  12 the inner header is not pulled for End.DT6
  13 stale SRH pointer if End.B6 fails late
  08 gso_size can drop to 0 or wrap around

skb dynptrs, where a write can reallocate the head of the skb:
  33 kfuncs that write through a dynptr
  34 bpf_dynptr_write() in a global function
  35 a dynptr passed to a global function
  36 the source of the write is a slice of the same skb
  37 overlapping source and destination
  38 slices as arguments of helpers that change the packet

The rest:
  03 inode leak if BPF_TOKEN_CREATE fails
  04 preallocated hash map hands out an element that is still linked
  07 BPF_JSET with a 64 bit mask, 32-bit kernels only
  09 probe names of a perf event are read without event_mutex
  17 unbounded recursion in the BTF show code
  31 BPF_PROG_TEST_RUN of sk_lookup uses the socket outside of RCU

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

 - 02, 06: helpers, kfuncs and global functions that may sleep are rejected
   in these callbacks.
 - 05: sleepable programs cannot be attached to the task_vma iterator.
 - 23: in sleepable callbacks, and for inner maps in sleepable programs,
   maps that rely on RCU need bpf_rcu_read_lock().
 - 24, 26: a program with a tail call cannot be attached to a map element
   iterator or to a writable tracepoint.
 - 27: an iterator program that writes over a special field cannot be
   attached to that map.
 - 33-35, 38: 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.
 - 04: a nested update of a preallocated hash map on the same CPU can fail
   with -EBUSY.
 - 08: bpf_skb_adjust_room() returns -ENOTSUPP if gso_size would not stay
   positive.

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 5689 programs of the selftests: same
verdicts, insns, states and stack depth.

Alexei Starovoitov (1):
  bpf: Clear sk_refcnt of a clone before its storage is linked

Josef Bacik (37):
  bpf: use the frame mem type in devmap egress XDP
  bpf: forbid sleeping in hash map for_each callbacks
  bpf: don't leak the inode when token creation fails
  bpf: don't hand out a linked element as extra element
  bpf: reject sleepable programs for task_vma iterator
  bpf: forbid sleeping in bpf_find_vma() callbacks
  bpf: look at all 64 bits of a BPF_JSET mask on 32-bit
  bpf: don't let bpf_skb_adjust_room() use up the MSS
  bpf: Read the probe of a perf event under event_mutex
  bpf: Don't saturate owner_refcnt on the last unpin
  bpf: Validate the SRH which End.DT6 uncovers in End.BPF
  bpf: Pull the inner header for End.DT6 in End.BPF
  bpf: Update the SRH state if End.B6 fails late
  bpf: Fix NULL deref in the sk_storage map iterator
  bpf: Hold the socket in the sk_storage map iterator
  bpf: Bound the nesting that the BTF show code follows
  bpf: Check the new program of an iterator link update
  bpf: Fix stale offload->prog pointer after realloc
  bpf: Don't bind a program to a netdev that goes away
  bpf: Keep a program until its bpf_wq callback has ended
  bpf: Count sleepable callbacks as sleepable map users
  bpf: Check the maps that sleepable callbacks use
  bpf: Assume no bounds for buffers behind a tail call
  bpf: Don't take an extension for an iterator program
  bpf: Assume no bounds for a writable tp with tail calls
  bpf: Keep iterator programs off special map fields
  bpf: Free what map_free() unlinks for sleepable readers
  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               |  24 +++-
 include/linux/bpf_local_storage.h |  13 ++
 include/linux/bpf_verifier.h      |   7 ++
 kernel/bpf/bpf_iter.c             |  31 ++++-
 kernel/bpf/bpf_local_storage.c    |  21 +++-
 kernel/bpf/btf.c                  |  14 ++-
 kernel/bpf/cfg.c                  |   4 +-
 kernel/bpf/core.c                 |  54 +++++---
 kernel/bpf/cpumap.c               |  35 ++++--
 kernel/bpf/devmap.c               |   1 +
 kernel/bpf/fixups.c               |   4 +
 kernel/bpf/hashtab.c              |  21 +++-
 kernel/bpf/helpers.c              |   9 +-
 kernel/bpf/map_in_map.c           |   7 +-
 kernel/bpf/map_iter.c             |  79 ++++++++----
 kernel/bpf/offload.c              |   9 +-
 kernel/bpf/states.c               |   3 +
 kernel/bpf/syscall.c              |  28 +++--
 kernel/bpf/task_iter.c            |  12 +-
 kernel/bpf/token.c                |   5 +-
 kernel/bpf/verifier.c             | 203 ++++++++++++++++++++++++------
 kernel/trace/bpf_trace.c          |  13 ++
 net/bpf/test_run.c                |  19 ++-
 net/core/bpf_sk_storage.c         |  96 ++++++++++++--
 net/core/filter.c                 |  58 +++++++--
 net/core/sock_map.c               |  17 ++-
 26 files changed, 631 insertions(+), 156 deletions(-)


base-commit: 645f5fcf7a5582f5d6ed2322f41cb34076c63b2c
-- 
2.55.0

[PATCH bpf-next 01/38] bpf: use the frame mem type in devmap egress XDP

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

A devmap egress program that removes a whole fragment of a redirected
xdp_frame with bpf_xdp_adjust_tail() or bpf_xdp_pull_data() frees it
with page_frag_free() whatever the frame's memory type is. For a
MEM_TYPE_PAGE_POOL frame from a page_pool based driver this drops the
page reference owned by the pool while the pool still accounts the
fragment ("page_pool leak" bad page, leaked pp reference).

kernel/bpf/devmap.c:dev_map_bpf_prog_run() points xdp.rxq at an
on-stack

    struct xdp_rxq_info rxq = { .dev = rx_dev };

whose mem.type is 0, i.e. MEM_TYPE_PAGE_SHARED, and
net/core/filter.c:bpf_xdp_shrink_data() releases frags according to
rxq->mem.type. cpumap, which runs programs on xdp_frames the same way,
already copies xdpf->mem_type into its private rxq for each frame.

With 9000 byte frames from virtio_net and an egress program that cuts the
fragments off:

  BUG: Bad page state in process swapper/3  pfn:10660b
  page dumped because: page_pool leak
   bad_page+0xbd/0x190
   __free_frozen_pages+0x5c7/0x900
   bpf_xdp_adjust_tail+0x3be/0x480
   bpf_prog_dad43f894a3faff7_egress+0x15c/0x1cd
   dev_map_bpf_prog_run+0x288/0x610

The pool waits for these pages for ever when its device is gone. That is
the harmless case. A page of the pool can hold more than one fragment
while its refcount is 1: the first page_frag_free() frees it under the
fragment which is still in use, and the second one runs into

  page dumped because: VM_BUG_ON_PAGE(page_ref_count(page) == 0)
  kernel BUG at include/linux/mm.h:1846!

veth is not affected. It takes references of its own on the pages and
redirects the frame as MEM_TYPE_PAGE_SHARED.

Fix by setting rxq.mem.type from each frame before running the devmap
program, as cpu_map_bpf_prog_run_xdp() does.

Fixes: ca9984c5f0ab ("bpf: devmap: provide rxq after redirect")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/devmap.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
index dc7b859e8bbf..1b8f0d7f8455 100644
--- a/kernel/bpf/devmap.c
+++ b/kernel/bpf/devmap.c
@@ -350,6 +350,7 @@ static int dev_map_bpf_prog_run(struct bpf_prog *xdp_prog,
 
 		xdp_convert_frame_to_buff(xdpf, &xdp);
 		xdp.txq = &txq;
+		rxq.mem.type = xdpf->mem_type;
 		xdp.rxq = &rxq;
 
 		act = bpf_prog_run_xdp(xdp_prog, &xdp);
-- 
2.55.0

[PATCH bpf-next 02/38] bpf: forbid sleeping in hash map for_each callbacks

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

A sleepable program (e.g. SEC("syscall"), lsm.s, fentry.s) can pass a
callback to bpf_for_each_map_elem() on a hash map and call a sleepable
helper or kfunc such as bpf_copy_from_user() from it. The verifier
accepts this, but at run time the callback is invoked from inside an RCU
read-side critical section:

    bpf_for_each_hash_elem()
      rcu_read_lock();
      callback_fn(map, key, val, ctx, 0);
        bpf_copy_from_user()       <- may fault and sleep
      rcu_read_unlock();

bpf_each_rhash_elem() does the same for BPF_MAP_TYPE_RHASH. Sleeping
there triggers the "Voluntary context switch within RCU read-side
critical section!" warning, delays grace periods, and with
CONFIG_PREEMPT_RCU=n silently ends the read-side critical section that
the bucket walk relies on.

Synchronous callbacks are verified with the caller's state, so
kernel/bpf/verifier.c:in_sleepable_context() only looks at the program's
sleepability and the locks the program took itself, and does not know
that the helper holds rcu_read_lock() around the callback.

With a SEC("syscall") program whose callback reads a page that has to be
read from disk first:

  BUG: sleeping function called from invalid context at include/linux/uaccess.h:169
  RCU nest depth: 1, expected: 0
   __might_fault+0x29/0x80
   _copy_from_user+0x26/0x90
   bpf_copy_from_user+0x23/0x60
   bpf_prog_17d44fa39eebd6cc_cb+0x35/0x46
   bpf_for_each_hash_elem+0xdb/0x280

Fix by marking the callback frame of hash map iteration as running under
RCU and making in_sleepable_context() false while such a frame is on the
call stack, so sleepable helpers, kfuncs and global functions are
rejected there as inside bpf_rcu_read_lock(). The rules for trusting RCU
protected pointers are left alone, which is conservative and keeps
callbacks that verify today and do not sleep working. The flag is
compared in func_states_equal() so that pruning never mixes the two
contexts. Array map callbacks run without any lock and keep the caller's
context.

Fixes: 314ee05e2fc6 ("bpf: Add hashtab support for bpf_for_each_map_elem() helper")
Fixes: 818e00848227 ("bpf: Implement iteration ops for resizable hashtab")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 include/linux/bpf_verifier.h |  5 +++++
 kernel/bpf/states.c          |  3 +++
 kernel/bpf/verifier.c        | 28 +++++++++++++++++++++++++++-
 3 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index 112ca1511da4..816948df8fff 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -379,6 +379,11 @@ struct bpf_func_state {
 	bool in_async_callback_fn;
 	bool in_exception_callback_fn;
 	bool no_stack_arg_load;
+	/*
+	 * The helper invokes this callback under rcu_read_lock(), so the
+	 * callback and its callees run in an RCU read-side critical section.
+	 */
+	bool in_rcu_callback_fn;
 	/* For callback calling functions that limit number of possible
 	 * callback executions (e.g. bpf_loop) keeps track of current
 	 * simulated iteration number.
diff --git a/kernel/bpf/states.c b/kernel/bpf/states.c
index 953700822fa7..b3d250caaaa6 100644
--- a/kernel/bpf/states.c
+++ b/kernel/bpf/states.c
@@ -995,6 +995,9 @@ static bool func_states_equal(struct bpf_verifier_env *env, struct bpf_func_stat
 	if (!old->no_stack_arg_load && cur->no_stack_arg_load)
 		return false;
 
+	if (old->in_rcu_callback_fn != cur->in_rcu_callback_fn)
+		return false;
+
 	for (i = 0; i < MAX_BPF_REG; i++)
 		if (((1 << i) & live_regs) &&
 		    !regsafe(env, &old->regs[i], &cur->regs[i],
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index ddfa0c6d2755..883cf148235c 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -11530,6 +11530,19 @@ static int set_map_elem_callback_state(struct bpf_verifier_env *env,
 	if (err)
 		return err;
 
+	switch (map->map_type) {
+	case BPF_MAP_TYPE_HASH:
+	case BPF_MAP_TYPE_PERCPU_HASH:
+	case BPF_MAP_TYPE_LRU_HASH:
+	case BPF_MAP_TYPE_LRU_PERCPU_HASH:
+	case BPF_MAP_TYPE_RHASH:
+		/* Hash tables run the callback under rcu_read_lock(). */
+		callee->in_rcu_callback_fn = true;
+		break;
+	default:
+		break;
+	}
+
 	callee->in_callback_fn = true;
 	callee->callback_ret_range = retval_range(0, 1);
 	return 0;
@@ -12273,10 +12286,22 @@ int bpf_get_helper_proto(struct bpf_verifier_env *env, int func_id,
 	return *ptr && (*ptr)->func ? 0 : -EINVAL;
 }
 
+/* Is any frame on the current path a callback that runs under RCU? */
+static bool in_rcu_callback(struct bpf_verifier_env *env)
+{
+	struct bpf_verifier_state *state = env->cur_state;
+	int i;
+
+	for (i = 0; i <= state->curframe; i++)
+		if (state->frame[i]->in_rcu_callback_fn)
+			return true;
+	return false;
+}
+
 /* Check if we're in a sleepable context. */
 static inline bool in_sleepable_context(struct bpf_verifier_env *env)
 {
-	return !in_rcu_cs(env);
+	return !in_rcu_cs(env) && !in_rcu_callback(env);
 }
 
 static const char *non_sleepable_context_description(struct bpf_verifier_env *env)
@@ -12290,6 +12315,7 @@ static const char *non_sleepable_context_description(struct bpf_verifier_env *en
 			return "IRQ-disabled region";
 		if (env->cur_state->active_locks)
 			return "lock region";
+		return "callback running under rcu_read_lock";
 	}
 	return "non-sleepable prog";
 }
-- 
2.55.0

[PATCH bpf-next 03/38] bpf: don't leak the inode when token creation fails

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

bpf_token_create() allocates a bpffs inode and hands it to
alloc_file_pseudo().  The inode reference only moves to the new file when
that call succeeds: alloc_file_pseudo() leaves the caller's reference alone
on failure, and FD_PREPARE() does not even evaluate the file expression
when it cannot get a descriptor.

Before the conversion to FD_PREPARE() the error path did iput(inode).  The
conversion dropped it, so each BPF_TOKEN_CREATE that fails with -EMFILE,
-ENFILE or -ENOMEM at this point leaks an inode.  A task that is allowed to
create tokens can hit this at will by lowering RLIMIT_NOFILE, and the
leaked inodes make umount of that bpffs instance complain about busy
inodes.

Put the inode again if no file was created.

Fixes: 981bec8f698b ("bpf: convert bpf_token_create() to FD_PREPARE()")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/token.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/kernel/bpf/token.c b/kernel/bpf/token.c
index e85a179523f0..19660ef95ddb 100644
--- a/kernel/bpf/token.c
+++ b/kernel/bpf/token.c
@@ -169,8 +169,11 @@ int bpf_token_create(union bpf_attr *attr)
 	FD_PREPARE(fdf, O_CLOEXEC,
 		   alloc_file_pseudo(inode, path.mnt, BPF_TOKEN_INODE_NAME,
 				     O_RDWR, &bpf_token_fops));
-	if (fdf.err)
+	if (fdf.err) {
+		/* no file took over the inode reference */
+		iput(inode);
 		return fdf.err;
+	}
 
 	token = kzalloc_obj(*token, GFP_USER);
 	if (!token)
-- 
2.55.0

[PATCH bpf-next 04/38] bpf: don't hand out a linked element as extra element

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

When htab_map_update_elem() replaces an element of a preallocated map,
alloc_htab_elem() takes the spare element of this cpu and puts the old
element into the per-cpu slot right away, long before the old element is
unlinked from its bucket.  The bucket lock disables interrupts, but it
does not keep out an NMI, or a tracing program on the return of
alloc_htab_elem().  bpf_prog_active only helps if both programs are
kprobe, tracepoint or perf_event programs.  A program that updates an
existing key in another bucket of the same map gets to use the slot:

  htab_map_update_elem(k1)              tc, xdp, fentry, raw_tp, ...
    alloc_htab_elem()
      l_new = *pl_new
      *pl_new = l_old                   still linked in bucket 1
      <NMI>
        htab_map_update_elem(k2)        perf_event program
          l_new = *pl_new               the l_old of the outer update
          hlist_nulls_add_head_rcu(l_new, bucket 2)
          hlist_nulls_del_rcu(k2's old element)
      </NMI>
    hlist_nulls_add_head_rcu(l_new, bucket 1)
    hlist_nulls_del_rcu(l_old)

If l_old was the first element of bucket 1, the outer add points its pprev
at l_new, and the delete takes it out of bucket 1 only.  l_new->next now
leads into bucket 2, where l_old stays with a poisoned pprev, on which a
delete of k2 crashes.  If it was not the first, the delete takes it out of
bucket 2: k2 is gone from the map, and the chain of bucket 1 still contains
l_old, whose next pointer leads into bucket 2.

Either way a walk of bucket 1 ends on the nulls value of bucket 2.
lookup_nulls_elem_raw() restarts whenever that happens, so every lookup in
bucket 1 which does not find its key spins forever:

  rcu: INFO: rcu_preempt detected stalls on CPUs/tasks:
   __htab_map_lookup_elem+0x354/0x4d0

  BUG: KASAN: wild-memory-access in htab_map_delete_elem+0x30c/0x670
  Write of size 8 at addr dead000000000122 by task extra/102

An NMI between the load and the store of the slot makes both updates use
the same element, with much the same result.

Empty the slot when the spare element is taken, in a way that is atomic
on this cpu, and put the old element in only after it has been unlinked.
A nested update that finds the slot empty, which is the case that
corrupted the map, fails with -EBUSY.

Fixes: 8c290e60fa2a ("bpf: fix hashmap extra_elems logic")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/hashtab.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
index 9d4791ee1741..923fec694747 100644
--- a/kernel/bpf/hashtab.c
+++ b/kernel/bpf/hashtab.c
@@ -1279,11 +1279,17 @@ static struct htab_elem *alloc_htab_elem(struct bpf_htab *htab, void *key,
 	if (prealloc) {
 		if (old_elem) {
 			/* if we're updating the existing element,
-			 * use per-cpu extra elems to avoid freelist_pop/push
+			 * use per-cpu extra elems to avoid freelist_pop/push.
+			 * The caller puts old_elem into the slot once it is
+			 * unlinked. Until then the slot is empty for an NMI
+			 * or a kprobe that updates this map on this cpu.
 			 */
 			pl_new = this_cpu_ptr(htab->extra_elems);
-			l_new = *pl_new;
-			*pl_new = old_elem;
+			l_new = READ_ONCE(*pl_new);
+			do {
+				if (!l_new)
+					return ERR_PTR(-EBUSY);
+			} while (!try_cmpxchg_local(pl_new, &l_new, NULL));
 		} else {
 			struct pcpu_freelist_node *l;
 
@@ -1459,11 +1465,14 @@ static long htab_map_update_elem(struct bpf_map *map, void *key, void *value,
 	if (l_old) {
 		hlist_nulls_del_rcu(&l_old->hash_node);
 
-		/* l_old has already been stashed in htab->extra_elems, cancel
-		 * its reusable special fields before it is available for reuse.
+		/* l_old replaces the extra element that alloc_htab_elem() took,
+		 * cancel its reusable special fields before the next update on
+		 * this cpu reuses it.
 		 */
-		if (htab_is_prealloc(htab))
+		if (htab_is_prealloc(htab)) {
+			WRITE_ONCE(*this_cpu_ptr(htab->extra_elems), l_old);
 			check_and_cancel_fields(htab, l_old);
+		}
 	}
 	htab_unlock_bucket(b, flags);
 	if (l_old && !htab_is_prealloc(htab))
-- 
2.55.0

Re: [PATCH bpf-next 04/38] bpf: don't hand out a linked element as extra element

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] bpf: l_old stored in extra_elems before check_and_cancel_fields()
--

commit 2a85541dd93d84eaa60a85ebbff00bfad598417f
Author: Josef Bacik <josef@toxicpanda.com>

bpf: don't hand out a linked element as extra element

This commit atomically clears the per-CPU extra_elems slot when claiming a
spare element during preallocated hash map updates, and delays storing the
replaced element into the slot until after it has been unlinked from its
bucket.

> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> index 9d4791ee17411..923fec694747b 100644
> --- a/kernel/bpf/hashtab.c
> +++ b/kernel/bpf/hashtab.c
> @@ -1279,11 +1279,17 @@ static struct htab_elem *alloc_htab_elem(struct bpf_htab *htab, void *key,
>  	if (prealloc) {
>  		if (old_elem) {
>  			/* if we're updating the existing element,
> -			 * use per-cpu extra elems to avoid freelist_pop/push
> +			 * use per-cpu extra elems to avoid freelist_pop/push.
> +			 * The caller puts old_elem into the slot once it is
> +			 * unlinked. Until then the slot is empty for an NMI
> +			 * or a kprobe that updates this map on this cpu.
>  			 */
>  			pl_new = this_cpu_ptr(htab->extra_elems);
> -			l_new = *pl_new;
> -			*pl_new = old_elem;
> +			l_new = READ_ONCE(*pl_new);
> +			do {
> +				if (!l_new)
> +					return ERR_PTR(-EBUSY);
> +			} while (!try_cmpxchg_local(pl_new, &l_new, NULL));
>  		} else {
>  			struct pcpu_freelist_node *l;
>  
> @@ -1459,11 +1465,14 @@ static long htab_map_update_elem(struct bpf_map *map, void *key, void *value,
>  	if (l_old) {
>  		hlist_nulls_del_rcu(&l_old->hash_node);
>  
> -		/* l_old has already been stashed in htab->extra_elems, cancel
> -		 * its reusable special fields before it is available for reuse.
> +		/* l_old replaces the extra element that alloc_htab_elem() took,
> +		 * cancel its reusable special fields before the next update on
> +		 * this cpu reuses it.
>  		 */
> -		if (htab_is_prealloc(htab))
> +		if (htab_is_prealloc(htab)) {
> +			WRITE_ONCE(*this_cpu_ptr(htab->extra_elems), l_old);
>  			check_and_cancel_fields(htab, l_old);
> +		}
>  	}
>  	htab_unlock_bucket(b, flags);
>  	if (l_old && !htab_is_prealloc(htab))

[Severity: High]
In kernel/bpf/hashtab.c:htab_map_update_elem(), l_old is written to
htab->extra_elems before check_and_cancel_fields(htab, l_old) is called:

    if (htab_is_prealloc(htab)) {
        WRITE_ONCE(*this_cpu_ptr(htab->extra_elems), l_old);
        check_and_cancel_fields(htab, l_old);
    }

Once WRITE_ONCE(*this_cpu_ptr(htab->extra_elems), l_old) executes, the
per-CPU slot is no longer NULL. If an NMI or a tracing program (for example,
fentry on bpf_obj_cancel_fields()) triggers on the same CPU and updates an
existing key in another bucket of the same preallocated map,
alloc_htab_elem() will claim l_old instead of returning -EBUSY.

When the map contains BTF special fields (BPF_TIMER, BPF_WORKQUEUE, or
BPF_TASK_WORK), the nested update can link l_old into the other bucket while
its previous timer, workqueue, or task_work fields are still active, and then
the outer check_and_cancel_fields(htab, l_old) will cancel and free the
special fields of the newly inserted live element.

Should check_and_cancel_fields(htab, l_old) be called before
WRITE_ONCE(*this_cpu_ptr(htab->extra_elems), l_old) so the slot stays NULL
until the special fields have been canceled?

титься

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

Re: [PATCH bpf-next 04/38] bpf: don't hand out a linked element as extra element

[bot+bpf-ci]

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

> bpf: don't hand out a linked element as extra element

TL;DR:
- htab_map_update_elem() fills the extra-elem slot before cancelling l_old's
  fields; a nested update may reuse it, so a live element's timer could be
  cancelled (not memory-unsafe)
- comment style nit

> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> index 9d4791ee1741..923fec694747 100644
> --- a/kernel/bpf/hashtab.c
> +++ b/kernel/bpf/hashtab.c

[ ... ]

> @@ -1459,11 +1465,14 @@ static long htab_map_update_elem(struct bpf_map *map, void *key, void *value,
>  	if (l_old) {
>  		hlist_nulls_del_rcu(&l_old->hash_node);
>  
> -		/* l_old has already been stashed in htab->extra_elems, cancel
> -		 * its reusable special fields before it is available for reuse.
> +		/* l_old replaces the extra element that alloc_htab_elem() took,
> +		 * cancel its reusable special fields before the next update on
> +		 * this cpu reuses it.
>  		 */

this isn't a bug, but this comment is rewritten completely and still keeps
its text on the opening line.  Could the opening /* go on its own line, as
is preferred in kernel/bpf/?

  /*
   * l_old replaces the extra element that alloc_htab_elem() took,
   * cancel its reusable special fields before the next update on
   * this cpu reuses it.
   */

> -		if (htab_is_prealloc(htab))
> +		if (htab_is_prealloc(htab)) {
> +			WRITE_ONCE(*this_cpu_ptr(htab->extra_elems), l_old);
>  			check_and_cancel_fields(htab, l_old);

The new comment says l_old's special fields are cancelled before the next
update on this cpu reuses it.  Is that what this code does?  The slot is
filled first and the fields are cancelled afterwards:

    WRITE_ONCE(*this_cpu_ptr(htab->extra_elems), l_old);
    check_and_cancel_fields(htab, l_old);

Can a nested update on this cpu run between these two calls?  That could be
an NMI (perf_event program), or an fentry program on bpf_obj_cancel_fields(),
which check_and_cancel_fields() calls after the slot is filled and which
tools/testing/selftests/bpf/progs/htab_update.c already attaches an updating
program to.

If that nested htab_map_update_elem() updates an existing key k2 in another
bucket, alloc_htab_elem() takes l_old from the slot.  copy_map_value() skips
the special fields, so the new element still carries the bpf_timer/bpf_wq/
task_work state of k1's replaced value.

The nested update then links l_old into k2's bucket.  When the outer update
resumes, check_and_cancel_fields(htab, l_old) runs:

    bpf_obj_free_timer() -> bpf_async_cancel_and_free()

on what is now k2's live element.

Timers are allowed in hash maps used by tracing/perf_event/fentry programs
(check_map_prog_compatibility() only restricts spin_lock and graph roots), so
this looks reachable.  In that window bpf_timer_init() on k2 returns -EBUSY
(__bpf_async_init() finds the stale async->cb), and any timer armed through
k2 is cancelled and freed right after.  This doesn't look memory-unsafe, and
it is narrower than before the patch, where the slot was filled in
alloc_htab_elem().

The order is also the reverse of free_htab_elem(), which cancels before
pcpu_freelist_push().  Would swapping the two statements (cancel first, then
WRITE_ONCE) make the comment true?  A nested update in that window would then
fail with -EBUSY, like the rest of the window this patch closes.

If the current order is intended, to keep the -EBUSY window short, should the
comment say that cancellation can happen after a nested reuse?

> +		}
>  	}
>  	htab_unlock_bucket(b, flags);
>  	if (l_old && !htab_is_prealloc(htab))


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

[PATCH bpf-next 05/38] bpf: reject sleepable programs for task_vma iterator

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

A sleepable "iter.s/task_vma" program can be attached, but its ->show()
callback runs with the mmap_lock of the iterated mm held for read. As soon
as the program runs, bpf_iter_run_prog() calls might_fault(), and any
bpf_copy_from_user() or bpf_copy_from_user_task() it does may really fault
or take mmap_lock again. With lockdep this is reported as recursive
locking of mmap_lock. Without lockdep, a fault on the reader's own mm, or
access_process_vm() on the iterated mm, has to take mmap_lock for read
again. If a writer queued up in the meantime that blocks behind the
writer, which in turn waits for the read lock the iterator already holds,
and the read() hangs:

    reader (iter.s/task_vma)        other thread of the same mm
    task_vma_seq_get_next()
      mmap_read_lock(mm)
                                    mmap_write_lock(mm)  <- waits
    bpf prog: copy_from_user()
      page fault
        mmap_read_lock(mm)          <- queued behind the writer

The reader waits killable, so the hang lasts until it gets a fatal signal.

bpf_iter_link_attach() allows sleepable programs for the targets that set
BPF_ITER_RESCHED, and task_vma sets it because cond_resched() between VMAs
is fine with mmap_lock held. Rescheduling between objects and running a
sleepable program inside ->show() are different properties, though, and
task_vma only has the first one. The open-coded task_vma iterator does not
have this problem since it hands out a snapshot of the VMA and holds no
lock when it returns to the program.

The vma handed to the program is only stable under mmap_lock, so the lock
can't be dropped around ->show(). Add a feature flag BPF_ITER_NOSLEEP for
targets which can't run sleepable programs although they can reschedule,
and set it for task_vma.

The check at attach time is not enough. BPF_LINK_UPDATE replaces the
program of a link, and bpf_iter_link_replace() only compares the type, the
attach type and the BTF id of the two programs, which are the same for a
sleepable and a non-sleepable one. Do the check there as well. That also
closes this way around the existing rule: so far a sleepable program could
be put on a link of a target that doesn't set BPF_ITER_RESCHED, like the
hash map element iterator, which runs it under rcu_read_lock().

BPF_LINK_CREATE of a sleepable program for the task_vma target, and
BPF_LINK_UPDATE to one, now fail with -EINVAL, also for programs that
don't touch user memory and have been working since sleepable iterator
programs exist. The programs can still be loaded. They have to be
loaded as non-sleepable programs, or use the open-coded iterator.

Fixes: b77fb25dcb34 ("bpf: Add support for bpf iterator programs to use sleepable helpers")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 include/linux/bpf.h    |  3 ++-
 kernel/bpf/bpf_iter.c  | 17 ++++++++++++++---
 kernel/bpf/task_iter.c |  6 +++++-
 3 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 54144372281c..46d325aa178e 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -3137,7 +3137,8 @@ typedef const struct bpf_func_proto *
 			     const struct bpf_prog *prog);
 
 enum bpf_iter_feature {
-	BPF_ITER_RESCHED	= BIT(0),
+	BPF_ITER_RESCHED	= BIT(0),	/* may cond_resched() between objects */
+	BPF_ITER_NOSLEEP	= BIT(1),	/* ->show() can't run sleepable progs */
 };
 
 #define BPF_ITER_CTX_ARG_MAX 2
diff --git a/kernel/bpf/bpf_iter.c b/kernel/bpf/bpf_iter.c
index b40eb404adab..fc3a75684ef7 100644
--- a/kernel/bpf/bpf_iter.c
+++ b/kernel/bpf/bpf_iter.c
@@ -72,6 +72,15 @@ static inline bool bpf_iter_target_support_resched(const struct bpf_iter_target_
 	return tinfo->reg_info->feature & BPF_ITER_RESCHED;
 }
 
+/* Only allow sleepable program for resched-able iterator that can run one */
+static bool bpf_iter_target_support_prog(const struct bpf_iter_target_info *tinfo,
+					 const struct bpf_prog *prog)
+{
+	return !prog->sleepable ||
+	       (bpf_iter_target_support_resched(tinfo) &&
+		!(tinfo->reg_info->feature & BPF_ITER_NOSLEEP));
+}
+
 static bool bpf_iter_support_resched(struct seq_file *seq)
 {
 	struct bpf_iter_priv_data *iter_priv;
@@ -409,6 +418,8 @@ static int bpf_iter_link_replace(struct bpf_link *link,
 				 struct bpf_prog *new_prog,
 				 struct bpf_prog *old_prog)
 {
+	struct bpf_iter_link *iter_link =
+		container_of(link, struct bpf_iter_link, link);
 	int ret = 0;
 
 	mutex_lock(&link_mutex);
@@ -419,7 +430,8 @@ static int bpf_iter_link_replace(struct bpf_link *link,
 
 	if (link->prog->type != new_prog->type ||
 	    link->prog->expected_attach_type != new_prog->expected_attach_type ||
-	    link->prog->aux->attach_btf_id != new_prog->aux->attach_btf_id) {
+	    link->prog->aux->attach_btf_id != new_prog->aux->attach_btf_id ||
+	    !bpf_iter_target_support_prog(iter_link->tinfo, new_prog)) {
 		ret = -EINVAL;
 		goto out_unlock;
 	}
@@ -544,8 +556,7 @@ int bpf_iter_link_attach(const union bpf_attr *attr, bpfptr_t uattr,
 	if (!tinfo)
 		return -ENOENT;
 
-	/* Only allow sleepable program for resched-able iterator */
-	if (prog->sleepable && !bpf_iter_target_support_resched(tinfo))
+	if (!bpf_iter_target_support_prog(tinfo, prog))
 		return -EINVAL;
 
 	link = kzalloc_obj(*link, GFP_USER | __GFP_NOWARN);
diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c
index 13e1aabe6f88..cf2bf388e181 100644
--- a/kernel/bpf/task_iter.c
+++ b/kernel/bpf/task_iter.c
@@ -737,7 +737,11 @@ static const struct bpf_iter_seq_info task_vma_seq_info = {
 static struct bpf_iter_reg task_vma_reg_info = {
 	.target			= "task_vma",
 	.attach_target		= bpf_iter_attach_task,
-	.feature		= BPF_ITER_RESCHED,
+	/*
+	 * ->show() runs with the mmap_lock of the iterated mm held, which a
+	 * sleepable program takes again when it faults on user memory.
+	 */
+	.feature		= BPF_ITER_RESCHED | BPF_ITER_NOSLEEP,
 	.ctx_arg_info_size	= 2,
 	.ctx_arg_info		= {
 		{ offsetof(struct bpf_iter__task_vma, task),
-- 
2.55.0

[PATCH bpf-next 06/38] bpf: forbid sleeping in bpf_find_vma() callbacks

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

A sleepable program can call bpf_find_vma() and use a sleepable helper
such as bpf_copy_from_user() or bpf_copy_from_user_task() in the
callback. The verifier accepts this, but bpf_find_vma() runs the callback
with the mmap_lock of the mm held for read. If the user copy faults on
that mm, the fault handler has to take mmap_lock again. Lockdep reports
recursive locking, and if a writer queued up in between, the fault blocks
behind the writer, which waits for the read lock the helper already
holds:

    sleepable prog                  other thread of the same mm
    bpf_find_vma(current, ...)
      mmap_read_trylock(mm)
                                    mmap_write_lock(mm)  <- waits
      callback: copy_from_user()
        page fault
          mmap_read_lock(mm)        <- queued behind the writer

This is the same problem as with sleepable task_vma iterator programs.
The verifier checks synchronous callbacks in the caller's context, so
in_sleepable_context() is true in the callback of a sleepable program.

Verify the callback like a hash map for_each callback, as running in an
RCU read-side critical section where sleeping is not allowed, and actually
hold rcu_read_lock() around it, so that global functions the callback
calls, which are then verified for a non-sleepable context, see the RCU
protection they are verified with.

Non-sleepable programs already run the callback under RCU, nothing changes
for them. A sleepable program whose bpf_find_vma() callback calls any
helper, kfunc or global function that may sleep is now rejected by the
verifier, including ones that would be harmless under mmap_lock as they
don't touch user memory.

Fixes: 7c7e3d31e785 ("bpf: Introduce helper bpf_find_vma")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/task_iter.c | 6 ++++++
 kernel/bpf/verifier.c  | 2 ++
 2 files changed, 8 insertions(+)

diff --git a/kernel/bpf/task_iter.c b/kernel/bpf/task_iter.c
index cf2bf388e181..2fa6b8dae8c4 100644
--- a/kernel/bpf/task_iter.c
+++ b/kernel/bpf/task_iter.c
@@ -810,8 +810,14 @@ BPF_CALL_5(bpf_find_vma, struct task_struct *, task, u64, start,
 	vma = find_vma(mm, start);
 
 	if (vma && vma->vm_start <= start && vma->vm_end > start) {
+		/*
+		 * The callback must not sleep while mmap_lock is held, and the
+		 * verifier checks it as an RCU read-side critical section.
+		 */
+		rcu_read_lock();
 		callback_fn((u64)(long)task, (u64)(long)vma,
 			    (u64)(long)callback_ctx, 0, 0);
+		rcu_read_unlock();
 		ret = 0;
 	}
 	bpf_mmap_unlock_mm(work, mm);
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 883cf148235c..430d5309eb5c 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -11629,6 +11629,8 @@ static int set_find_vma_callback_state(struct bpf_verifier_env *env,
 	bpf_mark_reg_not_init(env, &callee->regs[BPF_REG_4]);
 	bpf_mark_reg_not_init(env, &callee->regs[BPF_REG_5]);
 	callee->in_callback_fn = true;
+	/* bpf_find_vma() runs the callback under mmap_lock and RCU. */
+	callee->in_rcu_callback_fn = true;
 	callee->callback_ret_range = retval_range(0, 1);
 	return 0;
 }
-- 
2.55.0

[PATCH bpf-next 07/38] bpf: look at all 64 bits of a BPF_JSET mask on 32-bit

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

In the branch of a BPF_JSET which is taken, the verifier can learn
something about the register only if the mask has a single bit: then this
bit is set. regs_refine_cond_op() tests this with is_power_of_2(val) and
then ORs val into var_off.

val is a u64, but is_power_of_2() takes an unsigned long. On a 32-bit
kernel it only sees the lower half of the mask. For

	if r1 & 0xffffffff80000000 goto l

which is a BPF_JSET | BPF_K with an immediate of 0x80000000, the lower
half is a power of two. So the verifier takes all 33 bits for set at l,
while the branch is taken if any of them is. A mask of 0x100000001 from a
register does the same for bit 0.

The log of an i386 kernel, for r6 = bpf_get_prandom_u32() << 31:

	3: (45) if r6 & 0x80000000 goto pc+2
	from 3 to 6: R6=0xffffffff80000000 R10=fp0
	6: (77) r6 >>= 63                     ; R6=1

When the program runs, r6 >> 63 is 0.

From then on the verifier works with known bits which the register does
not have at run time. It accepts, for example, an offset into a map value
which it derives from these bits and believes to be constant, and the
program can access memory outside of the map value.

Count the bits of the whole value.

Only 32-bit kernels are affected. On a 64-bit kernel unsigned long holds
the whole mask, and this patch changes nothing.

Fixes: 960ea056561a ("bpf: verifier: teach the verifier to reason about the BPF_JSET instruction")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/verifier.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 430d5309eb5c..d0ba7156ba4f 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -18145,7 +18145,7 @@ static void regs_refine_cond_op(struct bpf_reg_state *reg1, struct bpf_reg_state
 		 * this restriction. I.e., !(r1 & 0x3) means neither bit 0 nor
 		 * bit 1 is set, which we can readily use in adjustments.
 		 */
-		if (!is_power_of_2(val))
+		if (hweight64(val) != 1)
 			break;
 		if (is_jmp32) {
 			t = tnum_or(tnum_subreg(reg1->var_off), tnum_const(val));
-- 
2.55.0

[PATCH bpf-next 08/38] bpf: don't let bpf_skb_adjust_room() use up the MSS

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

When bpf_skb_adjust_room() grows a TCP GSO packet without
BPF_F_ADJ_ROOM_FIXED_GSO, gso_size is lowered by the number of bytes
added. Nothing looks at how large it is.

gso_size is the size of the segments the peer has sent if the packet
comes out of GRO, and what the sender has written into the virtio header
if it comes from a tun device or a packet socket. If it equals len_diff
the result is 0, and a packet of up to 64 KB is no GSO packet any more. If
it is smaller, the 16 bit value wraps around.

One less than len_diff gives GSO_BY_FRAGS. skb_segment() then takes the
length of each segment from the frag_list. A packet with more than 64 KB
of payload gets that far, and one from a tun device has no frag_list:

  BUG: KASAN: null-ptr-deref in skb_segment+0xcf1/0x1a90
  BUG: kernel NULL pointer dereference, address: 0000000000000070
   tcp_gso_segment+0x1be/0x9f0
   inet_gso_segment+0x288/0x740
   inet_gso_segment+0x288/0x740
   skb_mac_gso_segment+0x179/0x2e0

Refuse to grow such a packet, with the error that other packets which
cannot be adjusted get.

Fixes: 2be7e212d541 ("bpf: add bpf_skb_adjust_room helper")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 net/core/filter.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/core/filter.c b/net/core/filter.c
index ba536be2915f..a18702f89feb 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -3612,6 +3612,11 @@ static int bpf_skb_net_grow(struct sk_buff *skb, u32 off, u32 len_diff,
 			return -ENOTSUPP;
 	}
 
+	/* The MSS is lowered by len_diff: something has to be left of it. */
+	if (skb_is_gso(skb) && !(flags & BPF_F_ADJ_ROOM_FIXED_GSO) &&
+	    skb_shinfo(skb)->gso_size <= len_diff)
+		return -ENOTSUPP;
+
 	ret = skb_cow_head(skb, meta_len + len_diff);
 	if (unlikely(ret < 0))
 		return ret;
-- 
2.55.0

Re: [PATCH bpf-next 08/38] bpf: don't let bpf_skb_adjust_room() use up the MSS

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] bpf: bpf_skb_net_grow() allows reducing TCP gso_size below TCP_MIN_GSO_SIZE
--

commit 0100f53f10670aa140c2329e248e324eafa85486
Author: Josef Bacik <josef@toxicpanda.com>

bpf: don't let bpf_skb_adjust_room() use up the MSS

This commit prevents bpf_skb_adjust_room() from growing a TCP GSO packet
when len_diff is greater than or equal to gso_size and
BPF_F_ADJ_ROOM_FIXED_GSO is not set. This avoids reducing gso_size to 0 or
wrapping the 16-bit value around (including to GSO_BY_FRAGS).

> diff --git a/net/core/filter.c b/net/core/filter.c
> index ba536be2915fb..a18702f89feb3 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -3612,6 +3612,11 @@ static int bpf_skb_net_grow(struct sk_buff *skb, u32 off, u32 len_diff,
>  			return -ENOTSUPP;
>  	}
>  
> +	/* The MSS is lowered by len_diff: something has to be left of it. */
> +	if (skb_is_gso(skb) && !(flags & BPF_F_ADJ_ROOM_FIXED_GSO) &&
> +	    skb_shinfo(skb)->gso_size <= len_diff)
> +		return -ENOTSUPP;

[Severity: High]
Should this check ensure that the remaining gso_size is at least
TCP_MIN_GSO_SIZE rather than just greater than zero?

In bpf_skb_net_grow(), non-TCP GSO packets without BPF_F_ADJ_ROOM_FIXED_GSO
are already rejected just above, so any packet matching this condition is a
TCP GSO packet. The networking stack relies on TCP GSO packets having
gso_size >= TCP_MIN_GSO_SIZE (8) so that the segment count fits in the
16-bit gso_segs field:

include/linux/virtio_net.h:__virtio_net_hdr_to_skb() {
    ...
			BUILD_BUG_ON(TCP_MIN_GSO_SIZE * GSO_MAX_SEGS < GSO_MAX_SIZE);
			gso_size = max(gso_size, TCP_MIN_GSO_SIZE);
    ...
}

With skb_shinfo(skb)->gso_size <= len_diff, bpf_skb_net_grow() still allows
skb_decrease_gso_size() to reduce gso_size into the range 1..7 while setting
SKB_GSO_DODGY and resetting gso_segs to 0. If a large TCP GSO packet (where
skb->len exceeds 65535 * gso_size) is then transmitted, can this overflow the
16-bit gso_segs field and corrupt qdisc_skb_cb(skb)->pkt_len in
qdisc_pkt_len_segs_init():

net/core/dev.c:qdisc_pkt_len_segs_init() {
    ...
		gso_segs = DIV_ROUND_UP(payload, shinfo->gso_size);
		shinfo->gso_segs = gso_segs;
		qdisc_skb_cb(skb)->pkt_segs = gso_segs;
	}
	qdisc_skb_cb(skb)->pkt_len += (gso_segs - 1) * hdr_len;
}

as well as overflow skb_shinfo(skb)->gso_segs in tcp_gso_segment() or trigger
DEBUG_NET_WARN_ON_ONCE(len / mss > GSO_MAX_SEGS) in skb_segment()?

> +
>  	ret = skb_cow_head(skb, meta_len + len_diff);
>  	if (unlikely(ret < 0))
>  		return ret;

endasi

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

[PATCH bpf-next 09/38] bpf: Read the probe of a perf event under event_mutex

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

For a perf event on a kprobe or uprobe event which was made in tracefs,
bpf_get_kprobe_info() and bpf_get_uprobe_info() look the probe up by
name in the list of dynamic events, and hand a pointer to its symbol or
file name back to the caller. None of the callers holds event_mutex.

The perf event keeps its trace event alive, nothing else. Other dynamic
events are removed from the list and freed during the walk. And a probe
which shares its event with other probes can be deleted while the event
is in use, so even the probe which is found can be freed, together with
the string, before the caller has measured and copied it. The result is
a use-after-free in the walk, in strlen() and in the copy to user space,
or a NULL pointer dereference.

The readers are BPF_OBJ_GET_INFO_BY_FD and fdinfo of a perf link, for
whoever holds a descriptor of the link, and BPF_TASK_FD_QUERY. The other
side is anybody who may write to dynamic_events, kprobe_events or
uprobe_events.

Take event_mutex in bpf_get_perf_event_info() and give the caller a copy
of the name, which it has to free.

Fixes: 41bdc4b40ed6 ("bpf: introduce bpf subcommand BPF_TASK_FD_QUERY")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/syscall.c     | 17 +++++++++++------
 kernel/trace/bpf_trace.c | 13 +++++++++++++
 2 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 654f896af265..4a9f28131791 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -4020,19 +4020,19 @@ static int bpf_perf_link_fill_common(const struct perf_event *event,
 		*ulenp = 1;
 	}
 	if (!uname)
-		return 0;
+		goto out;
 
 	if (buf) {
 		err = bpf_copy_to_user(uname, buf, ulen, len);
-		if (err)
-			return err;
 	} else {
 		char zero = '\0';
 
 		if (put_user(zero, uname))
-			return -EFAULT;
+			err = -EFAULT;
 	}
-	return 0;
+out:
+	kfree_const(buf);
+	return err;
 }
 
 #ifdef CONFIG_KPROBE_EVENTS
@@ -4089,6 +4089,7 @@ static void bpf_perf_link_fdinfo_kprobe(const struct perf_event *event,
 		   name, offset, missed, addr,
 		   type == BPF_FD_TYPE_KRETPROBE ?  "kretprobe" : "kprobe",
 		   event->bpf_cookie);
+	kfree_const(name);
 }
 #endif
 
@@ -4142,6 +4143,7 @@ static void bpf_perf_link_fdinfo_uprobe(const struct perf_event *event,
 		   name, offset, ref_ctr_offset,
 		   type == BPF_FD_TYPE_URETPROBE ?  "uretprobe" : "uprobe",
 		   event->bpf_cookie);
+	kfree_const(name);
 }
 #endif
 
@@ -4240,6 +4242,7 @@ static void bpf_tracepoint_link_show_fdinfo(const struct perf_event *event,
 		   "event_type:\t%s\n"
 		   "cookie:\t%llu\n",
 		   name, "tracepoint", event->bpf_cookie);
+	kfree_const(name);
 }
 
 static void bpf_probe_link_show_fdinfo(const struct perf_event *event,
@@ -5763,11 +5766,13 @@ static int bpf_task_fd_query(const union bpf_attr *attr,
 		err = bpf_get_perf_event_info(event, &prog_id, &fd_type,
 					      &buf, &probe_offset,
 					      &probe_addr, NULL);
-		if (!err)
+		if (!err) {
 			err = bpf_task_fd_query_copy(attr, uattr, prog_id,
 						     fd_type, buf,
 						     probe_offset,
 						     probe_addr);
+			kfree_const(buf);
+		}
 		goto put_file;
 	}
 
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 5465e7eba238..304b426bd44d 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -2209,6 +2209,7 @@ int bpf_probe_unregister(struct bpf_raw_event_map *btp, struct bpf_raw_tp_link *
 	return tracepoint_probe_unregister(btp->tp, (void *)btp->bpf_func, link);
 }
 
+/* On success the caller releases *buf with kfree_const() */
 int bpf_get_perf_event_info(const struct perf_event *event, u32 *prog_id,
 			    u32 *fd_type, const char **buf,
 			    u64 *probe_offset, u64 *probe_addr,
@@ -2218,6 +2219,12 @@ int bpf_get_perf_event_info(const struct perf_event *event, u32 *prog_id,
 	struct bpf_prog *prog;
 	int flags, err = 0;
 
+	/*
+	 * The list of dynamic events is walked, and a probe which has siblings
+	 * can be deleted, with its strings, while its event is in use.
+	 */
+	guard(mutex)(&event_mutex);
+
 	prog = event->prog;
 	if (!prog)
 		return -ENOENT;
@@ -2258,6 +2265,12 @@ int bpf_get_perf_event_info(const struct perf_event *event, u32 *prog_id,
 #endif
 	}
 
+	if (!err && *buf) {
+		*buf = kstrdup_const(*buf, GFP_KERNEL);
+		if (!*buf)
+			err = -ENOMEM;
+	}
+
 	return err;
 }
 
-- 
2.55.0

[PATCH bpf-next 10/38] bpf: Don't saturate owner_refcnt on the last unpin

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

bpf_local_storage_map_free() pins the owner with refcount_inc_not_zero()
while it uncharges an element.  The pin counts when
bpf_local_storage_destroy() could not get local_storage->lock: destroy()
then goes on, drops the owner's reference and spins until the count is
zero.  So the pin is the last reference whenever it is needed, and
refcount_dec() takes that for a bug: it warns "decrement hit 0; leaking
memory" and saturates the count.  Unless destroy() happens to read the
count between the two stores it spins for ever, under rcu_read_lock().

Drop the pin with refcount_dec_and_test().

Fixes: 5d800f87d0a5 ("bpf: Support lockless unlink when freeing map or local storage")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 include/linux/bpf_local_storage.h | 13 +++++++++++++
 kernel/bpf/bpf_local_storage.c    |  6 +++---
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/include/linux/bpf_local_storage.h b/include/linux/bpf_local_storage.h
index 9e4f5c45c974..6c643ac51079 100644
--- a/include/linux/bpf_local_storage.h
+++ b/include/linux/bpf_local_storage.h
@@ -104,6 +104,19 @@ struct bpf_local_storage {
 	refcount_t owner_refcnt;/* Used to pin owner when map_free is uncharging */
 };
 
+/*
+ * Drop a reference got by refcount_inc_not_zero(&local_storage->owner_refcnt).
+ * bpf_local_storage_destroy() may have dropped the owner's reference meanwhile
+ * and wait for the count to reach zero, so this one can be the last.
+ */
+static inline void
+bpf_local_storage_unpin_owner(struct bpf_local_storage *local_storage)
+{
+	bool last __maybe_unused;
+
+	last = refcount_dec_and_test(&local_storage->owner_refcnt);
+}
+
 /* U16_MAX is much more than enough for sk local storage
  * considering a tcp_sock is ~2k.
  */
diff --git a/kernel/bpf/bpf_local_storage.c b/kernel/bpf/bpf_local_storage.c
index 4642da062f0f..7721834e4bff 100644
--- a/kernel/bpf/bpf_local_storage.c
+++ b/kernel/bpf/bpf_local_storage.c
@@ -199,7 +199,7 @@ static void bpf_selem_unlink_storage_nolock_misc(struct bpf_local_storage_elem *
 	}
 
 	if (pin_owner)
-		refcount_dec(&local_storage->owner_refcnt);
+		bpf_local_storage_unpin_owner(local_storage);
 }
 
 /* local_storage->lock must be held and selem->local_storage == local_storage.
@@ -753,8 +753,8 @@ u32 bpf_local_storage_destroy(struct bpf_local_storage *local_storage)
 		while (refcount_read(&local_storage->owner_refcnt))
 			cpu_relax();
 		/*
-		 * Paired with refcount_dec() in bpf_selem_unlink_nofail()
-		 * to make sure destroy() sees the correct local_storage->mem_charge.
+		 * Paired with bpf_local_storage_unpin_owner() to make sure
+		 * destroy() sees the correct local_storage->mem_charge.
 		 */
 		smp_mb();
 	}
-- 
2.55.0

[PATCH bpf-next 11/38] bpf: Validate the SRH which End.DT6 uncovers in End.BPF

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

input_action_end_bpf() validates the SRH of a packet and pulls it into the
head before the program runs. If the program decapsulates the packet with
bpf_lwt_seg6_action(SEG6_LOCAL_ACTION_END_DT6), bpf_update_srh_state()
looks for a routing header in the inner packet and takes it for the new
SRH as it is.

Nobody has looked at that header, and whoever has sent the packet has
written it. ipv6_find_hdr() finds it in a fragment of the skb as well, but
hdrlen is then read at that offset from skb->data, past the end of the
head. The header need not be an SRH, and its hdrlen need not fit into the
packet, but it is marked as valid, and bpf_lwt_seg6_adjust_srh() and
bpf_lwt_seg6_store_bytes() check their offsets against it.

A program which then adds a TLV at the end of the SRH, where hdrlen has it,
makes bpf_lwt_seg6_adjust_srh() move and write memory behind the head of
the skb, as far as the sender likes within the 2 KB that hdrlen can say:

  BUG: KASAN: slab-out-of-bounds in bpf_skb_net_hdr_push+0x185/0x240
  Read of size 2088 at addr ffff888107d6f09e by task srh/102
  BUG: KASAN: slab-out-of-bounds in bpf_skb_net_hdr_push+0x1b5/0x240
  Write of size 8 at addr ffff888107d6f8be by task srh/102

Use seg6_get_srh(), which pulls and validates the header. Without a valid
SRH the state says that there is none, and the packet goes its way as
before. The pull can move the head, so compute the data pointers behind
it.

Fixes: 486cdf21583e ("bpf: add End.DT6 action to bpf_lwt_seg6_action helper")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 net/core/filter.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index a18702f89feb..6ac386dea4e8 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -7022,15 +7022,15 @@ static void bpf_update_srh_state(struct sk_buff *skb)
 {
 	struct seg6_bpf_srh_state *srh_state =
 		this_cpu_ptr(&seg6_bpf_srh_states);
-	int srhoff = 0;
 
-	if (ipv6_find_hdr(skb, &srhoff, IPPROTO_ROUTING, NULL, NULL) < 0) {
-		srh_state->srh = NULL;
-	} else {
-		srh_state->srh = (struct ipv6_sr_hdr *)(skb->data + srhoff);
+	/* Behind a decapsulation this is a header that nobody has looked at */
+	srh_state->srh = seg6_get_srh(skb, 0);
+	if (srh_state->srh) {
 		srh_state->hdrlen = srh_state->srh->hdrlen << 3;
 		srh_state->valid = true;
 	}
+	/* it may have been pulled into the head */
+	bpf_compute_data_pointers(skb);
 }
 
 BPF_CALL_4(bpf_lwt_seg6_action, struct sk_buff *, skb,
-- 
2.55.0

[PATCH bpf-next 12/38] bpf: Pull the inner header for End.DT6 in End.BPF

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

bpf_lwt_seg6_action(SEG6_LOCAL_ACTION_END_DT6) pulls the outer headers off
the packet and hands it to seg6_lookup_nexthop(), which reads the inner
IPv6 header at skb->data. Nothing says that this header is in the head of
the skb, or that there is one. If the head ends with the outer headers,
the route is looked up for whatever lies behind the head, and the packet
is dropped or leaves by the wrong route. If the program then pushes an SRH
with SEG6_LOCAL_ACTION_END_B6, seg6_do_srh_inline() pulls an IPv6 header
off a head which has none, and __skb_pull() hits its BUG(): the kernel
panics on a packet from the network.

GRO pulls the inner header of a plain IPv6 in IPv6 packet into the head. It
takes a packet which GRO does not look into that far, one with a fragment
header in front of the inner header for example, or a device without GRO,
and a driver which leaves the inner header in a fragment.

Make sure of the inner header together with the outer ones. The program
goes on behind an error of the helper, so do it before the packet is
changed. A pull which fails can still have moved the head, so look for the
SRH again.

Fixes: 486cdf21583e ("bpf: add End.DT6 action to bpf_lwt_seg6_action helper")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 net/core/filter.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index 6ac386dea4e8..be792b160e18 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -7063,9 +7063,14 @@ BPF_CALL_4(bpf_lwt_seg6_action, struct sk_buff *, skb,
 
 		if (ipv6_find_hdr(skb, &hdroff, IPPROTO_IPV6, NULL, NULL) < 0)
 			return -EBADMSG;
-		if (!pskb_pull(skb, hdroff))
+		/* As long as nothing has been changed, the program goes on */
+		if (!pskb_may_pull(skb, hdroff + sizeof(struct ipv6hdr))) {
+			/* but the head may have moved */
+			bpf_update_srh_state(skb);
 			return -EBADMSG;
+		}
 
+		__skb_pull(skb, hdroff);
 		skb_postpull_rcsum(skb, skb_network_header(skb), hdroff);
 		skb_reset_network_header(skb);
 		skb_reset_transport_header(skb);
-- 
2.55.0

[PATCH bpf-next 13/38] bpf: Update the SRH state if End.B6 fails late

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

For SEG6_LOCAL_ACTION_END_B6 and END_B6_ENCAP bpf_lwt_seg6_action() looks
for the SRH again only if bpf_push_seg6_encap() has succeeded. That
function pushes the new header, for which the head of the skb is
reallocated if it has not got the room, and then looks up the route to the
first segment. If there is none, it returns an error, and the pointer to
the SRH in seg6_bpf_srh_states still points into the old head, which has
been freed.

The program goes on behind the error. bpf_lwt_seg6_store_bytes() and
bpf_lwt_seg6_adjust_srh() read the SRH through that pointer.

Look for the SRH whatever bpf_push_seg6_encap() says.

Fixes: 486cdf21583e ("bpf: add End.DT6 action to bpf_lwt_seg6_action helper")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 net/core/filter.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index be792b160e18..bf01152d0cf9 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -7084,8 +7084,8 @@ BPF_CALL_4(bpf_lwt_seg6_action, struct sk_buff *, skb,
 			return -EBADMSG;
 		err = bpf_push_seg6_encap(skb, BPF_LWT_ENCAP_SEG6_INLINE,
 					  param, param_len);
-		if (!err)
-			bpf_update_srh_state(skb);
+		/* The header can be in place although err says otherwise */
+		bpf_update_srh_state(skb);
 
 		return err;
 	case SEG6_LOCAL_ACTION_END_B6_ENCAP:
@@ -7093,8 +7093,8 @@ BPF_CALL_4(bpf_lwt_seg6_action, struct sk_buff *, skb,
 			return -EBADMSG;
 		err = bpf_push_seg6_encap(skb, BPF_LWT_ENCAP_SEG6,
 					  param, param_len);
-		if (!err)
-			bpf_update_srh_state(skb);
+		/* The header can be in place although err says otherwise */
+		bpf_update_srh_state(skb);
 
 		return err;
 	default:
-- 
2.55.0

[PATCH bpf-next 14/38] bpf: Clear sk_refcnt of a clone before its storage is linked

[Alexei Starovoitov]

From: Alexei Starovoitov <ast@kernel.org>

sk_clone() sets sk_refcnt of the new socket some lines behind
bpf_sk_storage_clone(). But the elements which that function copies are
linked to their maps at once, and an element leads to its owner: the
iterator over a sk_storage map hands the owner to its program.
bpf_sk_storage_get() with BPF_SK_STORAGE_GET_F_CREATE and
bpf_sk_storage_delete() rely on refcount_inc_not_zero() to tell whether
they may touch the socket.

sk_refcnt is in the part of struct sock which sock_copy() leaves out, and
sk_prot_alloc() does not clear a clone unless init_on_alloc is on. It is
zero in an object which has been a socket before, and anything in one
which has not. If it is not zero, a helper gets a reference which
refcount_set() wipes out, and the helper's sock_put() then drops one of
the two references of the new socket, which is freed too early:

  IPv4: Attempt to release TCP socket in state 1
  BUG: KASAN: slab-use-after-free in __raw_spin_lock_init+0x47/0x90

In the meantime the helper changes a storage which bpf_sk_storage_clone()
fills without taking its lock.

Nothing else can find the clone at this point. Say that it is not alive
yet, before the first element is linked. Sockets whose parent has no
storage are left alone.

Fixes: a50a85e40c59 ("bpf: Expose bpf_sk_storage_* to iterator programs")
Suggested-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 net/core/bpf_sk_storage.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c
index 1d295a8769fa..b5349bec5695 100644
--- a/net/core/bpf_sk_storage.c
+++ b/net/core/bpf_sk_storage.c
@@ -164,6 +164,13 @@ int bpf_sk_storage_clone(const struct sock *sk, struct sock *newsk)
 	if (!sk_storage || hlist_empty(&sk_storage->list))
 		goto out;
 
+	/*
+	 * The maps lead to newsk as soon as an element is linked, but
+	 * sk_clone() sets sk_refcnt only later.  Until then it holds whatever
+	 * the slab object held before: no reference can be had yet.
+	 */
+	refcount_set(&newsk->sk_refcnt, 0);
+
 	hlist_for_each_entry_rcu(selem, &sk_storage->list, snode) {
 		struct bpf_local_storage_elem *copy_selem;
 		struct bpf_local_storage_map *smap;
-- 
2.55.0

[PATCH bpf-next 15/38] bpf: Fix NULL deref in the sk_storage map iterator

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

bpf_sk_storage_map_seq_find_next() hands out an element only if its
local_storage pointer is set. __bpf_sk_storage_map_seq_show() then reads
the pointer a second time and dereferences it without a test.

Since bpf_local_storage_destroy() unlinks the elements with
bpf_selem_unlink_nofail(), the pointer is reset to NULL when the socket
which owns the element is destroyed, so it can change between the two
reads. Reading the iterator while sockets with an element in the map are
closed ends in a NULL pointer dereference. The sockets need not belong to
the reader: where a program gives every new socket an element, an
unprivileged user who opens and closes sockets is enough.

Skip such an element, as the TCP and UDP iterators skip a socket which
has been unhashed.

Fixes: 0be08389c7f2 ("bpf: Switch to bpf_selem_unlink_nofail in bpf_local_storage_{map_free, destroy}")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 net/core/bpf_sk_storage.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c
index b5349bec5695..aea67b446c7c 100644
--- a/net/core/bpf_sk_storage.c
+++ b/net/core/bpf_sk_storage.c
@@ -813,6 +813,9 @@ static int __bpf_sk_storage_map_seq_show(struct seq_file *seq,
 		ctx.map = info->map;
 		if (selem) {
 			sk_storage = rcu_dereference(selem->local_storage);
+			/* reset since ->next() looked: the socket goes away */
+			if (!sk_storage)
+				return SEQ_SKIP;
 			ctx.sk = sk_storage->owner;
 			ctx.value = SDATA(selem)->data;
 		}
-- 
2.55.0

[PATCH bpf-next 16/38] bpf: Hold the socket in the sk_storage map iterator

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

The iterator over a sk_storage map passes the owner of an element to its
program as ctx->sk, without a reference. All that it holds is the RCU read
lock, which keeps the element and the storage, not the socket.

An element is unlinked by bpf_sk_storage_free(), which __sk_destruct()
calls right before it frees the socket. That is no better for a socket
with SOCK_RCU_FREE: the grace period comes in front of __sk_destruct(),
so a reader which starts behind call_rcu() still finds the element. The
program and the helpers it hands ctx->sk to, such as
bpf_sk_storage_get() or bpf_sk_storage_delete(), use a socket which is
being freed on another CPU. It takes nothing but reading the iterator
while sockets with an element in the map are closed, by whomever.

Take a reference for the time of the program, and skip the element if
there is none to be had. To look at the owner at all, do what
bpf_selem_unlink_nofail() does for bpf_local_storage_map_free(): as long
as the element is linked under the lock of the storage,
bpf_local_storage_destroy() has not got past it, unless it has given up
on the lock, and then it waits for owner_refcnt.

The last reference may now be dropped by the iterator. The end of a socket
may want process context: rfcomm_sock_destruct() takes a mutex. So only
drop the reference under RCU, and free a socket whose last one it was when
the RCU read lock has been released, which is at the end of a bucket and
of every read() at the latest.

Fixes: c69d2ddb2072 ("bpf: Using rcu_read_lock for bpf_sk_storage_map iterator")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 net/core/bpf_sk_storage.c | 73 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 68 insertions(+), 5 deletions(-)

diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c
index aea67b446c7c..ea22ec5ef5b1 100644
--- a/net/core/bpf_sk_storage.c
+++ b/net/core/bpf_sk_storage.c
@@ -699,8 +699,37 @@ struct bpf_iter_seq_sk_storage_map_info {
 	struct bpf_map *map;
 	unsigned int bucket_id;
 	unsigned skip_elems;
+	struct rcu_head *dead_sks;
 };
 
+/*
+ * sock_put() in two halves. The end of a socket may want process context:
+ * rfcomm_sock_destruct() takes a mutex. If the iterator drops the last
+ * reference, it keeps the socket until it has left the RCU read side critical
+ * section. Nobody uses sk_rcu before sk_destruct().
+ */
+static void
+bpf_sk_storage_map_seq_put_sk(struct bpf_iter_seq_sk_storage_map_info *info,
+			      struct sock *sk)
+{
+	if (refcount_dec_and_test(&sk->sk_refcnt)) {
+		sk->sk_rcu.next = info->dead_sks;
+		info->dead_sks = &sk->sk_rcu;
+	}
+}
+
+static void
+bpf_sk_storage_map_seq_free_sks(struct bpf_iter_seq_sk_storage_map_info *info)
+{
+	struct sock *sk;
+
+	while (info->dead_sks) {
+		sk = container_of(info->dead_sks, struct sock, sk_rcu);
+		info->dead_sks = sk->sk_rcu.next;
+		sk_free(sk);
+	}
+}
+
 static struct bpf_local_storage_elem *
 bpf_sk_storage_map_seq_find_next(struct bpf_iter_seq_sk_storage_map_info *info,
 				 struct bpf_local_storage_elem *prev_selem)
@@ -729,6 +758,7 @@ bpf_sk_storage_map_seq_find_next(struct bpf_iter_seq_sk_storage_map_info *info,
 			/* not found, unlock and go to the next bucket */
 			b = &smap->buckets[bucket_id++];
 			rcu_read_unlock();
+			bpf_sk_storage_map_seq_free_sks(info);
 			skip_elems = 0;
 			break;
 		}
@@ -796,12 +826,44 @@ DEFINE_BPF_ITER_FUNC(bpf_sk_storage_map, struct bpf_iter_meta *meta,
 		     struct bpf_map *map, struct sock *sk,
 		     void *value)
 
+/*
+ * Take a reference on the socket which owns @selem. RCU keeps the element, not
+ * its owner. As long as the element is linked under the lock of the storage,
+ * the owner has not got past bpf_sk_storage_free(), unless that has given up
+ * on the lock. Then it still waits for owner_refcnt.
+ */
+static struct sock *
+bpf_sk_storage_selem_get_sk(struct bpf_local_storage_elem *selem)
+{
+	struct bpf_local_storage *sk_storage;
+	struct sock *sk = NULL;
+	unsigned long flags;
+
+	sk_storage = rcu_dereference(selem->local_storage);
+	/* reset since ->next() looked: the socket goes away */
+	if (!sk_storage)
+		return NULL;
+
+	if (raw_res_spin_lock_irqsave(&sk_storage->lock, flags))
+		return NULL;
+
+	if (!hlist_unhashed(&selem->snode) &&
+	    refcount_inc_not_zero(&sk_storage->owner_refcnt)) {
+		sk = sk_storage->owner;
+		if (!refcount_inc_not_zero(&sk->sk_refcnt))
+			sk = NULL;
+		bpf_local_storage_unpin_owner(sk_storage);
+	}
+	raw_res_spin_unlock_irqrestore(&sk_storage->lock, flags);
+
+	return sk;
+}
+
 static int __bpf_sk_storage_map_seq_show(struct seq_file *seq,
 					 struct bpf_local_storage_elem *selem)
 {
 	struct bpf_iter_seq_sk_storage_map_info *info = seq->private;
 	struct bpf_iter__bpf_sk_storage_map ctx = {};
-	struct bpf_local_storage *sk_storage;
 	struct bpf_iter_meta meta;
 	struct bpf_prog *prog;
 	int ret = 0;
@@ -812,14 +874,14 @@ static int __bpf_sk_storage_map_seq_show(struct seq_file *seq,
 		ctx.meta = &meta;
 		ctx.map = info->map;
 		if (selem) {
-			sk_storage = rcu_dereference(selem->local_storage);
-			/* reset since ->next() looked: the socket goes away */
-			if (!sk_storage)
+			ctx.sk = bpf_sk_storage_selem_get_sk(selem);
+			if (!ctx.sk)
 				return SEQ_SKIP;
-			ctx.sk = sk_storage->owner;
 			ctx.value = SDATA(selem)->data;
 		}
 		ret = bpf_iter_run_prog(prog, &ctx);
+		if (ctx.sk)
+			bpf_sk_storage_map_seq_put_sk(info, ctx.sk);
 	}
 
 	return ret;
@@ -837,6 +899,7 @@ static void bpf_sk_storage_map_seq_stop(struct seq_file *seq, void *v)
 		(void)__bpf_sk_storage_map_seq_show(seq, v);
 	else
 		rcu_read_unlock();
+	bpf_sk_storage_map_seq_free_sks(seq->private);
 }
 
 static int bpf_iter_init_sk_storage_map(void *priv_data,
-- 
2.55.0

Re: [PATCH bpf-next 16/38] bpf: Hold the socket in the sk_storage map iterator

[bot+bpf-ci]

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

> bpf: Hold the socket in the sk_storage map iterator

TL;DR: The sk_storage iterator's new owner check may not hold for a storage
that lost the publish race in bpf_local_storage_alloc(), so it could take a
reference on a freed socket (use-after-free). The window is narrow.

> diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c
> --- a/net/core/bpf_sk_storage.c
> +++ b/net/core/bpf_sk_storage.c

[ ... ]

> @@ -796,12 +826,44 @@ DEFINE_BPF_ITER_FUNC(bpf_sk_storage_map, struct bpf_iter_meta *meta,
>  		     struct bpf_map *map, struct sock *sk,
>  		     void *value)
>
> +/*
> + * Take a reference on the socket which owns @selem. RCU keeps the element, not
> + * its owner. As long as the element is linked under the lock of the storage,
> + * the owner has not got past bpf_sk_storage_free(), unless that has given up
> + * on the lock. Then it still waits for owner_refcnt.
> + */
> +static struct sock *
> +bpf_sk_storage_selem_get_sk(struct bpf_local_storage_elem *selem)
> +{
> +	struct bpf_local_storage *sk_storage;
> +	struct sock *sk = NULL;
> +	unsigned long flags;
> +
> +	sk_storage = rcu_dereference(selem->local_storage);
> +	/* reset since ->next() looked: the socket goes away */
> +	if (!sk_storage)
> +		return NULL;
> +
> +	if (raw_res_spin_lock_irqsave(&sk_storage->lock, flags))
> +		return NULL;
> +
> +	if (!hlist_unhashed(&selem->snode) &&
> +	    refcount_inc_not_zero(&sk_storage->owner_refcnt)) {
> +		sk = sk_storage->owner;
                  ^^^^

Does the invariant in the comment above hold for every storage that can be
reached from a bucket list?

"As long as the element is linked under the lock of the storage, the owner
has not got past bpf_sk_storage_free()" is true for the storage published
in sk->sk_bpf_storage, since that is the only storage
bpf_local_storage_destroy() runs on and waits for.

It does not seem to hold for a storage that lost the publish race in
bpf_local_storage_alloc():

kernel/bpf/bpf_local_storage.c:bpf_local_storage_alloc() {
    ...
	prev_storage = cmpxchg(owner_storage_ptr, NULL, storage);
	if (unlikely(prev_storage)) {
		bpf_selem_unlink_map_nolock(first_selem);
		raw_res_spin_unlock_irqrestore(&b->lock, flags);
		err = -EAGAIN;
		goto uncharge;
	}
    ...
}

bpf_selem_link_map_nolock() has already put first_selem at the head of the
bucket list, so an RCU reader can see it.  On failure the element is
unlinked from the map only.  It stays on storage->list, so snode is still
hashed, first_selem->local_storage still points at the storage,
storage->owner is the socket, and owner_refcnt stays at 1.  Both are then
freed through kfree_rcu().

Consider this sequence.

CPU0 and CPU1 each create the first storage of the same socket at the same
time (bpf_sk_storage_get() with BPF_SK_STORAGE_GET_F_CREATE, or a map
update, for the same map or for two different maps).  CPU0 loses the
cmpxchg.

CPU2 is reading the iterator.  Under rcu_read_lock(),
bpf_sk_storage_map_seq_find_next() reads b->list while CPU0's element E is
linked there, sees E->local_storage != NULL and returns E.  CPU2 is then
delayed before ->show(), for example by preemption under
CONFIG_PREEMPT_RCU.

The socket is closed:

sock_put()
  sk_free()
    __sk_destruct()
      bpf_sk_storage_free()
        /* destroys only the winning storage, never waits on the losing
           storage's owner_refcnt */
    sk_prot_free()
      /* for AF_UNIX or AF_PACKET this is an immediate kmem_cache_free(),
         and the open RCU read section does not delay it */

CPU2 then runs bpf_sk_storage_selem_get_sk(E):

    if (!hlist_unhashed(&selem->snode) &&
        refcount_inc_not_zero(&sk_storage->owner_refcnt)) {
        sk = sk_storage->owner;
        if (!refcount_inc_not_zero(&sk->sk_refcnt))

The losing storage is still RCU-valid, so the lock is taken.
hlist_unhashed(&E->snode) is false, and refcount_inc_not_zero() on
owner_refcnt goes from 1 to 2.  sk is the freed socket, and
refcount_inc_not_zero(&sk->sk_refcnt) operates on freed memory.

If it succeeds, the program runs on the freed socket, and
bpf_sk_storage_map_seq_put_sk() later calls refcount_dec_and_test() on it
and may queue it for sk_free().  For TCP (SLAB_TYPESAFE_BY_RCU), the
iterator can instead take a reference on an unrelated socket that has
reused the object.

This window was already there before the patch, since the old code passed
storage->owner to the program without any check.  It is also narrow, since
E is in the bucket only for a few instructions under b->lock.  But the new
check is meant to make ctx->sk safe, and the iterator now also writes to
the stale owner itself.

Does bpf_local_storage_alloc() need to make the losing storage look dead
before it is freed, for example by dropping its owner_refcnt (or unlinking
first_selem->snode) under storage->lock in the cmpxchg failure path, so that
the iterator's check holds for every storage?

> +		if (!refcount_inc_not_zero(&sk->sk_refcnt))
> +			sk = NULL;
> +		bpf_local_storage_unpin_owner(sk_storage);
> +	}
> +	raw_res_spin_unlock_irqrestore(&sk_storage->lock, flags);
> +
> +	return sk;
> +}

[ ... ]


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

[PATCH bpf-next 17/38] bpf: Bound the nesting that the BTF show code follows

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

btf_struct_show() and btf_array_show() recurse for every nested struct
and array.  MAX_RESOLVE_DEPTH bounds one resolve walk when BTF is loaded,
not the nesting: a member whose type is resolved already is not walked
again, so BTF that defines its types from the inside out can nest them as
deep as it has types.

Whoever may load BTF, with CAP_BPF or a BPF token, can give such a type
to the value of a map.  Reading the file of that map in bpffs then
overruns the kernel stack: with a few hundred levels the stack guard page
is hit and the kernel panics.

Stop at 32 levels.  That is MAX_RESOLVE_DEPTH, and __btf_show_indent()
has as many columns.  Nothing is shown for a member that lies deeper, and
the status becomes -E2BIG, which bpf_snprintf_btf() and
bpf_seq_printf_btf() return.  The types of an x86_64 defconfig kernel nest
14 deep.

Fixes: b00b8daec828 ("bpf: btf: Add pretty print capability for data with BTF type info")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/btf.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 64c78434fa05..a9bf4c3c0a27 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -1081,6 +1081,8 @@ static const struct btf_type *btf_type_skip_qualifiers(const struct btf *btf,
 }
 
 #define BTF_SHOW_MAX_ITER	10
+/* nested structs and arrays shown; __btf_show_indent() has as many columns */
+#define BTF_SHOW_MAX_DEPTH	32
 
 #define BTF_KIND_BIT(kind)	(1ULL << kind)
 
@@ -1474,8 +1476,18 @@ static void *btf_show_start_aggr_type(struct btf_show *show,
 				      const struct btf_type *t,
 				      u32 type_id, void *data)
 {
-	void *safe_data = btf_show_start_type(show, t, type_id, data);
+	void *safe_data;
 
+	/*
+	 * Structs and arrays are shown by recursion.  BTF loading bounds the
+	 * depth of one resolve walk, not how deep resolved types may nest.
+	 */
+	if (show->state.depth >= BTF_SHOW_MAX_DEPTH) {
+		show->state.status = -E2BIG;
+		return NULL;
+	}
+
+	safe_data = btf_show_start_type(show, t, type_id, data);
 	if (!safe_data)
 		return safe_data;
 
-- 
2.55.0

Re: [PATCH bpf-next 17/38] bpf: Bound the nesting that the BTF show code follows

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] bpf: btf_show_obj_safe() clobbers -E2BIG set by btf_show_start_aggr_type()
- [Medium] bpf: btf_show_start_array_type() leaks array_encoding on max depth error
--

commit f2e2b108df464bcad9ef6d63627fcf113f33d61f
Author: Josef Bacik <josef@toxicpanda.com>

bpf: Bound the nesting that the BTF show code follows

This commit limits the recursion depth in btf_show_start_aggr_type() to
BTF_SHOW_MAX_DEPTH (32) and sets show->state.status to -E2BIG to prevent
kernel stack overflows when pretty-printing deeply nested BTF structs and
arrays.

> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 64c78434fa050..a9bf4c3c0a273 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -1081,6 +1081,8 @@ static const struct btf_type *btf_type_skip_qualifiers(const struct btf *btf,
>  }
>  
>  #define BTF_SHOW_MAX_ITER	10
> +/* nested structs and arrays shown; __btf_show_indent() has as many columns */
> +#define BTF_SHOW_MAX_DEPTH	32
>  
>  #define BTF_KIND_BIT(kind)	(1ULL << kind)
>  
> @@ -1474,8 +1476,18 @@ static void *btf_show_start_aggr_type(struct btf_show *show,
>  				      const struct btf_type *t,
>  				      u32 type_id, void *data)
>  {
> -	void *safe_data = btf_show_start_type(show, t, type_id, data);
> +	void *safe_data;
>  
> +	/*
> +	 * Structs and arrays are shown by recursion.  BTF loading bounds the
> +	 * depth of one resolve walk, not how deep resolved types may nest.
> +	 */
> +	if (show->state.depth >= BTF_SHOW_MAX_DEPTH) {
> +		show->state.status = -E2BIG;
> +		return NULL;
> +	}

[Severity: Medium]
Can show->state.status = -E2BIG get overwritten by a subsequent call to
btf_show_obj_safe()?

Returning NULL from btf_show_start_aggr_type() stops the current aggregate at
depth 32, but enclosing callers in __btf_struct_show() and __btf_array_show()
continue iterating over their remaining members or elements (as well as running
the second display pass after the depth_check pass in btf_struct_show() and
btf_array_show() when BTF_SHOW_ZERO is not set).

When bpf_snprintf_btf() or bpf_seq_printf_btf() runs, BTF_SHOW_UNSAFE is not
set. If any subsequent member or pass calls
btf_show_start_type()->btf_show_obj_safe() for data outside the current 32-byte
show->obj.safe window, btf_show_obj_safe() unconditionally overwrites
show->state.status:

kernel/bpf/btf.c:btf_show_obj_safe() {
	if (!safe) {
		size_left = btf_show_obj_size_left(show, data);
		if (size_left > BTF_SHOW_OBJ_SAFE_SIZE)
			size_left = BTF_SHOW_OBJ_SAFE_SIZE;
		show->state.status = copy_from_kernel_nofault(show->obj.safe,
							      data, size_left);
		if (!show->state.status) {
			show->obj.data = data;
			safe = show->obj.safe;
		}
	}
}

When copy_from_kernel_nofault() succeeds, show->state.status is reset to 0, so
btf_type_snprintf_show() and btf_type_seq_show_flags() will return success
instead of -E2BIG.

[Severity: Medium]
Also, when btf_show_start_aggr_type() returns NULL at BTF_SHOW_MAX_DEPTH, does
btf_show_start_array_type() leave show->state.array_encoding set?

In __btf_array_show(), a 1-byte integer array sets encoding = BTF_INT_CHAR and
calls btf_show_start_array_type(), which sets show->state.array_encoding before
calling btf_show_start_aggr_type():

kernel/bpf/btf.c:btf_show_start_array_type() {
	show->state.array_encoding = array_encoding;
	show->state.array_terminated = 0;
	return btf_show_start_aggr_type(show, t, type_id, data);
}

When btf_show_start_aggr_type() returns NULL, __btf_array_show() returns early
without calling btf_show_end_array_type() to clear show->state.array_encoding:

kernel/bpf/btf.c:__btf_array_show() {
	if (!btf_show_start_array_type(show, t, type_id, encoding, data))
		return;
}

When recursion unwinds to enclosing structs, any subsequent scalar 8-bit
integer member visited by btf_int_show() will see
show->state.array_encoding == BTF_INT_CHAR, causing printable bytes to be
formatted as character literals and a zero byte to set
show->state.array_terminated = 1, suppressing subsequent 8-bit integer fields:

kernel/bpf/btf.c:btf_int_show() {
	case 8:
		if (show->state.array_encoding == BTF_INT_CHAR) {
			/* check for null terminator */
			if (show->state.array_terminated)
				break;
			if (*(char *)data == '\0') {
				show->state.array_terminated = 1;
				break;
			}
}

> +
> +	safe_data = btf_show_start_type(show, t, type_id, data);
>  	if (!safe_data)
>  		return safe_data;
>

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

[PATCH bpf-next 18/38] bpf: Check the new program of an iterator link update

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

The bpf_map_elem, bpf_sk_storage_map and sockmap iterators hand the key
and the value of an element to the program as buffers.  The verifier only
records how far a program reaches into them, and ->attach_target()
compares that with the key and value size of the map.

bpf_iter_link_replace() does not.  BPF_LINK_UPDATE installs a program that
an attach refuses with -EACCES, and the program then reads, and for a
value writes, behind every element that the iterator visits.  It takes
CAP_BPF and CAP_PERFMON, or a BPF token.

Split the test out of the three ->attach_target() and ask it, as
->check_prog(), for the new program of an update as well.  Targets without
such a test take every program of their kind, as before.

Fixes: d6c4503cc296 ("bpf: Implement bpf iterator for hash maps")
Fixes: 5ce6e77c7edf ("bpf: Implement bpf iterator for sock local storage map")
Fixes: 0365351524d7 ("net: Allow iterating sockmap and sockhash")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 include/linux/bpf.h       |  4 +++
 kernel/bpf/bpf_iter.c     |  7 ++++++
 kernel/bpf/map_iter.c     | 51 ++++++++++++++++++++++-----------------
 net/core/bpf_sk_storage.c | 17 ++++++++++---
 net/core/sock_map.c       | 17 ++++++++++---
 5 files changed, 66 insertions(+), 30 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 46d325aa178e..3fb3c7d22d99 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -3128,6 +3128,8 @@ typedef int (*bpf_iter_attach_target_t)(struct bpf_prog *prog,
 					union bpf_iter_link_info *linfo,
 					struct bpf_iter_aux_info *aux);
 typedef void (*bpf_iter_detach_target_t)(struct bpf_iter_aux_info *aux);
+typedef int (*bpf_iter_check_prog_t)(const struct bpf_prog *prog,
+				     const struct bpf_iter_aux_info *aux);
 typedef void (*bpf_iter_show_fdinfo_t) (const struct bpf_iter_aux_info *aux,
 					struct seq_file *seq);
 typedef int (*bpf_iter_fill_link_info_t)(const struct bpf_iter_aux_info *aux,
@@ -3146,6 +3148,8 @@ struct bpf_iter_reg {
 	const char *target;
 	bpf_iter_attach_target_t attach_target;
 	bpf_iter_detach_target_t detach_target;
+	/* may the program run on what the link is attached to? */
+	bpf_iter_check_prog_t check_prog;
 	bpf_iter_show_fdinfo_t show_fdinfo;
 	bpf_iter_fill_link_info_t fill_link_info;
 	bpf_iter_get_func_proto_t get_func_proto;
diff --git a/kernel/bpf/bpf_iter.c b/kernel/bpf/bpf_iter.c
index fc3a75684ef7..4668c1696a08 100644
--- a/kernel/bpf/bpf_iter.c
+++ b/kernel/bpf/bpf_iter.c
@@ -436,6 +436,13 @@ static int bpf_iter_link_replace(struct bpf_link *link,
 		goto out_unlock;
 	}
 
+	if (iter_link->tinfo->reg_info->check_prog) {
+		ret = iter_link->tinfo->reg_info->check_prog(new_prog,
+							     &iter_link->aux);
+		if (ret)
+			goto out_unlock;
+	}
+
 	old_prog = xchg(&link->prog, new_prog);
 	bpf_prog_put(old_prog);
 
diff --git a/kernel/bpf/map_iter.c b/kernel/bpf/map_iter.c
index 2077d46d167c..16537356c1a2 100644
--- a/kernel/bpf/map_iter.c
+++ b/kernel/bpf/map_iter.c
@@ -97,13 +97,29 @@ static struct bpf_iter_reg bpf_map_reg_info = {
 	.seq_info		= &bpf_map_seq_info,
 };
 
+static int bpf_iter_map_check_prog(const struct bpf_prog *prog,
+				   const struct bpf_iter_aux_info *aux)
+{
+	const struct bpf_map *map = aux->map;
+	u32 value_size = map->value_size;
+
+	if (map->map_type == BPF_MAP_TYPE_PERCPU_HASH ||
+	    map->map_type == BPF_MAP_TYPE_LRU_PERCPU_HASH ||
+	    map->map_type == BPF_MAP_TYPE_PERCPU_ARRAY)
+		value_size = round_up(value_size, 8) * num_possible_cpus();
+
+	if (prog->aux->max_rdonly_access > map->key_size ||
+	    prog->aux->max_rdwr_access > value_size)
+		return -EACCES;
+
+	return 0;
+}
+
 static int bpf_iter_attach_map(struct bpf_prog *prog,
 			       union bpf_iter_link_info *linfo,
 			       struct bpf_iter_aux_info *aux)
 {
-	u32 key_acc_size, value_acc_size, key_size, value_size;
 	struct bpf_map *map;
-	bool is_percpu = false;
 	int err = -EINVAL;
 
 	if (!linfo->map.map_fd)
@@ -123,30 +139,20 @@ static int bpf_iter_attach_map(struct bpf_prog *prog,
 		goto put_map;
 	}
 
-	if (map->map_type == BPF_MAP_TYPE_PERCPU_HASH ||
-	    map->map_type == BPF_MAP_TYPE_LRU_PERCPU_HASH ||
-	    map->map_type == BPF_MAP_TYPE_PERCPU_ARRAY)
-		is_percpu = true;
-	else if (map->map_type != BPF_MAP_TYPE_HASH &&
-		 map->map_type != BPF_MAP_TYPE_LRU_HASH &&
-		 map->map_type != BPF_MAP_TYPE_ARRAY &&
-		 map->map_type != BPF_MAP_TYPE_RHASH)
+	if (map->map_type != BPF_MAP_TYPE_PERCPU_HASH &&
+	    map->map_type != BPF_MAP_TYPE_LRU_PERCPU_HASH &&
+	    map->map_type != BPF_MAP_TYPE_PERCPU_ARRAY &&
+	    map->map_type != BPF_MAP_TYPE_HASH &&
+	    map->map_type != BPF_MAP_TYPE_LRU_HASH &&
+	    map->map_type != BPF_MAP_TYPE_ARRAY &&
+	    map->map_type != BPF_MAP_TYPE_RHASH)
 		goto put_map;
 
-	key_acc_size = prog->aux->max_rdonly_access;
-	value_acc_size = prog->aux->max_rdwr_access;
-	key_size = map->key_size;
-	if (!is_percpu)
-		value_size = map->value_size;
-	else
-		value_size = round_up(map->value_size, 8) * num_possible_cpus();
-
-	if (key_acc_size > key_size || value_acc_size > value_size) {
-		err = -EACCES;
+	aux->map = map;
+	err = bpf_iter_map_check_prog(prog, aux);
+	if (err)
 		goto put_map;
-	}
 
-	aux->map = map;
 	return 0;
 
 put_map:
@@ -179,6 +185,7 @@ static const struct bpf_iter_reg bpf_map_elem_reg_info = {
 	.target			= "bpf_map_elem",
 	.attach_target		= bpf_iter_attach_map,
 	.detach_target		= bpf_iter_detach_map,
+	.check_prog		= bpf_iter_map_check_prog,
 	.show_fdinfo		= bpf_iter_map_show_fdinfo,
 	.fill_link_info		= bpf_iter_map_fill_link_info,
 	.ctx_arg_info_size	= 2,
diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c
index ea22ec5ef5b1..72b9ee4c98de 100644
--- a/net/core/bpf_sk_storage.c
+++ b/net/core/bpf_sk_storage.c
@@ -919,6 +919,15 @@ static void bpf_iter_fini_sk_storage_map(void *priv_data)
 	bpf_map_put_with_uref(seq_info->map);
 }
 
+static int bpf_iter_map_check_prog(const struct bpf_prog *prog,
+				   const struct bpf_iter_aux_info *aux)
+{
+	if (prog->aux->max_rdwr_access > aux->map->value_size)
+		return -EACCES;
+
+	return 0;
+}
+
 static int bpf_iter_attach_map(struct bpf_prog *prog,
 			       union bpf_iter_link_info *linfo,
 			       struct bpf_iter_aux_info *aux)
@@ -936,12 +945,11 @@ static int bpf_iter_attach_map(struct bpf_prog *prog,
 	if (map->map_type != BPF_MAP_TYPE_SK_STORAGE)
 		goto put_map;
 
-	if (prog->aux->max_rdwr_access > map->value_size) {
-		err = -EACCES;
+	aux->map = map;
+	err = bpf_iter_map_check_prog(prog, aux);
+	if (err)
 		goto put_map;
-	}
 
-	aux->map = map;
 	return 0;
 
 put_map:
@@ -972,6 +980,7 @@ static struct bpf_iter_reg bpf_sk_storage_map_reg_info = {
 	.target			= "bpf_sk_storage_map",
 	.attach_target		= bpf_iter_attach_map,
 	.detach_target		= bpf_iter_detach_map,
+	.check_prog		= bpf_iter_map_check_prog,
 	.show_fdinfo		= bpf_iter_map_show_fdinfo,
 	.fill_link_info		= bpf_iter_map_fill_link_info,
 	.ctx_arg_info_size	= 2,
diff --git a/net/core/sock_map.c b/net/core/sock_map.c
index 38df84284328..057e2f3c0265 100644
--- a/net/core/sock_map.c
+++ b/net/core/sock_map.c
@@ -1933,6 +1933,15 @@ int sock_map_link_create(const union bpf_attr *attr, struct bpf_prog *prog)
 	return ret;
 }
 
+static int sock_map_iter_check_prog(const struct bpf_prog *prog,
+				    const struct bpf_iter_aux_info *aux)
+{
+	if (prog->aux->max_rdonly_access > aux->map->key_size)
+		return -EACCES;
+
+	return 0;
+}
+
 static int sock_map_iter_attach_target(struct bpf_prog *prog,
 				       union bpf_iter_link_info *linfo,
 				       struct bpf_iter_aux_info *aux)
@@ -1951,12 +1960,11 @@ static int sock_map_iter_attach_target(struct bpf_prog *prog,
 	    map->map_type != BPF_MAP_TYPE_SOCKHASH)
 		goto put_map;
 
-	if (prog->aux->max_rdonly_access > map->key_size) {
-		err = -EACCES;
+	aux->map = map;
+	err = sock_map_iter_check_prog(prog, aux);
+	if (err)
 		goto put_map;
-	}
 
-	aux->map = map;
 	return 0;
 
 put_map:
@@ -1973,6 +1981,7 @@ static struct bpf_iter_reg sock_map_iter_reg = {
 	.target			= "sockmap",
 	.attach_target		= sock_map_iter_attach_target,
 	.detach_target		= sock_map_iter_detach_target,
+	.check_prog		= sock_map_iter_check_prog,
 	.show_fdinfo		= bpf_iter_map_show_fdinfo,
 	.fill_link_info		= bpf_iter_map_fill_link_info,
 	.ctx_arg_info_size	= 2,
-- 
2.55.0

[PATCH bpf-next 19/38] bpf: Fix stale offload->prog pointer after realloc

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

bpf_prog_realloc() moves a program that the rewrites of the verifier make
outgrow its pages.  It updates aux->prog, but not the offload->prog of a
device-bound program.  When the device is unregistered,
__bpf_offload_dev_netdev_unregister() hands the stale pointer to
__bpf_prog_offload_destroy(), which faults on the freed program:

  BUG: unable to handle page fault for address: ffffc90000071050
  RIP: 0010:__bpf_prog_offload_destroy+0x3f/0x310

It takes the right to load a device-bound XDP program, CAP_BPF and
CAP_NET_ADMIN or a BPF token, and a device that can be removed.

Commit a1aa9ef47c29 ("bpf: Fix stale offload->prog pointer after constant
blinding") repaired the other place that replaces a program.  Do the same
here.

Fixes: ab3f0063c48c ("bpf: offload: add infrastructure for loading programs for a specific netdev")
Fixes: 2b3486bc2d23 ("bpf: Introduce device-bound XDP programs")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 642f0ab8eb2d..2154c18a5a4d 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -269,6 +269,8 @@ struct bpf_prog *bpf_prog_realloc(struct bpf_prog *fp_old, unsigned int size,
 		memcpy(fp, fp_old, fp_old->pages * PAGE_SIZE);
 		fp->pages = pages;
 		fp->aux->prog = fp;
+		if (fp->aux->offload)
+			fp->aux->offload->prog = fp;
 
 		/* We keep fp->aux from fp_old around in the new
 		 * reallocated structure.
-- 
2.55.0

Re: [PATCH bpf-next 19/38] bpf: Fix stale offload->prog pointer after realloc

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] bpf: unlocked offload access in bpf_prog_realloc() causes use-after-free
--

commit 3691aa3c9c7d9f29e407360a3a32c4201c98c86f
Author: Josef Bacik <josef@toxicpanda.com>

bpf: Fix stale offload->prog pointer after realloc

This commit updates fp->aux->offload->prog in bpf_prog_realloc() when a
device-bound BPF program is reallocated during verifier rewrites so that
netdev unregistration does not pass a stale program pointer to
__bpf_prog_offload_destroy().

> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index 642f0ab8eb2d8..2154c18a5a4d8 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -269,6 +269,8 @@ struct bpf_prog *bpf_prog_realloc(struct bpf_prog *fp_old, unsigned int size,
>  		memcpy(fp, fp_old, fp_old->pages * PAGE_SIZE);
>  		fp->pages = pages;
>  		fp->aux->prog = fp;
> +		if (fp->aux->offload)
> +			fp->aux->offload->prog = fp;

[Severity: Critical]
Does this access to fp->aux->offload and the subsequent freeing of fp_old
need synchronization with bpf_devs_lock?

During bpf_prog_load(), bpf_prog_dev_bound_init() links offload into
ondev->progs under bpf_devs_lock and then releases the lock before
bpf_check() runs. As noted in kernel/bpf/offload.c:

kernel/bpf/offload.c:bpf_dev_bound_resolve_kfunc() {
	/* We don't hold bpf_devs_lock while resolving several
	 * kfuncs and can race with the unregister_netdevice().
	 * We rely on bpf_dev_bound_match() check at attach
	 * to render this program unusable.
	 */
	down_read(&bpf_devs_lock);
	if (!prog->aux->offload)
		goto out;
    ...
}

When verifier instruction rewrites grow the program via
bpf_check() -> bpf_patch_insn_data() -> bpf_patch_insn_single() ->
bpf_prog_realloc(), bpf_prog_realloc() runs without bpf_devs_lock while
concurrent netdev unregistration can run on another CPU under
down_write(&bpf_devs_lock):

kernel/bpf/offload.c:__bpf_offload_dev_netdev_unregister() {
    ...
		list_for_each_entry_safe(offload, ptmp, &ondev->progs, offloads)
			__bpf_prog_offload_destroy(offload->prog);
    ...
}

kernel/bpf/offload.c:__bpf_prog_offload_destroy() {
	struct bpf_prog_offload *offload = prog->aux->offload;

	if (offload->dev_state)
		offload->offdev->ops->destroy(prog);

	list_del_init(&offload->offloads);
	kfree(offload);
	prog->aux->offload = NULL;
}

Can this race in two ways?

1. bpf_prog_realloc() checks if (fp->aux->offload) and sees a non-NULL
   pointer, while __bpf_prog_offload_destroy() concurrently frees offload
   and sets prog->aux->offload = NULL, causing fp->aux->offload->prog = fp
   to write to freed slab memory or dereference NULL.

2. __bpf_offload_dev_netdev_unregister() reads offload->prog (still fp_old)
   and passes it to __bpf_prog_offload_destroy(fp_old), while
   bpf_prog_realloc() concurrently clears fp_old->aux = NULL and frees
   fp_old via __bpf_prog_free(fp_old), causing __bpf_prog_offload_destroy()
   to dereference a NULL prog->aux or freed fp_old.

>  
>  		/* We keep fp->aux from fp_old around in the new
>  		 * reallocated structure.

—
🤖 Generated bylinux-kernel-bot

Assisted-by: Google Gemini-3.1-Pro

---
Do youhave feedback on this review? Please let us know by replying to this email or filing an issue at https://github.com/google/linux-kernel-bot/issues.

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

Re: [PATCH bpf-next 19/38] bpf: Fix stale offload->prog pointer after realloc

[bot+bpf-ci]

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

> bpf: Fix stale offload->prog pointer after realloc

TL;DR: The new offload->prog update in bpf_prog_realloc() runs without
bpf_devs_lock, so it may race with device unregistration: possible write to
freed memory, and the stale-pointer use-after-free is only narrowed.

> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index 642f0ab8eb2d..2154c18a5a4d 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -269,6 +269,8 @@ struct bpf_prog *bpf_prog_realloc(struct bpf_prog *fp_old, unsigned int size,
>  		memcpy(fp, fp_old, fp_old->pages * PAGE_SIZE);
>  		fp->pages = pages;
>  		fp->aux->prog = fp;
> +		if (fp->aux->offload)
> +			fp->aux->offload->prog = fp;
>
>  		/* We keep fp->aux from fp_old around in the new
>  		 * reallocated structure.

Can this race with device unregistration?  The new lines read
fp->aux->offload and write offload->prog without holding bpf_devs_lock, and
unregistration can free that offload at the same moment, so the store may
land in freed memory.

bpf_prog_load() calls bpf_prog_dev_bound_init() before bpf_check().
bpf_prog_dev_bound_init() adds the offload to ondev->progs, then drops
bpf_devs_lock and does dev_put(netdev).  After that nothing holds off
unregistration while the verifier runs:

bpf_check()
  ...
  __bpf_patch_insn_data()
    bpf_patch_insn_single()
      bpf_prog_realloc()

At the same time unregister_netdevice_many_notify() ->
bpf_dev_bound_netdev_unregister() takes only rtnl and
down_write(&bpf_devs_lock) and runs:

    list_for_each_entry_safe(offload, ptmp, &ondev->progs, offloads)
        __bpf_prog_offload_destroy(offload->prog);

where __bpf_prog_offload_destroy() does:

    struct bpf_prog_offload *offload = prog->aux->offload;
    ...
    list_del_init(&offload->offloads);
    kfree(offload);
    prog->aux->offload = NULL;

Timeline 1, write to freed memory:

  CPU0 bpf_prog_realloc():
    if (fp->aux->offload)          <- sees offload O, not NULL
  CPU1 bpf_dev_bound_netdev_unregister():
    down_write(&bpf_devs_lock);
    __bpf_prog_offload_destroy(fp_old);   <- kfree(O), aux->offload = NULL
    up_write(&bpf_devs_lock);
  CPU0:
    fp->aux->offload->prog = fp;   <- 8-byte write at offset 0 of freed
                                      struct bpf_prog_offload

If the compiler reloads aux->offload instead of reusing the value it
tested, this becomes a NULL pointer write.

Timeline 2, the stale pointer is narrowed but not closed:

  CPU1 (under bpf_devs_lock) reads offload->prog == fp_old
  CPU0 stores fp into offload->prog, sets fp_old->aux = NULL, and calls
       __bpf_prog_free(fp_old), which ends in vfree(fp_old)
  CPU1 __bpf_prog_offload_destroy(fp_old) then reads fp_old->aux from
       vfree'd memory (or reads NULL)

That is the same fault quoted in the commit message.

Every other place that touches prog->aux->offload during verification takes
bpf_devs_lock and rechecks the pointer under it, because unregistration can
race with verification: bpf_prog_offload_replace_insn(),
bpf_prog_offload_verify_insn(), bpf_prog_offload_finalize(),
bpf_prog_offload_remove_insns() and bpf_dev_bound_resolve_kfunc().  The last
one carries the comment "We don't hold bpf_devs_lock while resolving several
kfuncs and can race with the unregister_netdevice()".  The new code does not
follow that pattern.

bpf_prog_realloc() can already sleep (__vmalloc with GFP_KERNEL), so could
the check and the store be done under down_read(&bpf_devs_lock), held until
before fp_old is freed?  bpf_devs_lock is static in kernel/bpf/offload.c, so
that would need a small helper there, with a stub for
!CONFIG_NET || !CONFIG_BPF_SYSCALL.  The unregister side holds the lock for
writing for its whole scan-and-destroy, so this would exclude both timelines.

The sibling fix a1aa9ef47c29 in bpf_jit_prog_release_other() has the same
unlocked pattern.  No later commit in the series range changes this code.


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

[PATCH bpf-next 20/38] bpf: Don't bind a program to a netdev that goes away

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

bpf_prog_dev_bound_init() looks the device up, takes bpf_devs_lock and
registers the device for the program.  The reference it holds does not
keep the device registered.  If bpf_dev_bound_netdev_unregister() has run
by the time the lock is taken, nobody takes the program off the device any
more: the bpf_offload_netdev and offload->netdev point to a net_device
that is freed as soon as the reference is dropped.
bpf_dev_bound_resolve_kfunc() dereferences it while the program is
verified, and a new device at the same address finds the old entry.

unregister_netdevice_many_notify() leaves NETREG_REGISTERED before it
calls bpf_dev_bound_netdev_unregister(), which takes bpf_devs_lock.  Look
at reg_state under that lock, and treat a device that goes like one that
is not there.

Fixes: 2b3486bc2d23 ("bpf: Introduce device-bound XDP programs")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/offload.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/kernel/bpf/offload.c b/kernel/bpf/offload.c
index d855399812ee..c615ed46f629 100644
--- a/kernel/bpf/offload.c
+++ b/kernel/bpf/offload.c
@@ -250,7 +250,14 @@ int bpf_prog_dev_bound_init(struct bpf_prog *prog, union bpf_attr *attr)
 	prog->aux->offload_requested = !(attr->prog_flags & BPF_F_XDP_DEV_BOUND_ONLY);
 
 	down_write(&bpf_devs_lock);
-	err = __bpf_prog_dev_bound_init(prog, netdev);
+	/*
+	 * Behind bpf_dev_bound_netdev_unregister() nobody would take the
+	 * program off the device, which goes once the reference is dropped.
+	 */
+	if (READ_ONCE(netdev->reg_state) == NETREG_REGISTERED)
+		err = __bpf_prog_dev_bound_init(prog, netdev);
+	else
+		err = -EINVAL;
 	up_write(&bpf_devs_lock);
 
 out:
-- 
2.55.0

[PATCH bpf-next 21/38] bpf: Keep a program until its bpf_wq callback has ended

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

bpf_wq_work() runs the callback under rcu_read_lock_trace() only, since
the callback may sleep.  The program it belongs to need not be sleepable,
and such a program is freed behind a plain RCU grace period.  The
reference that the bpf_wq holds is dropped when the map element goes, or
when another callback is set, no matter whether the work is running.

If that was the last reference the program is freed under the running
callback, and the worker goes on in the freed JIT image:

  Oops: int3: 0000 [#1] SMP NOPTI
  RIP: 0010:0xffffffffc0600947

All it takes is to close the map and the program while the callback runs.

Note in the program that it has a callback that runs sleepable, and wait
for an RCU Tasks Trace grace period, which implies a plain one, before
such a program is freed.  Let bpf_wq_work() enter its read side critical
section before it reads callback_fn.

bpf_task_work holds a reference to the program until the callback is done.

Fixes: 8e83da9732d9 ("bpf: add bpf_wq_start")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 include/linux/bpf.h   | 10 ++++++++++
 kernel/bpf/helpers.c  |  9 ++++++---
 kernel/bpf/syscall.c  |  4 ++--
 kernel/bpf/verifier.c |  2 ++
 4 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 3fb3c7d22d99..6f93bd1414e5 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1873,6 +1873,7 @@ struct bpf_prog_aux {
 	bool attach_btf_trace; /* true if attaching to BTF-enabled raw tp */
 	bool attach_tracing_prog; /* true if tracing another tracing program */
 	bool tramp_linked; /* true if it was ever linked to a trampoline */
+	bool sleepable_async_cb; /* has a bpf_wq or bpf_task_work callback */
 	bool func_proto_unreliable;
 	bool tail_call_reachable;
 	bool xdp_has_frags;
@@ -2006,6 +2007,15 @@ struct bpf_prog {
 	};
 };
 
+/*
+ * Does the program, or a callback of it, run under RCU Tasks Trace protection
+ * only, outside of any RCU read side critical section?
+ */
+static inline bool bpf_prog_runs_sleepable(const struct bpf_prog *prog)
+{
+	return prog->sleepable || prog->aux->sleepable_async_cb;
+}
+
 struct bpf_array_aux {
 	/* Programs with direct jumps into programs part of this array. */
 	struct list_head poke_progs;
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 439370d73cc5..648abe1ba63a 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -1262,18 +1262,21 @@ static void bpf_wq_work(struct work_struct *work)
 
 	BTF_TYPE_EMIT(struct bpf_wq);
 
+	/* keeps the program that callback_fn belongs to */
+	rcu_read_lock_trace();
+
 	callback_fn = READ_ONCE(cb->callback_fn);
 	if (!callback_fn)
-		return;
+		goto out;
 
 	key = map_key_from_value(map, value, &idx);
 
-        rcu_read_lock_trace();
-        migrate_disable();
+	migrate_disable();
 
 	callback_fn((u64)(long)map, (u64)(long)key, (u64)(long)value, 0, 0);
 
 	migrate_enable();
+out:
 	rcu_read_unlock_trace();
 }
 
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 4a9f28131791..755529888a72 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2477,7 +2477,7 @@ static void __bpf_prog_put_rcu_tasks(struct rcu_head *rcu)
 {
 	struct bpf_prog *prog = container_of(rcu, struct bpf_prog_aux, rcu)->prog;
 
-	if (prog->sleepable)
+	if (bpf_prog_runs_sleepable(prog))
 		call_rcu_tasks_trace(rcu, __bpf_prog_put_rcu);
 	else
 		call_rcu(rcu, __bpf_prog_put_rcu);
@@ -2497,7 +2497,7 @@ static void __bpf_prog_put_noref(struct bpf_prog *prog, bool deferred)
 	if (deferred) {
 		if (IS_ENABLED(CONFIG_TASKS_RCU) && prog->aux->tramp_linked)
 			call_rcu_tasks(&prog->aux->rcu, __bpf_prog_put_rcu_tasks);
-		else if (prog->sleepable)
+		else if (bpf_prog_runs_sleepable(prog))
 			call_rcu_tasks_trace(&prog->aux->rcu, __bpf_prog_put_rcu);
 		else
 			call_rcu(&prog->aux->rcu, __bpf_prog_put_rcu);
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index d0ba7156ba4f..6b15862a15e8 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -11211,6 +11211,8 @@ static int push_callback_call(struct bpf_verifier_env *env, struct bpf_insn *ins
 					 is_async_cb_sleepable(env, insn));
 		if (IS_ERR(async_cb))
 			return PTR_ERR(async_cb);
+		if (is_async_cb_sleepable(env, insn))
+			env->prog->aux->sleepable_async_cb = true;
 		callee = async_cb->frame[0];
 		callee->async_entry_cnt = state->frame[0]->async_entry_cnt + 1;
 
-- 
2.55.0

[PATCH bpf-next 22/38] bpf: Count sleepable callbacks as sleepable map users

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

map->sleepable_refcnt tells bpf_map_fd_put_ptr() whether an inner map that
is taken out of its outer map has to wait for an RCU Tasks Trace grace
period as well.  It counts the sleepable programs that use the outer map.
The bpf_wq or bpf_task_work callback of a program that is not sleepable
runs under rcu_read_lock_trace() only, and is not counted.

Such a callback can look an inner map up and keep it while user space
deletes it from the outer map.  The inner map is freed behind a plain RCU
grace period, under the callback:

  BUG: KASAN: slab-use-after-free in array_map_update_elem+0x6bc/0x7d0

Count a program with such a callback.  The maps are bound before the
verifier meets the callback, so raise the count of those that are bound
when it does.

Fixes: 8e83da9732d9 ("bpf: add bpf_wq_start")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/core.c     |  2 +-
 kernel/bpf/syscall.c  |  2 +-
 kernel/bpf/verifier.c | 17 +++++++++++++++--
 3 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 2154c18a5a4d..e98c79e8009c 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -3044,7 +3044,7 @@ void __bpf_free_used_maps(struct bpf_prog_aux *aux,
 	bool sleepable;
 	u32 i;
 
-	sleepable = aux->prog->sleepable;
+	sleepable = bpf_prog_runs_sleepable(aux->prog);
 	for (i = 0; i < len; i++) {
 		map = used_maps[i];
 		if (map->ops->map_poke_untrack)
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 755529888a72..3f215f2509b2 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -6253,7 +6253,7 @@ static int bpf_prog_bind_map(union bpf_attr *attr)
 	/* The bpf program will not access the bpf map, but for the sake of
 	 * simplicity, increase sleepable_refcnt for sleepable program as well.
 	 */
-	if (prog->sleepable)
+	if (bpf_prog_runs_sleepable(prog))
 		atomic64_inc(&map->sleepable_refcnt);
 	memcpy(used_maps_new, used_maps_old,
 	       sizeof(used_maps_old[0]) * prog->aux->used_map_cnt);
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 6b15862a15e8..1f2a846ae415 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -593,6 +593,19 @@ static bool is_async_cb_sleepable(struct bpf_verifier_env *env, struct bpf_insn
 	return false;
 }
 
+/* From here on the maps of the program count it as a sleepable user. */
+static void mark_sleepable_async_cb(struct bpf_verifier_env *env)
+{
+	int i;
+
+	if (bpf_prog_runs_sleepable(env->prog))
+		return;
+
+	env->prog->aux->sleepable_async_cb = true;
+	for (i = 0; i < env->used_map_cnt; i++)
+		atomic64_inc(&env->used_maps[i]->sleepable_refcnt);
+}
+
 bool bpf_is_may_goto_insn(struct bpf_insn *insn)
 {
 	return insn->code == (BPF_JMP | BPF_JCOND) && insn->src_reg == BPF_MAY_GOTO;
@@ -11212,7 +11225,7 @@ static int push_callback_call(struct bpf_verifier_env *env, struct bpf_insn *ins
 		if (IS_ERR(async_cb))
 			return PTR_ERR(async_cb);
 		if (is_async_cb_sleepable(env, insn))
-			env->prog->aux->sleepable_async_cb = true;
+			mark_sleepable_async_cb(env);
 		callee = async_cb->frame[0];
 		callee->async_entry_cnt = state->frame[0]->async_entry_cnt + 1;
 
@@ -20776,7 +20789,7 @@ static int __add_used_map(struct bpf_verifier_env *env, struct bpf_map *map)
 	if (err)
 		return err;
 
-	if (env->prog->sleepable)
+	if (bpf_prog_runs_sleepable(env->prog))
 		atomic64_inc(&map->sleepable_refcnt);
 
 	/* hold the map. If the program is rejected by verifier,
-- 
2.55.0

[PATCH bpf-next 23/38] bpf: Check the maps that sleepable callbacks use

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

Sleepable programs can only use the kinds of map that do without an RCU
read side critical section.  check_map_prog_compatibility() sees to that
when a map is bound to the program, and asks whether the program is
sleepable.  That misses the bpf_wq or bpf_task_work callback of a program
that is not, which runs sleepable all the same, and it misses an inner
map, which is not bound.

The bpf_wq callback of a tc program that looks a key up in a sockhash
gets as far as

  WARNING: net/core/sock_map.c:903 at __sock_hash_lookup_elem+0x18f/0x1e0

Ask the same question for the map that is handed to a helper in sleepable
context, unless the RCU read lock is held: by bpf_rcu_read_lock(), or by
the helper that runs the callback, as bpf_for_each_map_elem() does for a
hash map.  Programs that take the lock go on working.  Disabled preemption
or interrupts are not enough, since the maps ask for rcu_read_lock_held().
The other parts of the program may use such a map as before.

What is refused from now on runs into the warning above, or uses what the
map frees behind a plain RCU grace period: an entry of a devmap, of a perf
event array or of a cgroup array.

A bloom filter does without RCU.  Add it to the list, so that callbacks
which use one go on loading.  As a side effect sleepable programs can use
bloom filters as well.

A pointer to the value of a devmap which a program keeps beyond
bpf_rcu_read_unlock() is not dealt with here.

Fixes: 638e4b825d52 ("bpf: Allows per-cpu maps and map-in-map in sleepable programs")
Fixes: 8e83da9732d9 ("bpf: add bpf_wq_start")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/verifier.c | 80 +++++++++++++++++++++++++++----------------
 1 file changed, 51 insertions(+), 29 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 1f2a846ae415..09173d1d66d3 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -219,6 +219,7 @@ static int ref_set_non_owning(struct bpf_verifier_env *env,
 			      struct bpf_reg_state *reg);
 static bool is_trusted_reg(struct bpf_verifier_env *env, const struct bpf_reg_state *reg);
 static inline bool in_sleepable_context(struct bpf_verifier_env *env);
+static bool in_rcu_callback(struct bpf_verifier_env *env);
 static const char *non_sleepable_context_description(struct bpf_verifier_env *env);
 static void scalar32_min_max_add(struct bpf_reg_state *dst_reg, struct bpf_reg_state *src_reg);
 static void scalar_min_max_add(struct bpf_reg_state *dst_reg, struct bpf_reg_state *src_reg);
@@ -10379,12 +10380,57 @@ bool bpf_allow_tail_call_in_subprogs(struct bpf_verifier_env *env)
 	       bpf_jit_supports_subprog_tailcalls();
 }
 
+/* May the map be used outside of an RCU read side critical section? */
+static bool map_needs_no_rcu(const struct bpf_map *map)
+{
+	switch (map->map_type) {
+	case BPF_MAP_TYPE_HASH:
+	case BPF_MAP_TYPE_RHASH:
+	case BPF_MAP_TYPE_LRU_HASH:
+	case BPF_MAP_TYPE_ARRAY:
+	case BPF_MAP_TYPE_PERCPU_HASH:
+	case BPF_MAP_TYPE_PERCPU_ARRAY:
+	case BPF_MAP_TYPE_LRU_PERCPU_HASH:
+	case BPF_MAP_TYPE_LPM_TRIE:
+	case BPF_MAP_TYPE_ARRAY_OF_MAPS:
+	case BPF_MAP_TYPE_HASH_OF_MAPS:
+	case BPF_MAP_TYPE_RINGBUF:
+	case BPF_MAP_TYPE_USER_RINGBUF:
+	case BPF_MAP_TYPE_INODE_STORAGE:
+	case BPF_MAP_TYPE_SK_STORAGE:
+	case BPF_MAP_TYPE_TASK_STORAGE:
+	case BPF_MAP_TYPE_CGRP_STORAGE:
+	case BPF_MAP_TYPE_QUEUE:
+	case BPF_MAP_TYPE_STACK:
+	case BPF_MAP_TYPE_ARENA:
+	case BPF_MAP_TYPE_INSN_ARRAY:
+	case BPF_MAP_TYPE_PROG_ARRAY:
+	case BPF_MAP_TYPE_BLOOM_FILTER:
+		return true;
+	default:
+		return false;
+	}
+}
+
 static int check_map_func_compatibility(struct bpf_verifier_env *env,
 					struct bpf_map *map, int func_id)
 {
 	if (!map)
 		return 0;
 
+	/*
+	 * check_map_prog_compatibility() asks the program.  That misses the
+	 * sleepable callback of another program, and an inner map.  They may
+	 * use any map under the RCU read lock.  Disabled preemption or
+	 * interrupts will not do: the maps ask for rcu_read_lock_held().
+	 */
+	if (in_sleepable(env) && !env->cur_state->active_rcu_locks &&
+	    !in_rcu_callback(env) && !map_needs_no_rcu(map)) {
+		verbose(env, "map '%s' needs bpf_rcu_read_lock() in sleepable context\n",
+			map->name);
+		return -EINVAL;
+	}
+
 	/* We need a two way check, first is from map perspective ... */
 	switch (map->map_type) {
 	case BPF_MAP_TYPE_PROG_ARRAY:
@@ -20676,35 +20722,11 @@ static int check_map_prog_compatibility(struct bpf_verifier_env *env,
 		return -EINVAL;
 	}
 
-	if (prog->sleepable)
-		switch (map->map_type) {
-		case BPF_MAP_TYPE_HASH:
-		case BPF_MAP_TYPE_RHASH:
-		case BPF_MAP_TYPE_LRU_HASH:
-		case BPF_MAP_TYPE_ARRAY:
-		case BPF_MAP_TYPE_PERCPU_HASH:
-		case BPF_MAP_TYPE_PERCPU_ARRAY:
-		case BPF_MAP_TYPE_LRU_PERCPU_HASH:
-		case BPF_MAP_TYPE_LPM_TRIE:
-		case BPF_MAP_TYPE_ARRAY_OF_MAPS:
-		case BPF_MAP_TYPE_HASH_OF_MAPS:
-		case BPF_MAP_TYPE_RINGBUF:
-		case BPF_MAP_TYPE_USER_RINGBUF:
-		case BPF_MAP_TYPE_INODE_STORAGE:
-		case BPF_MAP_TYPE_SK_STORAGE:
-		case BPF_MAP_TYPE_TASK_STORAGE:
-		case BPF_MAP_TYPE_CGRP_STORAGE:
-		case BPF_MAP_TYPE_QUEUE:
-		case BPF_MAP_TYPE_STACK:
-		case BPF_MAP_TYPE_ARENA:
-		case BPF_MAP_TYPE_INSN_ARRAY:
-		case BPF_MAP_TYPE_PROG_ARRAY:
-			break;
-		default:
-			verbose(env,
-				"Sleepable programs can only use array, hash, ringbuf and local storage maps\n");
-			return -EINVAL;
-		}
+	if (prog->sleepable && !map_needs_no_rcu(map)) {
+		verbose(env,
+			"Sleepable programs can only use array, hash, ringbuf and local storage maps\n");
+		return -EINVAL;
+	}
 
 	if (bpf_map_is_cgroup_storage(map) &&
 	    bpf_cgroup_storage_assign(env->prog->aux, map)) {
-- 
2.55.0

Re: [PATCH bpf-next 23/38] bpf: Check the maps that sleepable callbacks use

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] bpf: global subprog call under preempt/irq disable bypasses map RCU check
--

commit 44c0901fdd3a21bde77f64657ef1a8f91005cd05
Author: Josef Bacik <josef@toxicpanda.com>

bpf: Check the maps that sleepable callbacks use

This commit restricts map helper calls in sleepable contexts (such as bpf_wq
or bpf_task_work callbacks, or with inner maps) to maps that do not require an
RCU read-side lock unless bpf_rcu_read_lock() is held or the helper runs inside
an RCU callback. It also adds bloom filters to the list of maps usable without
RCU.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 1f2a846ae415f..09173d1d66d39 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
[ ... ]
> @@ -10379,12 +10380,57 @@ bool bpf_allow_tail_call_in_subprogs(struct bpf_verifier_env *env)
>  	       bpf_jit_supports_subprog_tailcalls();
>  }
>  
> +/* May the map be used outside of an RCU read side critical section? */
> +static bool map_needs_no_rcu(const struct bpf_map *map)
> +{
[ ... ]
> +}
> +
>  static int check_map_func_compatibility(struct bpf_verifier_env *env,
>  					struct bpf_map *map, int func_id)
>  {
>  	if (!map)
>  		return 0;
>  
> +	/*
> +	 * check_map_prog_compatibility() asks the program.  That misses the
> +	 * sleepable callback of another program, and an inner map.  They may
> +	 * use any map under the RCU read lock.  Disabled preemption or
> +	 * interrupts will not do: the maps ask for rcu_read_lock_held().
> +	 */
> +	if (in_sleepable(env) && !env->cur_state->active_rcu_locks &&
> +	    !in_rcu_callback(env) && !map_needs_no_rcu(map)) {
> +		verbose(env, "map '%s' needs bpf_rcu_read_lock() in sleepable context\n",
> +			map->name);
> +		return -EINVAL;
> +	}

[Severity: High]
Can this check be bypassed if a sleepable callback or sleepable program
disables preemption or local IRQs and then calls a global subprogram that
accesses the map?

When a global subprogram is called in check_func_call(), the verifier records
the calling context using in_sleepable_context(env):

kernel/bpf/verifier.c:check_func_call() {
	...
	if (sub_aux->func_info_aux == BPF_FUNC_GLOBAL) {
		...
		sub_aux->called[in_sleepable_context(env)] = true;
		...
	}
}

However, in_sleepable_context() checks in_rcu_cs(), which treats disabled
preemption (active_preempt_locks) and disabled local IRQs (active_irq_id) as an
RCU critical section even when active_rcu_locks is zero:

kernel/bpf/verifier.c:in_rcu_cs() {
	return env->cur_state->active_rcu_locks ||
	       env->cur_state->active_preempt_locks ||
	       env->cur_state->active_locks ||
	       env->cur_state->active_irq_id ||
	       !in_sleepable(env);
}

kernel/bpf/verifier.c:in_sleepable_context() {
	return !in_rcu_cs(env) && !in_rcu_callback(env);
}

If a bpf_wq or bpf_task_work callback calls bpf_preempt_disable() or
bpf_local_irq_save() without bpf_rcu_read_lock() and then calls a global
subprogram, in_sleepable_context(env) returns false and sets
sub_aux->called[0] = true.

When do_check_subprogs() later verifies that global subprogram via
do_check_common(env, i, 0), state->in_sleepable is initialized to false. Inside
the global subprogram, in_sleepable(env) evaluates to false in
check_map_func_compatibility(), allowing helpers such as bpf_map_lookup_elem()
on a sockhash or an inner RCU-requiring map without holding rcu_read_lock() and
triggering the WARN_ON_ONCE(!rcu_read_lock_held()) in __sock_hash_lookup_elem()
at runtime.

[ ... ]

>
>
>
>

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

[PATCH bpf-next 24/38] bpf: Assume no bounds for buffers behind a tail call

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

The bpf_map_elem, bpf_sk_storage_map and sockmap iterators hand the key
and the value of an element to the program as buffers.  The verifier notes
how far a program reaches into them, and the iterator compares that with
the map when the program is attached.  A program that does not touch the
buffers passes.  It can tail-call another program of the same target,
which gets the same context, and whose reach nobody compares with the
map:

  BUG: KASAN: slab-out-of-bounds in ___bpf_prog_run+0xa46a/0xae80

The program array can be filled at any time, so there is nothing to look
at when the link is made.  For the same reason a program with a tail call
is taken to use the whole stack and the whole packet.  Take it to reach
as far as can be into the buffers as well, so that these iterators refuse
it with -EACCES.

Fixes: d6c4503cc296 ("bpf: Implement bpf iterator for hash maps")
Fixes: 5ce6e77c7edf ("bpf: Implement bpf iterator for sock local storage map")
Fixes: 0365351524d7 ("net: Allow iterating sockmap and sockhash")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/fixups.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
index 608621d7f225..002c3935a8eb 100644
--- a/kernel/bpf/fixups.c
+++ b/kernel/bpf/fixups.c
@@ -2151,6 +2151,8 @@ int bpf_do_misc_fixups(struct bpf_verifier_env *env)
 			if (!bpf_allow_tail_call_in_subprogs(env))
 				prog->aux->stack_depth = MAX_BPF_STACK;
 			prog->aux->max_pkt_offset = MAX_PACKET_OFF;
+			prog->aux->max_rdonly_access = U32_MAX;
+			prog->aux->max_rdwr_access = U32_MAX;
 
 			/* mark bpf_tail_call as different opcode to avoid
 			 * conditional branch in the interpreter for every normal
-- 
2.55.0

[PATCH bpf-next 25/38] bpf: Don't take an extension for an iterator program

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

An extension inherits the expected_attach_type of the program it extends.
If that is BPF_TRACE_ITER, check_attach_btf_id() hands the extension to
bpf_iter_prog_supported(), which goes by the name of the attach function.
For an extension that is the name of the subprogram it replaces, so the
load fails with -EINVAL, unless that subprogram is called like the function
of a target, bpf_iter_bpf_map_elem for example.

Then the extension gets the context of that target.  If the subprogram
has as many arguments, it can use the key and the value as buffers, but
how far it reaches is noted in the extension, where the test of the
iterator link does not look: it reads and writes behind the elements of
the map.  And the id of the subprogram in the BTF of the program is cached
as the id of the target, so that programs of that target which are loaded
already cannot be attached.

Refuse all extensions here.

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

diff --git a/kernel/bpf/bpf_iter.c b/kernel/bpf/bpf_iter.c
index 4668c1696a08..5fd376bd210b 100644
--- a/kernel/bpf/bpf_iter.c
+++ b/kernel/bpf/bpf_iter.c
@@ -351,6 +351,13 @@ int bpf_iter_prog_supported(struct bpf_prog *prog)
 	const char *prefix = BPF_ITER_FUNC_PREFIX;
 	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.
+	 */
+	if (prog->type == BPF_PROG_TYPE_EXT)
+		return -EINVAL;
+
 	if (strncmp(attach_fname, prefix, prefix_len))
 		return -EINVAL;
 
-- 
2.55.0

[PATCH bpf-next 26/38] bpf: Assume no bounds for a writable tp with tail calls

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

A writable raw tracepoint hands a buffer to the program.  The verifier
notes how far the program reaches into it, and bpf_probe_register() refuses
a program that reaches further than the tracepoint allows.  A program that
does not touch the buffer passes, and can tail-call one of its type that
reads and writes behind it.

Take a BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE program with a tail call to
reach as far as can be, as for the stack, the packet and the buffers of
iterators.  It cannot be attached any more.  Plain raw tracepoint programs,
for which the limit is zero, cannot get at the buffer, and a tail call
stays within one type: they are left alone.

Fixes: 9df1c28bb752 ("bpf: add writable context for raw tracepoints")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/fixups.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
index 002c3935a8eb..fe59e3114d82 100644
--- a/kernel/bpf/fixups.c
+++ b/kernel/bpf/fixups.c
@@ -2153,6 +2153,8 @@ int bpf_do_misc_fixups(struct bpf_verifier_env *env)
 			prog->aux->max_pkt_offset = MAX_PACKET_OFF;
 			prog->aux->max_rdonly_access = U32_MAX;
 			prog->aux->max_rdwr_access = U32_MAX;
+			if (prog->type == BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE)
+				prog->aux->max_tp_access = U32_MAX;
 
 			/* mark bpf_tail_call as different opcode to avoid
 			 * conditional branch in the interpreter for every normal
-- 
2.55.0

Re: [PATCH bpf-next 26/38] bpf: Assume no bounds for a writable tp with tail calls

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] bpf: max_tp_access check bypassed via BPF_PROG_TYPE_EXT on writable raw tp
--

commit 467f1021d59430c1b54a88fe6072cc36e60eb149
Author: Josef Bacik <josef@toxicpanda.com>

bpf: Assume no bounds for a writable tp with tail calls

This commit sets max_tp_access to U32_MAX when a
BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE program performs a tail call,
preventing it from attaching to a writable raw tracepoint and tail-calling a
program that accesses beyond the tracepoint buffer bounds.

> diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
> index 002c3935a8eb0..fe59e3114d828 100644
> --- a/kernel/bpf/fixups.c
> +++ b/kernel/bpf/fixups.c
> @@ -2153,6 +2153,8 @@ int bpf_do_misc_fixups(struct bpf_verifier_env *env)
>  			prog->aux->max_pkt_offset = MAX_PACKET_OFF;
>  			prog->aux->max_rdonly_access = U32_MAX;
>  			prog->aux->max_rdwr_access = U32_MAX;
> +			if (prog->type == BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE)
> +				prog->aux->max_tp_access = U32_MAX;

[Severity: High]
Can the max_tp_access check still be bypassed using a BPF_PROG_TYPE_EXT
(freplace) program attached to a BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE
target program?

A target program (tgt_prog) of type BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE
with prototype int prog(struct bpf_raw_tracepoint_args *ctx) (or a global
subprogram taking struct bpf_raw_tracepoint_args *ctx) that does not access
ctx->args[0] or call bpf_tail_call() is loaded with
tgt_prog->aux->max_tp_access == 0. Because
find_canonical_prog_ctx_type(BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE) resolves
to struct bpf_raw_tracepoint_args, btf_prepare_func_args() marks its BTF
signature as reliable.

When attaching tgt_prog, bpf_probe_register() in kernel/trace/bpf_trace.c
only validates max_tp_access on the root attached program:

kernel/trace/bpf_trace.c:bpf_probe_register() {
    ...
	if (prog->aux->max_tp_access > btp->writable_size)
		return -EINVAL;
    ...
}

This succeeds for any raw tracepoint (even when btp->writable_size is 0)
since tgt_prog->aux->max_tp_access is 0.

Meanwhile, a BPF_PROG_TYPE_EXT program (ext_prog) targeting tgt_prog or its
global subprogram passes bpf_check_attach_target(), inherits
raw_tracepoint_writable_verifier_ops in check_attach_btf_id(), and has its
struct bpf_raw_tracepoint_args *ctx argument typed as PTR_TO_CTX in
btf_prepare_func_args():

kernel/bpf/verifier.c:check_attach_btf_id() {
    ...
	if (tgt_prog && prog->type == BPF_PROG_TYPE_EXT) {
		/* to make freplace equivalent to their targets, they need to
		 * inherit env->ops and expected_attach_type for the rest of the
		 * verification
		 */
		env->ops = bpf_verifier_ops[tgt_prog->type];
		prog->expected_attach_type = tgt_prog->expected_attach_type;
	}
    ...
}

In bpf_do_misc_fixups(), the check added here tests
prog->type == BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE rather than
resolve_prog_type(prog), so it does not match when ext_prog
(prog->type == BPF_PROG_TYPE_EXT) calls bpf_tail_call() (which is permitted
when the replaced function has changes_pkt_data set, such as via
bpf_dynptr_write() or bpf_dynptr_memset() on a local dynptr, or when
CONFIG_NET=n). However, __bpf_prog_map_compatible() in kernel/bpf/core.c
uses resolve_prog_type(fp):

kernel/bpf/core.c:__bpf_prog_map_compatible() {
    ...
	enum bpf_prog_type prog_type = resolve_prog_type(fp);
    ...
}

This allows ext_prog to tail-call into a BPF_MAP_TYPE_PROG_ARRAY of
BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE programs that read and write past
btp->writable_size.

Furthermore, even if ext_prog directly accesses ctx->args[0] (which
raw_tp_writable_prog_is_valid_access() types as PTR_TO_TP_BUFFER) or has
ext_prog->aux->max_tp_access set to U32_MAX, check_tp_buffer_access() only
updates ext_prog->aux->max_tp_access, which is never validated against
btp->writable_size or propagated to tgt_prog.

Should BPF_PROG_TYPE_EXT programs be disallowed from targeting
BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE programs (similar to how iterator
programs reject extensions in commit "bpf: Don't take an extension for an
iterator program")?

>  
>  			/* mark bpf_tail_call as different opcode to avoid
>  			 * conditional branch in the interpreter for every normal

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

[PATCH bpf-next 27/38] bpf: Keep iterator programs off special map fields

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

The bpf_map_elem iterator, for all but the per-cpu maps, and the
bpf_sk_storage_map iterator hand the live value of an element to the
program as a writable buffer.  The verifier does not know the map when it
checks the program, so nothing keeps the program from writing over a
bpf_spin_lock, a bpf_timer, a kptr or another special field, which neither
a map update nor a pointer to a map value can touch.  With CAP_BPF and
CAP_PERFMON, or a BPF token, a program can so make the kernel follow a
pointer of its choice: after a write over a bpf_timer the kernel crashes
on the written value when the element is removed.

Offsets into a buffer are constants.  Let the verifier record the range of
bytes that a program may write through a read-write buffer, and refuse the
program with -EACCES, at attach and at a link update, if a special field
of the map lies in that range.

A program that only reads, or that writes beside the special fields, works
as before.  One that writes on both sides of a special field is refused,
since the range covers the field.

Fixes: d6c4503cc296 ("bpf: Implement bpf iterator for hash maps")
Fixes: 5ce6e77c7edf ("bpf: Implement bpf iterator for sock local storage map")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 include/linux/bpf.h       |  5 +++++
 kernel/bpf/map_iter.c     | 36 ++++++++++++++++++++++++++++++++----
 kernel/bpf/verifier.c     | 12 +++++++++---
 net/core/bpf_sk_storage.c |  2 +-
 4 files changed, 47 insertions(+), 8 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 6f93bd1414e5..8b26c4ec12a7 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1858,6 +1858,9 @@ struct bpf_prog_aux {
 	u32 ctx_arg_info_size;
 	u32 max_rdonly_access;
 	u32 max_rdwr_access;
+	/* [min, max): the bytes of a rdwr buffer that may be written */
+	u32 min_rdwr_write;
+	u32 max_rdwr_write;
 	u32 subprog_start;
 	struct btf *attach_btf;
 	struct bpf_ctx_arg_aux *ctx_arg_info;
@@ -3196,6 +3199,8 @@ void bpf_iter_map_show_fdinfo(const struct bpf_iter_aux_info *aux,
 			      struct seq_file *seq);
 int bpf_iter_map_fill_link_info(const struct bpf_iter_aux_info *aux,
 				struct bpf_link_info *info);
+int bpf_iter_map_check_value_write(const struct bpf_prog *prog,
+				   const struct bpf_map *map);
 
 int map_set_for_each_callback_args(struct bpf_verifier_env *env,
 				   struct bpf_func_state *caller,
diff --git a/kernel/bpf/map_iter.c b/kernel/bpf/map_iter.c
index 16537356c1a2..a58c148692e6 100644
--- a/kernel/bpf/map_iter.c
+++ b/kernel/bpf/map_iter.c
@@ -97,22 +97,50 @@ static struct bpf_iter_reg bpf_map_reg_info = {
 	.seq_info		= &bpf_map_seq_info,
 };
 
+/*
+ * For an iterator that hands the live map value to the program as a writable
+ * buffer: the verifier knows nothing of the map, so the special fields of the
+ * value have to lie outside of what the program may write.
+ */
+int bpf_iter_map_check_value_write(const struct bpf_prog *prog,
+				   const struct bpf_map *map)
+{
+	const struct btf_record *rec = map->record;
+	const struct btf_field *field;
+	u32 i;
+
+	if (IS_ERR_OR_NULL(rec) || !prog->aux->max_rdwr_write)
+		return 0;
+
+	for (i = 0; i < rec->cnt; i++) {
+		field = &rec->fields[i];
+		if (field->offset < prog->aux->max_rdwr_write &&
+		    field->offset + field->size > prog->aux->min_rdwr_write)
+			return -EACCES;
+	}
+
+	return 0;
+}
+
 static int bpf_iter_map_check_prog(const struct bpf_prog *prog,
 				   const struct bpf_iter_aux_info *aux)
 {
 	const struct bpf_map *map = aux->map;
 	u32 value_size = map->value_size;
+	bool is_percpu;
 
-	if (map->map_type == BPF_MAP_TYPE_PERCPU_HASH ||
-	    map->map_type == BPF_MAP_TYPE_LRU_PERCPU_HASH ||
-	    map->map_type == BPF_MAP_TYPE_PERCPU_ARRAY)
+	is_percpu = map->map_type == BPF_MAP_TYPE_PERCPU_HASH ||
+		    map->map_type == BPF_MAP_TYPE_LRU_PERCPU_HASH ||
+		    map->map_type == BPF_MAP_TYPE_PERCPU_ARRAY;
+	if (is_percpu)
 		value_size = round_up(value_size, 8) * num_possible_cpus();
 
 	if (prog->aux->max_rdonly_access > map->key_size ||
 	    prog->aux->max_rdwr_access > value_size)
 		return -EACCES;
 
-	return 0;
+	/* the program gets a copy of a per-cpu value */
+	return is_percpu ? 0 : bpf_iter_map_check_value_write(prog, map);
 }
 
 static int bpf_iter_attach_map(struct bpf_prog *prog,
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 09173d1d66d3..6ee9b5c6fe47 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -6053,9 +6053,10 @@ static int check_buffer_access(struct bpf_verifier_env *env,
 			       const struct bpf_reg_state *reg,
 			       argno_t argno, int off, int size,
 			       bool zero_size_allowed,
-			       u32 *max_access)
+			       u32 *max_access, bool write)
 {
 	const char *buf_info = type_is_rdonly_mem(reg->type) ? "rdonly" : "rdwr";
+	struct bpf_prog_aux *aux = env->prog->aux;
 	u32 access_end;
 	int err;
 
@@ -6064,6 +6065,11 @@ static int check_buffer_access(struct bpf_verifier_env *env,
 		return err;
 
 	*max_access = max(access_end, *max_access);
+	if (write && size) {
+		if (!aux->max_rdwr_write || access_end - size < aux->min_rdwr_write)
+			aux->min_rdwr_write = access_end - size;
+		aux->max_rdwr_write = max(access_end, aux->max_rdwr_write);
+	}
 
 	return 0;
 }
@@ -7339,7 +7345,7 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, struct b
 		}
 
 		err = check_buffer_access(env, reg, argno, off, size, false,
-					  max_access);
+					  max_access, t == BPF_WRITE);
 
 		if (!err && value_regno >= 0 && (rdonly_mem || t == BPF_READ))
 			mark_reg_unknown(env, regs, value_regno);
@@ -7852,7 +7858,7 @@ static int check_helper_mem_access(struct bpf_verifier_env *env, struct bpf_reg_
 		}
 		return check_buffer_access(env, reg, argno, 0,
 					   access_size, zero_size_allowed,
-					   max_access);
+					   max_access, access_type == BPF_WRITE);
 	case PTR_TO_STACK:
 		return check_stack_range_initialized(
 				env, reg,
diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c
index 72b9ee4c98de..1c59300af05c 100644
--- a/net/core/bpf_sk_storage.c
+++ b/net/core/bpf_sk_storage.c
@@ -925,7 +925,7 @@ static int bpf_iter_map_check_prog(const struct bpf_prog *prog,
 	if (prog->aux->max_rdwr_access > aux->map->value_size)
 		return -EACCES;
 
-	return 0;
+	return bpf_iter_map_check_value_write(prog, aux->map);
 }
 
 static int bpf_iter_attach_map(struct bpf_prog *prog,
-- 
2.55.0

[PATCH bpf-next 28/38] bpf: Free what map_free() unlinks for sleepable readers

[Alexei Starovoitov]

From: Josef Bacik <josef@toxicpanda.com>

bpf_local_storage_map_free() takes the elements of the map off the lists
of their owners, which are alive, and frees them, and the storage of an
owner with its last element, behind a plain RCU grace period.  Nobody uses
the map any more, but the other maps of these owners are in use.  A
sleepable program calls the get and delete helpers under
rcu_read_lock_trace() only.  While it walks the list of an owner for its
own map, or is about to lock the storage, the element it stands on, or the
storage, can be freed under it.

Whatever else takes an element off the list of a live owner frees it
behind an RCU Tasks Trace grace period, which implies a plain one.  Do so
in bpf_local_storage_map_free() as well.  It waits for these callbacks
before it frees the map.

This does not help a sleepable program that hands an owner to the helpers
which nobody holds, and which may be gone by then.

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>
---
 kernel/bpf/bpf_local_storage.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/kernel/bpf/bpf_local_storage.c b/kernel/bpf/bpf_local_storage.c
index 7721834e4bff..f234f671e75f 100644
--- a/kernel/bpf/bpf_local_storage.c
+++ b/kernel/bpf/bpf_local_storage.c
@@ -431,13 +431,17 @@ static void bpf_selem_unlink_nofail(struct bpf_local_storage_elem *selem,
 	 * then freed after an RCU grace period. However, if destroy() and map_free() are
 	 * racing or rqspinlock returns errors in unlikely situations (unlink != 2), free
 	 * the selem only after both map_free() and destroy() see the 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.
 	 */
 	if (unlink == 2 ||
 	    atomic_cmpxchg(&selem->state, SELEM_UNLINKED, SELEM_TOFREE) == SELEM_UNLINKED)
-		bpf_selem_free(selem, true);
+		bpf_selem_free(selem, !in_map_free);
 
 	if (free_storage)
-		bpf_local_storage_free(local_storage, true);
+		bpf_local_storage_free(local_storage, !in_map_free);
 }
 
 void __bpf_local_storage_insert_cache(struct bpf_local_storage *local_storage,
-- 
2.55.0

[PATCH bpf-next 29/38] 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.

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 | 11 ++++++++---
 kernel/bpf/map_in_map.c        |  7 +++++--
 3 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 8b26c4ec12a7..ad4ef937e112 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 f234f671e75f..3d0c455d258c 100644
--- a/kernel/bpf/bpf_local_storage.c
+++ b/kernel/bpf/bpf_local_storage.c
@@ -352,7 +352,7 @@ 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;
 	struct bpf_local_storage *local_storage;
 	struct bpf_local_storage_map *smap;
 	unsigned long flags;
@@ -362,6 +362,10 @@ 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) {
@@ -434,11 +438,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

Re: [PATCH bpf-next 29/38] bpf: Keep values of a dead owner for sleepable programs

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] bpf: premature bpf_obj_free_fields() in bpf_selem_unlink_nofail()
- [High] bpf: uncounted sleepable users in bpf_selem_unlink_nofail() via freplace
--

commit 8642076a3bb10a2ade07f6d402a14e8067337799
Author: Josef Bacik <josef@toxicpanda.com>

bpf: Keep values of a dead owner for sleepable programs

This patch defers freeing local storage elements behind an RCU Tasks Trace
grace period when an owner is destroyed if the storage map has sleepable
users or has been used as an inner map.

> diff --git a/kernel/bpf/bpf_local_storage.c b/kernel/bpf/bpf_local_storage.c
> index f234f671e75f8..3d0c455d258c2 100644
> --- a/kernel/bpf/bpf_local_storage.c
> +++ b/kernel/bpf/bpf_local_storage.c
> @@ -352,7 +352,7 @@ 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;
>  	struct bpf_local_storage *local_storage;
>  	struct bpf_local_storage_map *smap;
>  	unsigned long flags;
> @@ -362,6 +362,10 @@ 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));

[Severity: High]
Can a BPF_PROG_TYPE_EXT (freplace) program extending a sleepable target
program bypass this sleepable_users check and trigger a slab use-after-free
on SDATA(selem)->data?

In kernel/bpf/verifier.c:can_be_sleepable(), BPF_F_SLEEPABLE is rejected for
BPF_PROG_TYPE_EXT programs, so ext_prog->sleepable is always false:

kernel/bpf/verifier.c:can_be_sleepable() {
	if (prog->type == BPF_PROG_TYPE_TRACING) {
		...
	}
	if (prog->type == BPF_PROG_TYPE_LSM)
		return prog->expected_attach_type != BPF_LSM_CGROUP;

	return prog->type == BPF_PROG_TYPE_KPROBE /* only for uprobes */ ||
	       prog->type == BPF_PROG_TYPE_STRUCT_OPS ||
	       prog->type == BPF_PROG_TYPE_RAW_TRACEPOINT ||
	       prog->type == BPF_PROG_TYPE_TRACEPOINT;
}

When maps are bound to ext_prog in __add_used_map() or bpf_prog_bind_map(),
map->sleepable_refcnt is only incremented if bpf_prog_runs_sleepable(prog)
is true.

However, check_attach_btf_id() and __bpf_trampoline_link_prog() allow a
non-sleepable BPF_PROG_TYPE_EXT program (where ext_prog->aux->might_sleep is
false) to replace the main entry or a global subprogram of a sleepable
target program (where tgt_prog->sleepable is true) without propagating
sleepable status or incrementing map->sleepable_refcnt on ext_prog's
used_maps:

kernel/bpf/verifier.c:check_attach_btf_id() {
	if (tgt_prog && prog->type == BPF_PROG_TYPE_EXT) {
		env->ops = bpf_verifier_ops[tgt_prog->type];
		prog->expected_attach_type = tgt_prog->expected_attach_type;
	}
}

At runtime, tgt_prog enters via __bpf_prog_enter_sleepable() or
__bpf_prog_enter_sleepable_recur(), which only acquires
rcu_read_lock_trace() and migrate_disable(), and jumps directly into
ext_prog->bpf_func.

If ext_prog looks up a value from a local storage map bound only to ext_prog
(so sleepable_refcnt is 0 and was_inner_map is false) via
bpf_task_storage_get() and then releases the owner or exits
bpf_rcu_read_unlock(), bpf_local_storage_destroy() ->
bpf_selem_unlink_nofail() will see sleepable_users == false and free selem
via kfree_rcu() while ext_prog still accesses SDATA(selem)->data.

>  		b = b ? : select_bucket(smap, local_storage);
>  		err = raw_res_spin_lock_irqsave(&b->lock, flags);
>  		if (!err) {
> @@ -434,11 +438,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);

[Severity: Critical]
When sleepable_users is true, bpf_selem_free() defers freeing selem to
bpf_selem_free_trace_rcu() because a sleepable program may still be reading
or writing SDATA(selem)->data.  However, earlier in
bpf_selem_unlink_nofail(), bpf_obj_free_fields() is still called
immediately under b->lock and SDATA(selem)->smap is cleared to NULL:

kernel/bpf/bpf_local_storage.c:bpf_selem_unlink_nofail() {
		err = raw_res_spin_lock_irqsave(&b->lock, flags);
		if (!err) {
			if (likely(selem_linked_to_map(selem))) {
				hlist_del_init_rcu(&selem->map_node);
				bpf_obj_free_fields(smap->map.record, SDATA(selem)->data);
				unlink++;
			}
			raw_res_spin_unlock_irqrestore(&b->lock, flags);
		}
		WARN_ON_ONCE(err && in_map_free);
		if (!err || in_map_free)
			RCU_INIT_POINTER(SDATA(selem)->smap, NULL);
}

Doesn't calling bpf_obj_free_fields() before the RCU Tasks Trace grace
period cause a physical page use-after-free for BPF_UPTR fields and a kptr
leak for BPF_KPTR_REF or BPF_KPTR_PERCPU fields?

For a BPF_MAP_TYPE_TASK_STORAGE map with a BPF_UPTR field:

1. A sleepable program obtains SDATA(selem)->data (or loads the uptr's
   PTR_TO_MEM register via mark_uptr_ld_reg()) and releases the task
   reference or exits bpf_rcu_read_unlock().

2. When the task exits, free_task() -> bpf_task_storage_free() ->
   bpf_local_storage_destroy() -> bpf_selem_unlink_nofail() runs after
   exit_mm() has already unmapped the task's user address space.

3. bpf_obj_free_fields() immediately calls unpin_uptr_kaddr() ->
   unpin_user_page(virt_to_page(kaddr)) without waiting for the RCU Tasks
   Trace grace period and without clearing the pointer in SDATA(selem)->data,
   dropping the last reference to the physical page while the sleepable
   program can still read and write to it.

Similarly, for BPF_KPTR_REF or BPF_KPTR_PERCPU fields, the running sleepable
program can store a new kptr into SDATA(selem)->data via bpf_kptr_xchg()
after bpf_selem_unlink_nofail() has already called bpf_obj_free_fields() and
set SDATA(selem)->smap to NULL.  When bpf_selem_free_trace_rcu() later runs
after the RCU Tasks Trace grace period:

kernel/bpf/bpf_local_storage.c:bpf_selem_free_trace_rcu() {
	smap = rcu_dereference_check(SDATA(selem)->smap, 1);

	if (smap)
		bpf_obj_free_fields(smap->map.record, SDATA(selem)->data);
	kfree(selem);
}

SDATA(selem)->smap is already NULL, so bpf_selem_free_trace_rcu() skips
bpf_obj_free_fields() and leaks the newly exchanged kptr.

>  
>  	if (free_storage)
>  		bpf_local_storage_free(local_storage, !in_map_free);

[ ... ]

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

[PATCH bpf-next 30/38] 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 ad4ef937e112..c1d92fd10ef1 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1963,6 +1963,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 e98c79e8009c..956555fc5e9f 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -3087,12 +3087,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
@@ -3111,20 +3139,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 3f215f2509b2..c8bd92c95ac0 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 31/38] 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 32/38] 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 33/38] 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 816948df8fff..1ca03f8cfd83 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -1801,6 +1801,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 6ee9b5c6fe47..e002728abd04 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:
@@ -12635,8 +12643,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
 			 */
@@ -13685,9 +13692,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)
@@ -15625,7 +15659,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

Re: [PATCH bpf-next 33/38] bpf: Drop packet pointers behind kfuncs writing an skb

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] bpf: missing seen_direct_write for dynptr_write_kfuncs in check_kfunc_call()
--

commit 851ba1993e9264b3115b008c369c9dc215b291c2
Author: Josef Bacik <josef@toxicpanda.com>

bpf: Drop packet pointers behind kfuncs writing an skb

This commit invalidates packet pointers and skb dynptr slices when kfuncs
such as bpf_dynptr_copy(), bpf_dynptr_memset(),
bpf_probe_read_{user,kernel}[_str]_dynptr(), and
bpf_copy_from_user[_task][_str]_dynptr() write through an skb or skb_meta
dynptr.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 6ee9b5c6fe477..e002728abd041 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -15625,7 +15659,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);

[Severity: High]
When a kfunc in dynptr_write_kfuncs writes to a BPF_DYNPTR_TYPE_SKB or
BPF_DYNPTR_TYPE_SKB_META dynptr, does check_kfunc_call() also need to set
env->seen_direct_write (for instance via may_access_direct_pkt_data(env,
NULL, BPF_WRITE)) so that convert_ctx_accesses() emits
bpf_unclone_prologue()?

Unlike bpf_dynptr_write(), which always goes through __bpf_dynptr_write() ->
__bpf_skb_store_bytes() -> bpf_try_make_writable(), all of the kfuncs in
dynptr_write_kfuncs first try to obtain a writable slice in C via
bpf_dynptr_slice_rdwr() and write directly into it:

kernel/bpf/helpers.c:bpf_dynptr_memset() {
	...
	slice = bpf_dynptr_slice_rdwr(p, offset, NULL, size);
	if (likely(slice)) {
		memset(slice, val, size);
		return 0;
	}
	...
}

For linear skb data and skb metadata, bpf_dynptr_slice_rdwr() returns a
direct pointer into skb->head without uncloning the skb, relying on the
verifier having emitted bpf_unclone_prologue():

kernel/bpf/helpers.c:bpf_dynptr_slice_rdwr() {
	/*
	 * 1) The requested slice is in the head of the skb. In this case, the
	 * returned pointer is directly to skb data, and if the skb is cloned, the
	 * verifier will have uncloned it (see bpf_unclone_prologue()) already.
	 * The pointer can be directly written into.
	 */
	...
	return bpf_dynptr_slice(p, offset, buffer__nullable, buffer__szk);
}

When a program calls bpf_dynptr_slice_rdwr() directly, check_special_kfunc()
calls may_access_direct_pkt_data(env, NULL, BPF_WRITE), which sets
env->seen_direct_write = true. However, specialize_kfunc() restores
env->seen_direct_write to its previous value when specializing
KF_bpf_dynptr_from_skb:

kernel/bpf/verifier.c:specialize_kfunc() {
	...
	} else if (func_id == special_kfunc_list[KF_bpf_dynptr_from_skb]) {
		seen_direct_write = env->seen_direct_write;
		is_rdonly = !may_access_direct_pkt_data(env, NULL, BPF_WRITE);

		if (is_rdonly)
			addr = (unsigned long)bpf_dynptr_from_skb_rdonly;

		/* restore env->seen_direct_write to its original value, since
		 * may_access_direct_pkt_data mutates it
		 */
		env->seen_direct_write = seen_direct_write;
	}
}

If a program creates an skb or skb_meta dynptr and only modifies it through
one of the kfuncs in dynptr_write_kfuncs without performing other direct
packet writes, env->seen_direct_write remains false. Could this skip
bpf_unclone_prologue() and allow these kfuncs to overwrite shared skb->head
memory in place on cloned skbs?

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

[PATCH bpf-next 34/38] 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: b5964b968ac6 ("bpf: Add skb dynptrs")
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 35/38] 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.

Report such a dynptr as that of an skb to whoever asks for the type of the
argument of a call. The type in the register stays as it is, nothing
becomes forbidden for the dynptr itself. A clone inherits the caution.

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, 7 insertions(+), 1 deletion(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index e002728abd04..03b19829f27a 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -8416,6 +8416,7 @@ static int process_dynptr_func(struct bpf_verifier_env *env, struct bpf_reg_stat
 			       argno_t argno, int insn_idx, enum bpf_arg_type arg_type,
 			       struct bpf_call_arg_meta *meta)
 {
+	bool is_arg = reg->type == CONST_PTR_TO_DYNPTR;
 	int spi, err = 0;
 
 	/*  MEM_UNINIT - Points to memory that is an appropriate candidate for
@@ -8502,7 +8503,12 @@ static int process_dynptr_func(struct bpf_verifier_env *env, struct bpf_reg_stat
 			reg = &bpf_stack_slot(state, spi)->spilled_ptr;
 		}
 
-		meta->dynptr.type = reg->dynptr.type;
+		/*
+		 * The callers of a global function 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.
+		 */
+		meta->dynptr.type = is_arg ? BPF_DYNPTR_TYPE_SKB : reg->dynptr.type;
 		meta->dynptr.id = reg->id;
 		meta->dynptr.parent_id = reg->parent_id;
 	}
-- 
2.55.0

[PATCH bpf-next 36/38] 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. Make
the skb writable up front and 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 | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index bf01152d0cf9..54e9084b9263 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -1752,9 +1752,32 @@ static const struct bpf_func_proto bpf_skb_store_bytes_proto = {
 	.arg5_type	= ARG_ANYTHING,
 };
 
+/*
+ * 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;
+}
+
 int __bpf_skb_store_bytes(struct sk_buff *skb, u32 offset, const void *from,
 			  u32 len, u64 flags)
 {
+	if (unlikely(offset > INT_MAX))
+		return -EFAULT;
+	if (unlikely(bpf_try_make_writable_from(skb, offset + len, &from)))
+		return -EFAULT;
+
 	return ____bpf_skb_store_bytes(skb, offset, from, len, flags);
 }
 
@@ -12424,7 +12447,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

Re: [PATCH bpf-next 36/38] bpf: Follow the head if an skb is written from a slice

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] bpf: duplicate bpf_try_make_writable() call in __bpf_skb_store_bytes()
--

commit cd64e2e5d6a96038e5e37496663c8035d9bcd26c
Author: Josef Bacik <josef@toxicpanda.com>

bpf: Follow the head if an skb is written from a slice

This commit introduces bpf_try_make_writable_from() and updates
__bpf_skb_store_bytes() and __bpf_skb_meta_store_bytes() to adjust the source
pointer when making an skb writable reallocates its head.

> diff --git a/net/core/filter.c b/net/core/filter.c
> index bf01152d0cf9a..54e9084b9263a 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -1752,9 +1752,32 @@ static const struct bpf_func_proto bpf_skb_store_bytes_proto = {
>  	.arg5_type	= ARG_ANYTHING,
>  };
>  
> +/*
> + * 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;
> +}
> +
>  int __bpf_skb_store_bytes(struct sk_buff *skb, u32 offset, const void *from,
>  			  u32 len, u64 flags)
>  {
> +	if (unlikely(offset > INT_MAX))
> +		return -EFAULT;
> +	if (unlikely(bpf_try_make_writable_from(skb, offset + len, &from)))
> +		return -EFAULT;
> +
>  	return ____bpf_skb_store_bytes(skb, offset, from, len, flags);
>  }

[Severity: Medium]
Does calling ____bpf_skb_store_bytes() after bpf_try_make_writable_from() end
up calling bpf_try_make_writable() a second time without updating from?

Looking at ____bpf_skb_store_bytes() in net/core/filter.c:

    BPF_CALL_5(bpf_skb_store_bytes, struct sk_buff *, skb, u32, offset,
               const void *, from, u32, len, u64, flags)
    {
        void *ptr;

        if (unlikely(flags & ~(BPF_F_RECOMPUTE_CSUM | BPF_F_INVALIDATE_HASH)))
            return -EINVAL;
        if (unlikely(offset > INT_MAX))
            return -EFAULT;
        if (unlikely(bpf_try_make_writable(skb, offset + len)))
            return -EFAULT;

        ptr = skb->data + offset;
        if (flags & BPF_F_RECOMPUTE_CSUM)
            __skb_postpull_rcsum(skb, ptr, len, offset);

        memcpy(ptr, from, len);

When a BPF program calls bpf_dynptr_write() or bpf_dynptr_copy() on a
BPF_DYNPTR_TYPE_SKB dynptr using a slice of the same skb as the source,
__bpf_skb_store_bytes() calls bpf_try_make_writable_from() and then calls
____bpf_skb_store_bytes().

Inside ____bpf_skb_store_bytes(), the second bpf_try_make_writable() call
follows the chain:

    bpf_try_make_writable()
      __bpf_try_make_writable()
        skb_ensure_writable()
          pskb_may_pull()
            pskb_may_pull_reason()
              skb_might_realloc()
                pskb_expand_head()

When CONFIG_FAIL_SKB_REALLOC triggers, skb_might_realloc() reallocates and
frees skb->head during that second call without updating from, leaving from
pointing to the freed head before copying into ptr.

Also, because __bpf_skb_store_bytes() calls bpf_try_make_writable_from()
before ____bpf_skb_store_bytes() checks flags, invalid flags will only be
rejected after the skb has already been uncloned or pulled (and will return
-EFAULT instead of -EINVAL if skb_ensure_writable() fails, unlike
__bpf_skb_meta_store_bytes() which checks flags first).

Note that passing packet slices directly to the bpf_skb_store_bytes() or
bpf_lwt_seg6_store_bytes() helpers is prevented later in the series by commit
"bpf: Refuse packet slices for packet-changing helpers", so could
bpf_try_make_writable_from() be used directly inside ____bpf_skb_store_bytes()
instead of duplicating the checks in __bpf_skb_store_bytes()?

[ ... ]

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

[PATCH bpf-next 37/38] 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. The other kinds of dynptr 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 54e9084b9263..0e3261f8d322 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -1731,7 +1731,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 38/38] 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. bpf_skb_store_bytes() makes the
skb writable before it copies from its source, and that can reallocate the
head which the source 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. The
helper then reads the old head, which has been freed:
bpf_skb_store_bytes(), bpf_lwt_push_encap(), bpf_lwt_seg6_store_bytes()
and bpf_lwt_seg6_action().

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 03b19829f27a..1cf7f2e57a16 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -9693,6 +9693,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