Re: [PATCH bpf-next v5 2/3] bpf, sockmap: reject a packet-modifying SK_SKB stream parser

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The fix for the stream parser slab-use-after-free can be bypassed because `BPF_FUNC_dynptr_write` is missing from `bpf_helper_changes_pkt_data()`.
- [High] Lockless access to `link->prog` in `bpf_link_show_fdinfo` and `bpf_link_get_info_by_fd` can lead to a Use-After-Free if the program is updated concurrently.
--

commit bb301227c425ce89eb44f853ff40b55c28c66bcd
Author: Sechang Lim <rhkrqnwk98@gmail.com>

bpf, sockmap: reject a packet-modifying SK_SKB stream parser

The patch prevents a slab-use-after-free by rejecting SK_SKB stream parser
programs that modify the packet. It does this by checking if
prog->aux->changes_pkt_data is set at attach time.

> diff --git a/net/core/sock_map.c b/net/core/sock_map.c
> index 99e3789492a09..c60ba6d292f9a 100644
> --- a/net/core/sock_map.c
> +++ b/net/core/sock_map.c
> @@ -1515,6 +1515,17 @@ static int sock_map_prog_link_lookup(struct bpf_map *map, struct bpf_prog ***ppr
>  	return 0;
>  }
>  
> +static int sock_map_prog_attach_check(enum bpf_attach_type attach_type,
> +				      struct bpf_prog *prog)
> +{
> +	/* A stream parser must not modify the skb, only measure it. */
> +	if (prog && attach_type == BPF_SK_SKB_STREAM_PARSER &&
> +	    prog->aux->changes_pkt_data)
> +		return -EINVAL;

[Severity: High]
This isn't a bug introduced by this patch, but does this check completely
prevent packet modifications?

Looking at bpf_helper_changes_pkt_data() in net/core/filter.c, it seems
BPF_FUNC_dynptr_write is missing from the list. If a program acquires a
writable dynptr via bpf_dynptr_from_skb() and calls bpf_dynptr_write(), it
internally calls __bpf_skb_store_bytes() which can trigger pskb_may_pull() and
cause the exact same slab-use-after-free this patch is trying to fix.

> +
> +	return 0;
> +}
> +

[ ... ]

> @@ -1776,6 +1791,11 @@ static int sock_map_link_update_prog(struct bpf_link *link,
>  		ret = -EINVAL;
>  		goto out;
>  	}
> +
> +	ret = sock_map_prog_attach_check(link->attach_type, prog);
> +	if (ret)
> +		goto out;
> +

[Severity: High]
This isn't a bug introduced by this patch, but does the later program exchange
in this function race with lockless readers?

Later in sock_map_link_update_prog(), the program is updated using
xchg(&link->prog, prog) and the old program is freed via bpf_prog_put().
In kernel/bpf/syscall.c, bpf_link_show_fdinfo() and bpf_link_get_info_by_fd()
access link->prog locklessly without holding rcu_read_lock().

If a reader is preempted after fetching link->prog, could an RCU grace period
elapse and bpf_prog_put() free the old program? When the reader resumes,
dereferencing the freed memory (e.g., prog->tag or prog->aux->id) could lead
to a use-after-free.

>  	if (!sockmap_link->map) {
>  		ret = -ENOLINK;
>  		goto out;

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