Skip site navigation (1)Skip section navigation (2)
Date:      Tue, 20 Aug 2019 17:52:13 +0000 (UTC)
From:      Mark Johnston <markj@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   svn commit: r351262 - head/sys/dev/sound/midi
Message-ID:  <201908201752.x7KHqDN6066292@repo.freebsd.org>

next in thread | raw e-mail | index | archive | help
Author: markj
Date: Tue Aug 20 17:52:12 2019
New Revision: 351262
URL: https://svnweb.freebsd.org/changeset/base/351262

Log:
  Use a sleepable lock for midistat functions.
  
  Otherwise the mutex needs to be dropped when copying out the midistat
  sbuf, leading to a race which allows one to read kernel memory beyond
  the end of the sbuf buffer.
  
  Reported and tested by:	pho
  Security:	CVE-2019-5612

Modified:
  head/sys/dev/sound/midi/midi.c

Modified: head/sys/dev/sound/midi/midi.c
==============================================================================
--- head/sys/dev/sound/midi/midi.c	Tue Aug 20 17:51:32 2019	(r351261)
+++ head/sys/dev/sound/midi/midi.c	Tue Aug 20 17:52:12 2019	(r351262)
@@ -40,6 +40,7 @@
 __FBSDID("$FreeBSD$");
 
 #include <sys/param.h>
+#include <sys/systm.h>
 #include <sys/queue.h>
 #include <sys/kernel.h>
 #include <sys/lock.h>
@@ -49,10 +50,8 @@ __FBSDID("$FreeBSD$");
 #include <sys/conf.h>
 #include <sys/selinfo.h>
 #include <sys/sysctl.h>
-#include <sys/types.h>
 #include <sys/malloc.h>
-#include <sys/param.h>
-#include <sys/systm.h>
+#include <sys/sx.h>
 #include <sys/proc.h>
 #include <sys/fcntl.h>
 #include <sys/types.h>
@@ -187,10 +186,9 @@ TAILQ_HEAD(, snd_midi) midi_devs;
  * /dev/midistat variables and declarations, protected by midistat_lock
  */
 
-static struct mtx midistat_lock;
+static struct sx midistat_lock;
 static int      midistat_isopen = 0;
 static struct sbuf midistat_sbuf;
-static int      midistat_bufptr;
 static struct cdev *midistat_dev;
 
 /*
@@ -289,7 +287,7 @@ midi_init(kobj_class_t cls, int unit, int channel, voi
 	MIDI_TYPE *buf;
 
 	MIDI_DEBUG(1, printf("midiinit: unit %d/%d.\n", unit, channel));
-	mtx_lock(&midistat_lock);
+	sx_xlock(&midistat_lock);
 	/*
 	 * Protect against call with existing unit/channel or auto-allocate a
 	 * new unit number.
@@ -316,13 +314,8 @@ midi_init(kobj_class_t cls, int unit, int channel, voi
 		unit = i + 1;
 
 	MIDI_DEBUG(1, printf("midiinit #2: unit %d/%d.\n", unit, channel));
-	m = malloc(sizeof(*m), M_MIDI, M_NOWAIT | M_ZERO);
-	if (m == NULL)
-		goto err0;
-
-	m->synth = malloc(sizeof(*m->synth), M_MIDI, M_NOWAIT | M_ZERO);
-	if (m->synth == NULL)
-		goto err1;
+	m = malloc(sizeof(*m), M_MIDI, M_WAITOK | M_ZERO);
+	m->synth = malloc(sizeof(*m->synth), M_MIDI, M_WAITOK | M_ZERO);
 	kobj_init((kobj_t)m->synth, &midisynth_class);
 	m->synth->m = m;
 	kobj_init((kobj_t)m, cls);
@@ -331,7 +324,7 @@ midi_init(kobj_class_t cls, int unit, int channel, voi
 
 	MIDI_DEBUG(1, printf("midiinit queues %d/%d.\n", inqsize, outqsize));
 	if (!inqsize && !outqsize)
-		goto err2;
+		goto err1;
 
 	mtx_init(&m->lock, "raw midi", NULL, 0);
 	mtx_init(&m->qlock, "q raw midi", NULL, 0);
@@ -356,9 +349,8 @@ midi_init(kobj_class_t cls, int unit, int channel, voi
 
 	if ((inqsize && !MIDIQ_BUF(m->inq)) ||
 	    (outqsize && !MIDIQ_BUF(m->outq)))
-		goto err3;
+		goto err2;
 
-
 	m->busy = 0;
 	m->flags = 0;
 	m->unit = unit;
@@ -366,14 +358,14 @@ midi_init(kobj_class_t cls, int unit, int channel, voi
 	m->cookie = cookie;
 
 	if (MPU_INIT(m, cookie))
-		goto err3;
+		goto err2;
 
 	mtx_unlock(&m->lock);
 	mtx_unlock(&m->qlock);
 
 	TAILQ_INSERT_TAIL(&midi_devs, m, link);
 
-	mtx_unlock(&midistat_lock);
+	sx_xunlock(&midistat_lock);
 
 	m->dev = make_dev(&midi_cdevsw,
 	    MIDIMKMINOR(unit, MIDI_DEV_RAW, channel),
@@ -382,16 +374,19 @@ midi_init(kobj_class_t cls, int unit, int channel, voi
 
 	return m;
 
-err3:	mtx_destroy(&m->qlock);
+err2:
+	mtx_destroy(&m->qlock);
 	mtx_destroy(&m->lock);
 
 	if (MIDIQ_BUF(m->inq))
 		free(MIDIQ_BUF(m->inq), M_MIDI);
 	if (MIDIQ_BUF(m->outq))
 		free(MIDIQ_BUF(m->outq), M_MIDI);
-err2:	free(m->synth, M_MIDI);
-err1:	free(m, M_MIDI);
-err0:	mtx_unlock(&midistat_lock);
+err1:
+	free(m->synth, M_MIDI);
+	free(m, M_MIDI);
+err0:
+	sx_xunlock(&midistat_lock);
 	MIDI_DEBUG(1, printf("midi_init ended in error\n"));
 	return NULL;
 }
@@ -409,7 +404,7 @@ midi_uninit(struct snd_midi *m)
 	int err;
 
 	err = EBUSY;
-	mtx_lock(&midistat_lock);
+	sx_xlock(&midistat_lock);
 	mtx_lock(&m->lock);
 	if (m->busy) {
 		if (!(m->rchan || m->wchan))
@@ -428,8 +423,10 @@ midi_uninit(struct snd_midi *m)
 	if (!err)
 		goto exit;
 
-err:	mtx_unlock(&m->lock);
-exit:	mtx_unlock(&midistat_lock);
+err:
+	mtx_unlock(&m->lock);
+exit:
+	sx_xunlock(&midistat_lock);
 	return err;
 }
 
@@ -941,27 +938,22 @@ midistat_open(struct cdev *i_dev, int flags, int mode,
 	int error;
 
 	MIDI_DEBUG(1, printf("midistat_open\n"));
-	mtx_lock(&midistat_lock);
 
+	sx_xlock(&midistat_lock);
 	if (midistat_isopen) {
-		mtx_unlock(&midistat_lock);
+		sx_xunlock(&midistat_lock);
 		return EBUSY;
 	}
 	midistat_isopen = 1;
-	mtx_unlock(&midistat_lock);
-
 	if (sbuf_new(&midistat_sbuf, NULL, 4096, SBUF_AUTOEXTEND) == NULL) {
 		error = ENXIO;
-		mtx_lock(&midistat_lock);
 		goto out;
 	}
-	mtx_lock(&midistat_lock);
-	midistat_bufptr = 0;
 	error = (midistat_prepare(&midistat_sbuf) > 0) ? 0 : ENOMEM;
-
-out:	if (error)
+out:
+	if (error)
 		midistat_isopen = 0;
-	mtx_unlock(&midistat_lock);
+	sx_xunlock(&midistat_lock);
 	return error;
 }
 
@@ -969,40 +961,40 @@ static int
 midistat_close(struct cdev *i_dev, int flags, int mode, struct thread *td)
 {
 	MIDI_DEBUG(1, printf("midistat_close\n"));
-	mtx_lock(&midistat_lock);
+	sx_xlock(&midistat_lock);
 	if (!midistat_isopen) {
-		mtx_unlock(&midistat_lock);
+		sx_xunlock(&midistat_lock);
 		return EBADF;
 	}
 	sbuf_delete(&midistat_sbuf);
 	midistat_isopen = 0;
-
-	mtx_unlock(&midistat_lock);
+	sx_xunlock(&midistat_lock);
 	return 0;
 }
 
 static int
-midistat_read(struct cdev *i_dev, struct uio *buf, int flag)
+midistat_read(struct cdev *i_dev, struct uio *uio, int flag)
 {
-	int l, err;
+	long l;
+	int err;
 
 	MIDI_DEBUG(4, printf("midistat_read\n"));
-	mtx_lock(&midistat_lock);
+	sx_xlock(&midistat_lock);
 	if (!midistat_isopen) {
-		mtx_unlock(&midistat_lock);
+		sx_xunlock(&midistat_lock);
 		return EBADF;
 	}
-	l = min(buf->uio_resid, sbuf_len(&midistat_sbuf) - midistat_bufptr);
+	if (uio->uio_offset < 0 || uio->uio_offset > sbuf_len(&midistat_sbuf)) {
+		sx_xunlock(&midistat_lock);
+		return EINVAL;
+	}
 	err = 0;
+	l = lmin(uio->uio_resid, sbuf_len(&midistat_sbuf) - uio->uio_offset);
 	if (l > 0) {
-		mtx_unlock(&midistat_lock);
-		err = uiomove(sbuf_data(&midistat_sbuf) + midistat_bufptr, l,
-		    buf);
-		mtx_lock(&midistat_lock);
-	} else
-		l = 0;
-	midistat_bufptr += l;
-	mtx_unlock(&midistat_lock);
+		err = uiomove(sbuf_data(&midistat_sbuf) + uio->uio_offset, l,
+		    uio);
+	}
+	sx_xunlock(&midistat_lock);
 	return err;
 }
 
@@ -1015,7 +1007,7 @@ midistat_prepare(struct sbuf *s)
 {
 	struct snd_midi *m;
 
-	mtx_assert(&midistat_lock, MA_OWNED);
+	sx_assert(&midistat_lock, SA_XLOCKED);
 
 	sbuf_printf(s, "FreeBSD Midi Driver (midi2)\n");
 	if (TAILQ_EMPTY(&midi_devs)) {
@@ -1378,8 +1370,7 @@ midisynth_bender(void *n, uint8_t chn, uint16_t val)
 static int
 midi_destroy(struct snd_midi *m, int midiuninit)
 {
-
-	mtx_assert(&midistat_lock, MA_OWNED);
+	sx_assert(&midistat_lock, SA_XLOCKED);
 	mtx_assert(&m->lock, MA_OWNED);
 
 	MIDI_DEBUG(3, printf("midi_destroy\n"));
@@ -1405,8 +1396,8 @@ midi_destroy(struct snd_midi *m, int midiuninit)
 static int
 midi_load(void)
 {
-	mtx_init(&midistat_lock, "midistat lock", NULL, 0);
-	TAILQ_INIT(&midi_devs);		/* Initialize the queue. */
+	sx_init(&midistat_lock, "midistat lock");
+	TAILQ_INIT(&midi_devs);
 
 	midistat_dev = make_dev(&midistat_cdevsw,
 	    MIDIMKMINOR(0, MIDI_DEV_MIDICTL, 0),
@@ -1423,7 +1414,7 @@ midi_unload(void)
 
 	MIDI_DEBUG(1, printf("midi_unload()\n"));
 	retval = EBUSY;
-	mtx_lock(&midistat_lock);
+	sx_xlock(&midistat_lock);
 	if (midistat_isopen)
 		goto exit0;
 
@@ -1436,20 +1427,19 @@ midi_unload(void)
 		if (retval)
 			goto exit1;
 	}
-
-	mtx_unlock(&midistat_lock);	/* XXX */
-
+	sx_xunlock(&midistat_lock);
 	destroy_dev(midistat_dev);
+
 	/*
 	 * Made it here then unload is complete
 	 */
-	mtx_destroy(&midistat_lock);
+	sx_destroy(&midistat_lock);
 	return 0;
 
 exit1:
 	mtx_unlock(&m->lock);
 exit0:
-	mtx_unlock(&midistat_lock);
+	sx_xunlock(&midistat_lock);
 	if (retval)
 		MIDI_DEBUG(2, printf("midi_unload: failed\n"));
 	return retval;
@@ -1498,13 +1488,11 @@ midimapper_open(void *arg1, void **cookie)
 	int retval = 0;
 	struct snd_midi *m;
 
-	mtx_lock(&midistat_lock);
-
+	sx_xlock(&midistat_lock);
 	TAILQ_FOREACH(m, &midi_devs, link) {
 		retval++;
 	}
-
-	mtx_unlock(&midistat_lock);
+	sx_xunlock(&midistat_lock);
 	return retval;
 }
 
@@ -1520,17 +1508,15 @@ midimapper_fetch_synth(void *arg, void *cookie, int un
 	struct snd_midi *m;
 	int retval = 0;
 
-	mtx_lock(&midistat_lock);
-
+	sx_xlock(&midistat_lock);
 	TAILQ_FOREACH(m, &midi_devs, link) {
 		if (unit == retval) {
-			mtx_unlock(&midistat_lock);
+			sx_xunlock(&midistat_lock);
 			return (kobj_t)m->synth;
 		}
 		retval++;
 	}
-
-	mtx_unlock(&midistat_lock);
+	sx_xunlock(&midistat_lock);
 	return NULL;
 }
 



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201908201752.x7KHqDN6066292>