git: d97922c6c632 - main - unix/*: rewrite unp_internalize() cmsg parsing cycle

The branch main has been updated by glebius:

URL: https://cgit.FreeBSD.org/src/commit/?id=d97922c6c63202fc19c22c0f821c951f85fb9840

commit d97922c6c63202fc19c22c0f821c951f85fb9840
Author:     Gleb Smirnoff <glebius@FreeBSD.org>
AuthorDate: 2022-06-06 17:05:28 +0000
Commit:     Gleb Smirnoff <glebius@FreeBSD.org>
CommitDate: 2022-06-06 17:05:28 +0000

    unix/*: rewrite unp_internalize() cmsg parsing cycle
    
    Make it a complex, but a single for(;;) statement.  The previous cycle
    with some loop logic in the beginning and some loop logic at the end
    was confusing.  Both me and markj@ were misleaded to a conclusion that
    some checks are unnecessary, while they actually were necessary.
    
    While here, handle an edge case found by Mark, when on 64-bit platform
    an incorrect message from userland would underflow length counter, but
    return without any error.  Provide a test case for such message.
    
    Reviewed by:            markj
    Differential revision:  https://reviews.freebsd.org/D35375
---
 sys/kern/uipc_usrreq.c            | 37 ++++++++++++++-----------------------
 tests/sys/kern/unix_passfd_test.c | 37 ++++++++++++++++++++++++++++---------
 2 files changed, 42 insertions(+), 32 deletions(-)

diff --git a/sys/kern/uipc_usrreq.c b/sys/kern/uipc_usrreq.c
index 968631999d2c..16029e5c38c8 100644
--- a/sys/kern/uipc_usrreq.c
+++ b/sys/kern/uipc_usrreq.c
@@ -2193,22 +2193,20 @@ unp_internalize(struct mbuf **controlp, struct thread *td)
 	fdesc = p->p_fd;
 	error = 0;
 	control = *controlp;
-	clen = control->m_len;
 	*controlp = NULL;
 	initial_controlp = controlp;
-	for (cm = mtod(control, struct cmsghdr *); cm != NULL;) {
-		if (sizeof(*cm) > clen || cm->cmsg_level != SOL_SOCKET
-		    || cm->cmsg_len > clen || cm->cmsg_len < sizeof(*cm)) {
-			error = EINVAL;
-			goto out;
-		}
-		data = CMSG_DATA(cm);
-		datalen = (caddr_t)cm + cm->cmsg_len - (caddr_t)data;
+	for (clen = control->m_len, cm = mtod(control, struct cmsghdr *),
+	    data = CMSG_DATA(cm);
 
+	    clen >= sizeof(*cm) && cm->cmsg_level == SOL_SOCKET &&
+	    clen >= cm->cmsg_len && cm->cmsg_len >= sizeof(*cm) &&
+	    (char *)cm + cm->cmsg_len >= (char *)data;
+
+	    clen -= min(CMSG_SPACE(datalen), clen),
+	    cm = (struct cmsghdr *) ((char *)cm + CMSG_SPACE(datalen)),
+	    data = CMSG_DATA(cm)) {
+		datalen = (char *)cm + cm->cmsg_len - (char *)data;
 		switch (cm->cmsg_type) {
-		/*
-		 * Fill in credential information.
-		 */
 		case SCM_CREDS:
 			*controlp = sbcreatecontrol(NULL, sizeof(*cmcred),
 			    SCM_CREDS, SOL_SOCKET, M_WAITOK);
@@ -2228,7 +2226,7 @@ unp_internalize(struct mbuf **controlp, struct thread *td)
 		case SCM_RIGHTS:
 			oldfds = datalen / sizeof (int);
 			if (oldfds == 0)
-				break;
+				continue;
 			/* On some machines sizeof pointer is bigger than
 			 * sizeof int, so we need to check if data fits into
 			 * single mbuf.  We could allocate several mbufs, and
@@ -2334,17 +2332,10 @@ unp_internalize(struct mbuf **controlp, struct thread *td)
 			goto out;
 		}
 
-		if (*controlp != NULL)
-			controlp = &(*controlp)->m_next;
-		if (CMSG_SPACE(datalen) < clen) {
-			clen -= CMSG_SPACE(datalen);
-			cm = (struct cmsghdr *)
-			    ((caddr_t)cm + CMSG_SPACE(datalen));
-		} else {
-			clen = 0;
-			cm = NULL;
-		}
+		controlp = &(*controlp)->m_next;
 	}
+	if (clen > 0)
+		error = EINVAL;
 
 out:
 	if (error != 0 && initial_controlp != NULL)
diff --git a/tests/sys/kern/unix_passfd_test.c b/tests/sys/kern/unix_passfd_test.c
index 91030d84872b..fe46d815c6a1 100644
--- a/tests/sys/kern/unix_passfd_test.c
+++ b/tests/sys/kern/unix_passfd_test.c
@@ -744,14 +744,14 @@ ATF_TC_BODY(copyout_rights_error, tc)
 }
 
 /*
- * Verify that we can handle empty rights messages.  Try sending two SCM_RIGHTS
- * messages with a single call, one empty and one containing a single FD.
+ * Verify that we can handle empty rights messages.
  */
 ATF_TC_WITHOUT_HEAD(empty_rights_message);
 ATF_TC_BODY(empty_rights_message, tc)
 {
 	struct iovec iov;
 	struct msghdr msghdr;
+	struct cmsghdr cmsg;
 	char *cm, message[CMSG_SPACE(0) + CMSG_SPACE(sizeof(int))];
 	ssize_t len;
 	int error, fd[2], putfd;
@@ -759,21 +759,40 @@ ATF_TC_BODY(empty_rights_message, tc)
 	domainsocketpair(fd);
 	devnull(&putfd);
 
+	memset(&msghdr, 0, sizeof(msghdr));
+	iov.iov_base = NULL;
+	iov.iov_len = 0;
+	msghdr.msg_iov = &iov;
+	msghdr.msg_iovlen = 1;
+
+	/*
+	 * Try sending incorrect empty message.  On 64-bit platforms, where
+	 * CMSG_SPACE(0) > sizeof(struct cmsghdr), this will exercise
+	 * an edge case.
+	 */
+	cmsg = (struct cmsghdr ){
+	    .cmsg_len = sizeof(struct cmsghdr),	/* not CMSG_LEN(0)! */
+	    .cmsg_level = SOL_SOCKET,
+	    .cmsg_type = SCM_RIGHTS,
+	};
+	msghdr.msg_control = &cmsg;
+	msghdr.msg_controllen = CMSG_SPACE(0);
+
+	len = sendmsg(fd[0], &msghdr, 0);
+	if (CMSG_LEN(0) != sizeof(struct cmsghdr))
+		ATF_REQUIRE(len == -1 && errno == EINVAL);
+	else
+		ATF_REQUIRE(len == 0);
+
 	/*
-	 * First, try sending an empty message followed by a non-empty message.
+	 * Try sending an empty message followed by a non-empty message.
 	 */
 	cm = message;
 	putfds(cm, -1, 0);
 	cm += CMSG_SPACE(0);
 	putfds(cm, putfd, 1);
-
-	memset(&msghdr, 0, sizeof(msghdr));
-	iov.iov_base = NULL;
-	iov.iov_len = 0;
 	msghdr.msg_control = message;
 	msghdr.msg_controllen = sizeof(message);
-	msghdr.msg_iov = &iov;
-	msghdr.msg_iovlen = 1;
 
 	len = sendmsg(fd[0], &msghdr, 0);
 	ATF_REQUIRE_MSG(len == 0, "sendmsg failed: %s", strerror(errno));