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

ALSA: seq: Avoid concurrent access to queue flags 



The queue flags are represented in bit fields and the concurrent
access may result in unexpected results.  Although the current code
should be mostly OK as it's only reading a field while writing other
fields as KCSAN reported, it's safer to cover both with a proper
spinlock protection.

This patch fixes the possible concurrent read by protecting with
q->owner_lock.  Also the queue owner field is protected as well since
it's the field to be protected by the lock itself.

Reported-by: default avatar <syzbot+65c6c92d04304d0a8efc@syzkaller.appspotmail.com>
Reported-by: default avatar <syzbot+e60ddfa48717579799dd@syzkaller.appspotmail.com>
Link: https://lore.kernel.org/r/20200214111316.26939-2-tiwai@suse.de


Signed-off-by: default avatarTakashi Iwai <tiwai@suse.de>
parent 0fbb027b

sound/core/seq/seq_queue.c

+16 −4
Original line number Original line Diff line number Diff line
@@ -392,6 +392,7 @@ int snd_seq_queue_check_access(int queueid, int client) 
int snd_seq_queue_set_owner(int queueid, int client, int locked)

int snd_seq_queue_set_owner(int queueid, int client, int locked)

{

{

	struct snd_seq_queue *q = queueptr(queueid);

	struct snd_seq_queue *q = queueptr(queueid);

	unsigned long flags;





	if (q == NULL)

	if (q == NULL)

		return -EINVAL;

		return -EINVAL;
@@ -401,8 +402,10 @@ int snd_seq_queue_set_owner(int queueid, int client, int locked) 
		return -EPERM;

		return -EPERM;

	}

	}





	spin_lock_irqsave(&q->owner_lock, flags);

	q->locked = locked ? 1 : 0;

	q->locked = locked ? 1 : 0;

	q->owner = client;

	q->owner = client;

	spin_unlock_irqrestore(&q->owner_lock, flags);

	queue_access_unlock(q);

	queue_access_unlock(q);

	queuefree(q);

	queuefree(q);
@@ -539,15 +542,17 @@ void snd_seq_queue_client_termination(int client) 
	unsigned long flags;

	unsigned long flags;

	int i;

	int i;

	struct snd_seq_queue *q;

	struct snd_seq_queue *q;

	bool matched;





	for (i = 0; i < SNDRV_SEQ_MAX_QUEUES; i++) {

	for (i = 0; i < SNDRV_SEQ_MAX_QUEUES; i++) {

		if ((q = queueptr(i)) == NULL)

		if ((q = queueptr(i)) == NULL)

			continue;

			continue;

		spin_lock_irqsave(&q->owner_lock, flags);

		spin_lock_irqsave(&q->owner_lock, flags);

		if (q->owner == client)

		matched = (q->owner == client);

		if (matched)

			q->klocked = 1;

			q->klocked = 1;

		spin_unlock_irqrestore(&q->owner_lock, flags);

		spin_unlock_irqrestore(&q->owner_lock, flags);

		if (q->owner == client) {

		if (matched) {

			if (q->timer->running)

			if (q->timer->running)

				snd_seq_timer_stop(q->timer);

				snd_seq_timer_stop(q->timer);

			snd_seq_timer_reset(q->timer);

			snd_seq_timer_reset(q->timer);
@@ -739,6 +744,8 @@ void snd_seq_info_queues_read(struct snd_info_entry *entry, 
	int i, bpm;

	int i, bpm;

	struct snd_seq_queue *q;

	struct snd_seq_queue *q;

	struct snd_seq_timer *tmr;

	struct snd_seq_timer *tmr;

	bool locked;

	int owner;





	for (i = 0; i < SNDRV_SEQ_MAX_QUEUES; i++) {

	for (i = 0; i < SNDRV_SEQ_MAX_QUEUES; i++) {

		if ((q = queueptr(i)) == NULL)

		if ((q = queueptr(i)) == NULL)
@@ -750,9 +757,14 @@ void snd_seq_info_queues_read(struct snd_info_entry *entry, 
		else

		else

			bpm = 0;

			bpm = 0;





		spin_lock_irq(&q->owner_lock);

		locked = q->locked;

		owner = q->owner;

		spin_unlock_irq(&q->owner_lock);



		snd_iprintf(buffer, "queue %d: [%s]\n", q->queue, q->name);

		snd_iprintf(buffer, "queue %d: [%s]\n", q->queue, q->name);

		snd_iprintf(buffer, "owned by client    : %d\n", q->owner);

		snd_iprintf(buffer, "owned by client    : %d\n", owner);

		snd_iprintf(buffer, "lock status        : %s\n", q->locked ? "Locked" : "Free");

		snd_iprintf(buffer, "lock status        : %s\n", locked ? "Locked" : "Free");

		snd_iprintf(buffer, "queued time events : %d\n", snd_seq_prioq_avail(q->timeq));

		snd_iprintf(buffer, "queued time events : %d\n", snd_seq_prioq_avail(q->timeq));

		snd_iprintf(buffer, "queued tick events : %d\n", snd_seq_prioq_avail(q->tickq));

		snd_iprintf(buffer, "queued tick events : %d\n", snd_seq_prioq_avail(q->tickq));

		snd_iprintf(buffer, "timer state        : %s\n", tmr->running ? "Running" : "Stopped");

		snd_iprintf(buffer, "timer state        : %s\n", tmr->running ? "Running" : "Stopped");

CRA Git | Maintained and supported by SUSTech CRA and CCSE