From: Takashi Iwai Date: Tue, 9 Jun 2026 11:50:53 +0000 (+0200) Subject: ALSA: timer: Manage timer object with kref X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=ccd0db6671d2cae986b2daa1c538b6d541a9d62c;p=thirdparty%2Flinux.git 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 Link: https://patch.msgid.link/20260609115100.806869-2-tiwai@suse.de --- diff --git a/include/sound/timer.h b/include/sound/timer.h index 83bafe70cf33..e549eab7145b 100644 --- a/include/sound/timer.h +++ b/include/sound/timer.h @@ -75,6 +75,7 @@ struct snd_timer { struct list_head ack_list_head; struct list_head sack_list_head; /* slow ack list head */ struct work_struct task_work; + struct kref kref; int max_instances; /* upper limit of timer instances */ int num_instances; /* current number of timer instances */ }; @@ -131,4 +132,9 @@ int snd_timer_pause(struct snd_timer_instance *timeri); void snd_timer_interrupt(struct snd_timer *timer, unsigned long ticks_left); +struct snd_timer *snd_timeri_timer_get(struct snd_timer_instance *timeri); +void snd_timeri_timer_put(struct snd_timer *timer); + +DEFINE_FREE(snd_timeri_timer, struct snd_timer *, if (_T) snd_timeri_timer_put(_T)) + #endif /* __SOUND_TIMER_H */ diff --git a/sound/core/seq/seq_timer.c b/sound/core/seq/seq_timer.c index 9bef2f792498..4cd7211ccf48 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 3d72379e57a8..6d8fc1ee29f5 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)(®ister_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, ®ister_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)(®ister_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(¶ms, _params, sizeof(params))) diff --git a/sound/core/timer_compat.c b/sound/core/timer_compat.c index 4ae9eaeb5afb..25ee81c1668b 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));