From: Gleb Smirnoff Date: Tue, 30 Sep 2025 07:54:35 +0000 (-0700) Subject: privsep: enforce message boundaries with MSG_EOR on our messages (#533) X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=b709538cbbc7e480e479ac82149adb073bb01531;p=thirdparty%2Fdhcpcd.git privsep: enforce message boundaries with MSG_EOR on our messages (#533) privsep: enforce message boundaries with MSG_EOR on our messages The nature of the SOCK_SEQPACKET, that privsep modules uses, is stream. See: https://pubs.opengroup.org/onlinepubs/9799919799/functions/V2_chap02.html#tag_16_10_06 To guarantee that a reader will never read two messages in one read operation, the writer shall put end of record markers. The problem exposed itself in FreeBSD 15.0 that started to follow the specification better than before. Other SOCK_SEQPACKET usage considerations: a) as long as our reader provides a receive buffer that would fit the largest message our writer would ever send, we are good with regards to not a reading a partial message b) as long as our writer always write full messages with one write, we don't need use of MSG_WAITALL in reader. Fixes #530 Co-authored-by: Roy Marples --- diff --git a/src/dhcpcd.c b/src/dhcpcd.c index 3e65141e..0251ee85 100644 --- a/src/dhcpcd.c +++ b/src/dhcpcd.c @@ -393,7 +393,7 @@ dhcpcd_daemonise(struct dhcpcd_ctx *ctx) eloop_event_delete(ctx->eloop, ctx->fork_fd); exit_code = EXIT_SUCCESS; - if (write(ctx->fork_fd, &exit_code, sizeof(exit_code)) == -1) + if (send(ctx->fork_fd, &exit_code, sizeof(exit_code), MSG_EOR) == -1) logerr(__func__); close(ctx->fork_fd); ctx->fork_fd = -1; @@ -1449,8 +1449,8 @@ dhcpcd_signal_cb(int sig, void *arg) if (sig != SIGCHLD && ctx->options & DHCPCD_FORKED) { if (sig != SIGHUP && - write(ctx->fork_fd, &sig, sizeof(sig)) == -1) - logerr("%s: write", __func__); + send(ctx->fork_fd, &sig, sizeof(sig), MSG_EOR) == -1) + logerr("%s: send", __func__); return; } @@ -2712,8 +2712,8 @@ exit1: #ifdef USE_SIGNALS /* If still attached, detach from the launcher */ if (ctx.options & DHCPCD_STARTED && ctx.fork_fd != -1) { - if (write(ctx.fork_fd, &i, sizeof(i)) == -1) - logerr("%s: write", __func__); + if (send(ctx.fork_fd, &i, sizeof(i), MSG_EOR) == -1) + logerr("%s: send", __func__); } #endif diff --git a/src/logerr.c b/src/logerr.c index 98cd1c7e..7e8038bd 100644 --- a/src/logerr.c +++ b/src/logerr.c @@ -26,7 +26,9 @@ * SUCH DAMAGE. */ +#include #include + #include #include #include @@ -215,18 +217,25 @@ vlogmessage(int pri, const char *fmt, va_list args) int len = 0; if (ctx->log_fd != -1) { + pid_t pid = getpid(); char buf[LOGERR_SYSLOGBUF]; - pid_t pid; - - memcpy(buf, &pri, sizeof(pri)); - pid = getpid(); - memcpy(buf + sizeof(pri), &pid, sizeof(pid)); - len = vsnprintf(buf + sizeof(pri) + sizeof(pid), - sizeof(buf) - sizeof(pri) - sizeof(pid), - fmt, args); - if (len != -1) - len = (int)write(ctx->log_fd, buf, - ((size_t)++len) + sizeof(pri) + sizeof(pid)); + struct iovec iov[] = { + { .iov_base = &pri, .iov_len = sizeof(pri) }, + { .iov_base = &pid, .iov_len = sizeof(pid) }, + { .iov_base = buf }, + }; + + len = vsnprintf(buf, sizeof(buf), fmt, args); + if (len != -1) { + if ((size_t)len >= sizeof(buf)) + len = (int)sizeof(buf) - 1; + iov[2].iov_len = (size_t)(len + 1); + struct msghdr msg = { + .msg_iov = iov, + .msg_iovlen = sizeof(iov) / sizeof(iov[0]), + }; + len = (int)sendmsg(ctx->log_fd, &msg, MSG_EOR); + } return len; } @@ -390,24 +399,33 @@ int logreadfd(int fd) { struct logctx *ctx = &_logctx; - char buf[LOGERR_SYSLOGBUF]; int len, pri; - - len = (int)read(fd, buf, sizeof(buf)); - if (len == -1) + pid_t pid; + char buf[LOGERR_SYSLOGBUF] = { '\0' }; + struct iovec iov[] = { + { .iov_base = &pri, .iov_len = sizeof(pri) }, + { .iov_base = &pid, .iov_len = sizeof(pid) }, + { .iov_base = buf, .iov_len = sizeof(buf) }, + }; + struct msghdr msg = { + .msg_iov = iov, + .msg_iovlen = sizeof(iov) / sizeof(iov[0]) + }; + + len = (int)recvmsg(fd, &msg, MSG_WAITALL); + if (len == -1 || len == 0) return -1; - - /* Ensure we have pri, pid and a terminator */ - if (len < (int)(sizeof(pri) + sizeof(pid_t) + 1) || - buf[len - 1] != '\0') - { - errno = EINVAL; + /* Ensure we received the minimum and at least one character to log */ + if ((size_t)len < sizeof(pri) + sizeof(pid) + 1 || + msg.msg_flags & MSG_TRUNC) { + errno = EMSGSIZE; return -1; } + /* Ensure what we receive is NUL terminated */ + buf[(size_t)len - (sizeof(pri) + sizeof(pid)) - 1] = '\0'; - memcpy(&pri, buf, sizeof(pri)); - memcpy(&ctx->log_pid, buf + sizeof(pri), sizeof(ctx->log_pid)); - logmessage(pri, "%s", buf + sizeof(pri) + sizeof(ctx->log_pid)); + ctx->log_pid = pid; + logmessage(pri, "%s", buf); ctx->log_pid = 0; return len; } diff --git a/src/logerr.h b/src/logerr.h index 08c3c133..a6507c26 100644 --- a/src/logerr.h +++ b/src/logerr.h @@ -76,7 +76,7 @@ __printflike(2, 3) void logerrmessage(int pri, const char *fmt, ...); #define logerr(...) log_err(__VA_ARGS__) #define logerrx(...) log_errx(__VA_ARGS__) -/* For logging in a chroot */ +/* For logging in a chroot using SOCK_SEQPACKET */ int loggetfd(void); void logsetfd(int); int logreadfd(int); diff --git a/src/privsep-root.c b/src/privsep-root.c index 4e6b9538..35dec006 100644 --- a/src/privsep-root.c +++ b/src/privsep-root.c @@ -210,6 +210,7 @@ ps_root_writeerror(struct dhcpcd_ctx *ctx, ssize_t result, { .iov_base = &psr, .iov_len = sizeof(psr) }, { .iov_base = data, .iov_len = len }, }; + struct msghdr msg = { .msg_iov = iov, .msg_iovlen = __arraycount(iov) }; ssize_t err; int fd = PS_ROOT_FD(ctx); @@ -217,7 +218,7 @@ ps_root_writeerror(struct dhcpcd_ctx *ctx, ssize_t result, logdebugx("%s: result %zd errno %d", __func__, result, errno); #endif - err = writev(fd, iov, __arraycount(iov)); + err = sendmsg(fd, &msg, MSG_EOR); /* Error sending the message? Try sending the error of sending. */ if (err == -1) { @@ -227,7 +228,7 @@ ps_root_writeerror(struct dhcpcd_ctx *ctx, ssize_t result, psr.psr_errno = errno; iov[1].iov_base = NULL; iov[1].iov_len = 0; - err = writev(fd, iov, __arraycount(iov)); + err = sendmsg(fd, &msg, MSG_EOR); } return err; diff --git a/src/privsep.c b/src/privsep.c index 7b6fbbbc..380a4f62 100644 --- a/src/privsep.c +++ b/src/privsep.c @@ -895,7 +895,7 @@ ps_sendpsmmsg(struct dhcpcd_ctx *ctx, int fd, { .iov_base = NULL, }, /* payload 2 */ { .iov_base = NULL, }, /* payload 3 */ }; - int iovlen; + struct msghdr m = { .msg_iov = iov, .msg_iovlen = 1 }; ssize_t len; if (msg != NULL) { @@ -909,6 +909,7 @@ ps_sendpsmmsg(struct dhcpcd_ctx *ctx, int fd, iovp->iov_base = msg->msg_name; iovp->iov_len = msg->msg_namelen; iovp++; + m.msg_iovlen++; cmsg_padlen = CALC_CMSG_PADLEN(msg->msg_controllen, msg->msg_namelen); @@ -916,25 +917,26 @@ ps_sendpsmmsg(struct dhcpcd_ctx *ctx, int fd, iovp->iov_len = cmsg_padlen; iovp->iov_base = cmsg_padlen != 0 ? padding : NULL; iovp++; + m.msg_iovlen++; iovp->iov_base = msg->msg_control; iovp->iov_len = msg->msg_controllen; - iovlen = 4; + iovp++; + m.msg_iovlen++; for (i = 0; i < (int)msg->msg_iovlen; i++) { - if ((size_t)(iovlen + i) > __arraycount(iov)) { + if ((size_t)(m.msg_iovlen++) > __arraycount(iov)) { errno = ENOBUFS; return -1; } - iovp++; iovp->iov_base = msg->msg_iov[i].iov_base; iovp->iov_len = msg->msg_iov[i].iov_len; + iovp++; } - iovlen += i; - } else - iovlen = 1; + } + + len = sendmsg(fd, &m, MSG_EOR); - len = writev(fd, iov, iovlen); if (len == -1) { if (ctx->options & DHCPCD_FORKED && !(ctx->options & DHCPCD_PRIVSEPROOT)) @@ -1028,6 +1030,7 @@ ps_sendcmdmsg(int fd, uint16_t cmd, const struct msghdr *msg) { .iov_base = &psm, .iov_len = sizeof(psm) }, { .iov_base = data, .iov_len = 0 }, }; + struct msghdr m = { .msg_iov = iov, .msg_iovlen = __arraycount(iov) }; size_t dl = sizeof(data); socklen_t cmsg_padlen = CALC_CMSG_PADLEN(msg->msg_controllen, msg->msg_namelen); @@ -1063,7 +1066,8 @@ ps_sendcmdmsg(int fd, uint16_t cmd, const struct msghdr *msg) psm.ps_namelen + psm.ps_controllen + psm.ps_datalen + cmsg_padlen; if (psm.ps_datalen != 0) memcpy(p, msg->msg_iov[0].iov_base, psm.ps_datalen); - return writev(fd, iov, __arraycount(iov)); + + return sendmsg(fd, &m, MSG_EOR); nobufs: errno = ENOBUFS; @@ -1089,7 +1093,7 @@ ps_recvmsg(int rfd, unsigned short events, uint16_t cmd, int wfd) if (!(events & ELE_READ)) logerrx("%s: unexpected event 0x%04x", __func__, events); - len = recvmsg(rfd, &msg, 0); + len = recvmsg(rfd, &msg, MSG_WAITALL); if (len == -1) { logerr("%s: recvmsg", __func__); return len;