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_evR93rj1ZT_bzLKFqNQLPQ2BM0mzKnriGGsO5t07GAHQ@mail.gmail.com>
Date: Fri, 4 Apr 2025 10:22:38 -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 Fri, Apr 4, 2025 at 6:05 AM Ricardo Cañuelo Navarro <rcn@...lia.com> wrote:
>
> Thanks for the suggestion!
>
> On Thu, Apr 03 2025 at 14:44:18, Xin Long <lucien.xin@...il.com> wrote:
>
> > @@ -9234,7 +9236,7 @@ static int sctp_wait_for_sndbuf(struct
> > sctp_association *asoc, long *timeo_p,
> >                                           TASK_INTERRUPTIBLE);
> >                 if (asoc->base.dead)
> >                         goto do_dead;
> > -               if (!*timeo_p)
> > +               if (!*timeo_p || (t && t->dead))
> >                         goto do_nonblock;
> >                 if (sk->sk_err || asoc->state >= SCTP_STATE_SHUTDOWN_PENDING)
> >                         goto do_error;
>
> I suppose checking t->dead should be done after locking the socket
> again, where sctp_assoc_rm_peer() may have had a chance to run, rather
> than here?
>
It shouldn't matter, as long as it's protected by the socket lock.
The logic would be similar to checking asoc->base.dead.

> Something like this:
>
> @@ -9225,7 +9227,9 @@ static int sctp_wait_for_sndbuf(struct sctp_association *asoc, long *timeo_p,
>         pr_debug("%s: asoc:%p, timeo:%ld, msg_len:%zu\n", __func__, asoc,
>                  *timeo_p, msg_len);
>
> -       /* Increment the association's refcnt.  */
> +       /* Increment the transport and association's refcnt. */
> +       if (transport)
> +               sctp_transport_hold(transport);
>         sctp_association_hold(asoc);
>
>         /* Wait on the association specific sndbuf space. */
> @@ -9252,6 +9256,8 @@ static int sctp_wait_for_sndbuf(struct sctp_association *asoc, long *timeo_p,
>                 lock_sock(sk);
>                 if (sk != asoc->base.sk)
>                         goto do_error;
> +               if (transport && transport->dead)
> +                       goto do_nonblock;
>
>                 *timeo_p = current_timeo;
>         }
> @@ -9259,7 +9265,9 @@ static int sctp_wait_for_sndbuf(struct sctp_association *asoc, long *timeo_p,
>  out:
>         finish_wait(&asoc->wait, &wait);
>
> -       /* Release the association's refcnt.  */
> +       /* Release the transport and association's refcnt. */
> +       if (transport)
> +               sctp_transport_put(transport);
>         sctp_association_put(asoc);
>
>         return err;
>
>
> So by the time the sending thread re-claims the socket lock it can tell
> whether someone else removed the transport by checking transport->dead
> (set in sctp_transport_free()) and there's a guarantee that the
> transport hasn't been freed yet because we hold a reference to it.
>
> If the whole receive path through sctp_assoc_rm_peer() is protected by
> the same socket lock, as you said, this should be safe. The tests I ran
> seem to work fine. If you're ok with it I'll send another patch to
> supersede this one.
>
LGTM.

>
> > You will need to reintroduce the dead bit in struct sctp_transport and
> > set it in sctp_transport_free(). Note this field was previously removed in:
> >
> > commit 47faa1e4c50ec26e6e75dcd1ce53f064bd45f729
> > Author: Xin Long <lucien.xin@...il.com>
> > Date:   Fri Jan 22 01:49:09 2016 +0800
> >
> >     sctp: remove the dead field of sctp_transport
>
> I understand that none of the transport->dead checks from that commit
> are necessary anymore, since they were replaced by refcnt checks, and
> that we'll only bring the bit back for this particular check we're doing
> now, correct?
Correct, only the 'dead' bit and set it in sctp_transport_free().

Thanks.