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

[Xin Long]

On Thu, Oct 8, 2026 at 11:57 AM <netdev-bot+sashiko@xxxxxxxxxx> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
>
> Critical: 0 · High: 0 · Medium: 1 (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).
>
> - [Medium] sctp: UAPI breakage leaking -ESRCH to userspace on stream
>   validation failure
>
> Pre-existing issues:
> - [Medium] Pre-existing ownership problem on the sndbuf-wait error path
>   in net/sctp/socket.c, not introduced by this patch.
>
> --
>
> sctp: revalidate output stream after association connect wait
>
> This commit moves the output stream range check and the extension check
> into a new helper, sctp_sendmsg_check_stream(). It runs that helper again
> after both the sndbuf wait and the connect wait in sctp_sendmsg_to_asoc().
> If the check fails after the connect wait, the error becomes -ESRCH, so the
> caller does not free the auto-created, already established association.
>
> > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > index e35692dd8d303..321898f22d80d 100644
> > --- a/net/sctp/socket.c
> > +++ b/net/sctp/socket.c
> > @@ -1786,6 +1786,18 @@ static int sctp_sendmsg_check_sflags(struct sctp_association *asoc,
> >       return 1;
> >  }
> >
> > +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;
> > +}
> > +
>
> [ ... ]
>
> > @@ -1830,10 +1835,9 @@ static int sctp_sendmsg_to_asoc(struct sctp_association *asoc,
> >               err = sctp_wait_for_sndbuf(asoc, transport, &timeo, msg_len);
> >               if (err)
> >                       goto err;
> > -             if (unlikely(sinfo->sinfo_stream >= asoc->stream.outcnt)) {
> > -                     err = -EINVAL;
> > +             err = sctp_sendmsg_check_stream(asoc, sinfo);
> > +             if (err)
> >                       goto err;
> > -             }
> >       }
>
> [Severity: Medium]
> This is a pre-existing issue, not one introduced by this patch. Does the
> ownership reasoning from the connect wait also apply to this re-check after
> the sndbuf wait?
>
> The caller, sctp_sendmsg(), frees a new association on any error except
> -ESRCH. It does not check whether the association is still CLOSED:
>
>         err = sctp_sendmsg_to_asoc(asoc, msg, msg_len, transport, sinfo);
>         if (err < 0 && err != -ESRCH && new)
>                 sctp_association_free(asoc);
>
> sctp_assoc_add_peer() has already hashed the new association's transport at
> this point. On a UDP-style socket, another sender can find the association
> through sctp_endpoint_lookup_assoc() while this thread sleeps in
> sctp_wait_for_sndbuf() with the socket lock dropped.
>
> A possible sequence:
>
> thread A (creates the asoc, new == true)
> sctp_sendmsg_to_asoc()
>   sctp_wait_for_sndbuf()
>     release_sock()
>
I don't think sctp_wait_for_sndbuf() can be called for the first send
(new == true) because sctp_sendmsg_parse() already checks:

        if (msg_len > sk->sk_sndbuf)
                return -EMSGSIZE;

> thread B (sends to the same peer)
> sctp_sendmsg()
>   sctp_endpoint_lookup_assoc()   <- finds the CLOSED asoc
>   sctp_sendmsg_to_asoc()
>     sctp_primitive_ASSOCIATE()
>
> the peer's INIT-ACK shrinks asoc->stream.outcnt below A's sinfo_stream
>
> thread A
>     lock_sock()
>   sctp_sendmsg_check_stream()    <- returns -EINVAL
> sctp_sendmsg()
>   sctp_association_free()
>
> Would this tear down an association that the state machine now owns
> (COOKIE_ECHOED or ESTABLISHED)? No ABORT would be sent to the peer, and the
> counter and socket state would be left inconsistent. The commit message says
> it avoids this outcome for the connect wait by returning -ESRCH.
>
> The baseline already had the -EINVAL re-check here. The other
> sctp_wait_for_sndbuf() errors (-EAGAIN, -EPIPE, -ERESTARTSYS) also reach
> the same free. With this patch, sctp_stream_init_ext() failures (-ENOMEM or
> a scheduler init_sid error) reach it as well.
>
> Should sctp_sendmsg() check whether the association is still CLOSED before
> freeing it? Or should this path remap the error the same way the connect
> wait path does? Earlier commits c863850ce22e1b and 4f1da630d13de0 used a
> similar -ESRCH remapping.
>
> [ ... ]
>
> > @@ -1848,6 +1852,11 @@ static int sctp_sendmsg_to_asoc(struct sctp_association *asoc,
> >                               err = -ESRCH;
> >                               goto err;
> >                       }
> > +                     err = sctp_sendmsg_check_stream(asoc, sinfo);
> > +                     if (err) {
> > +                             err = -ESRCH;
> > +                             goto err;
> > +                     }
> >               } else {
> >                       wait_connect = true;
> >               }
>
> [Severity: Medium]
> Does this leak the internal -ESRCH sentinel to userspace?
>
> sctp_sendmsg() returns its result through sctp_error(), and
> sctp_error() only special-cases -EPIPE. Whatever
> sctp_sendmsg_to_asoc() returns here therefore reaches the sendmsg()
> caller unchanged.
>
> With this hunk, sending on an sinfo_stream that the peer did not grant
> can return ESRCH. That happens only on the first send of an implicitly
> created association with intl_enable set. The same request on an
> existing association, or after the sndbuf wait, returns EINVAL. An
> -ENOMEM from sctp_stream_init_ext(), or an error from the scheduler's
> init_sid hook, is also reported as ESRCH.
>
> ESRCH ("No such process") hides the real cause from the application.
> It also misleads, because the association is alive and ESTABLISHED at
> this point, and a later send on a valid stream would succeed. Since
> this is Cc stable, the errno change would also appear in stable
> kernels.
>
> The connect wait failure path just above already returns -ESRCH to
> userspace, but that case is pre-existing. This patch extends the
> behavior to an argument validation failure.
>
> The remapping only exists to keep sctp_sendmsg() from calling
> sctp_association_free(). Could the ownership decision be made
> separately from the errno? For example, sctp_sendmsg() could free the
> new association only while it is still CLOSED. Alternatively,
> sctp_sendmsg_to_asoc() could report ownership through a separate
> out-parameter. Either way the original -EINVAL or -ENOMEM would reach
> userspace. Or is there a reason the errno has to carry this?
>
This is not a typical user scenario, and this patch is sufficient to fix
the NULL pointer dereference.

I may address the original error returned to userspace in a separate patch
by passing &new to sctp_sendmsg_to_asoc() and setting it to false once the
association is established. This would also cover the failures reported in
v1, where sctp_datamsg_from_user() or sctp_primitive_SEND() may fail after
the wait and reach the same cleanup path.

Thanks.