From: Takashi Iwai <tiwai@suse.de>
To: linux-sound@vger.kernel.org
Subject: [PATCH] ALSA: timer: Don't take register_mutex with copy_from/to_user()
Date: Fri, 21 Mar 2025 18:26:52 +0100 [thread overview]
Message-ID: <20250321172653.14310-1-tiwai@suse.de> (raw)
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)(®ister_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: 3923de04c817 ("ALSA: pcm: oss: Use guard() for setup")
Reported-by: syzbot+2b96f44164236dda0f3b@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/67dd86c8.050a0220.25ae54.0059.GAE@google.com
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
sound/core/timer.c | 147 ++++++++++++++++++++++++---------------------
1 file changed, 77 insertions(+), 70 deletions(-)
diff --git a/sound/core/timer.c b/sound/core/timer.c
index fbada79380f9..d774b9b71ce2 100644
--- a/sound/core/timer.c
+++ b/sound/core/timer.c
@@ -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)(®ister_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, ®ister_mutex)
+ get_next_device(&id);
if (copy_to_user(_tid, &id, sizeof(*_tid)))
return -EFAULT;
return 0;
@@ -1620,23 +1626,24 @@ static int snd_timer_user_ginfo(struct file *file,
tid = ginfo->tid;
memset(ginfo, 0, sizeof(*ginfo));
ginfo->tid = tid;
- guard(mutex)(®ister_mutex);
- t = snd_timer_find(&tid);
- if (!t)
- return -ENODEV;
- ginfo->card = t->card ? t->card->number : -1;
- if (t->hw.flags & SNDRV_TIMER_HW_SLAVE)
- ginfo->flags |= SNDRV_TIMER_FLG_SLAVE;
- strscpy(ginfo->id, t->id, sizeof(ginfo->id));
- strscpy(ginfo->name, t->name, sizeof(ginfo->name));
- scoped_guard(spinlock_irq, &t->lock)
- ginfo->resolution = snd_timer_hw_resolution(t);
- if (t->hw.resolution_min > 0) {
- ginfo->resolution_min = t->hw.resolution_min;
- ginfo->resolution_max = t->hw.resolution_max;
- }
- list_for_each(p, &t->open_list_head) {
- ginfo->clients++;
+ scoped_guard(mutex, ®ister_mutex) {
+ t = snd_timer_find(&tid);
+ if (!t)
+ return -ENODEV;
+ ginfo->card = t->card ? t->card->number : -1;
+ if (t->hw.flags & SNDRV_TIMER_HW_SLAVE)
+ ginfo->flags |= SNDRV_TIMER_FLG_SLAVE;
+ strscpy(ginfo->id, t->id, sizeof(ginfo->id));
+ strscpy(ginfo->name, t->name, sizeof(ginfo->name));
+ scoped_guard(spinlock_irq, &t->lock)
+ ginfo->resolution = snd_timer_hw_resolution(t);
+ if (t->hw.resolution_min > 0) {
+ ginfo->resolution_min = t->hw.resolution_min;
+ ginfo->resolution_max = t->hw.resolution_max;
+ }
+ list_for_each(p, &t->open_list_head) {
+ ginfo->clients++;
+ }
}
if (copy_to_user(_ginfo, ginfo, sizeof(*ginfo)))
return -EFAULT;
@@ -1674,31 +1681,31 @@ 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)(®ister_mutex);
- t = snd_timer_find(&tid);
- if (t != NULL) {
- guard(spinlock_irq)(&t->lock);
- gstatus.resolution = snd_timer_hw_resolution(t);
- if (t->hw.precise_resolution) {
- t->hw.precise_resolution(t, &gstatus.resolution_num,
- &gstatus.resolution_den);
+ scoped_guard(mutex, ®ister_mutex) {
+ t = snd_timer_find(&tid);
+ if (t != NULL) {
+ guard(spinlock_irq)(&t->lock);
+ gstatus.resolution = snd_timer_hw_resolution(t);
+ if (t->hw.precise_resolution) {
+ t->hw.precise_resolution(t, &gstatus.resolution_num,
+ &gstatus.resolution_den);
+ } else {
+ gstatus.resolution_num = gstatus.resolution;
+ gstatus.resolution_den = 1000000000uL;
+ }
} else {
- gstatus.resolution_num = gstatus.resolution;
- gstatus.resolution_den = 1000000000uL;
+ return -ENODEV;
}
- } else {
- err = -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,
--
2.49.0
reply other threads:[~2025-03-21 17:26 UTC|newest]
Thread overview: [no followups] expand[flat|nested] mbox.gz Atom feed
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20250321172653.14310-1-tiwai@suse.de \
--to=tiwai@suse.de \
--cc=linux-sound@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.