[PATCH] ALSA: seq: 2nd attempt at fixing race creating a q

[PATCH] ALSA: seq: 2nd attempt at fixing race creating a q

[Daniel Mentz]

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.

This commit adds code to effectively prevent the removal of the queue by
calling snd_use_lock_use().

Reported-by: Dmitry Vyukov <dvyukov@google.com>
Cc: <stable@vger.kernel.org>
Cc: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Daniel Mentz <danielmentz@google.com>
---
 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

Re: [PATCH] ALSA: seq: 2nd attempt at fixing race creating a q

[Takashi Iwai]

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 <dvyukov@google.com>
> Cc: <stable@vger.kernel.org>
> Cc: Takashi Iwai <tiwai@suse.de>
> Signed-off-by: Daniel Mentz <danielmentz@google.com>
> ---
>  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
> 

Re: [PATCH] ALSA: seq: 2nd attempt at fixing race creating a q

[Daniel Mentz]

On Fri, Aug 11, 2017 at 7:42 AM, Takashi Iwai <tiwai@suse.de> wrote:
>
> 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.

queue_use() is defined to return void. I am assuming you're referring
to queueptr().

Where do you acquire the snd_use_lock i.e. where do you call
snd_use_lock_use()? My understanding is that it's called from
queueptr(). With that said, there is a tiny gap between the new queue
being added to the list in queue_list_add() (from
snd_seq_queue_alloc()) and the call to queueptr() in
snd_seq_ioctl_create_queue(). syzkaller specifically points to the
last instruction "return q->queue;" in snd_seq_queue_alloc(). This is
*after* q has been added to the list (and is therefore visible to
other threads) but *before* it has been locked through
snd_seq_ioctl_create_queue()->queueptr()->snd_use_lock_use(). Hence,
there is a possibility that the pointer q is no longer valid.
You could capture the return value from queue_list_add() which is
identical to q->queue and then return that. However, the other issue
is that queueptr() indeed returns NULL if the queue with that index
has been deleted, but it's theoretically possible that the queue has
been deleted and a *different* been created that now occupies the same
spot in the queue_list.

> Or do you actually see any crash or the wild behavior?

syzkaller reported a crash:

syzkaller hit the following crash on 7b2727c68878444d8f47d2d394395e4a11929624
https://android.googlesource.com/kernel/common/android-4.9
compiler: gcc (GCC) 7.1.1 20170620

==================================================================
BUG: KASAN: use-after-free in snd_seq_queue_alloc+0x47e/0x4a0
sound/core/seq/seq_queue.c:202 at addr ffff8801c7622000
Read of size 4 by task syz-executor1/23085
CPU: 1 PID: 23085 Comm: syz-executor1 Not tainted 4.9.40-g7b2727c #16
Hardware name: Google Google Compute Engine/Google Compute Engine,
BIOS Google 01/01/2011
 ffff8801cc49faa8 ffffffff81d8f109 ffff8801da001280 ffff8801c7622000
 ffff8801c7622200 ffffed0038ec4400 ffff8801c7622000 ffff8801cc49fad0
 ffffffff8153931c ffffed0038ec4400 ffff8801da001280 0000000000000000
Call Trace:
 [<ffffffff81d8f109>] __dump_stack lib/dump_stack.c:15 [inline]
 [<ffffffff81d8f109>] dump_stack+0xc1/0x128 lib/dump_stack.c:51
 [<ffffffff8153931c>] kasan_object_err+0x1c/0x70 mm/kasan/report.c:160
 [<ffffffff815395dc>] print_address_description mm/kasan/report.c:198 [inline]
 [<ffffffff815395dc>] kasan_report_error mm/kasan/report.c:287 [inline]
 [<ffffffff815395dc>] kasan_report.part.1+0x21c/0x500 mm/kasan/report.c:309
 [<ffffffff81539949>] kasan_report mm/kasan/report.c:329 [inline]
 [<ffffffff81539949>] __asan_report_load4_noabort+0x29/0x30
mm/kasan/report.c:329
 [<ffffffff82e096ee>] snd_seq_queue_alloc+0x47e/0x4a0
sound/core/seq/seq_queue.c:202
 [<ffffffff82dfdabd>] snd_seq_ioctl_create_queue+0xad/0x310
sound/core/seq/seq_clientmgr.c:1508
 [<ffffffff82e01146>] snd_seq_ioctl+0x226/0x4a0
sound/core/seq/seq_clientmgr.c:2131
 [<ffffffff815a922a>] vfs_ioctl fs/ioctl.c:43 [inline]
 [<ffffffff815a922a>] do_vfs_ioctl+0x1aa/0x10c0 fs/ioctl.c:679
 [<ffffffff815aa1cf>] SYSC_ioctl fs/ioctl.c:694 [inline]
 [<ffffffff815aa1cf>] SyS_ioctl+0x8f/0xc0 fs/ioctl.c:685
 [<ffffffff838a26c5>] entry_SYSCALL_64_fastpath+0x23/0xc6
Object at ffff8801c7622000, in cache kmalloc-512 size: 512
Allocated:
PID = 23085
 save_stack_trace+0x16/0x20 arch/x86/kernel/stacktrace.c:57
 save_stack+0x43/0xd0 mm/kasan/kasan.c:495
 set_track mm/kasan/kasan.c:507 [inline]
 kasan_kmalloc+0xad/0xe0 mm/kasan/kasan.c:598
 kmem_cache_alloc_trace+0xfb/0x2a0 mm/slub.c:2742
 kmalloc include/linux/slab.h:490 [inline]
 kzalloc include/linux/slab.h:636 [inline]
 queue_new sound/core/seq/seq_queue.c:113 [inline]
 snd_seq_queue_alloc+0x5d/0x4a0 sound/core/seq/seq_queue.c:193
 snd_seq_ioctl_create_queue+0xad/0x310 sound/core/seq/seq_clientmgr.c:1508
 snd_seq_ioctl+0x226/0x4a0 sound/core/seq/seq_clientmgr.c:2131
 vfs_ioctl fs/ioctl.c:43 [inline]
 do_vfs_ioctl+0x1aa/0x10c0 fs/ioctl.c:679
 SYSC_ioctl fs/ioctl.c:694 [inline]
 SyS_ioctl+0x8f/0xc0 fs/ioctl.c:685
 entry_SYSCALL_64_fastpath+0x23/0xc6
Freed:
PID = 23098
 save_stack_trace+0x16/0x20 arch/x86/kernel/stacktrace.c:57
 save_stack+0x43/0xd0 mm/kasan/kasan.c:495
 set_track mm/kasan/kasan.c:507 [inline]
 kasan_slab_free+0x73/0xc0 mm/kasan/kasan.c:571
 slab_free_hook mm/slub.c:1355 [inline]
 slab_free_freelist_hook mm/slub.c:1377 [inline]
 slab_free mm/slub.c:2958 [inline]
 kfree+0xf0/0x2f0 mm/slub.c:3878
 queue_delete+0x90/0xb0 sound/core/seq/seq_queue.c:156
 snd_seq_queue_delete+0x3c/0x50 sound/core/seq/seq_queue.c:215
 snd_seq_ioctl_delete_queue+0x6a/0x90 sound/core/seq/seq_clientmgr.c:1534
 snd_seq_ioctl+0x226/0x4a0 sound/core/seq/seq_clientmgr.c:2131
 vfs_ioctl fs/ioctl.c:43 [inline]
 do_vfs_ioctl+0x1aa/0x10c0 fs/ioctl.c:679
 SYSC_ioctl fs/ioctl.c:694 [inline]
 SyS_ioctl+0x8f/0xc0 fs/ioctl.c:685
 entry_SYSCALL_64_fastpath+0x23/0xc6
Memory state around the buggy address:
 ffff8801c7621f00: fb fb fb fb fb fb fb fb fc fc fc fc fb fb fb fb
 ffff8801c7621f80: fb fb fb fb fc fc fc fc fc fc fc fc fc fc fc fc
>ffff8801c7622000: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
                   ^
 ffff8801c7622080: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
 ffff8801c7622100: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
==================================================================

Re: [PATCH] ALSA: seq: 2nd attempt at fixing race creating a q

[Takashi Iwai]

On Fri, 11 Aug 2017 23:23:42 +0200,
Daniel Mentz wrote:
> 
> On Fri, Aug 11, 2017 at 7:42 AM, Takashi Iwai <tiwai@suse.de> wrote:
> >
> > 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.
> 
> queue_use() is defined to return void. I am assuming you're referring
> to queueptr().
> 
> Where do you acquire the snd_use_lock i.e. where do you call
> snd_use_lock_use()? My understanding is that it's called from
> queueptr(). With that said, there is a tiny gap between the new queue
> being added to the list in queue_list_add() (from
> snd_seq_queue_alloc()) and the call to queueptr() in
> snd_seq_ioctl_create_queue(). syzkaller specifically points to the
> last instruction "return q->queue;" in snd_seq_queue_alloc(). This is
> *after* q has been added to the list (and is therefore visible to
> other threads) but *before* it has been locked through
> snd_seq_ioctl_create_queue()->queueptr()->snd_use_lock_use(). Hence,
> there is a possibility that the pointer q is no longer valid.
> You could capture the return value from queue_list_add() which is
> identical to q->queue and then return that. However, the other issue
> is that queueptr() indeed returns NULL if the queue with that index
> has been deleted, but it's theoretically possible that the queue has
> been deleted and a *different* been created that now occupies the same
> spot in the queue_list.

OK, now I understood.  It's a small race window between
queue_list_add() and queueptr().  This should be clarified better in
the description.  No too much details are needed.  The only point is 
where the race window is opened against which.

> > Or do you actually see any crash or the wild behavior?
> 
> syzkaller reported a crash:

Then it's a real bug, so need to be fixed quickly.
Could you rephrase your changelog text including the crash information 
briefly, and resubmit?  The code change itself looks good.


thanks,

Takashi

[PATCH v2] ALSA: seq: 2nd attempt at fixing race creating a q

[Daniel Mentz]

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 <dvyukov@google.com>
Cc: <stable@vger.kernel.org>
Cc: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Daniel Mentz <danielmentz@google.com>
---
 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

Re: [PATCH v2] ALSA: seq: 2nd attempt at fixing race creating a q

[Takashi Iwai]

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 <dvyukov@google.com>
> Cc: <stable@vger.kernel.org>
> Cc: Takashi Iwai <tiwai@suse.de>
> Signed-off-by: Daniel Mentz <danielmentz@google.com>

Applied now.  Thanks!


Takashi

Re: [PATCH v2] ALSA: seq: 2nd attempt at fixing race creating a q

[Takashi Iwai]

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 <dvyukov@google.com>
> Cc: <stable@vger.kernel.org>
> Cc: Takashi Iwai <tiwai@suse.de>
> Signed-off-by: Daniel Mentz <danielmentz@google.com>

Applied now.  Thanks!


Takashi