Re: [PATCH net] sctp: revalidate output stream after association connect wait

[Xin Long]

On Mon, Oct 5, 2026 at 9:07 PM <netdev-bot+sashiko@xxxxxxxxxx> wrote:
>
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
>
> Critical: 0 · High: 1 · Medium: 1 · Low: 0
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [High] The patch only partly revalidates state after
>   sctp_wait_for_connect() in sctp_sendmsg_to_asoc()…
> - [Medium] The new -EINVAL in sctp_sendmsg_to_asoc()
>   (net/sctp/socket.c:1851-1855) is returned only after…
>
> --
>
> sctp: revalidate output stream after association connect wait
>
> This patch adds a check in sctp_sendmsg_to_asoc() after
> sctp_wait_for_connect() returns, for the message interleaving case. It
> compares sinfo->sinfo_stream against asoc->stream.outcnt again. The
> handshake can shrink outcnt and free the ext of removed streams while the
> socket lock is dropped, and that used to cause a NULL dereference in
> sctp_outq_tail().
>
> > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > index 4652fd90d9a6c..394d31cb698e0 100644
> > --- a/net/sctp/socket.c
> > +++ b/net/sctp/socket.c
> > @@ -1848,6 +1848,11 @@ static int sctp_sendmsg_to_asoc(struct sctp_association *asoc,
> >                               err = -ESRCH;
> >                               goto err;
> >                       }
> > +                     if (unlikely(sinfo->sinfo_stream >=
> > +                                  asoc->stream.outcnt)) {
> > +                             err = -EINVAL;
> > +                             goto err;
> > +                     }
>
> [Severity: High]
> Is the bound check enough on its own here? Could the stream's ext still be
> NULL when this check passes?
>
> The code before the wait checks two things:
>
> net/sctp/socket.c:sctp_sendmsg_to_asoc() {
>     ...
>         if (sinfo->sinfo_stream >= asoc->stream.outcnt) {
>                 err = -EINVAL;
>                 goto err;
>         }
>
>         if (unlikely(!SCTP_SO(&asoc->stream, sinfo->sinfo_stream)->ext)) {
>                 err = sctp_stream_init_ext(&asoc->stream, sinfo->sinfo_stream);
>     ...
> }
>
> Only the first check is repeated after sctp_wait_for_connect().
>
> While the lock is dropped, INIT-ACK processing can shrink outcnt:
>
> sctp_process_init()
>   sctp_stream_init()
>     sctp_stream_outq_migrate(stream, NULL, outcnt)
>       sctp_stream_free_ext()   /* kfree(ext); ext = NULL for sid >= outcnt */
>     sctp_stream_alloc_out()    /* only lowers stream->outcnt */
>
> After that, outcnt can be raised again without restoring ext. One way is
> for the peer to send a RECONF ADD_IN_STREAMS request, either bundled after
> COOKIE-ACK or sent right after it:
>
> sctp_sf_do_reconf()
>   sctp_process_strreset_addstrm_in()
>     sctp_stream_alloc_out(stream, outcnt, GFP_ATOMIC)
>     stream->outcnt = outcnt;
>
> The other way is for another local thread to call
> setsockopt(SCTP_ADD_STREAMS) while the lock is released:
>
> sctp_send_add_streams()
>   sctp_stream_alloc_out(stream, outcnt, GFP_KERNEL)
>
> On that path, outcnt is also not rolled back if sctp_send_reconf() fails.
This is a separate issue to check.

>
> sctp_stream_alloc_out() only calls genradix_prealloc(), so the existing
> entry for the earlier sid keeps ext == NULL.
>
> When the sender resumes, the new check passes because outcnt has grown
> back. Then this runs:
>
> sctp_primitive_SEND()
>   sctp_cmd_send_msg()
>     sctp_outq_tail()
>       sctp_outq_tail_data()
>         oute = SCTP_SO(&q->asoc->stream, stream)->ext;
>         list_add_tail(&ch->stream_list, &oute->outq);
>
> Doesn't this reach the same NULL ext dereference in sctp_outq_tail() that
> the commit message describes?
>
> Could the full pre-wait validation be repeated after each wait that drops
> the lock, perhaps in a shared helper? That means the bound check plus the
> !ext -> sctp_stream_init_ext() step.
>
> The existing outcnt-only recheck after sctp_wait_for_sndbuf() seems to
> have the same gap.
>
Please add a helper like:

static int sctp_sendmsg_check_stream(struct sctp_association *asoc,
                                     struct sctp_sndrcvinfo *sinfo)
{
        if (unlikely(sinfo->sinfo_stream >= asoc->stream.outcnt))
                return -EINVAL;

        if (unlikely(!SCTP_SO(&asoc->stream, sinfo->sinfo_stream)->ext))
                return sctp_stream_init_ext(&asoc->stream, sinfo->sinfo_stream);

        return 0;
}

then add or replace the stream checks in these places:
[1] at the beginning of sctp_sendmsg_to_asoc().
[2] after sctp_wait_for_sndbuf().
[3] after sctp_wait_for_connect() in asoc->ep->intl_enable branch.

> [Severity: Medium]
> What happens to the association when this new -EINVAL is returned?
>
> sctp_wait_for_connect() only returns 0 once the asoc is ESTABLISHED. If
> the association was created in this sendmsg() call, the caller then does:
>
> net/sctp/socket.c:sctp_sendmsg() {
>     ...
>         err = sctp_sendmsg_to_asoc(asoc, msg, msg_len, transport, sinfo);
>         if (err < 0 && err != -ESRCH && new)
>                 sctp_association_free(asoc);
>     ...
> }
>
> This frees a live, established association outside the state machine.
>
> No ABORT or SHUTDOWN is sent, so the peer is left with a half-open
> association. SCTP_MIB_CURRESTAB was incremented in sctp_sf_do_5_1E_ca()
> and is never decremented.
>
> On a TCP-style socket, sctp_cmd_new_state() has already set sk_state to
> SCTP_SS_ESTABLISHED. It stays there with no association behind it, so
> later sendmsg() calls fail with -EADDRNOTAVAIL or -EPIPE.
>
> The commit message says:
>
>     Return
>     EINVAL if it falls outside the negotiated range.  This matches the pre-wait
>     check.
>
> The -EINVAL before the wait frees a CLOSED asoc that never associated.
> This one frees an established asoc. A peer can trigger it just by
> advertising fewer inbound streams than the chosen sinfo_stream.
>
> Failures after the wait from sctp_datamsg_from_user() or
> sctp_primitive_SEND() already reach the same free path. This patch adds
> another way into it, and the peer controls that one.
Please set err = -ESRCH; before goto err; for the check [3].

For sctp_datamsg_from_user() and sctp_primitive_SEND(), we should not
change it now, in case it may break any existing applications.

Thanks.

>
> Since the association is already established at this point, should this
> case be handled differently from the pre-wait check?
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002010449.3689454-1-4ncienth%40gmail.com