On Wed, 13 Jan 2016 20:30:01 +0100, Dmitry Vyukov wrote:
On Wed, Jan 13, 2016 at 8:05 PM, Takashi Iwai tiwai@suse.de wrote:
On Wed, 13 Jan 2016 19:34:36 +0100, Dmitry Vyukov wrote:
On Wed, Jan 13, 2016 at 5:53 PM, Takashi Iwai tiwai@suse.de wrote:
This and your other relevant reports seem pointing the race of timer ioctls. Although snd_timer_close() itself calls snd_timer_stop(), there is no other protection against the concurrent execution.
If my guess is correct, a simplistic fix like below should work. It basically serializes the timer ioctl by using a new mutex (and replacing the old tread_sem mutex). They are no longtime blocking calls, so this shouldn't be a big problem. But certainly there can be a less intrusive way to paper over this if this really matters.
In this case for timer.c, I'd leave the final decision rather to Jaroslav. Jaroslav, what do you think?
After applying this patch I still see the following WARNINGS:
------------[ cut here ]------------ WARNING: CPU: 2 PID: 30398 at lib/list_debug.c:53 __list_del_entry+0x10b/0x1e0() list_del corruption, ffff880032d933b0->next is LIST_POISON1 (dead000000000100) Modules linked in: CPU: 2 PID: 30398 Comm: syz-executor Not tainted 4.4.0+ #241 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 00000000ffffffff ffff8800627778d8 ffffffff82926eed ffff880062777948 ffff880061c2af80 ffffffff8660b640 ffff880062777918 ffffffff81350c89 ffffffff8298e77b ffffed000c4eef25 ffffffff8660b640 0000000000000035 Call Trace: [< inline >] __dump_stack lib/dump_stack.c:15 [<ffffffff82926eed>] dump_stack+0x6f/0xa2 lib/dump_stack.c:50 [<ffffffff81350c89>] warn_slowpath_common+0xd9/0x140 kernel/panic.c:483 [<ffffffff81350d99>] warn_slowpath_fmt+0xa9/0xd0 kernel/panic.c:495 [<ffffffff8298e77b>] __list_del_entry+0x10b/0x1e0 lib/list_debug.c:51 [< inline >] list_del_init include/linux/list.h:145 [<ffffffff84ebd199>] _snd_timer_stop+0x119/0x450 sound/core/timer.c:501
This is
list_del_init(&timeri->active_list);right? Possibly the following oneliner covers it?
Yes, that is this line. Yes, these two patches fix use-after-frees and GPFs.
Tested-by: Dmitry Vyukov dvyukov@google.com
OK, I'm going to queue the list fix now, at least. Below is the patch with a proper description.
If there comes no objection, I'll queue the former ioctl mutex fix, too.
The only one that still happens is "sound: spinlock lockup in sound/core/timer.c".
Oh well, there is another :-<
thanks,
Takashi
-- 8< -- From: Takashi Iwai tiwai@suse.de Subject: [PATCH] ALSA: timer: Fix double unlink of active_list
ALSA timer instance object has a couple of linked lists and they are unlinked unconditionally at snd_timer_stop(). Meanwhile snd_timer_interrupt() unlinks it, but it calls list_del() which leaves the element list itself unchanged. This ends up with unlinking twice, and it was caught by syzkaller fuzzer.
The fix is to use list_del_init() variant properly there, too.
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/timer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/core/timer.c b/sound/core/timer.c index 31f40f03e5b7..9241784dfe7d 100644 --- a/sound/core/timer.c +++ b/sound/core/timer.c @@ -694,7 +694,7 @@ void snd_timer_interrupt(struct snd_timer * timer, unsigned long ticks_left) } else { ti->flags &= ~SNDRV_TIMER_IFLG_RUNNING; if (--timer->running) - list_del(&ti->active_list); + list_del_init(&ti->active_list); } if ((timer->hw.flags & SNDRV_TIMER_HW_TASKLET) || (ti->flags & SNDRV_TIMER_IFLG_FAST))