Re: [PATCH v4 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: bpf_tcp_ops_set_rcvlowat() allows out-of-bounds writes and data races via arbitrary sockets
--

commit f1017b98cfed57e0845856c2710e8fad69bfcbe9
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 a socket's sk_rcvlowat to suppress unnecessary wakeups. The function
determines whether to trigger a wakeup based on the BPF struct_ops hook
context.

> 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)
> +{
> +	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]
Does this code allow out-of-bounds writes if a non-TCP socket is passed?

The BPF verifier allows passing any trusted socket to kfuncs expecting a
struct sock *, such as those obtained via bpf_sk_lookup_udp() or
bpf_sk_lookup_tcp(). This means the sk argument is not restricted to the
locked context socket passed to the enqueue_rcvq or dequeue_rcvq hooks.

If a UDP socket is passed in, __tcp_set_rcvlowat() unsafely casts it to
struct tcp_sock and modifies tp->window_clamp:

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

Since struct udp_sock is smaller and lacks the window_clamp field, will this
write out of bounds? Should the signature take a struct tcp_sock *tp instead,
similar to bpf_tcp_ops_set_flags()?

Additionally, if a valid TCP socket is passed via a lookup, its socket lock
is not held. __tcp_set_rcvlowat() modifies sk_rcvlowat, sk_rcvbuf, and
window_clamp using logic that relies on the socket lock being held. Is there
a risk of data races and state corruption if it is called on an unlocked
socket?

Finally, passing an MPTCP subflow could bypass the intended restriction
since MPTCP subflows typically need to use mptcp_set_rcvlowat().

> +}

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