From patchwork Wed Jan 27 20:33:17 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kamal Mostafa X-Patchwork-Id: 574255 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from huckleberry.canonical.com (huckleberry.canonical.com [91.189.94.19]) by ozlabs.org (Postfix) with ESMTP id 533DD140B8C; Thu, 28 Jan 2016 07:40:26 +1100 (AEDT) Received: from localhost ([127.0.0.1] helo=huckleberry.canonical.com) by huckleberry.canonical.com with esmtp (Exim 4.76) (envelope-from ) id 1aOWt5-0007Jd-6K; Wed, 27 Jan 2016 20:40:23 +0000 Received: from youngberry.canonical.com ([91.189.89.112]) by huckleberry.canonical.com with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.76) (envelope-from ) id 1aOWrL-0006Ew-Jj for kernel-team@lists.ubuntu.com; Wed, 27 Jan 2016 20:38:35 +0000 Received: from 1.general.kamal.us.vpn ([10.172.68.52] helo=fourier) by youngberry.canonical.com with esmtpsa (TLS1.0:DHE_RSA_AES_128_CBC_SHA1:16) (Exim 4.76) (envelope-from ) id 1aOWrL-0003YT-7P; Wed, 27 Jan 2016 20:38:35 +0000 Received: from kamal by fourier with local (Exim 4.82) (envelope-from ) id 1aOWrI-0004rM-GR; Wed, 27 Jan 2016 12:38:32 -0800 From: Kamal Mostafa To: linux-kernel@vger.kernel.org, stable@vger.kernel.org, kernel-team@lists.ubuntu.com Subject: [PATCH 4.2.y-ckt 136/268] ALSA: timer: Harden slave timer list handling Date: Wed, 27 Jan 2016 12:33:17 -0800 Message-Id: <1453926929-17663-137-git-send-email-kamal@canonical.com> X-Mailer: git-send-email 1.9.1 In-Reply-To: <1453926929-17663-1-git-send-email-kamal@canonical.com> References: <1453926929-17663-1-git-send-email-kamal@canonical.com> X-Extended-Stable: 4.2 Cc: Takashi Iwai , Kamal Mostafa X-BeenThere: kernel-team@lists.ubuntu.com X-Mailman-Version: 2.1.14 Precedence: list List-Id: Kernel team discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: kernel-team-bounces@lists.ubuntu.com Sender: kernel-team-bounces@lists.ubuntu.com 4.2.8-ckt3 -stable review patch. If anyone has any objections, please let me know. ---8<------------------------------------------------------------ From: Takashi Iwai commit b5a663aa426f4884c71cd8580adae73f33570f0d upstream. A slave timer instance might be still accessible in a racy way while operating the master instance as it lacks of locking. Since the master operation is mostly protected with timer->lock, we should cope with it while changing the slave instance, too. Also, some linked lists (active_list and ack_list) of slave instances aren't unlinked immediately at stopping or closing, and this may lead to unexpected accesses. This patch tries to address these issues. It adds spin lock of timer->lock (either from master or slave, which is equivalent) in a few places. For avoiding a deadlock, we ensure that the global slave_active_lock is always locked at first before each timer lock. Also, ack and active_list of slave instances are properly unlinked at snd_timer_stop() and snd_timer_close(). Last but not least, remove the superfluous call of _snd_timer_stop() at removing slave links. This is a noop, and calling it may confuse readers wrt locking. Further cleanup will follow in a later patch. Actually we've got reports of use-after-free by syzkaller fuzzer, and this hopefully fixes these issues. Reported-by: Dmitry Vyukov Signed-off-by: Takashi Iwai Signed-off-by: Kamal Mostafa --- sound/core/timer.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/sound/core/timer.c b/sound/core/timer.c index 3810ee8..4e8d7bf 100644 --- a/sound/core/timer.c +++ b/sound/core/timer.c @@ -215,11 +215,13 @@ static void snd_timer_check_master(struct snd_timer_instance *master) slave->slave_id == master->slave_id) { list_move_tail(&slave->open_list, &master->slave_list_head); spin_lock_irq(&slave_active_lock); + spin_lock(&master->timer->lock); slave->master = master; slave->timer = master->timer; if (slave->flags & SNDRV_TIMER_IFLG_RUNNING) list_add_tail(&slave->active_list, &master->slave_active_head); + spin_unlock(&master->timer->lock); spin_unlock_irq(&slave_active_lock); } } @@ -346,15 +348,18 @@ int snd_timer_close(struct snd_timer_instance *timeri) timer->hw.close) timer->hw.close(timer); /* remove slave links */ + spin_lock_irq(&slave_active_lock); + spin_lock(&timer->lock); list_for_each_entry_safe(slave, tmp, &timeri->slave_list_head, open_list) { - spin_lock_irq(&slave_active_lock); - _snd_timer_stop(slave, 1, SNDRV_TIMER_EVENT_RESOLUTION); list_move_tail(&slave->open_list, &snd_timer_slave_list); slave->master = NULL; slave->timer = NULL; - spin_unlock_irq(&slave_active_lock); + list_del_init(&slave->ack_list); + list_del_init(&slave->active_list); } + spin_unlock(&timer->lock); + spin_unlock_irq(&slave_active_lock); mutex_unlock(®ister_mutex); } out: @@ -441,9 +446,12 @@ static int snd_timer_start_slave(struct snd_timer_instance *timeri) spin_lock_irqsave(&slave_active_lock, flags); timeri->flags |= SNDRV_TIMER_IFLG_RUNNING; - if (timeri->master) + if (timeri->master && timeri->timer) { + spin_lock(&timeri->timer->lock); list_add_tail(&timeri->active_list, &timeri->master->slave_active_head); + spin_unlock(&timeri->timer->lock); + } spin_unlock_irqrestore(&slave_active_lock, flags); return 1; /* delayed start */ } @@ -489,6 +497,8 @@ static int _snd_timer_stop(struct snd_timer_instance * timeri, if (!keep_flag) { spin_lock_irqsave(&slave_active_lock, flags); timeri->flags &= ~SNDRV_TIMER_IFLG_RUNNING; + list_del_init(&timeri->ack_list); + list_del_init(&timeri->active_list); spin_unlock_irqrestore(&slave_active_lock, flags); } goto __end;