From: Vitaliy Makkoveev <mvs@openbsd.org>
Subject: Re: sysctl: unlock KERN_CONSBUF and KERN_MSGBUF
To: Alexander Bluhm <bluhm@openbsd.org>
Cc: tech@openbsd.org
Date: Mon, 5 May 2025 20:19:23 +0300

On Mon, May 05, 2025 at 06:02:47PM +0200, Alexander Bluhm wrote:
> On Sat, May 03, 2025 at 01:55:30AM +0300, Vitaliy Makkoveev wrote:
> > We only need to keep buffer's header consistent. Store it to local
> > variable and deliver data with two steps.
> 
> I think you have to cover the cases if oldp == NULL or newp == NULL.
> sysctl_rdstruct() has if (newp) return (EPERM); and if (oldp) error
> = copyout(sp, oldp, len);
> 

Thanks for pointing. I kept the error paths order as it was in
sysctl_rdstruct().

> > Do we really need to fill `ump' with nulls and do manually field to
> > field copy? msgbuf header is all longs, it wouldn't be padded.
> 
> I would write: memset(&ump, 0, sizeof(ump)); ump = *mp; Then the
> compiler can figure out what to do and we are safe from leaking
> memory to userland.  In this particular case all fields long should
> be safe, but setting to 0 is more robust.  Copy kernel stack to
> userland is a common pitfall.
> 

I prefer to do this copying manually as we do in other similar places.
The proposed "memset(); ump = *mp;" is not obvious to me and could be
not obvious to others. The memset() and manual copying is doubtless.

Index: sys/kern/kern_sysctl.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_sysctl.c,v
retrieving revision 1.466
diff -u -p -r1.466 kern_sysctl.c
--- sys/kern/kern_sysctl.c	4 May 2025 13:42:07 -0000	1.466
+++ sys/kern/kern_sysctl.c	5 May 2025 17:13:22 -0000
@@ -574,10 +574,51 @@ kern_sysctl(int *name, u_int namelen, vo
 		return (sysctl_rdstruct(oldp, oldlenp, newp,
 		    &mbs, sizeof(mbs)));
 	}
-	case KERN_MSGBUFSIZE:
-	case KERN_CONSBUFSIZE: {
-		struct msgbuf *mp;
-		mp = (name[0] == KERN_MSGBUFSIZE) ? msgbufp : consbufp;
+	case KERN_CONSBUF:
+		if ((error = suser(p)))
+			return (error);
+		/* FALLTHROUGH */
+	case KERN_MSGBUF: {
+		extern struct mutex log_mtx;
+		const size_t hlen = offsetof(struct msgbuf, msg_bufc);
+		struct msgbuf ump, *mp = (name[0] == KERN_MSGBUF) ?
+		    msgbufp : consbufp;
+
+		/*
+		 * deal with cases where the message buffer has
+		 * become corrupted.
+		 */
+		if (!mp || mp->msg_magic != MSG_MAGIC)
+			return (ENXIO);
+		if (oldp && ((hlen + mp->msg_bufs) > *oldlenp))
+			return (ENOMEM);
+		if (newp)
+			return (EPERM);
+		if (oldp == NULL)
+			return (0);
+
+		memset(&ump, 0, sizeof(ump));
+		ump.msg_magic = mp->msg_magic;
+		ump.msg_bufs = mp->msg_bufs;
+		mtx_enter(&log_mtx);
+		ump.msg_bufx = mp->msg_bufx;
+		ump.msg_bufr = mp->msg_bufr;
+		ump.msg_bufd = mp->msg_bufd;
+		mtx_leave(&log_mtx);
+
+		/* copy header... */
+		if ((error = copyout(&ump, oldp, hlen)))
+			return (error);
+		/* ...and the data. */
+		error = copyout(mp->msg_bufc, oldp + hlen, mp->msg_bufs);
+
+		return (error);
+	}
+	case KERN_CONSBUFSIZE:
+	case KERN_MSGBUFSIZE: {
+		struct msgbuf *mp = (name[0] == KERN_MSGBUFSIZE) ?
+		    msgbufp : consbufp;
+
 		/*
 		 * deal with cases where the message buffer has
 		 * become corrupted.
@@ -668,22 +709,6 @@ kern_sysctl_locked(int *name, u_int name
 		if (newp && !error)
 			domainnamelen = newlen;
 		return (error);
-	case KERN_CONSBUF:
-		if ((error = suser(p)))
-			return (error);
-		/* FALLTHROUGH */
-	case KERN_MSGBUF: {
-		struct msgbuf *mp;
-		mp = (name[0] == KERN_MSGBUF) ? msgbufp : consbufp;
-		/*
-		 * deal with cases where the message buffer has
-		 * become corrupted.
-		 */
-		if (!mp || mp->msg_magic != MSG_MAGIC)
-			return (ENXIO);
-		return (sysctl_rdstruct(oldp, oldlenp, newp, mp,
-		    mp->msg_bufs + offsetof(struct msgbuf, msg_bufc)));
-	}
 	case KERN_CPTIME:
 	{
 		CPU_INFO_ITERATOR cii;