aboutsummaryrefslogtreecommitdiffstats
path: root/sound
diff options
authorTakashi Iwai <tiwai@suse.de>2026-06-09 13:50:53 +0200
committerTakashi Iwai <tiwai@suse.de>2026-06-10 09:36:28 +0200
commitccd0db6671d2cae986b2daa1c538b6d541a9d62c (patch)
treec46c45e75df4d63b18a19bdd8ebf02ddf6deacc4 /sound
parent98e157916f83c26a41448267180944048d2f1460 (diff)
downloadath-ccd0db6671d2cae986b2daa1c538b6d541a9d62c.tar.gz
ALSA: timer: Manage timer object with kref
So far we've tried to address UAFs in ALSA timer code by applying the locks at various places, but the fundamental problem is that the timer object may be released while the belonging timer instance objects are still present and accessing to it. This patch is a more proper fix to address that issue, namely, by refcounting and keeping the timer object. The basic implementation is to use kref for the refcount of the timer object, and take/release the reference at assigning/releasing the instance, as well as at referring from ioctls or ALSA sequencer code. The reference from ioctl or ALSA sequencer is abstracted with snd_timeri_timer auto-cleanup. Note that this change assumes that the code already took the fix commit da3039e91d1f ("ALSA: timer: Forcibly close timer instances at closing"); otherwise the refcount may be unbalanced when the timer is freed while slave instances are still present. Signed-off-by: Takashi Iwai <tiwai@suse.de> Link: https://patch.msgid.link/20260609115100.806869-2-tiwai@suse.de
Diffstat (limited to 'sound')
-rw-r--r--sound/core/seq/seq_timer.c11
-rw-r--r--sound/core/timer.c104
-rw-r--r--sound/core/timer_compat.c5
3 files changed, 89 insertions, 31 deletions
diff --git a/sound/core/seq/seq_timer.c b/sound/core/seq/seq_timer.c
index 9bef2f792498c..4cd7211ccf487 100644
--- a/sound/core/seq/seq_timer.c
+++ b/sound/core/seq/seq_timer.c
@@ -333,10 +333,10 @@ int snd_seq_timer_stop(struct snd_seq_timer *tmr)
static int initialize_timer(struct snd_seq_timer *tmr)
{
- struct snd_timer *t;
unsigned long freq;
- t = tmr->timeri->timer;
+ struct snd_timer *t __free(snd_timeri_timer) =
+ snd_timeri_timer_get(tmr->timeri);
if (!t)
return -EINVAL;
@@ -456,7 +456,12 @@ void snd_seq_info_timer_read(struct snd_info_entry *entry,
ti = tmr->timeri;
if (!ti)
break;
- snd_iprintf(buffer, "Timer for queue %i : %s\n", q->queue, ti->timer->name);
+
+ struct snd_timer *t __free(snd_timeri_timer) =
+ snd_timeri_timer_get(ti);
+ snd_iprintf(buffer, "Timer for queue %i : %s\n",
+ q->queue,
+ t ? t->name : "DEAD");
resolution = snd_timer_resolution(ti) * tmr->ticks;
snd_iprintf(buffer, " Period time : %lu.%09lu\n", resolution / 1000000000, resolution % 1000000000);
snd_iprintf(buffer, " Skew : %u / %u\n", tmr->skew, tmr->skew_base);
diff --git a/sound/core/timer.c b/sound/core/timer.c
index 3d72379e57a88..6d8fc1ee29f58 100644
--- a/sound/core/timer.c
+++ b/sound/core/timer.c
@@ -229,6 +229,44 @@ static void snd_timer_request(struct snd_timer_id *tid)
#endif
+/*
+ * refcount management of timer object
+ */
+static void snd_timer_kref_release(struct kref *kref);
+
+static inline void snd_timer_ref_get(struct snd_timer *timer)
+{
+ kref_get(&timer->kref);
+}
+
+static inline void snd_timer_ref_put(struct snd_timer *timer)
+{
+ kref_put(&timer->kref, snd_timer_kref_release);
+}
+
+/*
+ * Return the assigned timer for the instance, NULL if not present;
+ * the caller is responsible to call snd_timeri_timer_put(), or use auto-cleanup
+ */
+struct snd_timer *snd_timeri_timer_get(struct snd_timer_instance *timeri)
+{
+ struct snd_timer *t;
+
+ guard(spinlock_irqsave)(&slave_active_lock);
+ t = timeri->timer;
+ if (!t)
+ return NULL;
+ snd_timer_ref_get(t);
+ return t;
+}
+EXPORT_SYMBOL_GPL(snd_timeri_timer_get);
+
+void snd_timeri_timer_put(struct snd_timer *timer)
+{
+ snd_timer_ref_put(timer);
+}
+EXPORT_SYMBOL_GPL(snd_timeri_timer_put);
+
/* move the slave if it belongs to the master; return 1 if match */
static int check_matching_master_slave(struct snd_timer_instance *master,
struct snd_timer_instance *slave)
@@ -240,6 +278,7 @@ static int check_matching_master_slave(struct snd_timer_instance *master,
return -EBUSY;
list_move_tail(&slave->open_list, &master->slave_list_head);
master->timer->num_instances++;
+ snd_timer_ref_get(master->timer);
guard(spinlock_irq)(&slave_active_lock);
guard(spinlock)(&master->timer->lock);
slave->master = master;
@@ -386,6 +425,7 @@ int snd_timer_open(struct snd_timer_instance *timeri,
if (snd_timer_has_slave_key(timeri))
list_add_tail(&timeri->master_list, &snd_timer_master_list);
timer->num_instances++;
+ snd_timer_ref_get(timer);
err = snd_timer_check_master(timeri);
list_added:
if (err < 0)
@@ -412,6 +452,7 @@ static void remove_slave_links(struct snd_timer_instance *timeri,
list_for_each_entry_safe(slave, tmp, &timeri->slave_list_head, open_list) {
list_move_tail(&slave->open_list, &snd_timer_slave_list);
timer->num_instances--;
+ snd_timer_ref_put(timer);
slave->master = NULL;
slave->timer = NULL;
list_del_init(&slave->ack_list);
@@ -461,17 +502,16 @@ static void snd_timer_close_locked(struct snd_timer_instance *timeri,
remove_slave_links(timeri, timer);
/* slave doesn't need to release timer resources below */
- if (timeri->flags & SNDRV_TIMER_IFLG_SLAVE)
- timer = NULL;
- }
+ if (!(timeri->flags & SNDRV_TIMER_IFLG_SLAVE)) {
+ if (list_empty(&timer->open_list_head) && timer->hw.close)
+ timer->hw.close(timer);
+ /* release a card refcount for safe disconnection */
+ if (timer->card)
+ *card_devp_to_put = &timer->card->card_dev;
+ module_put(timer->module);
+ }
- if (timer) {
- if (list_empty(&timer->open_list_head) && timer->hw.close)
- timer->hw.close(timer);
- /* release a card refcount for safe disconnection */
- if (timer->card)
- *card_devp_to_put = &timer->card->card_dev;
- module_put(timer->module);
+ snd_timer_ref_put(timer);
}
}
@@ -503,12 +543,13 @@ static unsigned long snd_timer_hw_resolution(struct snd_timer *timer)
unsigned long snd_timer_resolution(struct snd_timer_instance *timeri)
{
- struct snd_timer * timer;
unsigned long ret = 0;
if (timeri == NULL)
return 0;
- timer = timeri->timer;
+
+ struct snd_timer *timer __free(snd_timeri_timer)
+ = snd_timeri_timer_get(timeri);
if (timer) {
guard(spinlock_irqsave)(&timer->lock);
ret = snd_timer_hw_resolution(timer);
@@ -961,6 +1002,7 @@ int snd_timer_new(struct snd_card *card, char *id, struct snd_timer_id *tid,
spin_lock_init(&timer->lock);
INIT_WORK(&timer->task_work, snd_timer_work);
timer->max_instances = 1000; /* default limit per timer */
+ kref_init(&timer->kref);
if (card != NULL) {
timer->module = card->module;
err = snd_device_new(card, SNDRV_DEV_TIMER, timer, &ops);
@@ -975,6 +1017,15 @@ int snd_timer_new(struct snd_card *card, char *id, struct snd_timer_id *tid,
}
EXPORT_SYMBOL(snd_timer_new);
+static void snd_timer_kref_release(struct kref *kref)
+{
+ struct snd_timer *timer = container_of(kref, struct snd_timer, kref);
+
+ if (timer->private_free)
+ timer->private_free(timer);
+ kfree(timer);
+}
+
static int snd_timer_free(struct snd_timer *timer)
{
struct snd_timer_instance *ti, *n;
@@ -982,20 +1033,19 @@ static int snd_timer_free(struct snd_timer *timer)
if (!timer)
return 0;
- guard(mutex)(&register_mutex);
- if (! list_empty(&timer->open_list_head)) {
- list_for_each_entry_safe(ti, n, &timer->open_list_head, open_list) {
- struct device *card_dev_to_put = NULL;
+ scoped_guard(mutex, &register_mutex) {
+ if (!list_empty(&timer->open_list_head)) {
+ list_for_each_entry_safe(ti, n, &timer->open_list_head, open_list) {
+ struct device *card_dev_to_put = NULL;
- snd_timer_close_locked(ti, &card_dev_to_put);
- put_device(card_dev_to_put);
+ snd_timer_close_locked(ti, &card_dev_to_put);
+ put_device(card_dev_to_put);
+ }
}
+ list_del(&timer->device_list);
}
- list_del(&timer->device_list);
- if (timer->private_free)
- timer->private_free(timer);
- kfree(timer);
+ snd_timer_ref_put(timer);
return 0;
}
@@ -1778,12 +1828,13 @@ static int snd_timer_user_info(struct file *file,
struct snd_timer_info __user *_info)
{
struct snd_timer_user *tu;
- struct snd_timer *t;
tu = file->private_data;
if (!tu->timeri)
return -EBADFD;
- t = tu->timeri->timer;
+
+ struct snd_timer *t __free(snd_timeri_timer) =
+ snd_timeri_timer_get(tu->timeri);
if (!t)
return -EBADFD;
@@ -1808,14 +1859,15 @@ static int snd_timer_user_params(struct file *file,
{
struct snd_timer_user *tu;
struct snd_timer_params params;
- struct snd_timer *t;
int err;
guard(mutex)(&register_mutex);
tu = file->private_data;
if (!tu->timeri)
return -EBADFD;
- t = tu->timeri->timer;
+
+ struct snd_timer *t __free(snd_timeri_timer) =
+ snd_timeri_timer_get(tu->timeri);
if (!t)
return -EBADFD;
if (copy_from_user(&params, _params, sizeof(params)))
diff --git a/sound/core/timer_compat.c b/sound/core/timer_compat.c
index 4ae9eaeb5afb1..25ee81c1668b7 100644
--- a/sound/core/timer_compat.c
+++ b/sound/core/timer_compat.c
@@ -49,12 +49,13 @@ static int snd_timer_user_info_compat(struct file *file,
{
struct snd_timer_user *tu;
struct snd_timer_info32 info;
- struct snd_timer *t;
tu = file->private_data;
if (!tu->timeri)
return -EBADFD;
- t = tu->timeri->timer;
+
+ struct snd_timer *t __free(snd_timeri_timer) =
+ snd_timeri_timer_get(tu->timeri);
if (!t)
return -EBADFD;
memset(&info, 0, sizeof(info));
generated by cgit 1.3-korg (git 2.53.0) at 2026-07-01 14:01:27 +0000