On Fri, Sep 9, 2016 at 9:19 PM, Timmons C. Player <
timmons.pla...@spirent.com> wrote:

> Simultaneously closing a socket from both the network and user
> space sides can trigger a use after free bug in soisdisconnected.
> This change uses the existing socket reference counting mechanism
> to prevent the socket from being freed until after this function
> has completed.
>
> Signed-off-by: Timmons C. Player <timmons.pla...@spirent.com>
> ---
>  bsd/sys/kern/uipc_socket.cc | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>
> diff --git a/bsd/sys/kern/uipc_socket.cc b/bsd/sys/kern/uipc_socket.cc
> index 1c49efc..194e74d 100644
> --- a/bsd/sys/kern/uipc_socket.cc
> +++ b/bsd/sys/kern/uipc_socket.cc
> @@ -3469,11 +3469,19 @@ void
>  soisdisconnected(struct socket *so)
>  {
>
> +       bool do_release = false;
>

Nitpick (not important now, since I think there's nothing else blocking
this patch): In modern C++, you don't need to define variables in the top
of the function - nicer to do this below where you actually calculate this
"do_release" so the reader can make better sense of it.

        /*
>          * Note: This code assumes that SOCK_LOCK(so) and
>          * SOCKBUF_LOCK(&so->so_rcv) are the same.
>          */
>         SOCK_LOCK(so);
> +       /*
> +        * If user space has already closed the socket, then it's possible
> +        * for some of these wakeups to trigger soclose.  We need to
> prevent
> +        * the socket from getting freed in the middle of this function, so
> +        * bump the reference count.
> +        */
> +       soref(so);
>         so->so_state &= ~(SS_ISCONNECTING|SS_ISCONNECTED|SS_
> ISDISCONNECTING);
>         so->so_state |= SS_ISDISCONNECTED;
>         so->so_rcv.sb_state |= SBS_CANTRCVMORE;
> @@ -3481,8 +3489,25 @@ soisdisconnected(struct socket *so)
>         so->so_snd.sb_state |= SBS_CANTSENDMORE;
>         sbdrop_locked(so, &so->so_snd, so->so_snd.sb_cc);
>         sowwakeup_locked(so);
> +       /*
> +        * If we have the only reference, then we need to call sorele to
> +        * free the socket.  If not, then we just quietly drop the ref
> +        * count ourselves to avoid taking the accept lock and possibly
> +        * deadlocking.
>
+        */
> +       if (so->so_count == 1) {
> +               do_release = true;
> +       } else {
> +               so->so_count--;
> +       }
>         SOCK_UNLOCK(so);
>         wakeup(&so->so_timeo);
> +
> +       if (do_release) {
> +               ACCEPT_LOCK();
> +               SOCK_LOCK(so);
> +               sorele(so);
> +       }
>

Thanks.
This logic appears correct to me, although it is really odd-looking and I
thought it must be wrong at first before I tried to follow the weird
spaghetti locking code in that file...
No wonder the comments in the code about sorele() says that "For a number
of reasons, these interfaces are not preferred, and should be avoided."

 }
>
>  /*
> --
> 2.7.4
>
> --
> You received this message because you are subscribed to the Google Groups
> "OSv Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to osv-dev+unsubscr...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

-- 
You received this message because you are subscribed to the Google Groups "OSv 
Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to osv-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Reply via email to