CVE-2016-2546: Concurrent Execution using Shared Resource with Improper Synchronization ('Race Condition') (CWE-362)
value: sound/core/timer.c in the Linux kernel before 4.4.1 uses an incorrect type of mutex, which allows local users to cause a denial of service (race condition, use-after-free, and system crash) via a crafted ioctl call.
CWE description: The product contains a code sequence that can run concurrently with other code, and the code sequence requires temporary, exclusive access to a shared resource, but a timing window exists in which the shared resource can be modified by another code sequence that is operating concurrently.
CVSS v3 base score: 5.1
Published: 2016-04-27T17:59Z
Repository: https://github.com/torvalds/linux
Language: C
Files changed: sound/core/timer.c
Commit date: 2016-01-14 14:07:21 +0100
Commit message: ALSA: timer: Fix race among timer ioctls

ALSA timer ioctls have an open race and this may lead to a
use-after-free of timer instance object.  A simplistic fix is to make
each ioctl exclusive.  We have already tread_sem for controlling the
tread, and extend this as a global mutex to be applied to each ioctl.

The downside is, of course, the worse concurrency.  But these ioctls
aren't to be parallel accessible, in anyway, so it should be fine to
serialize there.

Reported-by: Dmitry Vyukov <dvyukov@google.com>
Tested-by: Dmitry Vyukov <dvyukov@google.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>

Diff: diff --git a/sound/core/timer.c b/sound/core/timer.c
index 9241784dfe7dd..3810ee8f12051 100644
--- a/sound/core/timer.c
+++ b/sound/core/timer.c
@@ -71,11 +71,11 @@ struct snd_timer_user {
 	unsigned long last_resolution;
 	unsigned int filter;
 	struct timespec tstamp;		/* trigger tstamp */
 	wait_queue_head_t qchange_sleep;
 	struct fasync_struct *fasync;
-	struct mutex tread_sem;
+	struct mutex ioctl_lock;
 };
 
 /* list of timers */
 static LIST_HEAD(snd_timer_list);
 
@@ -1251,11 +1251,11 @@ static int snd_timer_user_open(struct inode *inode, struct file *file)
 	tu = kzalloc(sizeof(*tu), GFP_KERNEL);
 	if (tu == NULL)
 		return -ENOMEM;
 	spin_lock_init(&tu->qlock);
 	init_waitqueue_head(&tu->qchange_sleep);
-	mutex_init(&tu->tread_sem);
+	mutex_init(&tu->ioctl_lock);
 	tu->ticks = 1;
 	tu->queue_size = 128;
 	tu->queue = kmalloc(tu->queue_size * sizeof(struct snd_timer_read),
 			    GFP_KERNEL);
 	if (tu->queue == NULL) {
@@ -1271,12 +1271,14 @@ static int snd_timer_user_release(struct inode *inode, struct file *file)
 	struct snd_timer_user *tu;
 
 	if (file->private_data) {
 		tu = file->private_data;
 		file->private_data = NULL;
+		mutex_lock(&tu->ioctl_lock);
 		if (tu->timeri)
 			snd_timer_close(tu->timeri);
+		mutex_unlock(&tu->ioctl_lock);
 		kfree(tu->queue);
 		kfree(tu->tqueue);
 		kfree(tu);
 	}
 	return 0;
@@ -1510,11 +1512,10 @@ static int snd_timer_user_tselect(struct file *file,
 	struct snd_timer_select tselect;
 	char str[32];
 	int err = 0;
 
 	tu = file->private_data;
-	mutex_lock(&tu->tread_sem);
 	if (tu->timeri) {
 		snd_timer_close(tu->timeri);
 		tu->timeri = NULL;
 	}
 	if (copy_from_user(&tselect, _tselect, sizeof(tselect))) {
@@ -1554,11 +1555,10 @@ static int snd_timer_user_tselect(struct file *file,
 		tu->timeri->ccallback = snd_timer_user_ccallback;
 		tu->timeri->callback_data = (void *)tu;
 	}
 
       __err:
-      	mutex_unlock(&tu->tread_sem);
 	return err;
 }
 
 static int snd_timer_user_info(struct file *file,
 			       struct snd_timer_info __user *_info)
@@ -1767,11 +1767,11 @@ enum {
 	SNDRV_TIMER_IOCTL_STOP_OLD = _IO('T', 0x21),
 	SNDRV_TIMER_IOCTL_CONTINUE_OLD = _IO('T', 0x22),
 	SNDRV_TIMER_IOCTL_PAUSE_OLD = _IO('T', 0x23),
 };
 
-static long snd_timer_user_ioctl(struct file *file, unsigned int cmd,
+static long __snd_timer_user_ioctl(struct file *file, unsigned int cmd,
 				 unsigned long arg)
 {
 	struct snd_timer_user *tu;
 	void __user *argp = (void __user *)arg;
 	int __user *p = argp;
@@ -1784,21 +1784,15 @@ static long snd_timer_user_ioctl(struct file *file, unsigned int cmd,
 		return snd_timer_user_next_device(argp);
 	case SNDRV_TIMER_IOCTL_TREAD:
 	{
 		int xarg;
 
-		mutex_lock(&tu->tread_sem);
-		if (tu->timeri)	{	/* too late */
-			mutex_unlock(&tu->tread_sem);
+		if (tu->timeri)	/* too late */
 			return -EBUSY;
-		}
-		if (get_user(xarg, p)) {
-			mutex_unlock(&tu->tread_sem);
+		if (get_user(xarg, p))
 			return -EFAULT;
-		}
 		tu->tread = xarg ? 1 : 0;
-		mutex_unlock(&tu->tread_sem);
 		return 0;
 	}
 	case SNDRV_TIMER_IOCTL_GINFO:
 		return snd_timer_user_ginfo(file, argp);
 	case SNDRV_TIMER_IOCTL_GPARAMS:
@@ -1827,10 +1821,22 @@ static long snd_timer_user_ioctl(struct file *file, unsigned int cmd,
 		return snd_timer_user_pause(file);
 	}
 	return -ENOTTY;
 }
 
+static long snd_timer_user_ioctl(struct file *file, unsigned int cmd,
+				 unsigned long arg)
+{
+	struct snd_timer_user *tu = file->private_data;
+	long ret;
+
+	mutex_lock(&tu->ioctl_lock);
+	ret = __snd_timer_user_ioctl(file, cmd, arg);
+	mutex_unlock(&tu->ioctl_lock);
+	return ret;
+}
+
 static int snd_timer_user_fasync(int fd, struct file * file, int on)
 {
 	struct snd_timer_user *tu;
 
 	tu = file->private_data;

Vulnerable code: 	struct mutex tread_sem;
	mutex_init(&tu->tread_sem);
	mutex_lock(&tu->tread_sem);
      	mutex_unlock(&tu->tread_sem);
static long snd_timer_user_ioctl(struct file *file, unsigned int cmd,
		mutex_lock(&tu->tread_sem);
		if (tu->timeri)	{	/* too late */
			mutex_unlock(&tu->tread_sem);
		}
		if (get_user(xarg, p)) {
			mutex_unlock(&tu->tread_sem);
		}
		mutex_unlock(&tu->tread_sem);
Fixed code: 	struct mutex ioctl_lock;
	mutex_init(&tu->ioctl_lock);
		mutex_lock(&tu->ioctl_lock);
		mutex_unlock(&tu->ioctl_lock);
static long __snd_timer_user_ioctl(struct file *file, unsigned int cmd,
		if (tu->timeri)	/* too late */
		if (get_user(xarg, p))
static long snd_timer_user_ioctl(struct file *file, unsigned int cmd,
				 unsigned long arg)
{
	struct snd_timer_user *tu = file->private_data;
	long ret;

	mutex_lock(&tu->ioctl_lock);
	ret = __snd_timer_user_ioctl(file, cmd, arg);
	mutex_unlock(&tu->ioctl_lock);
	return ret;
}