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

[netdev-bot+sashiko@kernel.org]

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()

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?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261007035739.3472432-1-4ncienth%40gmail.com