From 44b83210c8ad21e907de61584cfeb2752abee19b Mon Sep 17 00:00:00 2001 From: Mark Johnston Date: Fri, 28 Aug 2026 11:04:20 +0000 Subject: [PATCH] unix: Fix a socket leak When connecting a unix domain stream socket, we 1. look up the peer (listening) socket, 2. allocate a new socket 3. add the new socket to the listening socket's queue Prior to commit 26147c51546e, this sequence of operations was synchronized by a pool mutex, also acquired in uipc_close(). After commit 26147c51546e, we drop the vnode pool lock immediately after finding the peer socket via a filesystem lookup. This creates a window where it's possible for a connection to add a new socket to the listening queue after the listening queue has been aborted. Fix the race by restoring the old behaviour of holding the pool lock across the solisten_enqueue() call. This is a bit ugly since we need to pass a mutex lock and a vnode through a couple of layers, but it seems like a low-risk solution. Alternately we could add some flag to the listening socket which indicates that no new connections are to be accepted, but I think this will require some changes to the generic socket code. Reported by: pho Fixes: 26147c51546e ("unix: pin the pathname peer by reference across the connect") Reviewed by: olce, kib, John Ericson Differential Revision: https://reviews.freebsd.org/D59201 --- sys/kern/uipc_usrreq.c | 108 ++++++++++++++++++++++++++--------------- 1 file changed, 70 insertions(+), 38 deletions(-) diff --git a/sys/kern/uipc_usrreq.c b/sys/kern/uipc_usrreq.c index 284997be3e04..f30119693a76 100644 --- a/sys/kern/uipc_usrreq.c +++ b/sys/kern/uipc_usrreq.c @@ -295,9 +295,9 @@ static int unp_connectat(int, struct socket *, const char *, int, static int unp_connect_peer(struct socket *, struct unpcb *, struct sockaddr **, struct thread *, bool); static int unp_connectat_peer(struct thread *, int, const char *, - struct socket **); + struct socket **, struct mtx **, struct vnode **); static int unp_vnode_peer(struct vnode *, struct thread *, - struct socket **); + struct socket **, struct mtx **, struct vnode **); static void unp_connect2(struct socket *, struct socket *, bool); static void unp_disconnect(struct unpcb *unp, struct unpcb *unp2); static void unp_dispose(struct socket *so); @@ -2955,6 +2955,8 @@ unp_connectat(int fd, struct socket *so, const char *path, int len, struct unpcb *unp; char buf[SOCK_MAXADDRLEN]; struct sockaddr *sa; + struct mtx *mtxp; + struct vnode *vp; int error; bool connreq; @@ -3006,17 +3008,36 @@ unp_connectat(int fd, struct socket *so, const char *path, int len, else sa = NULL; - error = unp_connectat_peer(td, fd, buf, &so2); - if (error != 0) - goto out; - error = unp_connect_peer(so, sotounpcb(so2), &sa, td, - referenced_peerp != NULL); - /* Transfer the reference; the caller releases it after unlocking. */ - if (error == 0 && referenced_peerp != NULL) - *referenced_peerp = so2; - else - sorele(so2); -out: + /* + * Find the peer socket we're connecting to, and connect to it. + * + * If the peer is bound to a name in the filesystem, then we hold + * the vnode pool lock until the connection is established, so as to + * avoid racing with a close of the peer listening socket. If the peer + * is referenced by a file descriptor, then that reference prevents the + * race, so no extra synchronization is needed. + */ + error = unp_connectat_peer(td, fd, buf, &so2, &mtxp, &vp); + if (error == 0) { + error = unp_connect_peer(so, sotounpcb(so2), &sa, td, + referenced_peerp != NULL); + if (error == 0 && referenced_peerp != NULL) { + *referenced_peerp = so2; + so2 = NULL; + } + + /* + * Release references only after the pool lock is dropped in + * order to avoid potential lock ordering issues. + */ + if (mtxp != NULL) { + mtx_unlock(mtxp); + vput(vp); + } + if (so2 != NULL) + sorele(so2); + } + free(sa, M_SONAME); if (__predict_false(error)) { UNP_PCB_LOCK(unp); @@ -3066,7 +3087,7 @@ unp_socket_fd_peer(struct thread *td, int fd, struct socket **so2p) * descriptor in td_dupfd and fails with ENODEV, the same convention open(2) * follows via dupfdopen() for /dev/fd. We honour it here and resolve that * descriptor as the peer, so a plain connect(2) to /dev/fd/N reaches the - * socket. Does not consume 'vp'. + * socket. Consumes the vnode reference. */ static int unp_dupfd_peer(struct vnode *vp, struct thread *td, struct socket **so2p) @@ -3079,13 +3100,14 @@ unp_dupfd_peer(struct vnode *vp, struct thread *td, struct socket **so2p) error = VOP_OPEN(vp, FREAD, td->td_ucred, td, NULL); dupfd = td->td_dupfd; td->td_dupfd = 0; - if (error == ENODEV && dupfd >= 0) - return (unp_socket_fd_peer(td, dupfd, so2p)); - if (error == 0) { + if (error == ENODEV && dupfd >= 0) { + error = unp_socket_fd_peer(td, dupfd, so2p); + } else if (error == 0) { /* Not the dupfd convention: an openable node is not a peer. */ (void)VOP_CLOSE(vp, FREAD, td->td_ucred, td); error = ECONNREFUSED; } + vput(vp); return (error); } @@ -3099,16 +3121,21 @@ unp_dupfd_peer(struct vnode *vp, struct thread *td, struct socket **so2p) * /dev/fd/N pathname fdescfs names a descriptor * ordinary pathname a bound socket looked up by path * - * The caller must release the returned socket with sorele(). + * The caller must release the returned socket with sorele(). If the mutex + * and vnode pointers are filled, they must be released as well. */ static int unp_connectat_peer(struct thread *td, int fd, const char *buf, - struct socket **so2p) + struct socket **so2p, struct mtx **mtxp, struct vnode **vpp) { struct nameidata nd; cap_rights_t rights; int error; + *so2p = NULL; + *mtxp = NULL; + *vpp = NULL; + /* * An empty sun_path means 'fd' names the peer directly. If it is a * socket, it is the peer, so return success (or its error) with no @@ -3131,10 +3158,10 @@ unp_connectat_peer(struct thread *td, int fd, const char *buf, NDFREE_PNBUF(&nd); /* - * Dispatch on the resolved vnode, then drop it: for the socket cases - * the returned reference keeps the peer stable, so the caller holds - * no vnode lock across unp_connect_peer() (which matters for the - * return_locked datagram fast path). + * Find our peer socket. If it comes from a socket on the filesystem, + * then we hold on to the vnode pool lock so as to interlock with a + * close of the listening socket. If the peer comes to us via an fd, + * then the fd reference itself keeps the peer stable. * * A synthetic descriptor node -- as fdescfs fabricates for a /dev/fd/N * path -- carries no type of its own (VNON); opening it yields the @@ -3149,8 +3176,7 @@ unp_connectat_peer(struct thread *td, int fd, const char *buf, if (nd.ni_vp->v_type == VNON) error = unp_dupfd_peer(nd.ni_vp, td, so2p); else - error = unp_vnode_peer(nd.ni_vp, td, so2p); - vput(nd.ni_vp); + error = unp_vnode_peer(nd.ni_vp, td, so2p, mtxp, vpp); return (error); } @@ -3160,13 +3186,10 @@ unp_connectat_peer(struct thread *td, int fd, const char *buf, * enforces the caller's authorization to reach the socket -- filesystem * permission (VOP_ACCESS) and MAC (mac_vnode_check_open) -- which bare readers * of the vnode->pcb binding, such as vfs_unp_reclaim(), deliberately skip. - * - * The returned reference keeps the peer stable for unp_connect_peer() once vp's - * per-vnode binding lock is dropped, so the caller must release it with - * sorele(). Does not consume 'vp'. */ static int -unp_vnode_peer(struct vnode *vp, struct thread *td, struct socket **so2p) +unp_vnode_peer(struct vnode *vp, struct thread *td, struct socket **so2p, + struct mtx **mtxp, struct vnode **vpp) { struct mtx *vplock; struct unpcb *unp2; @@ -3174,25 +3197,34 @@ unp_vnode_peer(struct vnode *vp, struct thread *td, struct socket **so2p) ASSERT_VOP_LOCKED(vp, __func__); - if (vp->v_type != VSOCK) - return (ENOTSOCK); + if (vp->v_type != VSOCK) { + error = ENOTSOCK; + goto fail; + } #ifdef MAC error = mac_vnode_check_open(td->td_ucred, vp, VWRITE | VREAD); if (error != 0) - return (error); + goto fail; #endif error = VOP_ACCESS(vp, VWRITE, td->td_ucred, td); if (error != 0) - return (error); + goto fail; vplock = mtx_pool_find(unp_vp_mtxpool, vp); mtx_lock(vplock); VOP_UNP_CONNECT(vp, &unp2); - if (unp2 == NULL) + if (unp2 == NULL) { + mtx_unlock(vplock); error = ECONNREFUSED; - else - soref(*so2p = unp2->unp_socket); - mtx_unlock(vplock); + goto fail; + } + soref(*so2p = unp2->unp_socket); + *mtxp = vplock; + *vpp = vp; + return (0); + +fail: + vput(vp); return (error); }