From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: [PATCH v2] ALSA: seq: 2nd attempt at fixing race creating a q Date: Tue, 15 Aug 2017 08:03:45 +0200 Message-ID: References: <20170811030734.37741-1-danielmentz@google.com> <20170814214601.3106-1-danielmentz@google.com> Mime-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mx1.suse.de (mx2.suse.de [195.135.220.15]) by alsa0.perex.cz (Postfix) with ESMTP id BA13E266AE1 for ; Tue, 15 Aug 2017 08:03:46 +0200 (CEST) In-Reply-To: <20170814214601.3106-1-danielmentz@google.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: Daniel Mentz Cc: alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org List-Id: alsa-devel@alsa-project.org 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