Commit 3424c8f5 authored by Takashi Iwai's avatar Takashi Iwai
Browse files

ALSA: timer: Don't take register_mutex with copy_from/to_user() 



The infamous mmap_lock taken in copy_from/to_user() can be often
problematic when it's called inside another mutex, as they might lead
to deadlocks.

In the case of ALSA timer code, the bad pattern is with
guard(mutex)(&register_mutex) that covers copy_from/to_user() -- which
was mistakenly introduced at converting to guard(), and it had been
carefully worked around in the past.

This patch fixes those pieces simply by moving copy_from/to_user() out
of the register mutex lock again.

Fixes: 3923de04 ("ALSA: pcm: oss: Use guard() for setup")
Reported-by: default avatar <syzbot+2b96f44164236dda0f3b@syzkaller.appspotmail.com>
Closes: https://lore.kernel.org/67dd86c8.050a0220.25ae54.0059.GAE@google.com
Link: https://patch.msgid.link/20250321172653.14310-1-tiwai@suse.de


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

sound/core/timer.c

+77 −70
Original line number Diff line number Diff line
@@ -1515,91 +1515,97 @@ static void snd_timer_user_copy_id(struct snd_timer_id *id, struct snd_timer *ti 
	id->subdevice = timer->tmr_subdevice;

}



static int snd_timer_user_next_device(struct snd_timer_id __user *_tid)

static void get_next_device(struct snd_timer_id *id)

{

	struct snd_timer_id id;

	struct snd_timer *timer;

	struct list_head *p;



	if (copy_from_user(&id, _tid, sizeof(id)))

		return -EFAULT;

	guard(mutex)(&register_mutex);

	if (id.dev_class < 0) {		/* first item */

	if (id->dev_class < 0) {		/* first item */

		if (list_empty(&snd_timer_list))

			snd_timer_user_zero_id(&id);

			snd_timer_user_zero_id(id);

		else {

			timer = list_entry(snd_timer_list.next,

					   struct snd_timer, device_list);

			snd_timer_user_copy_id(&id, timer);

			snd_timer_user_copy_id(id, timer);

		}

	} else {

		switch (id.dev_class) {

		switch (id->dev_class) {

		case SNDRV_TIMER_CLASS_GLOBAL:

			id.device = id.device < 0 ? 0 : id.device + 1;

			id->device = id->device < 0 ? 0 : id->device + 1;

			list_for_each(p, &snd_timer_list) {

				timer = list_entry(p, struct snd_timer, device_list);

				if (timer->tmr_class > SNDRV_TIMER_CLASS_GLOBAL) {

					snd_timer_user_copy_id(&id, timer);

					snd_timer_user_copy_id(id, timer);

					break;

				}

				if (timer->tmr_device >= id.device) {

					snd_timer_user_copy_id(&id, timer);

				if (timer->tmr_device >= id->device) {

					snd_timer_user_copy_id(id, timer);

					break;

				}

			}

			if (p == &snd_timer_list)

				snd_timer_user_zero_id(&id);

				snd_timer_user_zero_id(id);

			break;

		case SNDRV_TIMER_CLASS_CARD:

		case SNDRV_TIMER_CLASS_PCM:

			if (id.card < 0) {

				id.card = 0;

			if (id->card < 0) {

				id->card = 0;

			} else {

				if (id.device < 0) {

					id.device = 0;

				if (id->device < 0) {

					id->device = 0;

				} else {

					if (id.subdevice < 0)

						id.subdevice = 0;

					else if (id.subdevice < INT_MAX)

						id.subdevice++;

					if (id->subdevice < 0)

						id->subdevice = 0;

					else if (id->subdevice < INT_MAX)

						id->subdevice++;

				}

			}

			list_for_each(p, &snd_timer_list) {

				timer = list_entry(p, struct snd_timer, device_list);

				if (timer->tmr_class > id.dev_class) {

					snd_timer_user_copy_id(&id, timer);

				if (timer->tmr_class > id->dev_class) {

					snd_timer_user_copy_id(id, timer);

					break;

				}

				if (timer->tmr_class < id.dev_class)

				if (timer->tmr_class < id->dev_class)

					continue;

				if (timer->card->number > id.card) {

					snd_timer_user_copy_id(&id, timer);

				if (timer->card->number > id->card) {

					snd_timer_user_copy_id(id, timer);

					break;

				}

				if (timer->card->number < id.card)

				if (timer->card->number < id->card)

					continue;

				if (timer->tmr_device > id.device) {

					snd_timer_user_copy_id(&id, timer);

				if (timer->tmr_device > id->device) {

					snd_timer_user_copy_id(id, timer);

					break;

				}

				if (timer->tmr_device < id.device)

				if (timer->tmr_device < id->device)

					continue;

				if (timer->tmr_subdevice > id.subdevice) {

					snd_timer_user_copy_id(&id, timer);

				if (timer->tmr_subdevice > id->subdevice) {

					snd_timer_user_copy_id(id, timer);

					break;

				}

				if (timer->tmr_subdevice < id.subdevice)

				if (timer->tmr_subdevice < id->subdevice)

					continue;

				snd_timer_user_copy_id(&id, timer);

				snd_timer_user_copy_id(id, timer);

				break;

			}

			if (p == &snd_timer_list)

				snd_timer_user_zero_id(&id);

				snd_timer_user_zero_id(id);

			break;

		default:

			snd_timer_user_zero_id(&id);

			snd_timer_user_zero_id(id);

		}

	}

}



static int snd_timer_user_next_device(struct snd_timer_id __user *_tid)

{

	struct snd_timer_id id;



	if (copy_from_user(&id, _tid, sizeof(id)))

		return -EFAULT;

	scoped_guard(mutex, &register_mutex)

		get_next_device(&id);

	if (copy_to_user(_tid, &id, sizeof(*_tid)))

		return -EFAULT;

	return 0;
@@ -1620,7 +1626,7 @@ static int snd_timer_user_ginfo(struct file *file, 
	tid = ginfo->tid;

	memset(ginfo, 0, sizeof(*ginfo));

	ginfo->tid = tid;

	guard(mutex)(&register_mutex);

	scoped_guard(mutex, &register_mutex) {

		t = snd_timer_find(&tid);

		if (!t)

			return -ENODEV;
@@ -1638,6 +1644,7 @@ static int snd_timer_user_ginfo(struct file *file, 
		list_for_each(p, &t->open_list_head) {

			ginfo->clients++;

		}

	}

	if (copy_to_user(_ginfo, ginfo, sizeof(*ginfo)))

		return -EFAULT;

	return 0;
@@ -1674,14 +1681,13 @@ static int snd_timer_user_gstatus(struct file *file, 
	struct snd_timer_gstatus gstatus;

	struct snd_timer_id tid;

	struct snd_timer *t;

	int err = 0;



	if (copy_from_user(&gstatus, _gstatus, sizeof(gstatus)))

		return -EFAULT;

	tid = gstatus.tid;

	memset(&gstatus, 0, sizeof(gstatus));

	gstatus.tid = tid;

	guard(mutex)(&register_mutex);

	scoped_guard(mutex, &register_mutex) {

		t = snd_timer_find(&tid);

		if (t != NULL) {

			guard(spinlock_irq)(&t->lock);
@@ -1694,11 +1700,12 @@ static int snd_timer_user_gstatus(struct file *file, 
				gstatus.resolution_den = 1000000000uL;

			}

		} else {

		err = -ENODEV;

			return -ENODEV;

		}

	if (err >= 0 && copy_to_user(_gstatus, &gstatus, sizeof(gstatus)))

		err = -EFAULT;

	return err;

	}

	if (copy_to_user(_gstatus, &gstatus, sizeof(gstatus)))

		return -EFAULT;

	return 0;

}



static int snd_timer_user_tselect(struct file *file,