The branch main has been updated by glebius: URL: https://cgit.FreeBSD.org/src/commit/?id=d97922c6c63202fc19c22c0f821c951f85fb9840
commit d97922c6c63202fc19c22c0f821c951f85fb9840 Author: Gleb Smirnoff <[email protected]> AuthorDate: 2022-06-06 17:05:28 +0000 Commit: Gleb Smirnoff <[email protected]> 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));
