From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932683AbcAMTFr (ORCPT ); Wed, 13 Jan 2016 14:05:47 -0500 Received: from mx2.suse.de ([195.135.220.15]:56032 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932078AbcAMTFq (ORCPT ); Wed, 13 Jan 2016 14:05:46 -0500 Date: Wed, 13 Jan 2016 20:05:44 +0100 Message-ID: From: Takashi Iwai To: Dmitry Vyukov Cc: alsa-devel@alsa-project.org, Jie Yang , Mark Brown , Jaroslav Kysela , LKML , Eric Dumazet , Alexander Potapenko , Kostya Serebryany , syzkaller , Sasha Levin Subject: Re: sound: use-after-free in snd_timer_interrupt In-Reply-To: References: User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/24.5 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 13 Jan 2016 19:34:36 +0100, Dmitry Vyukov wrote: > > On Wed, Jan 13, 2016 at 5:53 PM, Takashi Iwai 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 > [] dump_stack+0x6f/0xa2 lib/dump_stack.c:50 > [] warn_slowpath_common+0xd9/0x140 kernel/panic.c:483 > [] warn_slowpath_fmt+0xa9/0xd0 kernel/panic.c:495 > [] __list_del_entry+0x10b/0x1e0 lib/list_debug.c:51 > [< inline >] list_del_init include/linux/list.h:145 > [] _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))