[PATCH bpf v2] bpf: Fix NULL pointer dereference in bpf_sk_storage_clone and diag paths

[PATCH bpf v2] bpf: Fix NULL pointer dereference in bpf_sk_storage_clone and diag paths

[Weiming Shi]

bpf_selem_unlink_nofail() sets SDATA(selem)->smap to NULL before
removing the selem from the storage hlist. A concurrent RCU reader in
bpf_sk_storage_clone() can observe the selem still on the list with
smap already NULL, causing a NULL pointer dereference.

 general protection fault, probably for non-canonical address 0xdffffc000000000a:
 KASAN: null-ptr-deref in range [0x0000000000000050-0x0000000000000057]
 RIP: 0010:bpf_sk_storage_clone+0x1cd/0xaa0 net/core/bpf_sk_storage.c:174
 Call Trace:
  <IRQ>
  sk_clone+0xfed/0x1980 net/core/sock.c:2591
  inet_csk_clone_lock+0x30/0x760 net/ipv4/inet_connection_sock.c:1222
  tcp_create_openreq_child+0x35/0x2680 net/ipv4/tcp_minisocks.c:571
  tcp_v4_syn_recv_sock+0x123/0xf90 net/ipv4/tcp_ipv4.c:1729
  tcp_check_req+0x8e1/0x2580 include/net/tcp.h:855
  tcp_v4_rcv+0x1845/0x3b80 net/ipv4/tcp_ipv4.c:2347

Add a NULL check for smap in bpf_sk_storage_clone().
bpf_sk_storage_diag_put_all() has the same unprotected dereference
pattern, add a NULL check there as well.

Fixes: 5d800f87d0a5 ("bpf: Support lockless unlink when freeing map or local storage")
Reported-by: Xiang Mei <xmei5@asu.edu>
Signed-off-by: Weiming Shi <bestswngs@gmail.com>
---
v2:
 drop the NULL check in diag_get(); The caller already checks smap for
NULL.

 net/core/bpf_sk_storage.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c
index f8338acebf077..67901de5b9c65 100644
--- a/net/core/bpf_sk_storage.c
+++ b/net/core/bpf_sk_storage.c
@@ -172,7 +172,7 @@ int bpf_sk_storage_clone(const struct sock *sk, struct sock *newsk)
 		struct bpf_map *map;
 
 		smap = rcu_dereference(SDATA(selem)->smap);
-		if (!(smap->map.map_flags & BPF_F_CLONE))
+		if (!smap || !(smap->map.map_flags & BPF_F_CLONE))
 			continue;
 
 		/* Note that for lockless listeners adding new element
@@ -599,6 +599,8 @@ static int bpf_sk_storage_diag_put_all(struct sock *sk, struct sk_buff *skb,
 	saved_len = skb->len;
 	hlist_for_each_entry_rcu(selem, &sk_storage->list, snode) {
 		smap = rcu_dereference(SDATA(selem)->smap);
+		if (!smap)
+			continue;
 		diag_size += nla_value_size(smap->map.value_size);
 
 		if (nla_stgs && diag_get(SDATA(selem), skb))
-- 
2.43.0

Re: [PATCH bpf v2] bpf: Fix NULL pointer dereference in bpf_sk_storage_clone and diag paths

[bot+bpf-ci]

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

> diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c
> index 14eb7812bda4..b2cd2b0292cb 100644
> --- a/net/core/bpf_sk_storage.c
> +++ b/net/core/bpf_sk_storage.c

[ ... ]

> @@ -596,6 +596,8 @@ static int bpf_sk_storage_diag_put_all(struct sock *sk, struct sk_buff *skb,
>  	saved_len = skb->len;
>  	hlist_for_each_entry_rcu(selem, &sk_storage->list, snode) {
>  		smap = rcu_dereference(SDATA(selem)->smap);
> +		if (!smap)
> +			continue;
>  		diag_size += nla_value_size(smap->map.value_size);
>
>  		if (nla_stgs && diag_get(SDATA(selem), skb))

Martin KaFai Lau pointed out in the v1 review that the NULL check for
smap doesn't fully address the race condition. The issue is that
diag_get() will re-dereference sdata->smap independently:

net/core/bpf_sk_storage.c:diag_get() {
    ...
    smap = rcu_dereference(sdata->smap);
    ...
}

Between the NULL check above and the call to diag_get(), smap could
become NULL again, leading to the same race condition.

Martin suggested passing smap as a parameter to diag_get() to avoid
the second dereference. This would ensure diag_get() uses the same
smap pointer that was already checked.

Could the fix be updated to pass smap to diag_get() as Martin
suggested?

Reference: https://lore.kernel.org/bpf/2026420182243.zAiN.martin.lau@linux.dev/


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

Re: [PATCH bpf v2] bpf: Fix NULL pointer dereference in bpf_sk_storage_clone and diag paths

[Amery Hung]

On Tue, Apr 21, 2026 at 9:24 AM <bot+bpf-ci@kernel.org> wrote:
>
> > diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c
> > index 14eb7812bda4..b2cd2b0292cb 100644
> > --- a/net/core/bpf_sk_storage.c
> > +++ b/net/core/bpf_sk_storage.c
>
> [ ... ]
>
> > @@ -596,6 +596,8 @@ static int bpf_sk_storage_diag_put_all(struct sock *sk, struct sk_buff *skb,
> >       saved_len = skb->len;
> >       hlist_for_each_entry_rcu(selem, &sk_storage->list, snode) {
> >               smap = rcu_dereference(SDATA(selem)->smap);
> > +             if (!smap)
> > +                     continue;
> >               diag_size += nla_value_size(smap->map.value_size);
> >
> >               if (nla_stgs && diag_get(SDATA(selem), skb))
>
> Martin KaFai Lau pointed out in the v1 review that the NULL check for
> smap doesn't fully address the race condition. The issue is that
> diag_get() will re-dereference sdata->smap independently:
>
> net/core/bpf_sk_storage.c:diag_get() {
>     ...
>     smap = rcu_dereference(sdata->smap);
>     ...
> }
>
> Between the NULL check above and the call to diag_get(), smap could
> become NULL again, leading to the same race condition.
>
> Martin suggested passing smap as a parameter to diag_get() to avoid
> the second dereference. This would ensure diag_get() uses the same
> smap pointer that was already checked.
>
> Could the fix be updated to pass smap to diag_get() as Martin
> suggested?
>
> Reference: https://lore.kernel.org/bpf/2026420182243.zAiN.martin.lau@linux.dev/
>

CI is right. Please pass smap to diag_get() as Martin suggested. It
will be the caller's responsibility to do rcu_dereference() and
null-check it. Note that there are more than one caller to diag_get().

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

Re: [PATCH bpf v2] bpf: Fix NULL pointer dereference in bpf_sk_storage_clone and diag paths

[sashiko-bot]

Sashiko AI review found 1 potential issue(s):
- [High] The NULL pointer dereference fix in the diag paths is incomplete due to a Time-Of-Check to Time-Of-Use (TOCTOU) race condition in `diag_get()`.
--

commit 89ba4092c0928afed16ec4db32d2b5ffaa88889b
Author: Weiming Shi <bestswngs@gmail.com>

bpf: Fix NULL pointer dereference in bpf_sk_storage_clone and diag paths

This commit adds NULL checks for the storage map pointer in
bpf_sk_storage_clone() and bpf_sk_storage_diag_put_all() to prevent a
NULL pointer dereference when racing with a concurrent storage map removal.

> diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c
> index 14eb7812bda4a..b2cd2b0292cb6 100644
> --- a/net/core/bpf_sk_storage.c
> +++ b/net/core/bpf_sk_storage.c

[ ... ]

> @@ -596,6 +596,8 @@ static int bpf_sk_storage_diag_put_all(struct sock *sk, struct sk_buff *skb,
>  	saved_len = skb->len;
>  	hlist_for_each_entry_rcu(selem, &sk_storage->list, snode) {
>  		smap = rcu_dereference(SDATA(selem)->smap);
> +		if (!smap)
> +			continue;
>  		diag_size += nla_value_size(smap->map.value_size);
>  
>  		if (nla_stgs && diag_get(SDATA(selem), skb))

Can this still trigger a NULL pointer dereference due to a race condition?

If a concurrent unlinking operation sets the storage map pointer to NULL
after the check above, but before diag_get() executes, it looks like
diag_get() could still read the NULL pointer:

net/core/bpf_sk_storage.c:diag_get() {
    ...
    smap = rcu_dereference(sdata->smap);
    if (nla_put_u32(skb, SK_DIAG_BPF_STORAGE_MAP_ID, smap->map.id))
    ...
}

Because rcu_dereference() re-fetches the pointer from memory, it could now
return NULL, leading to a crash on the smap->map.id access.

Would it be safer to pass the already-validated smap pointer directly to
diag_get() as an argument, or add a NULL check inside diag_get() itself?

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