Skip site navigation (1)Skip section navigation (2)
Date:      Tue, 7 Aug 2018 16:36:49 +0000 (UTC)
From:      Mark Johnston <markj@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   svn commit: r337423 - in head/sys: compat/cloudabi compat/freebsd32 compat/linux kern sys
Message-ID:  <201808071636.w77GanlD026372@repo.freebsd.org>

next in thread | raw e-mail | index | archive | help
Author: markj
Date: Tue Aug  7 16:36:48 2018
New Revision: 337423
URL: https://svnweb.freebsd.org/changeset/base/337423

Log:
  Improve handling of control message truncation.
  
  If a recvmsg(2) or recvmmsg(2) caller doesn't provide sufficient space
  for all control messages, the kernel sets MSG_CTRUNC in the message
  flags to indicate truncation of the control messages.  In the case
  of SCM_RIGHTS messages, however, we were failing to dispose of the
  rights that had already been externalized into the recipient's file
  descriptor table.  Add a new function and mbuf type to handle this
  cleanup task, and use it any time we fail to copy control messages
  out to the recipient.  To simplify cleanup, control message truncation
  is now only performed at control message boundaries.
  
  The change also fixes a few related bugs:
  - Rights could be leaked to the recipient process if an error occurred
    while copying out a message's contents.
  - We failed to set MSG_CTRUNC if the truncation occurred on a control
    message boundary, e.g., if the caller received two control messages
    and provided only the exact amount of buffer space needed for the
    first.
  
  PR:		131876
  Reviewed by:	ed (previous version)
  MFC after:	1 month
  Sponsored by:	The FreeBSD Foundation
  Differential Revision:	https://reviews.freebsd.org/D16561

Modified:
  head/sys/compat/cloudabi/cloudabi_sock.c
  head/sys/compat/freebsd32/freebsd32_misc.c
  head/sys/compat/linux/linux_socket.c
  head/sys/kern/uipc_syscalls.c
  head/sys/kern/uipc_usrreq.c
  head/sys/sys/mbuf.h

Modified: head/sys/compat/cloudabi/cloudabi_sock.c
==============================================================================
--- head/sys/compat/cloudabi/cloudabi_sock.c	Tue Aug  7 15:24:19 2018	(r337422)
+++ head/sys/compat/cloudabi/cloudabi_sock.c	Tue Aug  7 16:36:48 2018	(r337423)
@@ -120,24 +120,27 @@ cloudabi_sock_recv(struct thread *td, cloudabi_fd_t fd
 				    sizeof(int);
 				if (nfds > fdslen) {
 					/* Unable to store file descriptors. */
-					nfds = fdslen;
 					*rflags |=
 					    CLOUDABI_SOCK_RECV_FDS_TRUNCATED;
+					m_dispose_extcontrolm(control);
+					break;
 				}
 				error = copyout(CMSG_DATA(chdr), fds,
 				    nfds * sizeof(int));
-				if (error != 0) {
-					m_free(control);
-					return (error);
-				}
+				if (error != 0)
+					break;
 				fds += nfds;
 				fdslen -= nfds;
 				*rfdslen += nfds;
 			}
 		}
-		m_free(control);
+		if (control != NULL) {
+			if (error != 0)
+				m_dispose_extcontrolm(control);
+			m_free(control);
+		}
 	}
-	return (0);
+	return (error);
 }
 
 int

Modified: head/sys/compat/freebsd32/freebsd32_misc.c
==============================================================================
--- head/sys/compat/freebsd32/freebsd32_misc.c	Tue Aug  7 15:24:19 2018	(r337422)
+++ head/sys/compat/freebsd32/freebsd32_misc.c	Tue Aug  7 16:36:48 2018	(r337423)
@@ -1064,7 +1064,7 @@ freebsd32_copyoutmsghdr(struct msghdr *msg, struct msg
 				 FREEBSD32_ALIGN(sizeof(struct cmsghdr)))
 
 static size_t
-freebsd32_cmsg_convert(struct cmsghdr *cm, void *data, socklen_t datalen)
+freebsd32_cmsg_convert(const struct cmsghdr *cm, void *data, socklen_t datalen)
 {
 	size_t copylen;
 	union {
@@ -1122,7 +1122,7 @@ freebsd32_copy_msg_out(struct msghdr *msg, struct mbuf
 {
 	struct cmsghdr *cm;
 	void *data;
-	socklen_t clen, datalen, datalen_out;
+	socklen_t clen, datalen, datalen_out, oldclen;
 	int error;
 	caddr_t ctlbuf;
 	int len, maxlen, copylen;
@@ -1133,15 +1133,11 @@ freebsd32_copy_msg_out(struct msghdr *msg, struct mbuf
 	maxlen = msg->msg_controllen;
 	msg->msg_controllen = 0;
 
-	m = control;
 	ctlbuf = msg->msg_control;
-      
-	while (m && len > 0) {
+	for (m = control; m != NULL && len > 0; m = m->m_next) {
 		cm = mtod(m, struct cmsghdr *);
 		clen = m->m_len;
-
 		while (cm != NULL) {
-
 			if (sizeof(struct cmsghdr) > clen ||
 			    cm->cmsg_len > clen) {
 				error = EINVAL;
@@ -1152,34 +1148,36 @@ freebsd32_copy_msg_out(struct msghdr *msg, struct mbuf
 			datalen = (caddr_t)cm + cm->cmsg_len - (caddr_t)data;
 			datalen_out = freebsd32_cmsg_convert(cm, data, datalen);
 
-			/* Adjust message length */
-			cm->cmsg_len = FREEBSD32_ALIGN(sizeof(struct cmsghdr)) +
-			    datalen_out;
-
-			/* Copy cmsghdr */
+			/*
+			 * Copy out the message header.  Preserve the native
+			 * message size in case we need to inspect the message
+			 * contents later.
+			 */
 			copylen = sizeof(struct cmsghdr);
 			if (len < copylen) {
 				msg->msg_flags |= MSG_CTRUNC;
-				copylen = len;
+				m_dispose_extcontrolm(m);
+				goto exit;
 			}
-
+			oldclen = cm->cmsg_len;
+			cm->cmsg_len = FREEBSD32_ALIGN(sizeof(struct cmsghdr)) +
+			    datalen_out;
 			error = copyout(cm, ctlbuf, copylen);
-			if (error)
+			cm->cmsg_len = oldclen;
+			if (error != 0)
 				goto exit;
 
 			ctlbuf += FREEBSD32_ALIGN(copylen);
 			len    -= FREEBSD32_ALIGN(copylen);
 
-			if (len <= 0)
-				break;
-
-			/* Copy data */
 			copylen = datalen_out;
 			if (len < copylen) {
 				msg->msg_flags |= MSG_CTRUNC;
-				copylen = len;
+				m_dispose_extcontrolm(m);
+				break;
 			}
 
+			/* Copy out the message data. */
 			error = copyout(data, ctlbuf, copylen);
 			if (error)
 				goto exit;
@@ -1190,20 +1188,23 @@ freebsd32_copy_msg_out(struct msghdr *msg, struct mbuf
 			if (CMSG_SPACE(datalen) < clen) {
 				clen -= CMSG_SPACE(datalen);
 				cm = (struct cmsghdr *)
-					((caddr_t)cm + CMSG_SPACE(datalen));
+				    ((caddr_t)cm + CMSG_SPACE(datalen));
 			} else {
 				clen = 0;
 				cm = NULL;
 			}
-		}	
-		m = m->m_next;
+
+			msg->msg_controllen += FREEBSD32_ALIGN(sizeof(*cm)) +
+			    datalen_out;
+		}
 	}
+	if (len == 0 && m != NULL) {
+		msg->msg_flags |= MSG_CTRUNC;
+		m_dispose_extcontrolm(m);
+	}
 
-	msg->msg_controllen = (len <= 0) ? maxlen :  ctlbuf - (caddr_t)msg->msg_control;
-	
 exit:
 	return (error);
-
 }
 
 int
@@ -1240,19 +1241,22 @@ freebsd32_recvmsg(td, uap)
 	error = kern_recvit(td, uap->s, &msg, UIO_USERSPACE, controlp);
 	if (error == 0) {
 		msg.msg_iov = uiov;
-		
+
 		if (control != NULL)
 			error = freebsd32_copy_msg_out(&msg, control);
 		else
 			msg.msg_controllen = 0;
-		
+
 		if (error == 0)
 			error = freebsd32_copyoutmsghdr(&msg, uap->msg);
 	}
 	free(iov, M_IOV);
 
-	if (control != NULL)
+	if (control != NULL) {
+		if (error != 0)
+			m_dispose_extcontrolm(control);
 		m_freem(control);
+	}
 
 	return (error);
 }

Modified: head/sys/compat/linux/linux_socket.c
==============================================================================
--- head/sys/compat/linux/linux_socket.c	Tue Aug  7 15:24:19 2018	(r337422)
+++ head/sys/compat/linux/linux_socket.c	Tue Aug  7 16:36:48 2018	(r337423)
@@ -1263,7 +1263,7 @@ linux_recvmsg_common(struct thread *td, l_int s, struc
 	struct cmsgcred *cmcred;
 	struct l_cmsghdr *linux_cmsg = NULL;
 	struct l_ucred linux_ucred;
-	socklen_t datalen, outlen;
+	socklen_t datalen, maxlen, outlen;
 	struct l_msghdr linux_msg;
 	struct iovec *iov, *uiov;
 	struct mbuf *control = NULL;
@@ -1322,9 +1322,8 @@ linux_recvmsg_common(struct thread *td, l_int s, struc
 			goto bad;
 	}
 
-	outbuf = PTRIN(linux_msg.msg_control);
-	outlen = 0;
-
+	maxlen = linux_msg.msg_controllen;
+	linux_msg.msg_controllen = 0;
 	if (control) {
 		linux_cmsg = malloc(L_CMSG_HDRSZ, M_LINUX, M_WAITOK | M_ZERO);
 
@@ -1332,15 +1331,15 @@ linux_recvmsg_common(struct thread *td, l_int s, struc
 		msg->msg_controllen = control->m_len;
 
 		cm = CMSG_FIRSTHDR(msg);
-
+		outbuf = PTRIN(linux_msg.msg_control);
+		outlen = 0;
 		while (cm != NULL) {
 			linux_cmsg->cmsg_type =
 			    bsd_to_linux_cmsg_type(cm->cmsg_type);
 			linux_cmsg->cmsg_level =
 			    bsd_to_linux_sockopt_level(cm->cmsg_level);
-			if (linux_cmsg->cmsg_type == -1
-			    || cm->cmsg_level != SOL_SOCKET)
-			{
+			if (linux_cmsg->cmsg_type == -1 ||
+			    cm->cmsg_level != SOL_SOCKET) {
 				error = EINVAL;
 				goto bad;
 			}
@@ -1348,8 +1347,7 @@ linux_recvmsg_common(struct thread *td, l_int s, struc
 			data = CMSG_DATA(cm);
 			datalen = (caddr_t)cm + cm->cmsg_len - (caddr_t)data;
 
-			switch (cm->cmsg_type)
-			{
+			switch (cm->cmsg_type) {
 			case SCM_RIGHTS:
 				if (flags & LINUX_MSG_CMSG_CLOEXEC) {
 					fds = datalen / sizeof(int);
@@ -1394,14 +1392,13 @@ linux_recvmsg_common(struct thread *td, l_int s, struc
 				break;
 			}
 
-			if (outlen + LINUX_CMSG_LEN(datalen) >
-			    linux_msg.msg_controllen) {
+			if (outlen + LINUX_CMSG_LEN(datalen) > maxlen) {
 				if (outlen == 0) {
 					error = EMSGSIZE;
 					goto bad;
 				} else {
-					linux_msg.msg_flags |=
-					    LINUX_MSG_CTRUNC;
+					linux_msg.msg_flags |= LINUX_MSG_CTRUNC;
+					m_dispose_extcontrolm(control);
 					goto out;
 				}
 			}
@@ -1422,15 +1419,19 @@ linux_recvmsg_common(struct thread *td, l_int s, struc
 
 			cm = CMSG_NXTHDR(msg, cm);
 		}
+		linux_msg.msg_controllen = outlen;
 	}
 
 out:
-	linux_msg.msg_controllen = outlen;
 	error = copyout(&linux_msg, msghdr, sizeof(linux_msg));
 
 bad:
+	if (control != NULL) {
+		if (error != 0)
+			m_dispose_extcontrolm(control);
+		m_freem(control);
+	}
 	free(iov, M_IOV);
-	m_freem(control);
 	free(linux_cmsg, M_LINUX);
 
 	return (error);

Modified: head/sys/kern/uipc_syscalls.c
==============================================================================
--- head/sys/kern/uipc_syscalls.c	Tue Aug  7 15:24:19 2018	(r337422)
+++ head/sys/kern/uipc_syscalls.c	Tue Aug  7 16:36:48 2018	(r337423)
@@ -905,7 +905,7 @@ kern_recvit(struct thread *td, int s, struct msghdr *m
 {
 	struct uio auio;
 	struct iovec *iov;
-	struct mbuf *m, *control = NULL;
+	struct mbuf *control, *m;
 	caddr_t ctlbuf;
 	struct file *fp;
 	struct socket *so;
@@ -952,6 +952,7 @@ kern_recvit(struct thread *td, int s, struct msghdr *m
 	if (KTRPOINT(td, KTR_GENIO))
 		ktruio = cloneuio(&auio);
 #endif
+	control = NULL;
 	len = auio.uio_resid;
 	error = soreceive(so, &fromsa, &auio, NULL,
 	    (mp->msg_control || controlp) ? &control : NULL,
@@ -1015,30 +1016,22 @@ kern_recvit(struct thread *td, int s, struct msghdr *m
 			control->m_data += sizeof (struct cmsghdr);
 		}
 #endif
+		ctlbuf = mp->msg_control;
 		len = mp->msg_controllen;
-		m = control;
 		mp->msg_controllen = 0;
-		ctlbuf = mp->msg_control;
-
-		while (m && len > 0) {
-			unsigned int tocopy;
-
-			if (len >= m->m_len)
-				tocopy = m->m_len;
-			else {
-				mp->msg_flags |= MSG_CTRUNC;
-				tocopy = len;
-			}
-
-			if ((error = copyout(mtod(m, caddr_t),
-					ctlbuf, tocopy)) != 0)
+		for (m = control; m != NULL && len >= m->m_len; m = m->m_next) {
+			if ((error = copyout(mtod(m, caddr_t), ctlbuf,
+			    m->m_len)) != 0)
 				goto out;
 
-			ctlbuf += tocopy;
-			len -= tocopy;
-			m = m->m_next;
+			ctlbuf += m->m_len;
+			len -= m->m_len;
+			mp->msg_controllen += m->m_len;
 		}
-		mp->msg_controllen = ctlbuf - (caddr_t)mp->msg_control;
+		if (m != NULL) {
+			mp->msg_flags |= MSG_CTRUNC;
+			m_dispose_extcontrolm(m);
+		}
 	}
 out:
 	fdrop(fp, td);
@@ -1050,8 +1043,11 @@ out:
 
 	if (error == 0 && controlp != NULL)
 		*controlp = control;
-	else  if (control)
+	else if (control != NULL) {
+		if (error != 0)
+			m_dispose_extcontrolm(control);
 		m_freem(control);
+	}
 
 	return (error);
 }
@@ -1572,4 +1568,52 @@ getsockaddr(struct sockaddr **namp, caddr_t uaddr, siz
 		*namp = sa;
 	}
 	return (error);
+}
+
+/*
+ * Dispose of externalized rights from an SCM_RIGHTS message.  This function
+ * should be used in error or truncation cases to avoid leaking file descriptors
+ * into the recipient's (the current thread's) table.
+ */
+void
+m_dispose_extcontrolm(struct mbuf *m)
+{
+	struct cmsghdr *cm;
+	struct file *fp;
+	struct thread *td;
+	socklen_t clen, datalen;
+	int error, fd, *fds, nfd;
+
+	td = curthread;
+	for (; m != NULL; m = m->m_next) {
+		if (m->m_type != MT_EXTCONTROL)
+			continue;
+		cm = mtod(m, struct cmsghdr *);
+		clen = m->m_len;
+		while (clen > 0) {
+			if (clen < sizeof(*cm))
+				panic("%s: truncated mbuf %p", __func__, m);
+			datalen = CMSG_SPACE(cm->cmsg_len - CMSG_SPACE(0));
+			if (clen < datalen)
+				panic("%s: truncated mbuf %p", __func__, m);
+
+			if (cm->cmsg_level == SOL_SOCKET &&
+			    cm->cmsg_type == SCM_RIGHTS) {
+				fds = (int *)CMSG_DATA(cm);
+				nfd = (cm->cmsg_len - CMSG_SPACE(0)) /
+				    sizeof(int);
+
+				while (nfd-- > 0) {
+					fd = *fds++;
+					error = fget(td, fd, &cap_no_rights,
+					    &fp);
+					if (error == 0)
+						fdclose(td, fp, fd);
+				}
+			}
+			clen -= datalen;
+			cm = (struct cmsghdr *)((uint8_t *)cm + datalen);
+		}
+		m_chtype(m, MT_CONTROL);
+	}
 }

Modified: head/sys/kern/uipc_usrreq.c
==============================================================================
--- head/sys/kern/uipc_usrreq.c	Tue Aug  7 15:24:19 2018	(r337422)
+++ head/sys/kern/uipc_usrreq.c	Tue Aug  7 16:36:48 2018	(r337423)
@@ -2050,6 +2050,13 @@ unp_externalize(struct mbuf *control, struct mbuf **co
 				    &fdep[i]->fde_caps);
 				unp_externalize_fp(fdep[i]->fde_file);
 			}
+
+			/*
+			 * The new type indicates that the mbuf data refers to
+			 * kernel resources that may need to be released before
+			 * the mbuf is freed.
+			 */
+			m_chtype(*controlp, MT_EXTCONTROL);
 			FILEDESC_XUNLOCK(fdesc);
 			free(fdep[0], M_FILECAPS);
 		} else {

Modified: head/sys/sys/mbuf.h
==============================================================================
--- head/sys/sys/mbuf.h	Tue Aug  7 15:24:19 2018	(r337422)
+++ head/sys/sys/mbuf.h	Tue Aug  7 16:36:48 2018	(r337423)
@@ -568,7 +568,8 @@ struct mbuf {
 #define	MT_EXP4		12	/* for experimental use */
 
 #define	MT_CONTROL	14	/* extra-data protocol message */
-#define	MT_OOBDATA	15	/* expedited data  */
+#define	MT_EXTCONTROL	15	/* control message with externalized contents */
+#define	MT_OOBDATA	16	/* expedited data  */
 
 #define	MT_NOINIT	255	/* Not a type but a flag to allocate
 				   a non-initialized mbuf */
@@ -633,6 +634,7 @@ void		 m_demote_pkthdr(struct mbuf *);
 void		 m_demote(struct mbuf *, int, int);
 struct mbuf	*m_devget(char *, int, int, struct ifnet *,
 		    void (*)(char *, caddr_t, u_int));
+void		 m_dispose_extcontrolm(struct mbuf *m);
 struct mbuf	*m_dup(const struct mbuf *, int);
 int		 m_dup_pkthdr(struct mbuf *, const struct mbuf *, int);
 void		 m_extadd(struct mbuf *, char *, u_int, m_ext_free_t,



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201808071636.w77GanlD026372>