Received-SPF: softfail (google.com: domain of transitioning owner-tech+m56633=emaste=freebsd.org@openbsd.org does not designate 2001:1900:2254:206a::19:2 as permitted sender) client-ip=2001:1900:2254:206a::19:2;
Date: Fri, 24 Mar 2017 16:56:46 +0100
From: Alexander Bluhm <alexander.bluhm@gmx.net>
To: tech@openbsd.org
Subject: sendsyslog file race
Message-ID: <20170324155646.GD6537@t430s.bluhm.invalid>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
User-Agent: Mutt/1.8.0 (2017-02-23)
Precedence: list
Sender: owner-tech@openbsd.org

Hi,

There is a race in dosendsyslog() which resulted in a crash on a
5.9 system.  sosend(syslogf->f_data, ...) was called with a NULL
pointer.  So syslogf is not NULL, f_data is NULL and f_count is 1.

The file structure is ref counted, but the global variable syslogf
is not protected.  So it may change during sleep and dosendsyslog()
possibly uses a different socket at each access.

My crash happend during a reboot when init(8) is killing syslogd(8)
and some sort of super daemon tries to restart it constantly.
Although this design is questionable, it helps finding kernel bugs :-)

Solution is to access syslogf ony once, use a local copy, and do
the ref counting there.

ok?

bluhm

Index: kern/subr_log.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/kern/subr_log.c,v
retrieving revision 1.48
diff -u -p -r1.48 subr_log.c
--- kern/subr_log.c	23 Jun 2016 15:41:42 -0000	1.48
+++ kern/subr_log.c	24 Mar 2017 15:31:49 -0000
@@ -409,14 +409,17 @@ dosendsyslog(struct proc *p, const char 
 	struct iovec *ktriov = NULL;
 	int iovlen;
 #endif
+	struct file *fp;
 	char pri[6], *kbuf;
 	struct iovec aiov;
 	struct uio auio;
 	size_t i, len;
 	int error;
 
-	if (syslogf)
-		FREF(syslogf);
+	/* Global variable syslogf may change during sleep, use local copy. */
+	fp = syslogf;
+	if (fp)
+		FREF(fp);
 	else if (!ISSET(flags, LOG_CONS))
 		return (ENOTCONN);
 	else {
@@ -467,8 +470,8 @@ dosendsyslog(struct proc *p, const char 
 #endif
 
 	len = auio.uio_resid;
-	if (syslogf) {
-		error = sosend(syslogf->f_data, NULL, &auio, NULL, NULL, 0);
+	if (fp) {
+		error = sosend(fp->f_data, NULL, &auio, NULL, NULL, 0);
 		if (error == 0)
 			len -= auio.uio_resid;
 	} else if (constty || cn_devvp) {
@@ -515,8 +518,8 @@ dosendsyslog(struct proc *p, const char 
 		free(ktriov, M_TEMP, iovlen);
 	}
 #endif
-	if (syslogf)
-		FRELE(syslogf, p);
+	if (fp)
+		FRELE(fp, p);
 	else
 		error = ENOTCONN;
 	return (error);