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_f1sfYOxrgcoK-PhWy_vyG-jqRaO8F3CiB9xUTqu1D8pg@mail.gmail.com>
Date: Mon, 10 Nov 2025 11:28:24 -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 Mon, Nov 10, 2025 at 11:13 AM Eric Dumazet <edumazet@...gle.com> wrote:
>
> On Mon, Nov 10, 2025 at 8:10 AM Xin Long <lucien.xin@...il.com> wrote:
> >
> > On Mon, Nov 10, 2025 at 10:42 AM Eric Dumazet <edumazet@...gle.com> wrote:
> > >
> > > 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,
> > >
> > But you do see (u32) >> 31 can be 1, that makes the tp->rttvar/srtt
> > change slightly.
> > So the 'expecting no change' is from practical experience, right?
>
> EWMA31 on rtt is a NOP, unless rtt is more than 2^31
>
> A - (A >> 31) + (RTT >> 31)   -> A
>
> Is SCTP dealing with this level of RTT ?
>
I would not think so, the connection must have broken before that :-)

Acked-by: Xin Long <lucien.xin@...il.com>

THanks.