Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753392AbdHKOmx (ORCPT ); Fri, 11 Aug 2017 10:42:53 -0400 Received: from mx2.suse.de ([195.135.220.15]:35486 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752850AbdHKOmv (ORCPT ); Fri, 11 Aug 2017 10:42:51 -0400 Date: Fri, 11 Aug 2017 16:42:49 +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] ALSA: seq: 2nd attempt at fixing race creating a q In-Reply-To: <20170811030734.37741-1-danielmentz@google.com> References: <20170811030734.37741-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: 4650 Lines: 131 On Fri, 11 Aug 2017 05:07:34 +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. > " > > The last phrase in that commit message refers to calling queue_use(q, client, > 1) which increments queue->clients, but that does not prevent the > DELETE_QUEUE ioctl() and queue_delete() from kfree()ing the queue. > Hence, the commit is not effective at preventing the race. kfree() is performed only after snd_use_lock_sync(), thus by acquiring snd_use_lock() it doesn't race. If it were already deleted, queue_use() returns NULL so it's also fine. Or do you actually see any crash or the wild behavior? thanks, Takashi > > This commit adds code to effectively prevent the removal of the queue by > calling snd_use_lock_use(). > > Reported-by: Dmitry Vyukov > Cc: > Cc: Takashi Iwai > Signed-off-by: Daniel Mentz > --- > sound/core/seq/seq_clientmgr.c | 13 ++++--------- > sound/core/seq/seq_queue.c | 14 +++++++++----- > sound/core/seq/seq_queue.h | 2 +- > 3 files changed, 14 insertions(+), 15 deletions(-) > > diff --git a/sound/core/seq/seq_clientmgr.c b/sound/core/seq/seq_clientmgr.c > index 272c55fe17c8..ea2d0ae85bd3 100644 > --- a/sound/core/seq/seq_clientmgr.c > +++ b/sound/core/seq/seq_clientmgr.c > @@ -1502,16 +1502,11 @@ static int snd_seq_ioctl_unsubscribe_port(struct snd_seq_client *client, > static int snd_seq_ioctl_create_queue(struct snd_seq_client *client, void *arg) > { > struct snd_seq_queue_info *info = arg; > - int result; > struct snd_seq_queue *q; > > - result = snd_seq_queue_alloc(client->number, info->locked, info->flags); > - if (result < 0) > - return result; > - > - q = queueptr(result); > - if (q == NULL) > - return -EINVAL; > + q = snd_seq_queue_alloc(client->number, info->locked, info->flags); > + if (IS_ERR(q)) > + return PTR_ERR(q); > > info->queue = q->queue; > info->locked = q->locked; > @@ -1521,7 +1516,7 @@ static int snd_seq_ioctl_create_queue(struct snd_seq_client *client, void *arg) > if (!info->name[0]) > snprintf(info->name, sizeof(info->name), "Queue-%d", q->queue); > strlcpy(q->name, info->name, sizeof(q->name)); > - queuefree(q); > + snd_use_lock_free(&q->use_lock); > > return 0; > } > diff --git a/sound/core/seq/seq_queue.c b/sound/core/seq/seq_queue.c > index 450c5187eecb..79e0c5604ef8 100644 > --- a/sound/core/seq/seq_queue.c > +++ b/sound/core/seq/seq_queue.c > @@ -184,22 +184,26 @@ void __exit snd_seq_queues_delete(void) > static void queue_use(struct snd_seq_queue *queue, int client, int use); > > /* allocate a new queue - > - * return queue index value or negative value for error > + * return pointer to new queue or ERR_PTR(-errno) for error > + * The new queue's use_lock is set to 1. It is the caller's responsibility to > + * call snd_use_lock_free(&q->use_lock). > */ > -int snd_seq_queue_alloc(int client, int locked, unsigned int info_flags) > +struct snd_seq_queue *snd_seq_queue_alloc(int client, int locked, unsigned int info_flags) > { > struct snd_seq_queue *q; > > q = queue_new(client, locked); > if (q == NULL) > - return -ENOMEM; > + return ERR_PTR(-ENOMEM); > q->info_flags = info_flags; > queue_use(q, client, 1); > + snd_use_lock_use(&q->use_lock); > if (queue_list_add(q) < 0) { > + snd_use_lock_free(&q->use_lock); > queue_delete(q); > - return -ENOMEM; > + return ERR_PTR(-ENOMEM); > } > - return q->queue; > + return q; > } > > /* delete a queue - queue must be owned by the client */ > diff --git a/sound/core/seq/seq_queue.h b/sound/core/seq/seq_queue.h > index 30c8111477f6..719093489a2c 100644 > --- a/sound/core/seq/seq_queue.h > +++ b/sound/core/seq/seq_queue.h > @@ -71,7 +71,7 @@ void snd_seq_queues_delete(void); > > > /* create new queue (constructor) */ > -int snd_seq_queue_alloc(int client, int locked, unsigned int flags); > +struct snd_seq_queue *snd_seq_queue_alloc(int client, int locked, unsigned int flags); > > /* delete queue (destructor) */ > int snd_seq_queue_delete(int client, int queueid); > -- > 2.14.0.434.g98096fd7a8-goog >