Re: [PATCH v7 net-next 2/2] bpf: make tcp_tso_autosize() available to BPF congestion controls

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Exposing `tcp_tso_autosize` as a BPF kfunc allows a BPF program to trigger a kernel panic via division by zero by passing `mss_now = 0`.
- [Low] The selftest `tcp_ca_kfunc.c` fails to compile due to function redefinition and an invalid return type.

Pre-existing issues:
- [High] The already exposed kfunc `tcp_cong_avoid_ai` is vulnerable to a divide-by-zero kernel panic if a BPF program passes `w = 0`.
--

--- Patch [2]: [PATCH v7 net-next 2/2] bpf: make tcp_tso_autosize() available to BPF congestion controls ---
commit 8757143027760bc7421911e55324877a61cb35ac
Author: Chia-Yu Chang <[email protected]>

bpf: make tcp_tso_autosize() available to BPF congestion controls

Expose tcp_tso_autosize() as a BPF kfunc and register it in the TCP
congestion-control kfunc set. This allows BPF congestion controls to
reuse the kernel TSO autosizing logic while applying their own
minimum TSO segment policy. Add a selftest exercising tcp_tso_autosize()
from a BPF congestion control through the tso_segs() callback.

> diff --git a/net/ipv4/bpf_tcp_ca.c b/net/ipv4/bpf_tcp_ca.c
> index ed4fea98dfde1..9deed2244c2de 100644
> --- a/net/ipv4/bpf_tcp_ca.c
> +++ b/net/ipv4/bpf_tcp_ca.c
> @@ -194,6 +194,7 @@ BTF_ID_FLAGS(func, tcp_reno_cong_avoid)
>  BTF_ID_FLAGS(func, tcp_reno_undo_cwnd)
>  BTF_ID_FLAGS(func, tcp_slow_start)
>  BTF_ID_FLAGS(func, tcp_cong_avoid_ai)

[Severity: High]
This is a pre-existing issue, but since tcp_cong_avoid_ai() is already
exposed as a kfunc, is it vulnerable to a divide-by-zero kernel panic?

If a BPF program passes w = 0 to tcp_cong_avoid_ai() in net/ipv4/tcp_cong.c,
the condition tp->snd_cwnd_cnt >= w will evaluate to true, leading to:

    u32 delta = tp->snd_cwnd_cnt / w;

Since the BPF verifier does not enforce value range constraints on plain
scalar arguments, could a privileged user load a BPF program that crashes the
kernel this way?

> +BTF_ID_FLAGS(func, tcp_tso_autosize)
>  BTF_KFUNCS_END(bpf_tcp_ca_check_kfunc_ids)

[ ... ]

> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index 7d3e0e715c4bf..7914287dd5710 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
[ ... ]
> @@ -2253,8 +2253,8 @@ static bool tcp_nagle_check(bool partial, const struct tcp_sock *tp,
>   * for every 2^9 usec (aka 512 us) of RTT, so that the RTT-based allowance
>   * is below 1500 bytes after 6 * ~500 usec = 3ms.
>   */
> -u32 tcp_tso_autosize(const struct sock *sk, unsigned int mss_now,
> -		     int min_tso_segs)
> +__bpf_kfunc u32 tcp_tso_autosize(const struct sock *sk, unsigned int mss_now,
> +				 int min_tso_segs)

[Severity: High]
Similarly, does exposing tcp_tso_autosize() as a kfunc introduce a new
divide-by-zero risk?

Inside tcp_tso_autosize(), there is an unchecked division by mss_now:

    return max_t(u32, bytes / mss_now, min_tso_segs);

Could a BPF congestion control program pass 0 for mss_now, bypassing the
verifier's checks since it is an unconstrained scalar, and trigger a
divide-by-zero kernel panic?

>  {
>  	unsigned long bytes;
>  	u32 r;

[ ... ]

> diff --git a/tools/testing/selftests/bpf/progs/tcp_ca_kfunc.c b/tools/testing/selftests/bpf/progs/tcp_ca_kfunc.c
> index 58262e4903364..c0f8f20f3052c 100644
> --- a/tools/testing/selftests/bpf/progs/tcp_ca_kfunc.c
> +++ b/tools/testing/selftests/bpf/progs/tcp_ca_kfunc.c
[ ... ]
> @@ -109,6 +111,12 @@ void BPF_PROG(pkts_acked, struct sock *sk, const struct ack_sample *sample)
>  	cubictcp_acked(sk, sample);
>  }
>  
> +SEC("struct_ops")
> +void BPF_PROG(tso_segs, struct sock *sk, u32 mss_now)
> +{
> +	return tcp_tso_autosize(sk, mss_now, 1);
> +}

[Severity: Low]
Does this selftest compile correctly?

It looks like tso_segs is defined with a void return type, but then returns a
value from tcp_tso_autosize(), which violates C constraints. Additionally,
could this cause a function redefinition error if u32 BPF_PROG(tso_segs...)
is already defined earlier in this file?

> +
>  SEC(".struct_ops")
>  struct tcp_congestion_ops tcp_ca_kfunc = {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2