* Re: sound: use-after-free in snd_timer_stop [not found] <CACT4Y+borJj9XYEtXzLUbH9gUipPi9TQaj_O8Sw3tNUvFODPZA@mail.gmail.com> @ 2016-01-12 14:05 ` Takashi Iwai 2016-01-12 14:16 ` Takashi Iwai 0 siblings, 1 reply; 4+ messages in thread From: Takashi Iwai @ 2016-01-12 14:05 UTC (permalink / raw) To: Dmitry Vyukov Cc: alsa-devel, Jie Yang, Mark Brown, Jaroslav Kysela, LKML, Eric Dumazet, Alexander Potapenko, Kostya Serebryany, syzkaller, Sasha Levin 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? thanks, Takashi --- diff --git a/sound/core/seq/seq_queue.c b/sound/core/seq/seq_queue.c index 7dfd0f429410..8ea17d0a4299 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... */ @@ -470,7 +472,9 @@ int snd_seq_queue_timer_close(int queueid) queue = queueptr(queueid); if (queue == NULL) return -EINVAL; + mutex_lock(&queue->timer_mutex); snd_seq_timer_close(queue); + mutex_unlock(&queue->timer_mutex); queuefree(queue); return result; } ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: sound: use-after-free in snd_timer_stop 2016-01-12 14:05 ` sound: use-after-free in snd_timer_stop Takashi Iwai @ 2016-01-12 14:16 ` Takashi Iwai 2016-01-12 16:46 ` Dmitry Vyukov 0 siblings, 1 reply; 4+ messages in thread From: Takashi Iwai @ 2016-01-12 14:16 UTC (permalink / raw) To: Dmitry Vyukov Cc: alsa-devel, Jie Yang, Mark Brown, Jaroslav Kysela, LKML, Eric Dumazet, Alexander Potapenko, Kostya Serebryany, syzkaller, Sasha Levin 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). Takashi --- 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... */ ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: sound: use-after-free in snd_timer_stop 2016-01-12 14:16 ` Takashi Iwai @ 2016-01-12 16:46 ` Dmitry Vyukov 2016-01-12 16:52 ` Takashi Iwai 0 siblings, 1 reply; 4+ messages in thread From: Dmitry Vyukov @ 2016-01-12 16:46 UTC (permalink / raw) To: Takashi Iwai Cc: alsa-devel, Jie Yang, Mark Brown, Jaroslav Kysela, LKML, Eric Dumazet, Alexander Potapenko, Kostya Serebryany, syzkaller, Sasha Levin On Tue, Jan 12, 2016 at 3:16 PM, Takashi Iwai <tiwai@suse.de> 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@google.com> > --- > 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... */ ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: sound: use-after-free in snd_timer_stop 2016-01-12 16:46 ` Dmitry Vyukov @ 2016-01-12 16:52 ` Takashi Iwai 0 siblings, 0 replies; 4+ messages in thread From: Takashi Iwai @ 2016-01-12 16:52 UTC (permalink / raw) To: Dmitry Vyukov Cc: alsa-devel, Jie Yang, Mark Brown, Jaroslav Kysela, LKML, Eric Dumazet, Alexander Potapenko, Kostya Serebryany, syzkaller, Sasha Levin On Tue, 12 Jan 2016 17:46:15 +0100, Dmitry Vyukov wrote: > > On Tue, Jan 12, 2016 at 3:16 PM, Takashi Iwai <tiwai@suse.de> 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@google.com> Good to hear. FWIW, below is the final patch I'm going to queue. Thanks! Takashi -- 8< -- From: Takashi Iwai <tiwai@suse.de> 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@google.com> Tested-by: Dmitry Vyukov <dvyukov@google.com> Cc: <stable@vger.kernel.org> Signed-off-by: Takashi Iwai <tiwai@suse.de> --- 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-01-12 16:52 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CACT4Y+borJj9XYEtXzLUbH9gUipPi9TQaj_O8Sw3tNUvFODPZA@mail.gmail.com>
2016-01-12 14:05 ` sound: use-after-free in snd_timer_stop Takashi Iwai
2016-01-12 14:16 ` Takashi Iwai
2016-01-12 16:46 ` Dmitry Vyukov
2016-01-12 16:52 ` Takashi Iwai
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®