Re: [PATCH bpf-next 6/7] bpf: tcp: Add kfunc to adjust sk->sk_rcvlowat.

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] bpf: tcp: OOB access and type confusion in bpf_tcp_ops_set_rcvlowat()
--

commit c903e36ae4ed7b68c1aea9023743a57be672cd2a
Author: Kuniyuki Iwashima <kuniyu@google.com>

bpf: tcp: Add kfunc to adjust sk->sk_rcvlowat.

This commit adds a new BPF kfunc bpf_tcp_ops_set_rcvlowat() to dynamically
adjust the receive low watermark for TCP sockets. It clamps negative values
to INT_MAX and configures the wakeup flag appropriately depending on the
calling hook (enqueue vs dequeue).

> diff --git a/net/ipv4/bpf_tcp_ops.c b/net/ipv4/bpf_tcp_ops.c
> index b0e14b54917eb..3768b1440eb77 100644
> --- a/net/ipv4/bpf_tcp_ops.c
> +++ b/net/ipv4/bpf_tcp_ops.c
> @@ -359,8 +359,62 @@ static struct bpf_struct_ops bpf_tcp_ops = {
>  	.owner = THIS_MODULE,
>  };
>  
> +__bpf_kfunc_start_defs();
> +
> +__bpf_kfunc int bpf_tcp_ops_set_rcvlowat(struct sock *sk, int rcvlowat,
> +					 const struct bpf_prog_aux *aux)
> +{
> +	u32 moff = aux->attach_st_ops_member_off;
> +	bool wakeup = false;
> +
> +	if (moff == offsetof(struct bpf_tcp_ops, dequeue_rcvq))
> +		wakeup = true;
> +
> +	if (rcvlowat < 0)
> +		rcvlowat = INT_MAX;
> +
> +	return __tcp_set_rcvlowat(sk, rcvlowat, wakeup);

[Severity: High]
Could this lead to an out-of-bounds slab memory access if a BPF program
passes a non-TCP socket?

Because the kfunc accepts a generic struct sock *sk parameter, the BPF
verifier allows passing any socket type. For example, a trusted UDP
socket could be obtained via bpf_sk_lookup_udp() and passed directly here.

However, __tcp_set_rcvlowat() blindly casts the socket to struct tcp_sock *:

net/ipv4/tcp.c:__tcp_set_rcvlowat() {
    struct tcp_sock *tp = tcp_sk(sk);
    ...
    if (tp->window_clamp && tp->window_clamp < val)
        WRITE_ONCE(tp->window_clamp, val);
}

This writes to tp->window_clamp, which resides well beyond the bounds of a
UDP socket structure.

Additionally, does this permit concurrent modification of the socket state
since the looked-up socket isn't guaranteed to be locked?

> +}
> +
> +__bpf_kfunc_end_defs();

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260920195633.3033620-1-kuniyu@google.com?part=6