On Wed, 13 Jan 2016 20:30:01 +0100,
Dmitry Vyukov wrote:
>
> On Wed, Jan 13, 2016 at 8:05 PM, Takashi Iwai <tiwai@xxxxxxx> 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@xxxxxxx> 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@xxxxxxxxxx>
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@xxxxxxx>
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@xxxxxxxxxx>
Tested-by: Dmitry Vyukov <dvyukov@xxxxxxxxxx>
Cc: <stable@xxxxxxxxxxxxxxx>
Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>
---
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))
--
2.7.0
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
