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

[PATCH bpf v3] 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() and bpf_sk_storage_diag_put() have the
same unprotected dereference pattern, add NULL checks there as well and
pass the validated smap to diag_get() so it no longer performs its own
rcu_dereference.

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>
---
v3:
  pass smap to diag_get()
v2:
  drop the NULL check in diag_get(); The caller already checks smap for
NULL.

 net/core/bpf_sk_storage.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c
index f8338acebf077..1f9bd0005883b 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
@@ -534,10 +534,10 @@ bpf_sk_storage_diag_alloc(const struct nlattr *nla_stgs)
 }
 EXPORT_SYMBOL_GPL(bpf_sk_storage_diag_alloc);
 
-static int diag_get(struct bpf_local_storage_data *sdata, struct sk_buff *skb)
+static int diag_get(struct bpf_local_storage_map *smap,
+		    struct bpf_local_storage_data *sdata, struct sk_buff *skb)
 {
 	struct nlattr *nla_stg, *nla_value;
-	struct bpf_local_storage_map *smap;
 
 	/* It cannot exceed max nlattr's payload */
 	BUILD_BUG_ON(U16_MAX - NLA_HDRLEN < BPF_LOCAL_STORAGE_MAX_VALUE_SIZE);
@@ -546,7 +546,6 @@ static int diag_get(struct bpf_local_storage_data *sdata, struct sk_buff *skb)
 	if (!nla_stg)
 		return -EMSGSIZE;
 
-	smap = rcu_dereference(sdata->smap);
 	if (nla_put_u32(skb, SK_DIAG_BPF_STORAGE_MAP_ID, smap->map.id))
 		goto errout;
 
@@ -599,9 +598,11 @@ 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))
+		if (nla_stgs && diag_get(smap, SDATA(selem), skb))
 			/* Continue to learn diag_size */
 			err = -EMSGSIZE;
 	}
@@ -633,6 +634,7 @@ int bpf_sk_storage_diag_put(struct bpf_sk_storage_diag *diag,
 	unsigned int diag_size = nla_total_size(0);
 	struct bpf_local_storage *sk_storage;
 	struct bpf_local_storage_data *sdata;
+	struct bpf_local_storage_map *smap;
 	struct nlattr *nla_stgs;
 	unsigned int saved_len;
 	int err = 0;
@@ -668,7 +670,11 @@ int bpf_sk_storage_diag_put(struct bpf_sk_storage_diag *diag,
 
 		diag_size += nla_value_size(diag->maps[i]->value_size);
 
-		if (nla_stgs && diag_get(sdata, skb))
+		smap = rcu_dereference(sdata->smap);
+		if (!smap)
+			continue;
+
+		if (nla_stgs && diag_get(smap, sdata, skb))
 			/* Continue to learn diag_size */
 			err = -EMSGSIZE;
 	}
-- 
2.43.0

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

[sashiko-bot]

Sashiko AI review found 1 potential issue(s):
- [High] diag_get() leaks uninitialized kernel memory to user space when dumping BPF map values containing special fields (e.g., bpf_spin_lock, bpf_timer).
--

Note: The format of this review has been altered to provide a free-form
summary without quoting the original patch code directly due to recitation
restrictions.

commit 32307cca407ebdd5cf1feae057cd9956ec19daa4
Author: Weiming Shi <bestswngs@gmail.com>
Subject: bpf: Fix NULL pointer dereference in bpf_sk_storage_clone and diag paths

This patch addresses a race condition where a concurrent RCU reader in
bpf_sk_storage_clone() could observe a selem with a NULL smap, leading to a
NULL pointer dereference. It adds NULL checks in clone and diag paths and
updates diag_get() to take the validated smap directly.

While reviewing the changes to the diag_get() function, I noticed a
potential pre-existing issue regarding how map values are copied to the
netlink attribute.

Does this function leak uninitialized kernel memory to user space when
dumping BPF map values that contain special fields like bpf_spin_lock or
bpf_timer?

The function allocates space for the value using nla_reserve_64bit(), which
reserves space in the skb but does not initialize it. It then copies the
map value into this space using copy_map_value_locked() or copy_map_value().
However, those copy functions intentionally skip copying special fields,
leaving holes in the destination buffer.

In the standard bpf_map_lookup_elem() syscall path,
check_and_init_map_value() is called to explicitly zero out these holes
after copying. Since diag_get() does not appear to initialize these holes
before sending the netlink message, could uninitialized skb memory be
exposed to user space?

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

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

[Amery Hung]

On Thu, Apr 23, 2026 at 12:38 AM <sashiko-bot@kernel.org> wrote:
>
> Sashiko AI review found 1 potential issue(s):
> - [High] diag_get() leaks uninitialized kernel memory to user space when dumping BPF map values containing special fields (e.g., bpf_spin_lock, bpf_timer).
> --
>
> Note: The format of this review has been altered to provide a free-form
> summary without quoting the original patch code directly due to recitation
> restrictions.
>
> commit 32307cca407ebdd5cf1feae057cd9956ec19daa4
> Author: Weiming Shi <bestswngs@gmail.com>
> Subject: bpf: Fix NULL pointer dereference in bpf_sk_storage_clone and diag paths
>
> This patch addresses a race condition where a concurrent RCU reader in
> bpf_sk_storage_clone() could observe a selem with a NULL smap, leading to a
> NULL pointer dereference. It adds NULL checks in clone and diag paths and
> updates diag_get() to take the validated smap directly.
>
> While reviewing the changes to the diag_get() function, I noticed a
> potential pre-existing issue regarding how map values are copied to the
> netlink attribute.
>
> Does this function leak uninitialized kernel memory to user space when
> dumping BPF map values that contain special fields like bpf_spin_lock or
> bpf_timer?
>
> The function allocates space for the value using nla_reserve_64bit(), which
> reserves space in the skb but does not initialize it. It then copies the
> map value into this space using copy_map_value_locked() or copy_map_value().
> However, those copy functions intentionally skip copying special fields,
> leaving holes in the destination buffer.
>
> In the standard bpf_map_lookup_elem() syscall path,
> check_and_init_map_value() is called to explicitly zero out these holes
> after copying. Since diag_get() does not appear to initialize these holes
> before sending the netlink message, could uninitialized skb memory be
> exposed to user space?

Sashiko is correct. Special fields need to be scrubbed here as well.
Will send a fix.

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

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

[Amery Hung]

On Wed, Apr 22, 2026 at 12:37 AM Weiming Shi <bestswngs@gmail.com> wrote:
>
> 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() and bpf_sk_storage_diag_put() have the
> same unprotected dereference pattern, add NULL checks there as well and
> pass the validated smap to diag_get() so it no longer performs its own
> rcu_dereference.
>
> 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>

Acked-by: Amery Hung <ameryhung@gmail.com>

> ---
> v3:
>   pass smap to diag_get()
> v2:
>   drop the NULL check in diag_get(); The caller already checks smap for
> NULL.
>
>  net/core/bpf_sk_storage.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c
> index f8338acebf077..1f9bd0005883b 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
> @@ -534,10 +534,10 @@ bpf_sk_storage_diag_alloc(const struct nlattr *nla_stgs)
>  }
>  EXPORT_SYMBOL_GPL(bpf_sk_storage_diag_alloc);
>
> -static int diag_get(struct bpf_local_storage_data *sdata, struct sk_buff *skb)
> +static int diag_get(struct bpf_local_storage_map *smap,
> +                   struct bpf_local_storage_data *sdata, struct sk_buff *skb)
>  {
>         struct nlattr *nla_stg, *nla_value;
> -       struct bpf_local_storage_map *smap;
>
>         /* It cannot exceed max nlattr's payload */
>         BUILD_BUG_ON(U16_MAX - NLA_HDRLEN < BPF_LOCAL_STORAGE_MAX_VALUE_SIZE);
> @@ -546,7 +546,6 @@ static int diag_get(struct bpf_local_storage_data *sdata, struct sk_buff *skb)
>         if (!nla_stg)
>                 return -EMSGSIZE;
>
> -       smap = rcu_dereference(sdata->smap);
>         if (nla_put_u32(skb, SK_DIAG_BPF_STORAGE_MAP_ID, smap->map.id))
>                 goto errout;
>
> @@ -599,9 +598,11 @@ 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))
> +               if (nla_stgs && diag_get(smap, SDATA(selem), skb))
>                         /* Continue to learn diag_size */
>                         err = -EMSGSIZE;
>         }
> @@ -633,6 +634,7 @@ int bpf_sk_storage_diag_put(struct bpf_sk_storage_diag *diag,
>         unsigned int diag_size = nla_total_size(0);
>         struct bpf_local_storage *sk_storage;
>         struct bpf_local_storage_data *sdata;
> +       struct bpf_local_storage_map *smap;
>         struct nlattr *nla_stgs;
>         unsigned int saved_len;
>         int err = 0;
> @@ -668,7 +670,11 @@ int bpf_sk_storage_diag_put(struct bpf_sk_storage_diag *diag,
>
>                 diag_size += nla_value_size(diag->maps[i]->value_size);
>
> -               if (nla_stgs && diag_get(sdata, skb))
> +               smap = rcu_dereference(sdata->smap);
> +               if (!smap)
> +                       continue;
> +
> +               if (nla_stgs && diag_get(smap, sdata, skb))
>                         /* Continue to learn diag_size */
>                         err = -EMSGSIZE;
>         }
> --
> 2.43.0
>