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_dTX3c9wgMa8bDW-Hg-5gGJ7sJzN5s8xtGwwYW9FE=rcg@mail.gmail.com>
Date: Wed, 2 Apr 2025 15:40:56 -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 Wed, Apr 2, 2025 at 6:26 AM Ricardo Cañuelo Navarro <rcn@...lia.com> wrote:
>
> sctp_sendmsg() re-uses associations and transports when possible by
> doing a lookup based on the socket endpoint and the message destination
> address, and then sctp_sendmsg_to_asoc() sets the selected transport in
> all the message chunks to be sent.
>
> There's a possible race condition if another thread triggers the removal
> of that selected transport, for instance, by explicitly unbinding an
> address with setsockopt(SCTP_SOCKOPT_BINDX_REM), after the chunks have
> been set up and before the message is sent. This causes the access to
> the transport data in sctp_outq_select_transport(), when the association
> outqueue is flushed, to do a use-after-free read.
>
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.

> This patch addresses this scenario by checking if the transport still
> exists right after the chunks to be sent are set up to use it and before
> proceeding to sending them. If the transport was freed since it was
> found, the send is aborted. The reason to add the check here is that
> once the transport is assigned to the chunks, deleting that transport
> is safe, since it will also set chunk->transport to NULL in the affected
> chunks. This scenario is correctly handled already, see Fixes below.
>
> The bug was found by a private syzbot instance (see the error report [1]
> and the C reproducer that triggers it [2]).
>
> Link: https://people.igalia.com/rcn/kernel_logs/20250402__KASAN_slab-use-after-free_Read_in_sctp_outq_select_transport.txt [1]
> Link: https://people.igalia.com/rcn/kernel_logs/20250402__KASAN_slab-use-after-free_Read_in_sctp_outq_select_transport__repro.c [2]
> Cc: stable@...r.kernel.org
> Fixes: df132eff4638 ("sctp: clear the transport of some out_chunk_list chunks in sctp_assoc_rm_peer")
> Signed-off-by: Ricardo Cañuelo Navarro <rcn@...lia.com>
> ---
>  net/sctp/socket.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 36ee34f483d703ffcfe5ca9e6cc554fba24c75ef..9c5ff44fa73cae6a6a04790800cc33dfa08a8da9 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -1787,17 +1787,24 @@ static int sctp_sendmsg_check_sflags(struct sctp_association *asoc,
>         return 1;
>  }
>
> +static union sctp_addr *sctp_sendmsg_get_daddr(struct sock *sk,
> +                                              const struct msghdr *msg,
> +                                              struct sctp_cmsgs *cmsgs);
> +
>  static int sctp_sendmsg_to_asoc(struct sctp_association *asoc,
>                                 struct msghdr *msg, size_t msg_len,
>                                 struct sctp_transport *transport,
>                                 struct sctp_sndrcvinfo *sinfo)
>  {
> +       struct sctp_transport *aux_transport = NULL;
>         struct sock *sk = asoc->base.sk;
> +       struct sctp_endpoint *ep = sctp_sk(sk)->ep;
>         struct sctp_sock *sp = sctp_sk(sk);
>         struct net *net = sock_net(sk);
>         struct sctp_datamsg *datamsg;
>         bool wait_connect = false;
>         struct sctp_chunk *chunk;
> +       union sctp_addr *daddr;
>         long timeo;
>         int err;
>
> @@ -1869,6 +1876,15 @@ static int sctp_sendmsg_to_asoc(struct sctp_association *asoc,
>                 sctp_set_owner_w(chunk);
>                 chunk->transport = transport;
>         }
> +       /* Fail if transport was deleted after lookup in sctp_sendmsg() */
> +       daddr = sctp_sendmsg_get_daddr(sk, msg, NULL);
> +       if (daddr) {
> +               sctp_endpoint_lookup_assoc(ep, daddr, &aux_transport);
> +               if (!aux_transport || aux_transport != transport) {
> +                       sctp_datamsg_free(datamsg);
> +                       goto err;
> +               }
> +       }
>
We should avoid an extra hashtable lookup on this hot TX path, as it would
negatively impact performance.

Thanks.

>         err = sctp_primitive_SEND(net, asoc, datamsg);
>         if (err) {
>
> ---
> base-commit: 38fec10eb60d687e30c8c6b5420d86e8149f7557
> change-id: 20250402-kasan_slab-use-after-free_read_in_sctp_outq_select_transport-46c9c30bcb7d
>