[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

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

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

[Martin KaFai Lau]

On Tue, Apr 21, 2026 at 11:54:12PM -0700, Weiming Shi wrote:
> @@ -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);

This is the same issue pointed out as v1. diag_size is updated with
a smap in diag->maps[i]...

>  
> -		if (nla_stgs && diag_get(sdata, skb))
> +		smap = rcu_dereference(sdata->smap);
> +		if (!smap)
> +			continue;

... and then nothing is stored in skb, so diag_size is incorrectly inflated.

> +
> +		if (nla_stgs && diag_get(smap, sdata, skb))

diag->maps[i] can be directly used here. I fixed it up before applying.

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to bpf/bpf.git (master)
by Martin KaFai Lau <martin.lau@kernel.org>:

On Tue, 21 Apr 2026 23:54:12 -0700 you 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
> 
> [...]

Here is the summary with links:
  - [bpf,v3] bpf: Fix NULL pointer dereference in bpf_sk_storage_clone and diag paths
    https://git.kernel.org/bpf/bpf/c/375e4e33c18d

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html