Re: [PATCH bpf v4 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 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Incomplete mitigation for a Use-After-Free in the stream parser. The newly added attach-time check can be bypassed using `bpf_dynptr_write()`.
--

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

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

sk_psock_strp_parse() runs the BPF_PROG_TYPE_SK_SKB stream-parser program
to find the length of the next message. strparser assembles a message out
of several received skbs by chaining them onto the head's frag_list and
recording where to append the next one in strp->skb_nextp. A stream parser
is only meant to measure the next message, so this patch rejects a parser
whose program can change packet data 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)
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^
[Severity: High]
This isn't a bug introduced by this patch, but could this attach-time check be
bypassed by using bpf_dynptr_write()?

Because BPF_FUNC_dynptr_write is missing from the
bpf_helper_changes_pkt_data() switch statement in net/core/filter.c, and
bpf_dynptr_from_skb() is not flagged by bpf_is_kfunc_pkt_changing(), the
verifier never sets changes_pkt_data for programs using dynptrs to modify
the packet.

If a BPF program obtains a writable dynptr to the SKB using the
bpf_dynptr_from_skb() kfunc, it can modify the packet using the
bpf_dynptr_write() helper.

For SKB dynptrs, bpf_dynptr_write() calls __bpf_dynptr_write() in
kernel/bpf/helpers.c:

	case BPF_DYNPTR_TYPE_SKB:
		return __bpf_skb_store_bytes(dst->data, dst->offset + offset, src, len,
					     flags);

This modifies the SKB and triggers skb_ensure_writable(). If the write
exceeds skb_headlen(skb), this invokes __pskb_pull_tail() and unconditionally
frees the frag_list skbs, causing the exact use-after-free in the stream
parser that this patch intends to fix.

> +		return -EINVAL;
> +
> +	return 0;
> +}
> +

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