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

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.

[ ... ]

                  ^^^^

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?

[ ... ]


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