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: <CANn89iLUiYteUoLV_AiZeW8rJe5ns5uu5gmQHbjmdQPD3sZy5A@mail.gmail.com>
Date: Mon, 10 Nov 2025 07:42:21 -0800
From: Eric Dumazet <edumazet@...gle.com>
To: Xin Long <lucien.xin@...il.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 Mon, Nov 10, 2025 at 7:36 AM Xin Long <lucien.xin@...il.com> wrote:
>
> 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.

Skipping or not the update is a matter of taste really.

If someone was setting 30 or more, it was expecting tp->rttvar and
tp->srtt to not change at all,


>
> > +                       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
> >