[PATCH net 1/2] tcp: fix shift-out-of-bounds in SYN-ACK retransmit timers

[PATCH net 1/2] tcp: fix shift-out-of-bounds in SYN-ACK retransmit timers

[Jiayuan Chen]

tcp_synack_retries is a u8 sysctl with no upper bound (0..255), and
TCP_DEFER_ACCEPT can derive a comparable count. Both feed the
max_retries / max_syn_ack_retries that decide how long a request_sock
keeps retransmitting SYN-ACKs. req->num_timeout is bumped on every
SYN-ACK timeout and then used as a shift amount:

	/* tcp_reqsk_timeout() */
	u64 timeout = (u64)req->timeout << req->num_timeout;
	/* tcp_fastopen_synack_timer() */
	req->timeout << req->num_timeout

Set tcp_synack_retries high enough and num_timeout climbs past the width
of the shifted type, which is UB:

  UBSAN: shift-out-of-bounds in ./include/net/tcp.h:917:34
  shift exponent 64 is too large for 64-bit type 'long long unsigned int'
  Call Trace:
   <TASK>
   dump_stack_lvl+0xc1/0xf0
   dump_stack+0x10/0x20
   __ubsan_handle_shift_out_of_bounds+0x1d1/0x390
   reqsk_timer_handler.cold+0x1c/0x5d
   call_timer_fn+0x17d/0x510
   __run_timers+0x662/0xab0
   run_timer_softirq+0xdf/0x260
   handle_softirqs+0x204/0x9b0
   __irq_exit_rcu+0x13c/0x220
   irq_exit_rcu+0xe/0x20
   sysvec_apic_timer_interrupt+0x40/0xd0
   asm_sysvec_apic_timer_interrupt+0x1b/0x20

num_timeout is also a 7-bit field (max 127). Once max_retries goes above
127 the "num_timeout >= max_retries" expiry check can never be true, so
the reqsk keeps retransmitting SYN-ACKs and never gives up.

Leave the one-byte sysctl range alone and fix both in the consumers:

 - bound the shift amount in tcp_reqsk_timeout_sk(). The result is
   clamped to tcp_rto_max() anyway and a num_timeout well under 30
   already saturates that, so capping the shift at 30 changes no
   reachable behaviour and drops the UB.
 - cap max_retries / max_syn_ack_retries at MAX_TCP_SYNACK_RETRIES (127,
   the num_timeout field width) so the reqsk can actually expire.
 - route the TFO path through the same bounded helper.

Fixes: e6c022a4fa2d ("tcp: better retrans tracking for defer-accept")
Cc: stable@vger.kernel.org
Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
---
 Documentation/networking/ip-sysctl.rst |  4 +++-
 include/net/tcp.h                      | 16 ++++++++++++----
 net/ipv4/inet_connection_sock.c        |  2 ++
 net/ipv4/tcp.c                         |  2 +-
 net/ipv4/tcp_timer.c                   |  3 ++-
 5 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/Documentation/networking/ip-sysctl.rst b/Documentation/networking/ip-sysctl.rst
index 4b19c8d3efda..eac33072be2d 100644
--- a/Documentation/networking/ip-sysctl.rst
+++ b/Documentation/networking/ip-sysctl.rst
@@ -956,7 +956,9 @@ tcp_stdurg - BOOLEAN
 
 tcp_synack_retries - INTEGER
 	Number of times SYNACKs for a passive TCP connection attempt will
-	be retransmitted. Should not be higher than 255. Default value
+	be retransmitted. The sysctl accepts 0-255, but values above 127 are
+	silently capped by the SYN-ACK timers to match the 7-bit
+	request_sock::num_timeout field. Default value
 	is 5, which corresponds to 31seconds till the last retransmission
 	with the current initial RTO of 1second. With this the final timeout
 	for a passive TCP connection will happen after 63seconds.
diff --git a/include/net/tcp.h b/include/net/tcp.h
index 0265c27a5fd7..986b7ea05e4a 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -183,6 +183,8 @@ static_assert((1 << ATO_BITS) > TCP_DELACK_MAX);
 #define MAX_TCP_KEEPINTVL	32767
 #define MAX_TCP_KEEPCNT		127
 #define MAX_TCP_SYNCNT		127
+/* request_sock::num_timeout is a 7-bit field. */
+#define MAX_TCP_SYNACK_RETRIES	127
 
 /* Ensure that TCP PAWS checks are relaxed after ~2147 seconds
  * to avoid overflows. This assumes a clock smaller than 1 Mhz.
@@ -912,12 +914,18 @@ static inline u32 __tcp_set_rto(const struct tcp_sock *tp)
 	return usecs_to_jiffies((tp->srtt_us >> 3) + tp->rttvar_us);
 }
 
-static inline unsigned long tcp_reqsk_timeout(struct request_sock *req)
+static inline unsigned long tcp_reqsk_timeout_sk(const struct sock *sk,
+						 const struct request_sock *req)
 {
-	u64 timeout = (u64)req->timeout << req->num_timeout;
+	u64 timeout = (u64)READ_ONCE(req->timeout) <<
+		      min_t(u8, req->num_timeout, 30);
+
+	return (unsigned long)min_t(u64, timeout, tcp_rto_max(sk));
+}
 
-	return (unsigned long)min_t(u64, timeout,
-				    tcp_rto_max(req->rsk_listener));
+static inline unsigned long tcp_reqsk_timeout(struct request_sock *req)
+{
+	return tcp_reqsk_timeout_sk(req->rsk_listener, req);
 }
 
 u32 tcp_delack_max(const struct sock *sk);
diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
index 8151c638a24c..ee7b64f3141c 100644
--- a/net/ipv4/inet_connection_sock.c
+++ b/net/ipv4/inet_connection_sock.c
@@ -1072,6 +1072,8 @@ static void reqsk_timer_handler(struct timer_list *t)
 	net = sock_net(sk_listener);
 	max_syn_ack_retries = READ_ONCE(icsk->icsk_syn_retries) ? :
 		READ_ONCE(net->ipv4.sysctl_tcp_synack_retries);
+	max_syn_ack_retries = min_t(int, max_syn_ack_retries,
+				    MAX_TCP_SYNACK_RETRIES);
 	/* Normally all the openreqs are young and become mature
 	 * (i.e. converted to established socket) for first timeout.
 	 * If synack was not acknowledged for 1 second, it means
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 99d7237a623d..a309fd45ad42 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -358,7 +358,7 @@ static u8 secs_to_retrans(int seconds, int timeout, int rto_max)
 		int period = timeout;
 
 		res = 1;
-		while (seconds > period && res < 255) {
+		while (seconds > period && res < MAX_TCP_SYNACK_RETRIES) {
 			res++;
 			timeout <<= 1;
 			if (timeout > rto_max)
diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
index e56eae4bc341..f7b100334a89 100644
--- a/net/ipv4/tcp_timer.c
+++ b/net/ipv4/tcp_timer.c
@@ -469,6 +469,7 @@ static void tcp_fastopen_synack_timer(struct sock *sk, struct request_sock *req)
 	 */
 	max_retries = READ_ONCE(icsk->icsk_syn_retries) ? :
 		READ_ONCE(sock_net(sk)->ipv4.sysctl_tcp_synack_retries) + 1;
+	max_retries = min_t(int, max_retries, MAX_TCP_SYNACK_RETRIES);
 
 	if (req->num_timeout >= max_retries) {
 		tcp_write_err(sk);
@@ -490,7 +491,7 @@ static void tcp_fastopen_synack_timer(struct sock *sk, struct request_sock *req)
 	if (!tp->retrans_stamp)
 		tp->retrans_stamp = tcp_time_stamp_ts(tp);
 	tcp_reset_xmit_timer(sk, ICSK_TIME_RETRANS,
-			  req->timeout << req->num_timeout, false);
+			     tcp_reqsk_timeout_sk(sk, req), false);
 }
 
 static bool tcp_rtx_probe0_timed_out(const struct sock *sk,
-- 
2.43.0

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

[Jiayuan Chen]

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).
inet_csk_rto_backoff() then uses it as a shift amount:

	u64 when = (u64)icsk->icsk_rto << icsk->icsk_backoff;

Set tcp_retries2 to 64 or more and keep the peer silent, icsk_backoff
climbs past 63 and the shift is UB. For example an ICMP host unreachable
arriving on such a socket goes through tcp_ld_RTO_revert():

  UBSAN: shift-out-of-bounds in ./include/net/inet_connection_sock.h:258:40
  shift exponent 73 is too large for 64-bit type 'long long unsigned int'
  Call Trace:
   <IRQ>
   dump_stack_lvl+0xc1/0xf0
   dump_stack+0x10/0x20
   __ubsan_handle_shift_out_of_bounds+0x1d1/0x390
   tcp_ld_RTO_revert.cold+0x16/0x92
   tcp_v4_err+0xdb3/0x13e0
   icmp_socket_deliver+0x1a7/0x390
   icmp_unreach+0x2dd/0xe50
   icmp_rcv+0x93e/0xe30
   ip_protocol_deliver_rcu+0x36b/0x3e0
   ip_local_deliver_finish+0x37d/0x620
   ip_local_deliver+0x165/0x1d0
   ip_rcv+0x284/0x320
   __netif_receive_skb_one_core+0x16b/0x1c0
   __netif_receive_skb+0x21/0x150
   process_backlog+0x37e/0x1200
   __napi_poll.constprop.0+0xa5/0x5e0
   net_rx_action+0x4c0/0xf70
   handle_softirqs+0x204/0x9b0
   __irq_exit_rcu+0x13c/0x220
   irq_exit_rcu+0xe/0x20
   sysvec_apic_timer_interrupt+0x92/0xd0
   </IRQ>

The orphan check in tcp_probe_timer() calls the same helper.

tcp_probe0_when() got a similar clamp in 2020, but this helper was left
as is. Cap the shift at 32 like it was suggested back then: icsk_rto is
at least 1 jiffy, so 1 << 32 jiffies is already far above any max_when,
and the min_t() result is unchanged. Unlike an ilog2(RTO_MAX/RTO_MIN)
bound, 32 still holds when tcp_rto_min_us is lowered.

Fixes: fcdd1cf4dd63 ("tcp: avoid possible arithmetic overflows")
Cc: stable@vger.kernel.org
Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
---
 include/net/inet_connection_sock.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/net/inet_connection_sock.h b/include/net/inet_connection_sock.h
index 903499581db7..562e12033a5d 100644
--- a/include/net/inet_connection_sock.h
+++ b/include/net/inet_connection_sock.h
@@ -255,9 +255,10 @@ static inline unsigned long
 inet_csk_rto_backoff(const struct inet_connection_sock *icsk,
 		     unsigned long max_when)
 {
-        u64 when = (u64)icsk->icsk_rto << icsk->icsk_backoff;
+	u8 backoff = min_t(u8, 32U, icsk->icsk_backoff);
+	u64 when = (u64)icsk->icsk_rto << backoff;
 
-        return (unsigned long)min_t(u64, when, max_when);
+	return (unsigned long)min_t(u64, when, max_when);
 }
 
 struct sock *inet_csk_accept(struct sock *sk, struct proto_accept_arg *arg);
-- 
2.43.0

Re: [PATCH net 1/2] tcp: fix shift-out-of-bounds in SYN-ACK retransmit timers

[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 commit message gets the TCP_DEFER_ACCEPT path wrong and
  does not mention a user-visible change.

--

tcp: fix shift-out-of-bounds in SYN-ACK retransmit timers

The patch caps the shift amount in the request_sock SYN-ACK timeout helper
at 30. It also limits max_retries / max_syn_ack_retries to 127, the width
of the num_timeout field, and sends the TFO SYN-ACK timer through the same
bounded helper. A further hunk lowers the TCP_DEFER_ACCEPT conversion cap
in secs_to_retrans() from 255 to 127.

[ ... ]

> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 99d7237a623d8..a309fd45ad425 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -358,7 +358,7 @@ static u8 secs_to_retrans(int seconds, int timeout, int rto_max)
>  		int period = timeout;
>  
>  		res = 1;
> -		while (seconds > period && res < 255) {
> +		while (seconds > period && res < MAX_TCP_SYNACK_RETRIES) {

[Severity: Medium]
This hunk lowers the TCP_DEFER_ACCEPT cap from 255 to 127. It seems to be
the actual fix for the defer-accept trigger, but none of the three bullets
in the commit message describe it. Could the change list mention it?

The commit message also says:

  TCP_DEFER_ACCEPT can derive a comparable count. Both feed the
  max_retries / max_syn_ack_retries that decide how long a request_sock
  keeps retransmitting SYN-ACKs.

Is that accurate? do_tcp_setsockopt() stores the converted value in
icsk_accept_queue.rskq_defer_accept, and syn_ack_recalc() checks it as a
separate condition:

net/ipv4/inet_connection_sock.c:syn_ack_recalc() {
    ...
	*expire = req->num_timeout >= max_syn_ack_retries &&
		  (!inet_rsk(req)->acked || req->num_timeout >= rskq_defer_accept);
    ...
}

tcp_check_req() also compares req->num_timeout against rskq_defer_accept
directly. The value never passes through max_retries or
max_syn_ack_retries, so the new min_t() clamps in reqsk_timer_handler() and
tcp_fastopen_synack_timer() don't apply to it.

Before this patch, take rskq_defer_accept above 127 and a peer that sends a
bare ACK (acked=1) but no data. The 7-bit num_timeout can never reach
rskq_defer_accept, so expire stays false. reqsk_timer_handler() then keeps
re-arming through tcp_reqsk_timeout(), and the shift goes out of bounds once
num_timeout reaches 64.

That path only needs setsockopt(TCP_DEFER_ACCEPT) on the caller's own
listener, with the default tcp_synack_retries=5. The commit message gives
the trigger only as "Set tcp_synack_retries high enough", which needs root.
Would it help stable backporters to say that this is reachable without
privileges?

There is also a user-visible change here. setsockopt(TCP_DEFER_ACCEPT) now
stores at most 127 retransmits, and larger requested values are truncated
without notice. As a result, the largest value getsockopt(TCP_DEFER_ACCEPT)
can return drops from retrans_to_secs(255, 1, 120) = 29887 seconds to
retrans_to_secs(127, 1, 120) = 14527 seconds.

The ip-sysctl.rst update only covers tcp_synack_retries. Should the commit
message or the documentation mention the new TCP_DEFER_ACCEPT limit?

>  			res++;
>  			timeout <<= 1;
>  			if (timeout > rto_max)

[ ... ]

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

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