Commit d0f83306 authored by Takashi Iwai's avatar Takashi Iwai
Browse files

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.

Reviewed-by: default avatarNicolai Stange <nstange@suse.de>
Cc: <stable@vger.kernel.org>
Signed-off-by: default avatarTakashi Iwai <tiwai@suse.de>
parent 099fd6ca

sound/core/seq/seq_prioq.c

+14 −14
Original line number Diff line number Diff line
@@ -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)

{

sound/core/seq/seq_prioq.h

+2 −4
Original line number Diff line number Diff line
@@ -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);

sound/core/seq/seq_queue.c

+9 −19
Original line number Diff line number Diff line
@@ -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 */

CRA Git | Maintained and supported by SUSTech CRA and CCSE