[alsa-devel] [PATCH 1/2] ALSA: seq: Fix possible UAF in snd_seq_check_queue()
Although we've covered the races between concurrent write() and ioctl() in the previous patch series, there is still a possible UAF in the following scenario:
A: user client closed B: timer irq -> snd_seq_release() -> snd_seq_timer_interrupt() -> snd_seq_free_client() -> snd_seq_check_queue() -> cell = snd_seq_prioq_cell_peek() -> snd_seq_prioq_leave() .... removing all cells -> snd_seq_pool_done() .... vfree() -> snd_seq_compare_tick_time(cell) ... Oops
So the problem is that a cell is peeked and accessed without any protection until it's retrieved from the queue again via snd_seq_prioq_cell_out().
This patch tries to address it, also cleans up the code by a slight refactoring. snd_seq_prioq_cell_out() now receives an extra pointer argument. When it's non-NULL, the function checks the event timestamp with the given pointer. The caller needs to pass the right reference either to snd_seq_tick or snd_seq_realtime depending on the event timestamp type.
A good news is that the above change allows us to remove the snd_seq_prioq_cell_peek(), too, thus the patch actually reduces the code size.
Cc: stable@vger.kernel.org Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/seq/seq_prioq.c | 28 ++++++++++++++-------------- sound/core/seq/seq_prioq.h | 6 ++---- sound/core/seq/seq_queue.c | 28 +++++++++------------------- 3 files changed, 25 insertions(+), 37 deletions(-)
diff --git a/sound/core/seq/seq_prioq.c b/sound/core/seq/seq_prioq.c index bc1c8488fc2a..2bc6759e4adc 100644 --- a/sound/core/seq/seq_prioq.c +++ b/sound/core/seq/seq_prioq.c @@ -87,7 +87,7 @@ void snd_seq_prioq_delete(struct snd_seq_prioq **fifo) if (f->cells > 0) { /* drain prioQ */ while (f->cells > 0) - snd_seq_cell_free(snd_seq_prioq_cell_out(f)); + snd_seq_cell_free(snd_seq_prioq_cell_out(f, NULL)); } kfree(f); @@ -214,8 +214,18 @@ int snd_seq_prioq_cell_in(struct snd_seq_prioq * f, return 0; }
+/* return 1 if the current time >= event timestamp */ +static int event_is_ready(struct snd_seq_event *ev, void *current_time) +{ + if ((ev->flags & SNDRV_SEQ_TIME_STAMP_MASK) == SNDRV_SEQ_TIME_STAMP_TICK) + return snd_seq_compare_tick_time(current_time, &ev->time.tick); + else + return snd_seq_compare_real_time(current_time, &ev->time.time); +} + /* dequeue cell from prioq */ -struct snd_seq_event_cell *snd_seq_prioq_cell_out(struct snd_seq_prioq *f) +struct snd_seq_event_cell *snd_seq_prioq_cell_out(struct snd_seq_prioq *f, + void *current_time) { struct snd_seq_event_cell *cell; unsigned long flags; @@ -227,6 +237,8 @@ struct snd_seq_event_cell *snd_seq_prioq_cell_out(struct snd_seq_prioq *f) spin_lock_irqsave(&f->lock, flags);
cell = f->head; + if (cell && current_time && !event_is_ready(&cell->event, current_time)) + cell = NULL; if (cell) { f->head = cell->next;
@@ -252,18 +264,6 @@ int snd_seq_prioq_avail(struct snd_seq_prioq * f) return f->cells; }
- -/* peek at cell at the head of the prioq */ -struct snd_seq_event_cell *snd_seq_prioq_cell_peek(struct snd_seq_prioq * f) -{ - if (f == NULL) { - pr_debug("ALSA: seq: snd_seq_prioq_cell_in() called with NULL prioq\n"); - return NULL; - } - return f->head; -} - - static inline int prioq_match(struct snd_seq_event_cell *cell, int client, int timestamp) { diff --git a/sound/core/seq/seq_prioq.h b/sound/core/seq/seq_prioq.h index d38bb78d9345..2c315ca10fc4 100644 --- a/sound/core/seq/seq_prioq.h +++ b/sound/core/seq/seq_prioq.h @@ -44,14 +44,12 @@ void snd_seq_prioq_delete(struct snd_seq_prioq **fifo); int snd_seq_prioq_cell_in(struct snd_seq_prioq *f, struct snd_seq_event_cell *cell);
/* dequeue cell from prioq */ -struct snd_seq_event_cell *snd_seq_prioq_cell_out(struct snd_seq_prioq *f); +struct snd_seq_event_cell *snd_seq_prioq_cell_out(struct snd_seq_prioq *f, + void *current_time);
/* return number of events available in prioq */ int snd_seq_prioq_avail(struct snd_seq_prioq *f);
-/* peek at cell at the head of the prioq */ -struct snd_seq_event_cell *snd_seq_prioq_cell_peek(struct snd_seq_prioq *f); - /* client left queue */ void snd_seq_prioq_leave(struct snd_seq_prioq *f, int client, int timestamp);
diff --git a/sound/core/seq/seq_queue.c b/sound/core/seq/seq_queue.c index 0428e9061b47..b377f5048352 100644 --- a/sound/core/seq/seq_queue.c +++ b/sound/core/seq/seq_queue.c @@ -277,30 +277,20 @@ void snd_seq_check_queue(struct snd_seq_queue *q, int atomic, int hop)
__again: /* Process tick queue... */ - while ((cell = snd_seq_prioq_cell_peek(q->tickq)) != NULL) { - if (snd_seq_compare_tick_time(&q->timer->tick.cur_tick, - &cell->event.time.tick)) { - cell = snd_seq_prioq_cell_out(q->tickq); - if (cell) - snd_seq_dispatch_event(cell, atomic, hop); - } else { - /* event remains in the queue */ + for (;;) { + cell = snd_seq_prioq_cell_out(q->tickq, + &q->timer->tick.cur_tick); + if (!cell) break; - } + snd_seq_dispatch_event(cell, atomic, hop); }
- /* Process time queue... */ - while ((cell = snd_seq_prioq_cell_peek(q->timeq)) != NULL) { - if (snd_seq_compare_real_time(&q->timer->cur_time, - &cell->event.time.time)) { - cell = snd_seq_prioq_cell_out(q->timeq); - if (cell) - snd_seq_dispatch_event(cell, atomic, hop); - } else { - /* event remains in the queue */ + for (;;) { + cell = snd_seq_prioq_cell_out(q->timeq, &q->timer->cur_time); + if (!cell) break; - } + snd_seq_dispatch_event(cell, atomic, hop); }
/* free lock */
When releasing a client, we need to clear the clienttab[] entry at first, then call snd_seq_queue_client_leave(). Otherwise, the in-flight cell in the queue might be picked up by the timer interrupt via snd_seq_check_queue() before calling snd_seq_queue_client_leave(), and it's delivered to another queue while the client is clearing queues. This may eventually result in an uncleared cell remaining in a queue, and the later snd_seq_pool_delete() may need to wait for a long time until the event gets really processed.
By moving the clienttab[] clearance at the beginning of release, any event delivery of a cell belonging to this client will fail at a later point, since snd_seq_client_ptr() returns NULL. Thus the cell that was picked up by the timer interrupt will be returned immediately without further delivery, and the long stall of snd_seq_delete_pool() can be avoided, too.
Cc: stable@vger.kernel.org Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/seq/seq_clientmgr.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/core/seq/seq_clientmgr.c b/sound/core/seq/seq_clientmgr.c index 918338dea5b9..61a07fe34cd2 100644 --- a/sound/core/seq/seq_clientmgr.c +++ b/sound/core/seq/seq_clientmgr.c @@ -255,12 +255,12 @@ static int seq_free_client1(struct snd_seq_client *client)
if (!client) return 0; - snd_seq_delete_all_ports(client); - snd_seq_queue_client_leave(client->number); spin_lock_irqsave(&clients_lock, flags); clienttablock[client->number] = 1; clienttab[client->number] = NULL; spin_unlock_irqrestore(&clients_lock, flags); + snd_seq_delete_all_ports(client); + snd_seq_queue_client_leave(client->number); snd_use_lock_sync(&client->use_lock); snd_seq_queue_client_termination(client->number); if (client->pool)
participants (1)
-
Takashi Iwai