Re: sound: use-after-free in snd_timer_stop

[Takashi Iwai]

On Tue, 12 Jan 2016 17:46:15 +0100,
Dmitry Vyukov wrote:
> 
> On Tue, Jan 12, 2016 at 3:16 PM, Takashi Iwai <tiwai@xxxxxxx> wrote:
> > On Tue, 12 Jan 2016 15:05:24 +0100,
> > Takashi Iwai wrote:
> >>
> >> On Tue, 12 Jan 2016 11:22:09 +0100,
> >> Dmitry Vyukov wrote:
> >> >
> >> > Hello,
> >> >
> >> > I've hit the following use-after-free while running syzkaller fuzzer.
> >> > It is followed by a splat of other reports and finally kernel death.
> >> > I wasn't able to reproduce it with a standalone C program (there is
> >> > probably some global state  involved). But it reproduces by replaying
> >> > fuzzer logs in a loop (you will need Go toolchain):
> >> >
> >> > $ go get github.com/google/syzkaller
> >> > $ cd $GOPATH/src/github.com/google/syzkaller
> >> > $ make executor execprog
> >>
> >> Just a note: I had to run "mkdir bin" beforehand.
> >> Also for my distro, I had to remove -static.
> >>
> >> > $ scp bin/syz-executor bin/syz-execprog your@machine
> >> > $ scp snd_timer_stop your@machine # the attached file
> >> > on test machine:
> >> > $ ./syz-execprog -executor ./syz-executor -cover=0 -repeat=0 -procs=16
> >> > snd_timer_stop
> >>
> >> I tried this but couldn't reproduce the issue on my machine,
> >> unfortunately.  (Instead I hit another kernel panic regarding apparmor
> >> :)
> >>
> >> But looking through your log, it seems like a missing race
> >> protection.  Does the patch below work for you?
> >
> > Gah, scratch this.  The second hunk causes a deadlock.
> > The revised patch is below (just containing the first hunk).
> 
> Yes, this patch fixes the crashes for me.
> Reproduction may require running syz-execprog for several minutes.
> 
> Tested-by: Dmitry Vyukov <dvyukov@xxxxxxxxxx>

Good to hear.  FWIW, below is the final patch I'm going to queue.
Thanks!


Takashi

-- 8< --
From: Takashi Iwai <tiwai@xxxxxxx>
Subject: [PATCH] ALSA: seq: Fix race at timer setup and close

ALSA sequencer code has an open race between the timer setup ioctl and
the close of the client.  This was triggered by syzkaller fuzzer, and
a use-after-free was caught there as a result.

This patch papers over it by adding a proper queue->timer_mutex lock
around the timer-related calls in the relevant code path.

Reported-by: Dmitry Vyukov <dvyukov@xxxxxxxxxx>
Tested-by: Dmitry Vyukov <dvyukov@xxxxxxxxxx>
Cc: <stable@xxxxxxxxxxxxxxx>
Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>
---
 sound/core/seq/seq_queue.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/sound/core/seq/seq_queue.c b/sound/core/seq/seq_queue.c
index 7dfd0f429410..0bec02e89d51 100644
--- a/sound/core/seq/seq_queue.c
+++ b/sound/core/seq/seq_queue.c
@@ -142,8 +142,10 @@ static struct snd_seq_queue *queue_new(int owner, int locked)
 static void queue_delete(struct snd_seq_queue *q)
 {
 	/* stop and release the timer */
+	mutex_lock(&q->timer_mutex);
 	snd_seq_timer_stop(q->timer);
 	snd_seq_timer_close(q);
+	mutex_unlock(&q->timer_mutex);
 	/* wait until access free */
 	snd_use_lock_sync(&q->use_lock);
 	/* release resources... */
-- 
2.7.0