Re: [PATCH bpf-next v2 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 4 potential issue(s) to consider:
- [High] Incorrect pointer arithmetic on the BPF context array causes NULL pointer dereferences on 32-bit architectures.
- [High] A missing bounds check in TCP option parsing combined with a TOCTOU double-fetch allows a malicious BPF program to cause a heap buffer overflow.
- [Medium] The struct_ops TCP option reservation is gated by a completely unrelated cgroup BPF static key, breaking the feature for established connections.
- [Low] Multi-line comments in BPF subsystem files violate the required comment style.
--

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

Add the TCP header option callbacks to the bpf_tcp_ops struct_ops type

This patch exposes TCP header option callbacks as struct_ops members to
allow BPF programs to parse, reserve, and write TCP header options. It
mirrors the legacy BPF_SOCK_OPS callbacks and shares the reserved window.

> The hdr_opt_len fast path in tcp_established_options() is gated by
> cgroup_bpf_enabled(CGROUP_TCP_SOCK_OPS). Note this is a global,
> per-attach-type static branch: it is enabled whenever any bpf_tcp_ops is
> attached, even one that does not implement hdr_opt_len or that is attached
> to a different cgroup.

[Severity: Medium]
Is this description accurate? Struct_ops do not enable this cgroup static
key. Will this check evaluate to false in non-cgroup-bpf deployments,
skipping space reservation for established connections but still calling
write_hdr_opt later?

> diff --git a/include/net/tcp.h b/include/net/tcp.h
> --- 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);
> +
[Severity: Low]
This isn't a bug, but does the BPF subsystem prefer the opening /* to be on
its own line for multi-line comments?

> +	/* 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.
> +	 */
> +	void (*parse_hdr)(struct sock *sk, struct sk_buff *skb);
> +
[ ... ]
> diff --git a/net/ipv4/bpf_tcp_ops.c b/net/ipv4/bpf_tcp_ops.c
> --- a/net/ipv4/bpf_tcp_ops.c
> +++ b/net/ipv4/bpf_tcp_ops.c
[ ... ]
> @@ -66,6 +87,99 @@ static struct bpf_tcp_ops __bpf_tcp_ops = {
>  	.retrans = retrans_stub,
>  	.connect = connect_stub,
>  	.listen = listen_stub,
> +	.parse_hdr = parse_hdr_stub,
> +	.hdr_opt_len = hdr_opt_len_stub,
> +	.write_hdr_opt = write_hdr_opt_stub,
> +};
> +
> +BPF_CALL_4(bpf_tcp_ops_store_hdr_opt, void *, ctx, const void *, from,
> +	   u32, len, u64, flags)
> +{
[Severity: High]
Can this pointer cast lead to reading the wrong offset on 32-bit systems?
The ctx array contains 64-bit integers, so casting directly to a pointer
array might result in reading the upper 32 bits (which are zero) on
little-endian architectures, causing a NULL pointer dereference.

> +	struct sk_buff *skb = ((struct sk_buff **)ctx)[1];
> +	struct bpf_sock_ops_kern sock_ops = {};
> +	u32 opt_off = ((u64 *)ctx)[5];
> +	u8 *op, *opend;
> +
[Severity: Low]
This isn't a bug, but multi-line comments in the BPF subsystem typically
place the opening /* on a separate line.

> +	/* 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.
> +	 */
> +	op = skb->data + opt_off;
> +	opend = skb->data + tcp_hdrlen(skb);
> +	while (op < opend && *op != TCPOPT_NOP) {
[Severity: High]
Does this loop prevent op from advancing past opend if the option length
byte is forged? If op[1] is maliciously modified concurrently in the BPF
map memory before __bpf_sock_ops_store_hdr_opt() does its memcpy, could
the next call to bpf_tcp_ops_store_hdr_opt() advance op past opend and
cause an out-of-bounds memcpy?

> +		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);
> +}
[ ... ]
> +BPF_CALL_4(bpf_tcp_ops_load_hdr_opt, void *, ctx, void *, search_res,
> +	   u32, len, u64, flags)
> +{
[Severity: High]
Will this cast misalign the array index on 32-bit architectures? Treating
the 64-bit integer array as an array of 32-bit pointers will cause the
wrong bytes to be read for the context argument.

> +	struct sk_buff *skb = ((struct sk_buff **)ctx)[1];
> +	struct bpf_sock_ops_kern sock_ops = {};
[ ... ]
> +BPF_CALL_3(bpf_tcp_ops_reserve_hdr_opt, void *, ctx, u32, len, u64, flags)
> +{
[Severity: High]
Could casting ctx to a pointer array here also lead to a NULL pointer
dereference on 32-bit systems due to reading the upper 32 bits of the
previous 64-bit argument?

> +	unsigned int *remaining = ((unsigned int **)ctx)[5];
> +
> +	if (flags || len < 2)
> +		return -EINVAL;
[ ... ]
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
[ ... ]
> @@ -1244,6 +1281,15 @@ static unsigned int tcp_established_options(struct sock *sk, struct sk_buff *skb
>  		size = MAX_TCP_OPTION_SPACE - remaining;
>  	}
>  
[Severity: Medium]
Does this cgroup check evaluate to false for struct_ops in non-cgroup-bpf
deployments? If so, does it silently skip reserving space in
bpf_tcp_ops_hdr_opt_len() for established connections, while still calling
write_hdr_opt unconditionally later?

> +	if (cgroup_bpf_enabled(CGROUP_TCP_SOCK_OPS)) {
> +		unsigned int remaining = MAX_TCP_OPTION_SPACE - size;
> +
> +		remaining = bpf_tcp_ops_hdr_opt_len(sk, skb, NULL, NULL, 0, opts,
> +						    remaining);
> +
> +		size = MAX_TCP_OPTION_SPACE - remaining;
> +	}
> +
>  	return size;
>  }

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