On Wed, 13 Jan 2016 21:48:47 +0100, Dmitry Vyukov wrote: > > On Wed, Jan 13, 2016 at 9:30 PM, Takashi Iwai <tiwai@xxxxxxx> wrote: > > On Wed, 13 Jan 2016 20:41:30 +0100, > > Dmitry Vyukov wrote: > >> > >> On Wed, Jan 13, 2016 at 8:30 PM, Dmitry Vyukov <dvyukov@xxxxxxxxxx> 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> > >> > >> > >> I've re-tested the programs that I reported. But when I started the > >> fuzzer again I hit a similar use-after-free in snd_timer_interrupt: > > > > Is it the result with all patches, i.e. four patches (two for > > sequencer and two for timer)? > > Yes, with 4 recent patches. OK, then this might be a possible race at the current snd_timer_stop() implementation. There is no sync action there, so the ISR might be still alive after snd_timer_close() call. Or might be another race. This pattern looks a bit different, as it's involved with hrtimer. I'll take a look at it tomorrow. thanks, Takashi _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
