netdev - Re: [PATCH net] sctp: prevent possible shift-out-of-bounds in sctp_transport_update

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CADvbK_fZABufnbF9vsS_GZ6OgYfKb7nT3NDdT+iO-C7Rw9K6mw@mail.gmail.com>
Date: Mon, 10 Nov 2025 10:36:34 -0500
From: Xin Long <lucien.xin@...il.com>
To: Eric Dumazet <edumazet@...gle.com>
Cc: "David S . Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>, 
	Marcelo Ricardo Leitner <marcelo.leitner@...il.com>, netdev@...r.kernel.org, eric.dumazet@...il.com, 
	syzbot+f8c46c8b2b7f6e076e99@...kaller.appspotmail.com, 
	Daniel Borkmann <daniel@...earbox.net>
Subject: Re: [PATCH net] sctp: prevent possible shift-out-of-bounds in sctp_transport_update_rto

On Thu, Nov 6, 2025 at 6:10 AM Eric Dumazet <edumazet@...gle.com> wrote:
>
> syzbot reported a possible shift-out-of-bounds [1]
>
> Blamed commit added rto_alpha_max and rto_beta_max set to 1000.
>
> It is unclear if some sctp users are setting very large rto_alpha
> and/or rto_beta.
>
> In order to prevent user regression, perform the test at run time.
>
> Also add READ_ONCE() annotations as sysctl values can change under us.
>
> [1]
>
> UBSAN: shift-out-of-bounds in net/sctp/transport.c:509:41
> shift exponent 64 is too large for 32-bit type 'unsigned int'
> CPU: 0 UID: 0 PID: 16704 Comm: syz.2.2320 Not tainted syzkaller #0 PREEMPT(full)
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 10/02/2025
> Call Trace:
>  <TASK>
>   __dump_stack lib/dump_stack.c:94 [inline]
>   dump_stack_lvl+0x16c/0x1f0 lib/dump_stack.c:120
>   ubsan_epilogue lib/ubsan.c:233 [inline]
>   __ubsan_handle_shift_out_of_bounds+0x27f/0x420 lib/ubsan.c:494
>   sctp_transport_update_rto.cold+0x1c/0x34b net/sctp/transport.c:509
>   sctp_check_transmitted+0x11c4/0x1c30 net/sctp/outqueue.c:1502
>   sctp_outq_sack+0x4ef/0x1b20 net/sctp/outqueue.c:1338
>   sctp_cmd_process_sack net/sctp/sm_sideeffect.c:840 [inline]
>   sctp_cmd_interpreter net/sctp/sm_sideeffect.c:1372 [inline]
>
> Fixes: b58537a1f562 ("net: sctp: fix permissions for rto_alpha and rto_beta knobs")
> Reported-by: syzbot+f8c46c8b2b7f6e076e99@...kaller.appspotmail.com
> Closes: https://lore.kernel.org/netdev/690c81ae.050a0220.3d0d33.014e.GAE@google.com/T/#u
> Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> Cc: Daniel Borkmann <daniel@...earbox.net>
> ---
>  net/sctp/transport.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/net/sctp/transport.c b/net/sctp/transport.c
> index 0d48c61fe6adefc1a9c56ca1b8ab00072825d9e6..0c56d9673cc137e3f1a64311e79bd41db2cb1282 100644
> --- a/net/sctp/transport.c
> +++ b/net/sctp/transport.c
> @@ -486,6 +486,7 @@ void sctp_transport_update_rto(struct sctp_transport *tp, __u32 rtt)
>
>         if (tp->rttvar || tp->srtt) {
>                 struct net *net = tp->asoc->base.net;
> +               unsigned int rto_beta, rto_alpha;
>                 /* 6.3.1 C3) When a new RTT measurement R' is made, set
>                  * RTTVAR <- (1 - RTO.Beta) * RTTVAR + RTO.Beta * |SRTT - R'|
>                  * SRTT <- (1 - RTO.Alpha) * SRTT + RTO.Alpha * R'
> @@ -497,10 +498,14 @@ void sctp_transport_update_rto(struct sctp_transport *tp, __u32 rtt)
>                  * For example, assuming the default value of RTO.Alpha of
>                  * 1/8, rto_alpha would be expressed as 3.
>                  */
> -               tp->rttvar = tp->rttvar - (tp->rttvar >> net->sctp.rto_beta)
> -                       + (((__u32)abs((__s64)tp->srtt - (__s64)rtt)) >> net->sctp.rto_beta);
> -               tp->srtt = tp->srtt - (tp->srtt >> net->sctp.rto_alpha)
> -                       + (rtt >> net->sctp.rto_alpha);
> +               rto_beta = READ_ONCE(net->sctp.rto_beta);
> +               if (rto_beta < 32)
Wouldn't be better to do:

rto_beta = min(READ_ONCE(net->sctp.rto_beta), 31U); ?

so that when rto_alpha >= 32, the update will not be skipped entirely.

> +                       tp->rttvar = tp->rttvar - (tp->rttvar >> rto_beta)
> +                               + (((__u32)abs((__s64)tp->srtt - (__s64)rtt)) >> rto_beta);
> +               rto_alpha = READ_ONCE(net->sctp.rto_alpha);
> +               if (rto_alpha < 32)
> +                       tp->srtt = tp->srtt - (tp->srtt >> rto_alpha)
> +                               + (rtt >> rto_alpha);
>         } else {
>                 /* 6.3.1 C2) When the first RTT measurement R is made, set
>                  * SRTT <- R, RTTVAR <- R/2.
> --
> 2.51.2.1026.g39e6a42477-goog
>