aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorDaniel Mentz2017-08-14 16:46:01 -0500
committerGreg Kroah-Hartman2017-08-24 19:02:35 -0500
commit735aa043bf00c809a93d01c7ef4039115f1ef590 (patch)
treef5e198ab6614077f7c1e4614ee26df2349fd18e7
parentae4743cac8d771a614da982ed51ffd396041c3ec (diff)
downloadkernel-omap-735aa043bf00c809a93d01c7ef4039115f1ef590.tar.gz
kernel-omap-735aa043bf00c809a93d01c7ef4039115f1ef590.tar.xz
kernel-omap-735aa043bf00c809a93d01c7ef4039115f1ef590.zip
ALSA: seq: 2nd attempt at fixing race creating a queue
commit 7e1d90f60a0d501c8503e636942ca704a454d910 upstream. 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). Fixes: 4842e98f26dd ("ALSA: seq: Fix race at creating a queue") Reported-by: Dmitry Vyukov <dvyukov@google.com> Signed-off-by: Daniel Mentz <danielmentz@google.com> Signed-off-by: Takashi Iwai <tiwai@suse.de> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
-rw-r--r--sound/core/seq/seq_clientmgr.c13
-rw-r--r--sound/core/seq/seq_queue.c14
-rw-r--r--sound/core/seq/seq_queue.h2
3 files changed, 14 insertions, 15 deletions
diff --git a/sound/core/seq/seq_clientmgr.c b/sound/core/seq/seq_clientmgr.c
index c67f9c212dd1..e326c1d80416 100644
--- a/sound/core/seq/seq_clientmgr.c
+++ b/sound/core/seq/seq_clientmgr.c
@@ -1530,19 +1530,14 @@ static int snd_seq_ioctl_create_queue(struct snd_seq_client *client,
1530 void __user *arg) 1530 void __user *arg)
1531{ 1531{
1532 struct snd_seq_queue_info info; 1532 struct snd_seq_queue_info info;
1533 int result;
1534 struct snd_seq_queue *q; 1533 struct snd_seq_queue *q;
1535 1534
1536 if (copy_from_user(&info, arg, sizeof(info))) 1535 if (copy_from_user(&info, arg, sizeof(info)))
1537 return -EFAULT; 1536 return -EFAULT;
1538 1537
1539 result = snd_seq_queue_alloc(client->number, info.locked, info.flags); 1538 q = snd_seq_queue_alloc(client->number, info.locked, info.flags);
1540 if (result < 0) 1539 if (IS_ERR(q))
1541 return result; 1540 return PTR_ERR(q);
1542
1543 q = queueptr(result);
1544 if (q == NULL)
1545 return -EINVAL;
1546 1541
1547 info.queue = q->queue; 1542 info.queue = q->queue;
1548 info.locked = q->locked; 1543 info.locked = q->locked;
@@ -1552,7 +1547,7 @@ static int snd_seq_ioctl_create_queue(struct snd_seq_client *client,
1552 if (! info.name[0]) 1547 if (! info.name[0])
1553 snprintf(info.name, sizeof(info.name), "Queue-%d", q->queue); 1548 snprintf(info.name, sizeof(info.name), "Queue-%d", q->queue);
1554 strlcpy(q->name, info.name, sizeof(q->name)); 1549 strlcpy(q->name, info.name, sizeof(q->name));
1555 queuefree(q); 1550 snd_use_lock_free(&q->use_lock);
1556 1551
1557 if (copy_to_user(arg, &info, sizeof(info))) 1552 if (copy_to_user(arg, &info, sizeof(info)))
1558 return -EFAULT; 1553 return -EFAULT;
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)
184static void queue_use(struct snd_seq_queue *queue, int client, int use); 184static void queue_use(struct snd_seq_queue *queue, int client, int use);
185 185
186/* allocate a new queue - 186/* allocate a new queue -
187 * return queue index value or negative value for error 187 * return pointer to new queue or ERR_PTR(-errno) for error
188 * The new queue's use_lock is set to 1. It is the caller's responsibility to
189 * call snd_use_lock_free(&q->use_lock).
188 */ 190 */
189int snd_seq_queue_alloc(int client, int locked, unsigned int info_flags) 191struct snd_seq_queue *snd_seq_queue_alloc(int client, int locked, unsigned int info_flags)
190{ 192{
191 struct snd_seq_queue *q; 193 struct snd_seq_queue *q;
192 194
193 q = queue_new(client, locked); 195 q = queue_new(client, locked);
194 if (q == NULL) 196 if (q == NULL)
195 return -ENOMEM; 197 return ERR_PTR(-ENOMEM);
196 q->info_flags = info_flags; 198 q->info_flags = info_flags;
197 queue_use(q, client, 1); 199 queue_use(q, client, 1);
200 snd_use_lock_use(&q->use_lock);
198 if (queue_list_add(q) < 0) { 201 if (queue_list_add(q) < 0) {
202 snd_use_lock_free(&q->use_lock);
199 queue_delete(q); 203 queue_delete(q);
200 return -ENOMEM; 204 return ERR_PTR(-ENOMEM);
201 } 205 }
202 return q->queue; 206 return q;
203} 207}
204 208
205/* delete a queue - queue must be owned by the client */ 209/* 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);
71 71
72 72
73/* create new queue (constructor) */ 73/* create new queue (constructor) */
74int snd_seq_queue_alloc(int client, int locked, unsigned int flags); 74struct snd_seq_queue *snd_seq_queue_alloc(int client, int locked, unsigned int flags);
75 75
76/* delete queue (destructor) */ 76/* delete queue (destructor) */
77int snd_seq_queue_delete(int client, int queueid); 77int snd_seq_queue_delete(int client, int queueid);