On Fri, Sep 9, 2016 at 9:19 PM, Timmons C. Player < [email protected]> 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 <[email protected]> > --- > 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 [email protected]. > 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 [email protected]. For more options, visit https://groups.google.com/d/optout.
