[alsa-devel] [PATCH] ALSA: seq: 2nd attempt at fixing race creating a q
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);
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,
- 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);- snd_use_lock_use(&q->use_lock); if (queue_list_add(q) < 0) {
snd_use_lock_free(&q->use_lock);
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
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,
- 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 ==================================================================
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,
- 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
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);
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
participants (2)
-
Daniel Mentz -
Takashi Iwai