Re: [PATCH net v2] sctp: validate stream count in sctp_process_strreset_inreq()

[Xin Long]

On Fri, Jul 10, 2026 at 4:26 AM David Laight
<david.laight.linux@xxxxxxxxx> wrote:
>
> On Thu,  9 Jul 2026 21:07:18 -0400
> "Cen Zhang (Microsoft)" <blbllhy@xxxxxxxxx> wrote:
>
> > When processing a RESET_IN_REQUEST from a peer,
> > sctp_process_strreset_inreq() derives the stream count from the
> > parameter length but does not check whether the resulting
> > RESET_OUT_REQUEST would exceed SCTP_MAX_CHUNK_LEN.
> >
> > The OUT request header (sctp_strreset_outreq, 16 bytes) is 8 bytes
> > larger than the IN request header (sctp_strreset_inreq, 8 bytes).
> > Generally, the IP payload is bounded to 65535 bytes, so the stream
> > list cannot be large enough to trigger the overflow. However, on
> > interfaces with MTU > 65535 (e.g., loopback with IPv6 jumbograms), a
> > stream list that fits within the incoming IN parameter can cause a
> > __u16 overflow in sctp_make_strreset_req() when computing the OUT
> > request size, leading to an undersized skb allocation and a kernel
> > BUG:
> >
> >   net/core/skbuff.c:207         skb_panic
> >   net/core/skbuff.c:2625        skb_put
> >   net/sctp/sm_make_chunk.c:1535 sctp_addto_chunk
> >   net/sctp/sm_make_chunk.c:3695 sctp_make_strreset_req
> >   net/sctp/stream.c:655         sctp_process_strreset_inreq
> >
> > The local setsockopt path validates the generated reset request size.
> > However, for an incoming-only reset, it accounts for the smaller IN
> > request even though the peer must generate an OUT request with the same
> > stream list. Such a request cannot be completed successfully by the
> > peer.
> >
> > Reject peer IN requests whose corresponding OUT request would exceed
> > SCTP_MAX_CHUNK_LEN. Also tighten the local check so it does not send an
> > IN request that would require an oversized OUT request from the peer.
> >
> > Fixes: 7f9d68ac944e ("sctp: implement sender-side procedures for SSN Reset Request Parameter")
> > Reported-by: AutonomousCodeSecurity@xxxxxxxxxxxxx
> > Closes: https://lore.kernel.org/all/20260707203215.2752-1-blbllhy@xxxxxxxxx/
> > Suggested-by: Xin Long <lucien.xin@xxxxxxxxx>
> > Signed-off-by: Cen Zhang (Microsoft) <blbllhy@xxxxxxxxx>
> > ---
> > v2: Add the OUT request length check to the send path, as suggested by Xin Long.
> >
> >  net/sctp/stream.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/sctp/stream.c b/net/sctp/stream.c
> > index 5c2fdedea088..34ffe6c945a4 100644
> > --- a/net/sctp/stream.c
> > +++ b/net/sctp/stream.c
> > @@ -308,7 +308,8 @@ int sctp_send_reset_streams(struct sctp_association *asoc,
> >                                       goto out;
> >
> >                       param_len += str_nums * sizeof(__u16) +
> > -                                  sizeof(struct sctp_strreset_inreq);
> > +                                  (out ? sizeof(struct sctp_strreset_inreq)
> > +                                       : sizeof(struct sctp_strreset_outreq));
>
> Does it really make any sense to have a connection with the 32k streams
> that would be needed in order to send a maximal length request?
> (Or more likely a user requesting the same streams be reset multiple times.)
> So an initial check that str_nums < SOME_CONSTANT_JUST_BELOW_32K would do.
>
Yes, that would be a simpler fix.

However, 32K is not a limit defined by the RFC. While 32K streams may seem
excessive in practice, we cannot say that such a configuration is invalid.
If an application legitimately needs more than that, it would be difficult
to argue that it is not using SCTP correctly.

> Looking at the code I'm sure the kmalloc() shouldn't be done in the
> 'str_nums == 0' case either.
> In fact it is probably worth doing the kmalloc() earlier to avoid two
> scans of the array.
> I even wonder if it should be possible to allocate the chunk without filling
> in the data and then put the values in afterwards (freeing the chunk if there
> is an error).
>
> Then there is the code that reverts the state to OPEN if sctp_send_reconf()
> fails - nothing check that is the original state.
>
That would be another issue that we can address separately.
It would be great if you could follow up on this one. :-)

>         David
>
>
> >               }
> >
> >               if (param_len > SCTP_MAX_CHUNK_LEN -
> > @@ -639,6 +640,9 @@ struct sctp_chunk *sctp_process_strreset_inreq(
> >
> >       nums = (ntohs(param.p->length) - sizeof(*inreq)) / sizeof(__u16);
> >       str_p = inreq->list_of_streams;
> > +     if (nums * sizeof(__u16) + sizeof(struct sctp_strreset_outreq) >
> > +         SCTP_MAX_CHUNK_LEN - sizeof(struct sctp_reconf_chunk))
> > +             goto out;
> >       for (i = 0; i < nums; i++) {
> >               if (ntohs(str_p[i]) >= stream->outcnt) {
> >                       result = SCTP_STRRESET_ERR_WRONG_SSN;
>

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

Thanks.