Re: sound: use-after-free in snd_timer_interrupt

[Takashi Iwai]

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?


thanks,

Takashi

---
diff --git a/sound/core/timer.c b/sound/core/timer.c
index b03a9e489286..3810ee8f1205 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))