Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1576403 > unrolled thread

sound: use-after-free in snd_seq_queue_alloc

Back to article view | Back to linux.kernel

Contents

  sound: use-after-free in snd_seq_queue_alloc Dmitry Vyukov <dvyukov@google.com> - 2017-02-08 10:50 +0100
    Re: sound: use-after-free in snd_seq_queue_alloc Takashi Iwai <tiwai@suse.de> - 2017-02-08 13:10 +0100

#1576403 — sound: use-after-free in snd_seq_queue_alloc

Hello,


I've got the following report while running syzkaller fuzzer on
8b1b41ee74f9712c355d66dc105bbea663ae0afd:

BUG: KASAN: use-after-free in snd_seq_queue_alloc+0x663/0x690
sound/core/seq/seq_queue.c:200 at addr ffff880086ba1d00
Read of size 4 by task syz-executor2/31851
CPU: 2 PID: 31851 Comm: syz-executor2 Not tainted 4.10.0-rc5+ #201
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011

Call Trace:
 __asan_report_load4_noabort+0x3e/0x40 mm/kasan/report.c:327
 snd_seq_queue_alloc+0x663/0x690 sound/core/seq/seq_queue.c:200
 snd_seq_ioctl_create_queue+0xad/0x310 sound/core/seq/seq_clientmgr.c:1508
 snd_seq_ioctl+0x2da/0x4d0 sound/core/seq/seq_clientmgr.c:2130
 vfs_ioctl fs/ioctl.c:43 [inline]
 do_vfs_ioctl+0x1bf/0x1790 fs/ioctl.c:683
 SYSC_ioctl fs/ioctl.c:698 [inline]
 SyS_ioctl+0x8f/0xc0 fs/ioctl.c:689
 entry_SYSCALL_64_fastpath+0x1f/0xc2

Allocated:
PID = 31851
[<ffffffff83483a17>] kzalloc include/linux/slab.h:490 [inline]
[<ffffffff83483a17>] queue_new sound/core/seq/seq_queue.c:113 [inline]
[<ffffffff83483a17>] snd_seq_queue_alloc+0x107/0x690
sound/core/seq/seq_queue.c:191
[<ffffffff834748dd>] snd_seq_ioctl_create_queue+0xad/0x310
sound/core/seq/seq_clientmgr.c:1508
[<ffffffff8347878a>] snd_seq_ioctl+0x2da/0x4d0
sound/core/seq/seq_clientmgr.c:2130
[<ffffffff81aa41cf>] vfs_ioctl fs/ioctl.c:43 [inline]
[<ffffffff81aa41cf>] do_vfs_ioctl+0x1bf/0x1790 fs/ioctl.c:683
[<ffffffff81aa582f>] SYSC_ioctl fs/ioctl.c:698 [inline]
[<ffffffff81aa582f>] SyS_ioctl+0x8f/0xc0 fs/ioctl.c:689
[<ffffffff841c9c81>] entry_SYSCALL_64_fastpath+0x1f/0xc2

Freed:
PID = 31854
[<ffffffff81a0b5c3>] kfree+0xd3/0x250 mm/slab.c:3822
[<ffffffff834817e0>] queue_delete+0x90/0xb0 sound/core/seq/seq_queue.c:156
[<ffffffff834826cc>] snd_seq_queue_delete+0x3c/0x50
sound/core/seq/seq_queue.c:213
[<ffffffff8347480a>] snd_seq_ioctl_delete_queue+0x6a/0x90
sound/core/seq/seq_clientmgr.c:1534
[<ffffffff8347878a>] snd_seq_ioctl+0x2da/0x4d0
sound/core/seq/seq_clientmgr.c:2130
[<ffffffff81aa41cf>] vfs_ioctl fs/ioctl.c:43 [inline]
[<ffffffff81aa41cf>] do_vfs_ioctl+0x1bf/0x1790 fs/ioctl.c:683
[<ffffffff81aa582f>] SYSC_ioctl fs/ioctl.c:698 [inline]
[<ffffffff81aa582f>] SyS_ioctl+0x8f/0xc0 fs/ioctl.c:689



Looking at the code:

int 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;
        q->info_flags = info_flags;
        if (queue_list_add(q) < 0) {
                queue_delete(q);
                return -ENOMEM;
        }
        snd_seq_queue_use(q->queue, client, 1); /* use this queue */
        return q->queue;
}

After queue_list_add(q) q can be deleted by another thread, so
snd_seq_queue_use(q->queue, client, 1) already potentially operates on
deleted object.

[toc] | [next] | [standalone]

#1576486

On Wed, 08 Feb 2017 10:41:14 +0100,
Dmitry Vyukov wrote:
> 
> Hello,
> 
> 
> I've got the following report while running syzkaller fuzzer on
> 8b1b41ee74f9712c355d66dc105bbea663ae0afd:
> 
> BUG: KASAN: use-after-free in snd_seq_queue_alloc+0x663/0x690
> sound/core/seq/seq_queue.c:200 at addr ffff880086ba1d00
> Read of size 4 by task syz-executor2/31851
> CPU: 2 PID: 31851 Comm: syz-executor2 Not tainted 4.10.0-rc5+ #201
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> 
> Call Trace:
>  __asan_report_load4_noabort+0x3e/0x40 mm/kasan/report.c:327
>  snd_seq_queue_alloc+0x663/0x690 sound/core/seq/seq_queue.c:200
>  snd_seq_ioctl_create_queue+0xad/0x310 sound/core/seq/seq_clientmgr.c:1508
>  snd_seq_ioctl+0x2da/0x4d0 sound/core/seq/seq_clientmgr.c:2130
>  vfs_ioctl fs/ioctl.c:43 [inline]
>  do_vfs_ioctl+0x1bf/0x1790 fs/ioctl.c:683
>  SYSC_ioctl fs/ioctl.c:698 [inline]
>  SyS_ioctl+0x8f/0xc0 fs/ioctl.c:689
>  entry_SYSCALL_64_fastpath+0x1f/0xc2
> 
> Allocated:
> PID = 31851
> [<ffffffff83483a17>] kzalloc include/linux/slab.h:490 [inline]
> [<ffffffff83483a17>] queue_new sound/core/seq/seq_queue.c:113 [inline]
> [<ffffffff83483a17>] snd_seq_queue_alloc+0x107/0x690
> sound/core/seq/seq_queue.c:191
> [<ffffffff834748dd>] snd_seq_ioctl_create_queue+0xad/0x310
> sound/core/seq/seq_clientmgr.c:1508
> [<ffffffff8347878a>] snd_seq_ioctl+0x2da/0x4d0
> sound/core/seq/seq_clientmgr.c:2130
> [<ffffffff81aa41cf>] vfs_ioctl fs/ioctl.c:43 [inline]
> [<ffffffff81aa41cf>] do_vfs_ioctl+0x1bf/0x1790 fs/ioctl.c:683
> [<ffffffff81aa582f>] SYSC_ioctl fs/ioctl.c:698 [inline]
> [<ffffffff81aa582f>] SyS_ioctl+0x8f/0xc0 fs/ioctl.c:689
> [<ffffffff841c9c81>] entry_SYSCALL_64_fastpath+0x1f/0xc2
> 
> Freed:
> PID = 31854
> [<ffffffff81a0b5c3>] kfree+0xd3/0x250 mm/slab.c:3822
> [<ffffffff834817e0>] queue_delete+0x90/0xb0 sound/core/seq/seq_queue.c:156
> [<ffffffff834826cc>] snd_seq_queue_delete+0x3c/0x50
> sound/core/seq/seq_queue.c:213
> [<ffffffff8347480a>] snd_seq_ioctl_delete_queue+0x6a/0x90
> sound/core/seq/seq_clientmgr.c:1534
> [<ffffffff8347878a>] snd_seq_ioctl+0x2da/0x4d0
> sound/core/seq/seq_clientmgr.c:2130
> [<ffffffff81aa41cf>] vfs_ioctl fs/ioctl.c:43 [inline]
> [<ffffffff81aa41cf>] do_vfs_ioctl+0x1bf/0x1790 fs/ioctl.c:683
> [<ffffffff81aa582f>] SYSC_ioctl fs/ioctl.c:698 [inline]
> [<ffffffff81aa582f>] SyS_ioctl+0x8f/0xc0 fs/ioctl.c:689
> 
> 
> 
> Looking at the code:
> 
> int 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;
>         q->info_flags = info_flags;
>         if (queue_list_add(q) < 0) {
>                 queue_delete(q);
>                 return -ENOMEM;
>         }
>         snd_seq_queue_use(q->queue, client, 1); /* use this queue */
>         return q->queue;
> }
> 
> After queue_list_add(q) q can be deleted by another thread, so
> snd_seq_queue_use(q->queue, client, 1) already potentially operates on
> deleted object.

A good catch!  The fix patch is below.


thanks,

Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] ALSA: seq: Fix race at creating a queue

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.

Reported-by: Dmitry Vyukov <dvyukov@google.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/core/seq/seq_queue.c | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/sound/core/seq/seq_queue.c b/sound/core/seq/seq_queue.c
index 0bec02e89d51..450c5187eecb 100644
--- a/sound/core/seq/seq_queue.c
+++ b/sound/core/seq/seq_queue.c
@@ -181,6 +181,8 @@ 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
  */
@@ -192,11 +194,11 @@ int snd_seq_queue_alloc(int client, int locked, unsigned int info_flags)
 	if (q == NULL)
 		return -ENOMEM;
 	q->info_flags = info_flags;
+	queue_use(q, client, 1);
 	if (queue_list_add(q) < 0) {
 		queue_delete(q);
 		return -ENOMEM;
 	}
-	snd_seq_queue_use(q->queue, client, 1); /* use this queue */
 	return q->queue;
 }
 
@@ -502,19 +504,9 @@ int snd_seq_queue_timer_set_tempo(int queueid, int client,
 	return result;
 }
 
-
-/* use or unuse this queue -
- * if it is the first client, starts the timer.
- * if it is not longer used by any clients, stop the timer.
- */
-int snd_seq_queue_use(int queueid, int client, int use)
+/* use or unuse this queue */
+static void queue_use(struct snd_seq_queue *queue, int client, int use)
 {
-	struct snd_seq_queue *queue;
-
-	queue = queueptr(queueid);
-	if (queue == NULL)
-		return -EINVAL;
-	mutex_lock(&queue->timer_mutex);
 	if (use) {
 		if (!test_and_set_bit(client, queue->clients_bitmap))
 			queue->clients++;
@@ -529,6 +521,21 @@ int snd_seq_queue_use(int queueid, int client, int use)
 	} else {
 		snd_seq_timer_close(queue);
 	}
+}
+
+/* use or unuse this queue -
+ * if it is the first client, starts the timer.
+ * if it is not longer used by any clients, stop the timer.
+ */
+int snd_seq_queue_use(int queueid, int client, int use)
+{
+	struct snd_seq_queue *queue;
+
+	queue = queueptr(queueid);
+	if (queue == NULL)
+		return -EINVAL;
+	mutex_lock(&queue->timer_mutex);
+	queue_use(queue, client, use);
 	mutex_unlock(&queue->timer_mutex);
 	queuefree(queue);
 	return 0;
-- 
2.11.0

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web