From: Alexander Bluhm <bluhm@openbsd.org>
Subject: Re: sysctl: unlock KERN_CONSBUF and KERN_MSGBUF
To: Vitaliy Makkoveev <mvs@openbsd.org>
Cc: tech@openbsd.org
Date: Tue, 6 May 2025 20:15:23 +0200

On Tue, May 06, 2025 at 08:46:11PM +0300, Vitaliy Makkoveev wrote:
> I don't understand you. The type of long `msg_bufs' is immutable, we set
> it only once during bootstrap for both `msgbufp' and `consbufp'.

Ah, sorry.  I missread the code.  Everything is fine.

OK bluhm@

> 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	6 May 2025 17:25:13 -0000
> @@ -574,10 +574,52 @@ 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 (newp)
> +			return (EPERM);
> +		if (oldp) {
> +			if ((hlen + mp->msg_bufs) > *oldlenp)
> +				return (ENOMEM);
> +		} else 
> +			return (0);
> +
> +		memset(&ump, 0, sizeof(ump));
> +		mtx_enter(&log_mtx);
> +		ump.msg_magic = mp->msg_magic;
> +		ump.msg_bufs = mp->msg_bufs;
> +		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 +710,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;