* [PATCH net v2 2/2] tcp: fix shift-out-of-bounds in inet_csk_rto_backoff()
2026-10-26 12:32 [PATCH net v2 1/2] tcp: fix shift-out-of-bounds in SYN-ACK retransmit timers Jiayuan Chen
@ 2026-10-26 12:32 ` Jiayuan Chen
2026-10-26 13:36 ` Neal Cardwell
2026-10-26 13:27 ` [PATCH net v2 1/2] tcp: fix shift-out-of-bounds in SYN-ACK retransmit timers Neal Cardwell
1 sibling, 1 reply; 4+ messages in thread
From: Jiayuan Chen @ 2026-10-26 12:32 UTC (permalink / raw)
To: netdev
Cc: Jiayuan Chen, stable, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Simon Horman, Jonathan Corbet,
Shuah Khan, Randy Dunlap, Neal Cardwell, Kuniyuki Iwashima,
linux-doc, linux-kernel
icsk_backoff is incremented by tcp_retransmit_timer() on every backed-off
RTO with no cap of its own, and by tcp_send_probe0() up to tcp_retries2.
It is used as a shift amount in inet_csk_rto_backoff():
u64 when = (u64)icsk->icsk_rto << icsk->icsk_backoff;
Once it climbs past 63 the shift is UB. Seen with tcp_retries2=255 and a
silent peer, when an ICMP host unreachable 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. On x86-64
the oversized shift count is taken modulo 64, so an orphan that should
be dead can be seen as alive, and tcp_ld_RTO_revert() can end up with a
smaller RTO than intended.
Cap the shift at 32: any non-zero icsk_rto << 32 exceeds TCP_RTO_MAX, so
the min_t() result is what the unbounded math would give. The
ilog2(TCP_RTO_MAX / TCP_RTO_MIN) + 1 bound of tcp_probe0_when() does not
fit here, as icsk_rto has no TCP_RTO_MIN floor the way tcp_probe0_base()
has.
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH net v2 1/2] tcp: fix shift-out-of-bounds in SYN-ACK retransmit timers
2026-10-26 12:32 [PATCH net v2 1/2] tcp: fix shift-out-of-bounds in SYN-ACK retransmit timers Jiayuan Chen
2026-10-26 12:32 ` [PATCH net v2 2/2] tcp: fix shift-out-of-bounds in inet_csk_rto_backoff() Jiayuan Chen
@ 2026-10-26 13:27 ` Neal Cardwell
1 sibling, 0 replies; 4+ messages in thread
From: Neal Cardwell @ 2026-10-26 13:27 UTC (permalink / raw)
To: Jiayuan Chen
Cc: netdev, stable, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Simon Horman, Jonathan Corbet, Shuah Khan,
Randy Dunlap, Kuniyuki Iwashima, linux-doc, linux-kernel
On Mon, Oct 26, 2026 at 8:32 AM Jiayuan Chen <jiayuan.chen@linux.dev> wrote:
>
> req->num_timeout counts the SYN-ACK timeouts of a request and is 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
>
> Once it climbs past the width of the shifted type the shift is UB. Seen
> with tcp_synack_retries=255 and a peer that never completes the
> handshake:
>
> 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 a 7-bit field. Once a retransmit limit is above 127 the
> "num_timeout >= limit" check can never be true, the request is never
> expired by that limit, and after the field wraps to 0
> reqsk_timer_handler() decrements queue->young again on the next expiry.
>
> - split tcp_reqsk_timeout() into tcp_reqsk_timeout_sk() and bound the
> shift there; a num_timeout well under 30 already saturates the
> tcp_rto_max() clamp, so nothing reachable changes for the u64 path.
> - cap max_retries / max_syn_ack_retries at 127, the largest value the
> field can hold, instead of narrowing the sysctl; 128..255 are still
> accepted and act as 127.
> - cap the TCP_DEFER_ACCEPT conversion in secs_to_retrans() at 127 too;
> larger values kept an acked request from ever expiring. getsockopt()
> now reports at most 14527 instead of 29887 seconds.
> - route the TFO path through the same helper; its u32 shift could also
> wrap to 0 before reaching UB.
> - document that tcp_synack_retries values above 127 act as 127.
>
> Fixes: e6c022a4fa2d ("tcp: better retrans tracking for defer-accept")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
> ---
> v1 -> v2: reworded the commit message, no code change.
> v1: https://lore.kernel.org/netdev/20261023064915.401112-1-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
The following review comments are from an LLM, look legitimate, and
IMHO are worth addressing.
Please either incorporate each piece of feedback or respond to
indicate why you think it's not appropriate.
---
Thanks for the fix. The logic looks right to me, and I could reproduce
both splats and confirm the patch fixes them. A few comments:
1) The changelog suggests tcp_synack_retries (or TCP_DEFER_ACCEPT) must be
raised. But a listener can also set TCP_SYNCNT up to MAX_TCP_SYNCNT
(127), and that also bounds its SYN-ACK retries. So num_timeout reaches
the UB range with default sysctls. With TCP_SYNCNT=127 plus
TCP_RTO_MAX_MS=1000 on the listener, the tcp.h:917 splat (shift
exponent 64) shows up after about 64 seconds. For TFO it fires already
at num_timeout 32, because "req->timeout << req->num_timeout" is a
32-bit shift (req->timeout is u32). Please mention TCP_SYNCNT. It also
shows the shift clamp is needed independently of the 0..255 sysctl
range.
2) Before the shift amount goes out of range, the shifted value already
wraps. With HZ=1000 and req->timeout = 1000:
(u64)1000 << 61..63 == 0, and (u32)1000 << 29..31 == 0.
Both give zero-length timers. On an unpatched kernel I see bursts of
three back-to-back SYN-ACK retransmits about 1 ms apart:
- regular path: at num_timeout 61-63, and again at 125-127 once x86
masks the shift count;
- TFO path: at num_timeout 29-31, 61-63, 93-95 and 125-127.
That is user-visible on production (non-UBSAN) kernels and is also
fixed by your clamp. It may be worth a sentence in the changelog.
3) READ_ONCE(req->timeout): as far as I can see, req->timeout is only
written in reqsk_queue_hash_req() and tcp_fastopen_create_child(),
both before the request is visible to other CPUs. If this is meant as
a data-race annotation, please explain it in the changelog (and pair
it with WRITE_ONCE()). Otherwise I would drop it from this fix.
4) Nit: this patch caps the shift at 30 and patch 2 caps it at 32, for the
same reason. Maybe use one bound in both, plus a one-line comment on
why it cannot change the clamped result. 32 also works here, since
(u64)U32_MAX << 32 still fits in a u64.
5) Nit: MAX_TCP_SYNACK_RETRIES duplicates MAX_TCP_SYNCNT (127), which
already limits the listener's SYN-ACK retries via TCP_SYNCNT. Maybe
reuse it, or define one in terms of the other, with the 7-bit
num_timeout comment.
6) Have you considered limiting the sysctl itself, as 651e92716aaa
("sysctl net: Keep tcp_syn_retries inside the boundary") did for
tcp_syn_retries? That would make the limit visible to admins instead
of silently capping it. The consumer-side caps would still be needed
for TCP_DEFER_ACCEPT and for the TFO "+ 1" (128 can never be reached
by a 7-bit counter), so your approach is reasonable. It would be good
to say in the changelog why the silent cap was preferred.
7) Minor: once num_timeout wraps to 0, the
"if (req->num_timeout++ == 0) atomic_dec(&queue->young)" in
reqsk_timer_handler() and reqsk_queue_removed() decrement queue->young
again. That is one more symptom fixed by making the request expire at
127.
8) The documentation text exposes an internal field name
("request_sock::num_timeout"). Maybe simply: "Values larger than 127
are treated as 127."
9) Small user-visible change worth a note in the changelog: for large
values, getsockopt(TCP_DEFER_ACCEPT) now reports at most
retrans_to_secs(127) = 14527 seconds instead of 29887.
10) Per Documentation/process/maintainer-netdev.rst ("Bug fixes"), please
state how the issue was found, how it was validated (the reproducer
steps are short enough to include), and how the fix was tested.
---
Thanks,
neal
^ permalink raw reply [flat|nested] 4+ messages in thread