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

[netdev-bot+sashiko@kernel.org]

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.

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.

[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.

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