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;