CVE-2016-2548: Improper Input Validation (CWE-20)
value: sound/core/timer.c in the Linux kernel before 4.4.1 retains certain linked lists after a close or stop action, which allows local users to cause a denial of service (system crash) via a crafted ioctl call, related to the (1) snd_timer_close and (2) _snd_timer_stop functions.
CWE description: The product receives input or data, but it does
        not validate or incorrectly validates that the input has the
        properties that are required to process the data safely and
        correctly.
CVSS v3 base score: 6.2
Published: 2016-04-27T17:59Z
Repository: https://github.com/torvalds/linux
Language: C
Files changed: sound/core/timer.c
Commit date: 2016-01-15 15:50:34 +0100
Commit message: ALSA: timer: Harden slave timer list handling

A slave timer instance might be still accessible in a racy way while
operating the master instance as it lacks of locking.  Since the
master operation is mostly protected with timer->lock, we should cope
with it while changing the slave instance, too.  Also, some linked
lists (active_list and ack_list) of slave instances aren't unlinked
immediately at stopping or closing, and this may lead to unexpected
accesses.

This patch tries to address these issues.  It adds spin lock of
timer->lock (either from master or slave, which is equivalent) in a
few places.  For avoiding a deadlock, we ensure that the global
slave_active_lock is always locked at first before each timer lock.

Also, ack and active_list of slave instances are properly unlinked at
snd_timer_stop() and snd_timer_close().

Last but not least, remove the superfluous call of _snd_timer_stop()
at removing slave links.  This is a noop, and calling it may confuse
readers wrt locking.  Further cleanup will follow in a later patch.

Actually we've got reports of use-after-free by syzkaller fuzzer, and
this hopefully fixes these issues.

Reported-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 3810ee8f12051..4e8d7bfffff6b 100644
--- a/sound/core/timer.c
+++ b/sound/core/timer.c
@@ -213,15 +213,17 @@ static void snd_timer_check_master(struct snd_timer_instance *master)
 	list_for_each_entry_safe(slave, tmp, &snd_timer_slave_list, open_list) {
 		if (slave->slave_class == master->slave_class &&
 		    slave->slave_id == master->slave_id) {
 			list_move_tail(&slave->open_list, &master->slave_list_head);
 			spin_lock_irq(&slave_active_lock);
+			spin_lock(&master->timer->lock);
 			slave->master = master;
 			slave->timer = master->timer;
 			if (slave->flags & SNDRV_TIMER_IFLG_RUNNING)
 				list_add_tail(&slave->active_list,
 					      &master->slave_active_head);
+			spin_unlock(&master->timer->lock);
 			spin_unlock_irq(&slave_active_lock);
 		}
 	}
 }
 
@@ -344,19 +346,22 @@ int snd_timer_close(struct snd_timer_instance *timeri)
 		list_del(&timeri->open_list);
 		if (timer && list_empty(&timer->open_list_head) &&
 		    timer->hw.close)
 			timer->hw.close(timer);
 		/* remove slave links */
+		spin_lock_irq(&slave_active_lock);
+		spin_lock(&timer->lock);
 		list_for_each_entry_safe(slave, tmp, &timeri->slave_list_head,
 					 open_list) {
-			spin_lock_irq(&slave_active_lock);
-			_snd_timer_stop(slave, 1, SNDRV_TIMER_EVENT_RESOLUTION);
 			list_move_tail(&slave->open_list, &snd_timer_slave_list);
 			slave->master = NULL;
 			slave->timer = NULL;
-			spin_unlock_irq(&slave_active_lock);
+			list_del_init(&slave->ack_list);
+			list_del_init(&slave->active_list);
 		}
+		spin_unlock(&timer->lock);
+		spin_unlock_irq(&slave_active_lock);
 		mutex_unlock(&register_mutex);
 	}
  out:
 	if (timeri->private_free)
 		timeri->private_free(timeri);
@@ -439,13 +444,16 @@ static int snd_timer_start_slave(struct snd_timer_instance *timeri)
 {
 	unsigned long flags;
 
 	spin_lock_irqsave(&slave_active_lock, flags);
 	timeri->flags |= SNDRV_TIMER_IFLG_RUNNING;
-	if (timeri->master)
+	if (timeri->master && timeri->timer) {
+		spin_lock(&timeri->timer->lock);
 		list_add_tail(&timeri->active_list,
 			      &timeri->master->slave_active_head);
+		spin_unlock(&timeri->timer->lock);
+	}
 	spin_unlock_irqrestore(&slave_active_lock, flags);
 	return 1; /* delayed start */
 }
 
 /*
@@ -487,10 +495,12 @@ static int _snd_timer_stop(struct snd_timer_instance * timeri,
 
 	if (timeri->flags & SNDRV_TIMER_IFLG_SLAVE) {
 		if (!keep_flag) {
 			spin_lock_irqsave(&slave_active_lock, flags);
 			timeri->flags &= ~SNDRV_TIMER_IFLG_RUNNING;
+			list_del_init(&timeri->ack_list);
+			list_del_init(&timeri->active_list);
 			spin_unlock_irqrestore(&slave_active_lock, flags);
 		}
 		goto __end;
 	}
 	timer = timeri->timer;

Vulnerable code: 			spin_lock_irq(&slave_active_lock);
			_snd_timer_stop(slave, 1, SNDRV_TIMER_EVENT_RESOLUTION);
			spin_unlock_irq(&slave_active_lock);
	if (timeri->master)
Fixed code: 			spin_lock(&master->timer->lock);
			spin_unlock(&master->timer->lock);
		spin_lock_irq(&slave_active_lock);
		spin_lock(&timer->lock);
			list_del_init(&slave->ack_list);
			list_del_init(&slave->active_list);
		spin_unlock(&timer->lock);
		spin_unlock_irq(&slave_active_lock);
	if (timeri->master && timeri->timer) {
		spin_lock(&timeri->timer->lock);
		spin_unlock(&timeri->timer->lock);
	}
			list_del_init(&timeri->ack_list);
			list_del_init(&timeri->active_list);