[alsa-devel] sound: use-after-free in snd_timer_interrupt

[Dmitry Vyukov]

On Wed, Jan 13, 2016 at 9:30 PM, Takashi Iwai <tiwai at suse.de> 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 at google.com> wrote:
>> > On Wed, Jan 13, 2016 at 8:05 PM, Takashi Iwai <tiwai at 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 at 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 at google.com>
>>
>>
>> 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.