Re: [PATCH net 2/2] tcp: fix shift-out-of-bounds in inet_csk_rto_backoff()

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The code change is correct.

--

tcp: fix shift-out-of-bounds in inet_csk_rto_backoff()

This commit caps the shift amount in inet_csk_rto_backoff() at 32, so a
large icsk_backoff no longer causes an out-of-range shift of icsk_rto.
The clamp covers both callers, tcp_ld_RTO_revert() and the orphan check
in tcp_probe_timer().

> icsk_backoff is bumped on every RTO and zero window probe, and the only
> thing bounding it is tcp_retries2 (a u8 sysctl, 0..255).

[ ... ]

> Set tcp_retries2 to 64 or more and keep the peer silent, icsk_backoff
> climbs past 63 and the shift is UB.

[Severity: Medium]
The code change looks correct to me. Is this description of how the bug
is reached accurate, though?

tcp_retries2 caps icsk_backoff only in the zero window probe path:

net/ipv4/tcp_output.c:tcp_send_probe0() {
    ...
	if (err <= 0) {
		if (icsk->icsk_backoff < READ_ONCE(net->ipv4.sysctl_tcp_retries2))
			icsk->icsk_backoff++;
    ...
}

In the RTO path there is no limit on the count:

net/ipv4/tcp_timer.c:tcp_retransmit_timer() {
    ...
		icsk->icsk_backoff++;
		icsk->icsk_rto = min(icsk->icsk_rto << 1, tcp_rto_max(sk));
    ...
}

The connection is killed only by tcp_write_timeout(), which calls
retransmits_timed_out(sk, retry_until, icsk_user_timeout). When
TCP_USER_TIMEOUT is non-zero, the tcp_retries2 based model is skipped,
and only elapsed time is compared against the user timeout:

net/ipv4/tcp_timer.c:retransmits_timed_out() {
    ...
	if (likely(timeout == 0)) {
		...
		timeout = tcp_model_timeout(sk, boundary, rto_base);
	}
    ...
}

Even when the user timeout is 0, the model bounds elapsed time and not
the icsk_backoff count.

No privilege is needed for either socket option. tcp_sock_set_user_timeout()
accepts any value >= 0, and TCP_RTO_MAX_MS accepts values down to 1000 ms
in do_tcp_setsockopt().

So an unprivileged process can trigger this with default sysctls:

  setsockopt(TCP_RTO_MAX_MS, 1000)
  setsockopt(TCP_USER_TIMEOUT, a few minutes)
  peer goes silent
    tcp_retransmit_timer() fires about once a second
    icsk_backoff passes 63 in about a minute
  ICMP unreachable arrives
    tcp_v4_err()->tcp_ld_RTO_revert()->inet_csk_rto_backoff()

Without TCP_RTO_MAX_MS, a TCP_USER_TIMEOUT of a few hours is enough. An
orphaned socket reaches the same shift through tcp_probe_timer().

Is the impact also larger than a UBSAN warning? On x86-64 the hardware
masks the shift count. tcp_ld_RTO_revert() could then compute an
icsk_rto that is far too small, and the orphan alive check in
tcp_probe_timer() could give the wrong answer.

Could the commit message say that unprivileged socket options and a
network ICMP are enough to trigger this, and describe the RTO impact,
instead of saying it needs tcp_retries2 >= 64?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261023064915.401112-1-jiayuan.chen%40linux.dev