[Bug 290658] race condition in SOCK_SEQPACKET unix sockets

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=290658

            Bug ID: 290658
           Summary: race condition in SOCK_SEQPACKET unix sockets
           Product: Base System
           Version: 15.0-CURRENT
          Hardware: Any
                OS: Any
            Status: New
          Severity: Affects Only Me
          Priority: ---
         Component: kern
          Assignee: bugs@FreeBSD.org
          Reporter: markj@FreeBSD.org

Suppose I try to read from a SEQPACKET PF_LOCAL socket with MSG_PEEK set.  We
find the extent of the mbuf chain whose contents we're going to return:

1482                         if (m->m_flags & M_EOR) {                          
1483                                 last = STAILQ_NEXT(m, m_stailq);           
1484                                 lastlen = 0;                               
1485                                 flags |= MSG_EOR;                          
1486                                 break;                                     
1487                         }

Later, we loop over mbufs in the socket buffer and stop once we see "last". 
The problem is that we drop the rx socket buffer buffer lock before that loop. 
So, if "last" is NULL, and the sender adds new a new record to the buffer
before the loop (possible because the socket buffer lock is not held by the
receiver), then we will return more than one record's worth of data:

1632         for (m = first; m != last; m = next) {                             
1633                 next = STAILQ_NEXT(m, m_stailq);                           
1634                 error = uiomove(mtod(m, char *), m->m_len, uio);           
1635                 if (__predict_false(error)) {                              
1636                         SOCK_IO_RECV_UNLOCK(so);                           
1637                         if (!peek)                                         
1638                                 for (; m != last; m = next) {              
1639                                         next = STAILQ_NEXT(m, m_stailq);   
1640                                         m_free(m);                         
1641                                 }                                          
1642                         return (error);                                    
1643                 }                                                          
1644                 if (!peek)                                                 
1645                         m_free(m);                                         
1646         }

That is, the "m != last" check only works as intended if last != NULL or
MSG_PEEK is not set.  If last == NULL, then we will traverse the whole socket
buffer.

More generally, this pattern is sketchy: how do we know that some other routine
isn't going to free the mbufs out from under us?  The recv I/O lock
synchronizes with other recv() calls, but there are other uipc functions like
uipc_sendfile() which fiddle with the receive buffer without that lock held.

-- 
You are receiving this mail because:
You are the assignee for the bug.