d/freebsd-dev
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

unix/*: rewrite unp_internalize() cmsg parsing cycle

Browse Source 
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
This commit is contained in:
Gleb Smirnoff 2022-06-06 10:05:28 -07:00
parent 8d95f50052
commit d97922c6c6
2 changed files with 42 additions and 32 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

37
sys/kern/uipc_usrreq.c
View File

@ -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)

37
tests/sys/kern/unix_passfd_test.c
View File

@ -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;
/*
* First, try sending an empty message followed by a non-empty message.
* 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);
/*
* 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));