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 <roy@marples.name>

Author: glebius

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 @@ main(int argc, char **argv, char **envp)
 #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
 

src/logerr.c

@@ -26,7 +26,9 @@
  * SUCH DAMAGE.
  */
 
+#include <sys/socket.h>
 #include <sys/time.h>
+
 #include <errno.h>
 #include <stdbool.h>
 #include <stdarg.h>
@@ -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;
 }

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

src/privsep-root.c

@@ -210,14 +210,15 @@ 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);
 
 #ifdef PRIVSEP_DEBUG
 	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;

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,32 +909,34 @@ 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);
 		assert(cmsg_padlen <= sizeof(padding));
 		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;