Re: [PATCH v5 bpf-next 09/10] 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 write and data race in bpf_tcp_ops_set_rcvlowat()
--

commit 7deb5622ddf6a03f72f031cf7e714bbd4cca6f9e
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 sk->sk_rcvlowat from bpf_tcp_ops.{enqueue,dequeue}_rcvq(). It handles
negative values and calls __tcp_set_rcvlowat() to update the socket.

> diff --git a/net/ipv4/bpf_tcp_ops.c b/net/ipv4/bpf_tcp_ops.c
> index 8182037c42690..2ba73dd6c52cc 100644
> --- a/net/ipv4/bpf_tcp_ops.c
> +++ b/net/ipv4/bpf_tcp_ops.c
[ ... ]
> @@ -361,12 +361,31 @@ __bpf_kfunc int bpf_tcp_ops_set_flags(struct tcp_sock *tp, u32 enable, u32 disab
>  	return 0;
>  }
>  
> +__bpf_kfunc int bpf_tcp_ops_set_rcvlowat(struct sock *sk, int rcvlowat,
> +					 const struct bpf_prog_aux *aux)
> +{

[Severity: High]
Could this allow a BPF program to pass a non-TCP or unlocked socket here?

Since the argument is a generic "struct sock *sk", a BPF program might use
bpf_map_lookup_elem() on a sockmap or bpf_sk_lookup_udp() to obtain a
trusted PTR_TO_SOCKET for a UDP socket or an unlocked TCP socket.

If an arbitrary socket is passed through to __tcp_set_rcvlowat(), it
unsafely casts the socket to a tcp_sock:

net/ipv4/tcp.c:__tcp_set_rcvlowat() {
    struct tcp_sock *tp = tcp_sk(sk);
    ...
    if (space > sk->sk_rcvbuf) {
        WRITE_ONCE(sk->sk_rcvbuf, space);

        if (tp->window_clamp && tp->window_clamp < val)
            WRITE_ONCE(tp->window_clamp, val);
    }
}

Since udp_sock is smaller than tcp_sock, writing to tp->window_clamp
could cause an out-of-bounds write. Additionally, modifying sk_rcvbuf and
window_clamp on an arbitrary looked-up socket could cause a data race
if the socket is not implicitly locked by the current context.

> +	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);
> +}
> +
>  __bpf_kfunc_end_defs();
>

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