Re: [PATCH bpf-next v3 12/15] bpf: tcp: Support parse/len/write header option hooks in bpf_tcp_ops

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Out-of-bounds write in bpf_tcp_ops_store_hdr_opt via TOCTOU race condition.
- [Medium] Logical starvation of bpf_tcp_ops header option space due to shared reservation allocation.
- [Low] BPF multi-line comment style violation.
--

commit 16cbab0861ea029c828cf9218ec942f85b8d7050
Author: Amery Hung <ameryhung@gmail.com>

bpf: tcp: Support parse/len/write header option hooks in bpf_tcp_ops

This commit adds TCP header option callbacks to the bpf_tcp_ops struct_ops
type. These mirror the legacy sockops callbacks but expose them as
struct_ops members for regular function signatures.

> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 2102f9f2afd63..7bf702117602d 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -3005,6 +3005,45 @@ struct bpf_tcp_ops {
>  
>  	/* Called on listen(2), right after the socket enters TCP_LISTEN. */
>  	void (*listen)(struct sock *sk);
> +
> +	/* Parse the TCP header options of an incoming skb received on an
> +	 * established connection. Use bpf_dynptr_from_skb()/bpf_skb_load_bytes()
> +	 * to access the options.
> +	 */

[Severity: Low]
This isn't a bug, but does this multi-line comment start on the same line as
the opening /*? The BPF subsystem coding style prefers the text to start on
the next line.

[ ... ]
> diff --git a/net/ipv4/bpf_tcp_ops.c b/net/ipv4/bpf_tcp_ops.c
> index 3febbc8dd1a06..b27613898337e 100644
> --- a/net/ipv4/bpf_tcp_ops.c
> +++ b/net/ipv4/bpf_tcp_ops.c
> @@ -66,6 +87,102 @@ static struct bpf_tcp_ops __bpf_tcp_ops = {
[ ... ]
> +BPF_CALL_4(bpf_tcp_ops_store_hdr_opt, void *, ctx, const void *, from,
> +	   u32, len, u64, flags)
> +{
> +	u64 *args = ctx;
> +	struct sk_buff *skb = (void *)(unsigned long)args[1];
> +	struct bpf_sock_ops_kern sock_ops = {};
> +	u32 opt_off = args[5];
> +	u8 *op, *opend;
> +
> +	/* bpf_tcp_ops does not keep track of the end of the written TCP header
> +	 * options, so search for it every time the helper is called. The free
> +	 * space is NOP-filled, so a TCPOPT_NOP ends the search rather than being
> +	 * skipped as in a normal option walk in sockops.
> +	 */

[Severity: Low]
This isn't a bug, but does this multi-line comment start on the same line as
the opening /*? The BPF subsystem coding style prefers the text to start on
the next line.

> +	op = skb->data + opt_off;
> +	opend = skb->data + tcp_hdrlen(skb);
> +	while (op < opend && *op != TCPOPT_NOP) {
> +		if (*op == TCPOPT_EOL || op + 1 >= opend || op[1] < 2)
> +			break;
> +		op += op[1];
> +	}
> +
> +	sock_ops.skb = skb;
> +	sock_ops.skb_data_end = op;
> +	sock_ops.remaining_opt_len = opend - op;
> +
> +	return __bpf_sock_ops_store_hdr_opt(&sock_ops, from, len, flags);
> +}

[Severity: High]
If a concurrent BPF program modifies the map value containing the TCP option
length during the helper execution, could the copied length cause the loop to
advance op beyond opend?

If op exceeds opend, would the calculation opend - op underflow the u8
remaining_opt_len into a large positive value? This could lead to an
out-of-bounds write when __bpf_sock_ops_store_hdr_opt() calls memcpy() using
skb_data_end.

[ ... ]
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index 978d8474460b0..15915d452d6a1 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -573,6 +573,13 @@ static void bpf_skops_write_hdr_opt(struct sock *sk, struct sk_buff *skb,
>  	if (nr_written < max_opt_len)
>  		memset(skb->data + first_opt_off + nr_written, TCPOPT_NOP,
>  		       max_opt_len - nr_written);
> +
> +	/* bpf_tcp_ops portion is NOP-filled (everything past the sockops
> +	 * writer's bytes). The writer find the append point by scanning from
> +	 * first_opt_off + nr_written to the first NOP.
> +	 */
> +	bpf_tcp_ops_call(write_hdr_opt, sk, skb, req, syn_skb, synack_type,
> +			 first_opt_off + nr_written);
>  }

[Severity: Medium]
Does max_opt_len contain the combined space reserved by both the legacy hook
and the new bpf_tcp_ops hook?

If a legacy program dynamically writes more options than it initially reserved,
could it incorrectly consume the space intended for bpf_tcp_ops, causing later
store_hdr_opt calls to fail with -ENOSPC?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706171918.317102-1-ameryhung@gmail.com?part=12