From nobody Mon Jun 06 17:05:45 2022 X-Original-To: dev-commits-src-main@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 636221BD5283; Mon, 6 Jun 2022 17:05:56 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4LH0Hl6Dcyz4lCG; Mon, 6 Jun 2022 17:05:55 +0000 (UTC) (envelope-from git@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1654535155; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=cav7JggsE/WQ5vI7n0K47hsj1v5+9vTgfaWurn+4G9M=; b=Xsq7Y9eY+uK3xXvJunxGDMYFOkCniZPLjQ977rL7rDG8VlztpJsFX+eCdXmnDbgqeUl8bK apVQMfBwo9UNZbdD5hW2kOEaAM1xpsWRbvHgcXSPPeWVTpn3NwvWtoWHf0C6QCA/ardGOP fW9JwskIpWyTIDdwvyQWp+qmQ59HSxHKx6+JoPi0Eyp2Bd7SnkOtOZnk0MdIMxdXEB+oRp wyLjawJvzQRPic1mpGYcx76xWT6SRBaEXCQArPmZl2jhJNCuSMQIi5a0p4Y08c/WQZ1T8E Z/UT2X2Qfr7RpOIOjMxfV2wgZ+vnaGV5S5dsKUVPqVrKySEzdUhLefSVob+Fdw== Received: from gitrepo.freebsd.org (gitrepo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 311C91D60; Mon, 6 Jun 2022 17:05:52 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org ([127.0.1.44]) by gitrepo.freebsd.org (8.16.1/8.16.1) with ESMTP id 256H5jLX086876; Mon, 6 Jun 2022 17:05:45 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.16.1/8.16.1/Submit) id 256H5j5I086875; Mon, 6 Jun 2022 17:05:45 GMT (envelope-from git) Date: Mon, 6 Jun 2022 17:05:45 GMT Message-Id: <202206061705.256H5j5I086875@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org From: Gleb Smirnoff Subject: git: d97922c6c632 - main - unix/*: rewrite unp_internalize() cmsg parsing cycle List-Id: Commit messages for the main branch of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-main List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-main@freebsd.org X-BeenThere: dev-commits-src-main@freebsd.org MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Git-Committer: glebius X-Git-Repository: src X-Git-Refname: refs/heads/main X-Git-Reftype: branch X-Git-Commit: d97922c6c63202fc19c22c0f821c951f85fb9840 Auto-Submitted: auto-generated ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1654535155; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=cav7JggsE/WQ5vI7n0K47hsj1v5+9vTgfaWurn+4G9M=; b=v7XcRMgYfqyKiwl2GX+p/SIkNr9+95DwKfR5nNWZcRMy52w8azyfh4xllkKSYenjHLW/8v L6Cx3PfqozoLkJTo5ckxlF0wcBOm5NltTgMESQPkWyTwk2c4aHQaqgnhcSquNMvLtwbX5R rqRlvofoAt0Xc9ld7a4bw8uC6F5DcBDuj52XlMBchIEXrtx7VLR3p25Na+LViyfg8Xd2Hv 2o+D8qZ+Z1b7vlOoUvHMqZ1ywBwl9EJ+YL7lW6D058K7a7NUA63XupMoTAr96Pp5l5Aclp Cy6iOqkENaK88XNXXXP0Wnu4hxMWLnHlGAy21IPvtEvnyUYLBGWrfx5jLswkdw== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1654535155; a=rsa-sha256; cv=none; b=sxdEis6ELeuCZyeqPftP8YIdNQK1c1pbfzQZOX8fFZRKiYBS0jHKLDhDcap0d4e7HcMo6l qzTbNQjWzjZG0OgbuVyJcnEULUhRItcB9r+UiF2G2OcBDrhQ9b+kJdlvIMWnGcKR5SmmHN O6a1JGrqxM0OdCbJ4A8E+4fV9Vuzm6B+ZP2hv8BKx4FESftJoi/kahubrp8KShzoatVtWN iz+vBveq12Clr3sPUyP2y9WHWXPNFR+WfB6LNk3nxJhthrWA+TCDDkN6Go6iN5BWAVZK4Y Duo6l2VMscQhcHkoh0xi6/lrBqIuvqn7K7noPuPKQTjU/+K2vd8WS3Zms+5ZXg== ARC-Authentication-Results: i=1; mx1.freebsd.org; none X-ThisMailContainsUnwantedMimeParts: N The branch main has been updated by glebius: URL: https://cgit.FreeBSD.org/src/commit/?id=d97922c6c63202fc19c22c0f821c951f85fb9840 commit d97922c6c63202fc19c22c0f821c951f85fb9840 Author: Gleb Smirnoff AuthorDate: 2022-06-06 17:05:28 +0000 Commit: Gleb Smirnoff 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));