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

> 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.

bluhm

> Index: sys/kern/kern_sysctl.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/kern_sysctl.c,v
> retrieving revision 1.465
> diff -u -p -r1.465 kern_sysctl.c
> --- sys/kern/kern_sysctl.c	27 Apr 2025 00:58:55 -0000	1.465
> +++ sys/kern/kern_sysctl.c	2 May 2025 22:47:32 -0000
> @@ -576,8 +576,9 @@ kern_sysctl(int *name, u_int namelen, vo
>  	}
>  	case KERN_MSGBUFSIZE:
>  	case KERN_CONSBUFSIZE: {
> -		struct msgbuf *mp;
> -		mp = (name[0] == KERN_MSGBUFSIZE) ? msgbufp : consbufp;
> +		struct msgbuf *mp = (name[0] == KERN_MSGBUFSIZE) ?
> +		    msgbufp : consbufp;
> +
>  		/*
>  		 * deal with cases where the message buffer has
>  		 * become corrupted.
> @@ -586,6 +587,38 @@ kern_sysctl(int *name, u_int namelen, vo
>  			return (ENXIO);
>  		return (sysctl_rdint(oldp, oldlenp, newp, mp->msg_bufs));
>  	}
> +	case KERN_CONSBUF:
> +		if ((error = suser(p)))
> +			return (error);
> +		/* FALLTHROUGH */
> +	case KERN_MSGBUF: {
> +		extern struct mutex log_mtx;
> +		struct msgbuf ump, *mp = (name[0] == KERN_MSGBUF) ?
> +		    msgbufp : consbufp;
> +		size_t hlen = offsetof(struct msgbuf, msg_bufc), tlen;
> +
> +		/*
> +		 * deal with cases where the message buffer has
> +		 * become corrupted.
> +		 */
> +		if (!mp || mp->msg_magic != MSG_MAGIC)
> +			return (ENXIO);
> +
> +		if ((tlen = hlen + mp->msg_bufs) > *oldlenp)
> +			return (ENOMEM);
> +
> +		mtx_enter(&log_mtx);
> +		memcpy(&ump, mp, hlen);
> +		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_TIMEOUT_STATS:
>  		return (timeout_sysctl(oldp, oldlenp, newp, newlen));
>  	case KERN_OSREV:
> @@ -656,22 +689,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;