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

[sashiko-bot@kernel.org]

Sashiko AI review found 1 potential issue(s):
- [Medium] diag_get() returns -EMSGSIZE when a storage element is concurrently deleted, which falsely signals an out-of-buffer condition to the netlink subsystem.
--

commit 522b6760e7969af1ad0bb15843d5c06084d686f7
Author: Weiming Shi <bestswngs@gmail.com>

bpf: Fix NULL pointer dereference in bpf_sk_storage_clone and diag paths

This commit addresses a NULL pointer dereference in bpf_sk_storage_clone
and diagnostic paths by adding checks for when a concurrent RCU reader
observes a storage element with a NULL smap.

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

[ ... ]

> @@ -544,6 +544,8 @@ static int diag_get(struct bpf_local_storage_data *sdata, struct sk_buff *skb)
>  		return -EMSGSIZE;
>  
>  	smap = rcu_dereference(sdata->smap);
> +	if (!smap)
> +		goto errout;

When a storage element is concurrently deleted, smap evaluates to NULL here
and goes to errout, which returns -EMSGSIZE.

In netlink diagnostics, doesn't -EMSGSIZE specifically signal an
out-of-buffer condition?

Could propagating -EMSGSIZE for a concurrently deleted element cause socket
dumps to unnecessarily abort the current message and retry, or cause
exact-match diagnostic queries to falsely fail with an EMSGSIZE error
returned to userspace?

Would it be better to return 0 after cancelling the nested attribute to
gracefully skip the deleted element, or perhaps pass the already validated
smap from the caller directly into diag_get()?

>  	if (nla_put_u32(skb, SK_DIAG_BPF_STORAGE_MAP_ID, smap->map.id))
>  		goto errout;

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