ALSA: seq: Fix possible UAF in snd_seq_check_queue()

commit d0f833065221cbfcbadf19fd4102bcfa9330006a upstream.

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.

Reviewed-by: Nicolai Stange <nstange@suse.de>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
[bwh: Backported to 3.2: Deleted function had different log message]
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>

diff --git a/sound/core/seq/seq_prioq.c b/sound/core/seq/seq_prioq.c
index 29896ab..15c5d67 100644 (file)
--- a/sound/core/seq/seq_prioq.c
+++ b/sound/core/seq/seq_prioq.c
@@ -89,7 +89,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);
@@ -216,8 +216,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;
@@ -229,6 +239,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;
 
@@ -255,17 +267,6 @@ 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)
-{
-       if (f == NULL) {
-               snd_printd("oops: 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 d38bb78..2c315ca 100644 (file)
--- 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 17fe04d..cfe942c 100644 (file)
--- a/sound/core/seq/seq_queue.c
+++ b/sound/core/seq/seq_queue.c
@@ -275,30 +275,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 */