Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 7e1d90f6 authored by Daniel Mentz's avatar Daniel Mentz Committed by Takashi Iwai
Browse files

ALSA: seq: 2nd attempt at fixing race creating a queue 



commit 4842e98f ("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: 4842e98f ("ALSA: seq: Fix race at creating a queue")
Reported-by: default avatarDmitry Vyukov <dvyukov@google.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: default avatarDaniel Mentz <danielmentz@google.com>
Signed-off-by: default avatarTakashi Iwai <tiwai@suse.de>
parent 8df4b003

sound/core/seq/seq_clientmgr.c

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

static int snd_seq_ioctl_create_queue(struct snd_seq_client *client, void *arg)

{

{

	struct snd_seq_queue_info *info = arg;

	struct snd_seq_queue_info *info = arg;

	int result;

	struct snd_seq_queue *q;

	struct snd_seq_queue *q;





	result = snd_seq_queue_alloc(client->number, info->locked, info->flags);

	q = snd_seq_queue_alloc(client->number, info->locked, info->flags);

	if (result < 0)

	if (IS_ERR(q))

		return result;

		return PTR_ERR(q);



	q = queueptr(result);

	if (q == NULL)

		return -EINVAL;





	info->queue = q->queue;

	info->queue = q->queue;

	info->locked = q->locked;

	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])

	if (!info->name[0])

		snprintf(info->name, sizeof(info->name), "Queue-%d", q->queue);

		snprintf(info->name, sizeof(info->name), "Queue-%d", q->queue);

	strlcpy(q->name, info->name, sizeof(q->name));

	strlcpy(q->name, info->name, sizeof(q->name));

	queuefree(q);

	snd_use_lock_free(&q->use_lock);





	return 0;

	return 0;

}

}

sound/core/seq/seq_queue.c

+9 −5
Original line number Original line Diff line number Diff line
@@ -184,22 +184,26 @@ void __exit snd_seq_queues_delete(void) 
static void queue_use(struct snd_seq_queue *queue, int client, int use);

static void queue_use(struct snd_seq_queue *queue, int client, int use);





/* allocate a new queue -

/* 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;

	struct snd_seq_queue *q;





	q = queue_new(client, locked);

	q = queue_new(client, locked);

	if (q == NULL)

	if (q == NULL)

		return -ENOMEM;

		return ERR_PTR(-ENOMEM);

	q->info_flags = info_flags;

	q->info_flags = info_flags;

	queue_use(q, client, 1);

	queue_use(q, client, 1);

	snd_use_lock_use(&q->use_lock);

	if (queue_list_add(q) < 0) {

	if (queue_list_add(q) < 0) {

		snd_use_lock_free(&q->use_lock);

		queue_delete(q);

		queue_delete(q);

		return -ENOMEM;

		return ERR_PTR(-ENOMEM);

	}

	}

	return q->queue;

	return q;

}

}





/* delete a queue - queue must be owned by the client */

/* delete a queue - queue must be owned by the client */

sound/core/seq/seq_queue.h

+1 −1
Original line number Original line Diff line number Diff line
@@ -71,7 +71,7 @@ void snd_seq_queues_delete(void); 








/* create new queue (constructor) */

/* 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) */

/* delete queue (destructor) */

int snd_seq_queue_delete(int client, int queueid);

int snd_seq_queue_delete(int client, int queueid);