linux-kernel - Re: [PATCH] sctp: check transport existence before processing a send primitive

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CADvbK_c69AoVyFDX2YduebF9DG8YyZM7aP7aMrMyqJi7vMmiSA@mail.gmail.com>
Date: Thu, 3 Apr 2025 10:42:08 -0400
From: Xin Long <lucien.xin@...il.com>
To: Ricardo Cañuelo Navarro <rcn@...lia.com>
Cc: Marcelo Ricardo Leitner <marcelo.leitner@...il.com>, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Simon Horman <horms@...nel.org>, kernel-dev@...lia.com, linux-sctp@...r.kernel.org, 
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH] sctp: check transport existence before processing a send primitive

On Thu, Apr 3, 2025 at 5:58 AM Ricardo Cañuelo Navarro <rcn@...lia.com> wrote:
>
> Thanks for reviewing, answers below:
>
> On Wed, Apr 02 2025 at 15:40:56, Xin Long <lucien.xin@...il.com> wrote:
> > The data send path:
> >
> >   sctp_endpoint_lookup_assoc() ->
> >   sctp_sendmsg_to_asoc()
> >
> > And the transport removal path:
> >
> >   sctp_sf_do_asconf() ->
> >   sctp_process_asconf() ->
> >   sctp_assoc_rm_peer()
> >
> > are both protected by the same socket lock.
> >
> > Additionally, when a path is removed, sctp_assoc_rm_peer() updates the
> > transport of all existing chunks in the send queues (peer->transmitted
> > and asoc->outqueue.out_chunk_list) to NULL.
> >
> > It will be great if you can reproduce the issue locally and help check
> > how the potential race occurs.
>
> That's true but if there isn't enough space in the send buffer, then
> sctp_sendmsg_to_asoc() will release the lock temporarily.
>
Oh right, I missed that. Thanks.

> The scenario that the reproducer generates is the following:
>
>         Thread A                                  Thread B
>         --------------------                      --------------------
> (1)     sctp_sendmsg()
>           lock_sock()
>           sctp_sendmsg_to_asoc()
>             sctp_wait_for_sndbuf()
>               release_sock()
>                                                   sctp_setsockopt(SCTP_SOCKOPT_BINDX_REM)
>                                                     lock_sock()
>                                                     sctp_setsockopt_bindx()
>                                                     sctp_send_asconf_del_ip()
>                                                       ...
>                                                     release_sock()
>                                                       process rcv backlog:
>                                                         sctp_do_sm()
>                                                           sctp_sf_do_asconf()
>                                                             ...
>                                                               sctp_assoc_rm_peer()
>               lock_sock()
> (2)          chunk->transport = transport
>              sctp_primitive_SEND()
>                ...
>                sctp_outq_select_transport()
> *BUG*            switch (new_transport->state)
>
>
> Notes:
> ------
>
> Both threads operate on the same socket.
>
> 1. Here, sctp_endpoint_lookup_assoc() finds and returns an existing
> association and transport.
>
> 2. At this point, `transport` is already deleted. chunk->transport is
> not set to NULL because sctp_assoc_rm_peer() ran _before_ the transport
> was assigned to the chunk.
>
> > We should avoid an extra hashtable lookup on this hot TX path, as it would
> > negatively impact performance.
>
> Good point. I can't really tell the performance impact of the lookup
> here, my experience with the SCTP implementation is very limited. Do you
> have any suggestions or alternatives about how to deal with this?
>
I think the correct approach is to follow how sctp_assoc_rm_peer()
handles this.

You can use asoc->peer.last_sent_to (which isn't really used elsewhere)
to temporarily store the transport before releasing the socket lock and
sleeping in sctp_sendmsg_to_asoc(). After waking up and reacquiring the
lock, restore the transport back to asoc->peer.last_sent_to.

Additionally, during an ASCONF update, ensure asoc->peer.last_sent_to
is set to a valid transport if it matches the transport being removed.

For example:

in sctp_wait_for_sndbuf():

    asoc->peer.last_sent_to = *tp;
    release_sock(sk);
    current_timeo = schedule_timeout(current_timeo);
    lock_sock(sk);
    *tp = asoc->peer.last_sent_to;
    asoc->peer.last_sent_to = NULL;

in sctp_assoc_rm_peer():

    if (asoc->peer.last_sent_to == peer)
        asoc->peer.last_sent_to = transport;