Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753136AbdHOGDt (ORCPT ); Tue, 15 Aug 2017 02:03:49 -0400 Received: from mx2.suse.de ([195.135.220.15]:57522 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753038AbdHOGDr (ORCPT ); Tue, 15 Aug 2017 02:03:47 -0400 Date: Tue, 15 Aug 2017 08:03:45 +0200 Message-ID: From: Takashi Iwai To: Daniel Mentz Cc: alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org, perex@perex.cz, stable@vger.kernel.org Subject: Re: [PATCH v2] ALSA: seq: 2nd attempt at fixing race creating a q In-Reply-To: <20170814214601.3106-1-danielmentz@google.com> References: <20170811030734.37741-1-danielmentz@google.com> <20170814214601.3106-1-danielmentz@google.com> 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/25.2 (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 Content-Length: 1666 Lines: 42 On Mon, 14 Aug 2017 23:46:01 +0200, Daniel Mentz wrote: > > commit 4842e98f26dd80be3623c4714a244ba52ea096a8 ("ALSA: seq: Fix race at > creating a queue") attempted to fix a race reported by syzkaller. That > fix has been described as follows: > > " > When a sequencer queue is created in snd_seq_queue_alloc(),it adds the > new queue element to the public list before referencing it. Thus the > queue might be deleted before the call of snd_seq_queue_use(), and it > results in the use-after-free error, as spotted by syzkaller. > > The fix is to reference the queue object at the right time. > " > > Even with that fix in place, syzkaller reported a use-after-free error. > It specifically pointed to the last instruction "return q->queue" in > snd_seq_queue_alloc(). The pointer q is being used after kfree() has > been called on it. > > It turned out that there is still a small window where a race can > happen. The window opens at > snd_seq_ioctl_create_queue()->snd_seq_queue_alloc()->queue_list_add() > and closes at > snd_seq_ioctl_create_queue()->queueptr()->snd_use_lock_use(). Between > these two calls, a different thread could delete the queue and possibly > re-create a different queue in the same location in queue_list. > > This change prevents this situation by calling snd_use_lock_use() from > snd_seq_queue_alloc() prior to calling queue_list_add(). It is then the > caller's responsibility to call snd_use_lock_free(&q->use_lock). > > Reported-by: Dmitry Vyukov > Cc: > Cc: Takashi Iwai > Signed-off-by: Daniel Mentz Applied now. Thanks! Takashi