Re: sound: use-after-free in snd_timer_interrupt

[Takashi Iwai <tiwai@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 2120 bytes --]

On Wed, 20 Apr 2016 10:08:55 +0200,
Takashi Iwai wrote:
> 
> On Wed, 20 Apr 2016 09:56:04 +0200,
> Dmitry Vyukov wrote:
> > 
> > On Sun, Apr 3, 2016 at 8:33 AM, Takashi Iwai <tiwai@suse.de> wrote:
> > >> >> It is not easily reproducible. I've hit several times while running
> > >> >> fuzzer for a week. Here is one of the logs for the record:
> > >> >> https://gist.githubusercontent.com/dvyukov/c84798ee55721563ecb537c4d51dc9f5/raw/f00b865a85877656f13b41917f7321730f140d35/gistfile1.txt
> > >> >
> > >> > There are a few more fixes in sound/core/timer.c since 4.5, and they
> > >> > possibly already cover this.
> > >> >
> > >> > Please let me know if this is still seen on the upcoming 4.6-rc2.
> > >>
> > >> Hi Takashi,
> > >>
> > >> I've updated fuzzer to 05cf8077e54b20dddb756eaa26f3aeb5c38dd3cf (Apr
> > >> 1) yesterday. Let's see if it still happens.
> > >>
> > >> Out of curiosity, how was the bug found?
> > >
> > > Well, I'm not entirely sure whether they really cover.  It's just a
> > > hope, as these are patches to close some possible races :)
> > >
> > > 9984d1b5835ca29fc7025186a891ee7398d21cc7
> > >     ALSA: timer: Protect the whole snd_timer_close() with open race
> > > f65e0d299807d8a11812845c972493c3f9a18e10
> > >     ALSA: timer: Call notifier in the same spinlock
> > > 4a07083ed613644c96c34a7dd2853dc5d7c70902
> > >     ALSA: timer: Use mod_timer() for rearming the system timer
> > 
> > 
> > Hi Takashi,
> > 
> > I've hit it again on 806fdcce017dc98c4dbf8ed001750a0d7d2bb0af (Apr
> > 14), all 3 commits are already in my tree.
> > 
> > [  343.222218] ------------[ cut here ]------------
> > [  343.222218] WARNING: CPU: 3 PID: 7040 at kernel/time/hrtimer.c:837
> > hrtimer_forward+0x26a/0x3e0
> 
> This is a different warning.  The previous was use-after-free, and
> this is a warning about re-arming the queued hrtimer.
> Maybe there is a slightly remaining race about hrtimer_start() and the
> interrupt handler in snd-hrtimer.

Could you check whether two patches below help anything?
This should harden against the race between hrtimer callback and
another start/stop calls.


Takashi


[-- Attachment #2: 0001-ALSA-timer-Allow-backend-disabling-start-stop-from-h.patch --]
[-- Type: application/octet-stream, Size: 4254 bytes --]

>From 18dee7aebf62375a224025ee0ee287c36784aa6d Mon Sep 17 00:00:00 2001
From: Takashi Iwai <tiwai@suse.de>
Date: Wed, 20 Apr 2016 12:02:36 +0200
Subject: [PATCH 1/2] ALSA: timer: Allow backend disabling start/stop from
 handler

Some timer backend doesn't particularly like (re)start / stop calls
from its interrupt handler.  For example, hrtimer can't stop properly
with sync, and we still seem to have some open race.

This patch introduced a new flag, SNDRV_TIMER_HW_RET_CTRL, so that the
timer backend can specify whether snd_timer_interrupt() should call hw
start() and hw.stop() callbacks or not.  If the new flag is set,
snd_timer_interrupt() won't call hw.start() and hw.stop() callbacks
but return SNDRV_TIMER_RET_START and SNDRV_TIMER_RET_STOP,
respectively.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 include/sound/timer.h | 12 +++++++++++-
 sound/core/timer.c    | 24 ++++++++++++++++++------
 2 files changed, 29 insertions(+), 7 deletions(-)

diff --git a/include/sound/timer.h b/include/sound/timer.h
index c4d76ff056c6..6ca6ed4169da 100644
--- a/include/sound/timer.h
+++ b/include/sound/timer.h
@@ -37,6 +37,7 @@
 #define SNDRV_TIMER_HW_SLAVE	0x00000004	/* only slave timer (variable resolution) */
 #define SNDRV_TIMER_HW_FIRST	0x00000008	/* first tick can be incomplete */
 #define SNDRV_TIMER_HW_TASKLET	0x00000010	/* timer is called from tasklet */
+#define SNDRV_TIMER_HW_RET_CTRL	0x00000020	/* don't start/stop at irq handler */
 
 #define SNDRV_TIMER_IFLG_SLAVE	  0x00000001
 #define SNDRV_TIMER_IFLG_RUNNING  0x00000002
@@ -50,6 +51,15 @@
 #define SNDRV_TIMER_FLG_CHANGE	0x00000001
 #define SNDRV_TIMER_FLG_RESCHED	0x00000002	/* need reschedule */
 
+/* return value from snd_timer_interrupt();
+ * START and STOP are returned only when SNDRV_TIMER_HW_RET_CTRL is set
+ */
+enum {
+	SNDRV_TIMER_RET_NONE = 0,
+	SNDRV_TIMER_RET_START = 1,
+	SNDRV_TIMER_RET_STOP = 2,
+};
+
 struct snd_timer;
 
 struct snd_timer_hardware {
@@ -139,6 +149,6 @@ int snd_timer_stop(struct snd_timer_instance *timeri);
 int snd_timer_continue(struct snd_timer_instance *timeri);
 int snd_timer_pause(struct snd_timer_instance *timeri);
 
-void snd_timer_interrupt(struct snd_timer *timer, unsigned long ticks_left);
+int snd_timer_interrupt(struct snd_timer *timer, unsigned long ticks_left);
 
 #endif /* __SOUND_TIMER_H */
diff --git a/sound/core/timer.c b/sound/core/timer.c
index 6469bedda2f3..c653c409d74d 100644
--- a/sound/core/timer.c
+++ b/sound/core/timer.c
@@ -683,19 +683,20 @@ static void snd_timer_tasklet(unsigned long arg)
  * ticks_left is usually equal to timer->sticks.
  *
  */
-void snd_timer_interrupt(struct snd_timer * timer, unsigned long ticks_left)
+int snd_timer_interrupt(struct snd_timer *timer, unsigned long ticks_left)
 {
 	struct snd_timer_instance *ti, *ts, *tmp;
 	unsigned long resolution, ticks;
 	struct list_head *p, *ack_list_head;
 	unsigned long flags;
 	int use_tasklet = 0;
+	int ret = 0;
 
 	if (timer == NULL)
-		return;
+		return -ENODEV;
 
 	if (timer->card && timer->card->shutdown)
-		return;
+		return -ENODEV;
 
 	spin_lock_irqsave(&timer->lock, flags);
 
@@ -747,17 +748,26 @@ void snd_timer_interrupt(struct snd_timer * timer, unsigned long ticks_left)
 		snd_timer_reschedule(timer, timer->sticks);
 	if (timer->running) {
 		if (timer->hw.flags & SNDRV_TIMER_HW_STOP) {
-			timer->hw.stop(timer);
+			if (timer->hw.flags & SNDRV_TIMER_HW_RET_CTRL)
+				ret = SNDRV_TIMER_RET_STOP;
+			else
+				timer->hw.stop(timer);
 			timer->flags |= SNDRV_TIMER_FLG_CHANGE;
 		}
 		if (!(timer->hw.flags & SNDRV_TIMER_HW_AUTO) ||
 		    (timer->flags & SNDRV_TIMER_FLG_CHANGE)) {
 			/* restart timer */
 			timer->flags &= ~SNDRV_TIMER_FLG_CHANGE;
-			timer->hw.start(timer);
+			if (timer->hw.flags & SNDRV_TIMER_HW_RET_CTRL)
+				ret = SNDRV_TIMER_RET_START;
+			else
+				timer->hw.start(timer);
 		}
 	} else {
-		timer->hw.stop(timer);
+		if (timer->hw.flags & SNDRV_TIMER_HW_RET_CTRL)
+			ret = SNDRV_TIMER_RET_STOP;
+		else
+			timer->hw.stop(timer);
 	}
 
 	/* now process all fast callbacks */
@@ -785,6 +795,8 @@ void snd_timer_interrupt(struct snd_timer * timer, unsigned long ticks_left)
 
 	if (use_tasklet)
 		tasklet_schedule(&timer->task_queue);
+
+	return ret;
 }
 
 /*
-- 
2.8.1


[-- Attachment #3: 0002-ALSA-hrtimer-Use-manual-start-stop-in-callback.patch --]
[-- Type: application/octet-stream, Size: 3931 bytes --]

>From 360230be1a52e0f81470820cf2be4e7da50e25f8 Mon Sep 17 00:00:00 2001
From: Takashi Iwai <tiwai@suse.de>
Date: Wed, 20 Apr 2016 12:10:10 +0200
Subject: [PATCH 2/2] ALSA: hrtimer: Use manual start/stop in callback

With the new SNDRV_TIMER_HW_RET_CTRL flag, hrtimer can manage the
callback behavior more correctly.  Now it gets a return value from
snd_timer_interrupt() whether to reprogram or stop the timer, and it
can choose the right return value.

The biggest bonus by this change is that we can protect the whole
interrupt with a spinlock against start/stop calls from another
thread.  The patch adds a spinlock and converts the former atomic flag
into a normal bool flag.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/core/hrtimer.c | 51 ++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 38 insertions(+), 13 deletions(-)

diff --git a/sound/core/hrtimer.c b/sound/core/hrtimer.c
index 656d9a9032dc..8a1b51473fd5 100644
--- a/sound/core/hrtimer.c
+++ b/sound/core/hrtimer.c
@@ -38,7 +38,8 @@ static unsigned int resolution;
 struct snd_hrtimer {
 	struct snd_timer *timer;
 	struct hrtimer hrt;
-	atomic_t running;
+	spinlock_t lock;
+	bool running;
 };
 
 static enum hrtimer_restart snd_hrtimer_callback(struct hrtimer *hrt)
@@ -46,16 +47,33 @@ static enum hrtimer_restart snd_hrtimer_callback(struct hrtimer *hrt)
 	struct snd_hrtimer *stime = container_of(hrt, struct snd_hrtimer, hrt);
 	struct snd_timer *t = stime->timer;
 	unsigned long oruns;
+	int irq_ret;
+	enum hrtimer_restart ret = HRTIMER_NORESTART;
+	unsigned long flags;
 
-	if (!atomic_read(&stime->running))
-		return HRTIMER_NORESTART;
+	spin_lock_irqsave(&stime->lock, flags);
+	if (!stime->running)
+		goto unlock;
 
 	oruns = hrtimer_forward_now(hrt, ns_to_ktime(t->sticks * resolution));
-	snd_timer_interrupt(stime->timer, t->sticks * oruns);
+	irq_ret = snd_timer_interrupt(stime->timer, t->sticks * oruns);
+
+	switch (irq_ret) {
+	case SNDRV_TIMER_RET_START:
+		hrtimer_start(&stime->hrt, ns_to_ktime(t->sticks * resolution),
+			      HRTIMER_MODE_REL);
+		/* fallthru */
+	case SNDRV_TIMER_RET_NONE:
+		ret = HRTIMER_RESTART;
+		break;
+	default:
+		stime->running = false;
+		break; /* HRTIMER_NORESTART */
+	}
 
-	if (!atomic_read(&stime->running))
-		return HRTIMER_NORESTART;
-	return HRTIMER_RESTART;
+ unlock:
+	spin_unlock_irqrestore(&stime->lock, flags);
+	return  ret;
 }
 
 static int snd_hrtimer_open(struct snd_timer *t)
@@ -68,7 +86,8 @@ static int snd_hrtimer_open(struct snd_timer *t)
 	hrtimer_init(&stime->hrt, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
 	stime->timer = t;
 	stime->hrt.function = snd_hrtimer_callback;
-	atomic_set(&stime->running, 0);
+	spin_lock_init(&stime->lock);
+	stime->running = false;
 	t->private_data = stime;
 	return 0;
 }
@@ -88,25 +107,31 @@ static int snd_hrtimer_close(struct snd_timer *t)
 static int snd_hrtimer_start(struct snd_timer *t)
 {
 	struct snd_hrtimer *stime = t->private_data;
+	unsigned long flags;
 
-	atomic_set(&stime->running, 0);
-	hrtimer_try_to_cancel(&stime->hrt);
+	spin_lock_irqsave(&stime->lock, flags);
 	hrtimer_start(&stime->hrt, ns_to_ktime(t->sticks * resolution),
 		      HRTIMER_MODE_REL);
-	atomic_set(&stime->running, 1);
+	stime->running = true;
+	spin_unlock_irqrestore(&stime->lock, flags);
 	return 0;
 }
 
 static int snd_hrtimer_stop(struct snd_timer *t)
 {
 	struct snd_hrtimer *stime = t->private_data;
-	atomic_set(&stime->running, 0);
+	unsigned long flags;
+
+	spin_lock_irqsave(&stime->lock, flags);
 	hrtimer_try_to_cancel(&stime->hrt);
+	stime->running = false;
+	spin_unlock_irqrestore(&stime->lock, flags);
 	return 0;
 }
 
 static struct snd_timer_hardware hrtimer_hw = {
-	.flags =	SNDRV_TIMER_HW_AUTO | SNDRV_TIMER_HW_TASKLET,
+	.flags =	SNDRV_TIMER_HW_AUTO | SNDRV_TIMER_HW_TASKLET |
+			SNDRV_TIMER_HW_RET_CTRL,
 	.open =		snd_hrtimer_open,
 	.close =	snd_hrtimer_close,
 	.start =	snd_hrtimer_start,
-- 
2.8.1