Re: [PATCH bpf] bpf: Fix NULL pointer dereference in __bpf_sk_storage_map_seq_show

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Socket local storage elements are removed from the RCU-protected BPF map inside the socket's destruction path (`__sk_destruct`), violating RCU-001 rules and causing a use-after-free of the socket because the socket can be freed while a BPF iterator is still using it.
- [High] `bpf_selem_unlink_nofail()` concurrently sets `SDATA(selem)->smap` and `selem->local_storage` to NULL, causing guaranteed NULL pointer dereferences in concurrent `bpf_selem_unlink()` and `bpf_selem_unlink_map()` paths.
--

commit cabffb2f3787054c5c6ba2a413eaf5dc639ba23d
Author: Cen Zhang (Microsoft Security FORGE Labs) <blbllhy@gmail.com>
bpf: Fix NULL pointer dereference in __bpf_sk_storage_map_seq_show

This commit addresses a NULL pointer dereference in
__bpf_sk_storage_map_seq_show() during concurrent socket closure. It adds a
NULL check after re-reading selem->local_storage to prevent dereferencing a
cleared pointer.

> diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c
> index 1d295a8769fad..7228520aa2b6e 100644
> --- a/net/core/bpf_sk_storage.c
> +++ b/net/core/bpf_sk_storage.c
> @@ -806,6 +806,8 @@ 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);
> +			if (!sk_storage)
> +				return 0;
>  			ctx.sk = sk_storage->owner;

[Severity: High]
This is a pre-existing issue, but does this patch fully address the
underlying TOCTOU race?

If a socket without SOCK_RCU_FREE (e.g., AF_UNIX) is closed, __sk_destruct()
in net/core/sock.c calls bpf_sk_storage_free(), which unlinks the storage
from the BPF map.

Couldn't a concurrent BPF iterator in __bpf_sk_storage_map_seq_show()
operating under rcu_read_lock() find the storage element before it is
unlinked, and read ctx.sk = sk_storage->owner? The socket is then
synchronously freed by __sk_destruct() without waiting for an RCU grace
period, leaving the iterator to pass a dangling pointer to the BPF program
via bpf_iter_run_prog(prog, &ctx).

This appears to violate the remove-before-reclaim ordering (RCU-001) since
the socket is removed from the RCU-protected BPF map inside the socket's
destruction path.

[Severity: High]
This is a pre-existing issue, but might this only fix a downstream symptom
in the iterator path while leaving other concurrent map operations
vulnerable?

When a socket goes away, bpf_selem_unlink_nofail() in
kernel/bpf/bpf_local_storage.c sets SDATA(selem)->smap to NULL without
holding local_storage->lock.

If a concurrent map deletion calls bpf_selem_unlink(), it acquires
local_storage->lock first, sees selem->snode is still hashed, and calls
bpf_selem_unlink_map(). Could bpf_selem_unlink_map() then unconditionally
dereference the now-NULL SDATA(selem)->smap, crashing inside
select_bucket()?

kernel/bpf/bpf_local_storage.c:bpf_selem_unlink_map() {
    ...
    smap = rcu_dereference_check(SDATA(selem)->smap,
                                 bpf_rcu_lock_held());
    b = select_bucket(smap, local_storage);
    ...
}

Similarly, it looks like bpf_selem_unlink() could use a potentially NULL
local_storage for lock acquisition:

kernel/bpf/bpf_local_storage.c:bpf_selem_unlink() {
    ...
    local_storage = rcu_dereference_check(selem->local_storage,
                                          bpf_rcu_lock_held());
    err = raw_res_spin_lock_irqsave(&local_storage->lock, flags);
    ...
}

>  			ctx.value = SDATA(selem)->data;
>  		}
>  		ret = bpf_iter_run_prog(prog, &ctx);

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