Skip to content

Commit 375e4e3

Browse files
winminMartin KaFai Lau
authored andcommitted
bpf: Fix NULL pointer dereference in bpf_sk_storage_clone and diag paths
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 issue. Add a NULL check and pass the validated smap directly to diag_get(), which is refactored to take smap as a parameter instead of reading it internally. bpf_sk_storage_diag_put() uses diag->maps[i] which is always valid under its refcount, so diag->maps[i] is passed directly to diag_get(). Fixes: 5d800f8 ("bpf: Support lockless unlink when freeing map or local storage") Reported-by: Xiang Mei <xmei5@asu.edu> Acked-by: Amery Hung <ameryhung@gmail.com> Signed-off-by: Weiming Shi <bestswngs@gmail.com> Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org> Link: https://patch.msgid.link/20260422065411.1007737-2-bestswngs@gmail.com
1 parent cd0eb48 commit 375e4e3

1 file changed

Lines changed: 7 additions & 6 deletions

File tree

net/core/bpf_sk_storage.c

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -172,7 +172,7 @@ int bpf_sk_storage_clone(const struct sock *sk, struct sock *newsk)
172172
struct bpf_map *map;
173173

174174
smap = rcu_dereference(SDATA(selem)->smap);
175-
if (!(smap->map.map_flags & BPF_F_CLONE))
175+
if (!smap || !(smap->map.map_flags & BPF_F_CLONE))
176176
continue;
177177

178178
/* Note that for lockless listeners adding new element
@@ -531,10 +531,10 @@ bpf_sk_storage_diag_alloc(const struct nlattr *nla_stgs)
531531
}
532532
EXPORT_SYMBOL_GPL(bpf_sk_storage_diag_alloc);
533533

534-
static int diag_get(struct bpf_local_storage_data *sdata, struct sk_buff *skb)
534+
static int diag_get(struct bpf_local_storage_map *smap,
535+
struct bpf_local_storage_data *sdata, struct sk_buff *skb)
535536
{
536537
struct nlattr *nla_stg, *nla_value;
537-
struct bpf_local_storage_map *smap;
538538

539539
/* It cannot exceed max nlattr's payload */
540540
BUILD_BUG_ON(U16_MAX - NLA_HDRLEN < BPF_LOCAL_STORAGE_MAX_VALUE_SIZE);
@@ -543,7 +543,6 @@ static int diag_get(struct bpf_local_storage_data *sdata, struct sk_buff *skb)
543543
if (!nla_stg)
544544
return -EMSGSIZE;
545545

546-
smap = rcu_dereference(sdata->smap);
547546
if (nla_put_u32(skb, SK_DIAG_BPF_STORAGE_MAP_ID, smap->map.id))
548547
goto errout;
549548

@@ -596,9 +595,11 @@ static int bpf_sk_storage_diag_put_all(struct sock *sk, struct sk_buff *skb,
596595
saved_len = skb->len;
597596
hlist_for_each_entry_rcu(selem, &sk_storage->list, snode) {
598597
smap = rcu_dereference(SDATA(selem)->smap);
598+
if (!smap)
599+
continue;
599600
diag_size += nla_value_size(smap->map.value_size);
600601

601-
if (nla_stgs && diag_get(SDATA(selem), skb))
602+
if (nla_stgs && diag_get(smap, SDATA(selem), skb))
602603
/* Continue to learn diag_size */
603604
err = -EMSGSIZE;
604605
}
@@ -665,7 +666,7 @@ int bpf_sk_storage_diag_put(struct bpf_sk_storage_diag *diag,
665666

666667
diag_size += nla_value_size(diag->maps[i]->value_size);
667668

668-
if (nla_stgs && diag_get(sdata, skb))
669+
if (nla_stgs && diag_get((struct bpf_local_storage_map *)diag->maps[i], sdata, skb))
669670
/* Continue to learn diag_size */
670671
err = -EMSGSIZE;
671672
}

0 commit comments

Comments
 (0)