[alsa-devel] [PATCH 0/4] ALSA: yet more timer cleanup and fixes
Hi,
while looking at the old syzkaller reports, I noticed that there are still some possible races around closing the timer object. Here is a patch set to cover it as well as some relevant cleanups / fixes.
I haven't set stable to Cc in them since they are really corner cases, and they haven't been tested much. But they should be safe (and easy) to backport, too, if any.
Takashi
===
Takashi Iwai (4): ALSA: timer: Unify timer callback process code ALSA: timer: Make sure to clear pending ack list ALSA: timer: Check ack_list emptiness instead of bit flag ALSA: timer: Make snd_timer_close() really kill pending actions
include/sound/timer.h | 1 - sound/core/timer.c | 123 ++++++++++++++++++++++++++++++-------------------- 2 files changed, 74 insertions(+), 50 deletions(-)
The timer core has two almost identical code for processing callbacks: once in snd_timer_interrupt() for fast callbacks and another in snd_timer_tasklet() for delayed callbacks. Let's unify them.
In the new version, the resolution is read from ti->resolution at each call, and this must be fine; ti->resolution is set in the preparation step in snd_timer_interrupt().
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/timer.c | 62 +++++++++++++++++++++++------------------------------- 1 file changed, 26 insertions(+), 36 deletions(-)
diff --git a/sound/core/timer.c b/sound/core/timer.c index 61a0cec6e1f6..fdcddfb756b4 100644 --- a/sound/core/timer.c +++ b/sound/core/timer.c @@ -720,29 +720,19 @@ static void snd_timer_reschedule(struct snd_timer * timer, unsigned long ticks_l timer->sticks = ticks; }
-/* - * timer tasklet - * - */ -static void snd_timer_tasklet(unsigned long arg) +/* call callbacks in timer ack list */ +static void snd_timer_process_callbacks(struct snd_timer *timer, + struct list_head *head) { - struct snd_timer *timer = (struct snd_timer *) arg; struct snd_timer_instance *ti; - struct list_head *p; unsigned long resolution, ticks; - unsigned long flags;
- if (timer->card && timer->card->shutdown) - return; - - spin_lock_irqsave(&timer->lock, flags); - /* now process all callbacks */ - while (!list_empty(&timer->sack_list_head)) { - p = timer->sack_list_head.next; /* get first item */ - ti = list_entry(p, struct snd_timer_instance, ack_list); + while (!list_empty(head)) { + ti = list_first_entry(head, struct snd_timer_instance, + ack_list);
/* remove from ack_list and make empty */ - list_del_init(p); + list_del_init(&ti->ack_list);
ticks = ti->pticks; ti->pticks = 0; @@ -755,6 +745,22 @@ static void snd_timer_tasklet(unsigned long arg) spin_lock(&timer->lock); ti->flags &= ~SNDRV_TIMER_IFLG_CALLBACK; } +} + +/* + * timer tasklet + * + */ +static void snd_timer_tasklet(unsigned long arg) +{ + struct snd_timer *timer = (struct snd_timer *) arg; + unsigned long flags; + + if (timer->card && timer->card->shutdown) + return; + + spin_lock_irqsave(&timer->lock, flags); + snd_timer_process_callbacks(timer, &timer->sack_list_head); spin_unlock_irqrestore(&timer->lock, flags); }
@@ -767,8 +773,8 @@ static void snd_timer_tasklet(unsigned long arg) void snd_timer_interrupt(struct snd_timer * timer, unsigned long ticks_left) { struct snd_timer_instance *ti, *ts, *tmp; - unsigned long resolution, ticks; - struct list_head *p, *ack_list_head; + unsigned long resolution; + struct list_head *ack_list_head; unsigned long flags; int use_tasklet = 0;
@@ -839,23 +845,7 @@ void snd_timer_interrupt(struct snd_timer * timer, unsigned long ticks_left) }
/* now process all fast callbacks */ - while (!list_empty(&timer->ack_list_head)) { - p = timer->ack_list_head.next; /* get first item */ - ti = list_entry(p, struct snd_timer_instance, ack_list); - - /* remove from ack_list and make empty */ - list_del_init(p); - - ticks = ti->pticks; - ti->pticks = 0; - - ti->flags |= SNDRV_TIMER_IFLG_CALLBACK; - spin_unlock(&timer->lock); - if (ti->callback) - ti->callback(ti, resolution, ticks); - spin_lock(&timer->lock); - ti->flags &= ~SNDRV_TIMER_IFLG_CALLBACK; - } + snd_timer_process_callbacks(timer, &timer->ack_list_head);
/* do we have any slow callbacks? */ use_tasklet = !list_empty(&timer->sack_list_head);
When a card is under disconnection, we bail out immediately at each timer interrupt or tasklet. This might leave some items left in ack list. For a better integration of the upcoming change to check ack_list emptiness, clear out the whole list upon the emergency exit route.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/timer.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/sound/core/timer.c b/sound/core/timer.c index fdcddfb756b4..107d8ebeeb2e 100644 --- a/sound/core/timer.c +++ b/sound/core/timer.c @@ -747,6 +747,18 @@ static void snd_timer_process_callbacks(struct snd_timer *timer, } }
+/* clear pending instances from ack list */ +static void snd_timer_clear_callbacks(struct snd_timer *timer, + struct list_head *head) +{ + unsigned long flags; + + spin_lock_irqsave(&timer->lock, flags); + while (!list_empty(head)) + list_del_init(head->next); + spin_unlock_irqrestore(&timer->lock, flags); +} + /* * timer tasklet * @@ -756,8 +768,10 @@ static void snd_timer_tasklet(unsigned long arg) struct snd_timer *timer = (struct snd_timer *) arg; unsigned long flags;
- if (timer->card && timer->card->shutdown) + if (timer->card && timer->card->shutdown) { + snd_timer_clear_callbacks(timer, &timer->sack_list_head); return; + }
spin_lock_irqsave(&timer->lock, flags); snd_timer_process_callbacks(timer, &timer->sack_list_head); @@ -781,8 +795,10 @@ void snd_timer_interrupt(struct snd_timer * timer, unsigned long ticks_left) if (timer == NULL) return;
- if (timer->card && timer->card->shutdown) + if (timer->card && timer->card->shutdown) { + snd_timer_clear_callbacks(timer, &timer->ack_list_head); return; + }
spin_lock_irqsave(&timer->lock, flags);
For checking the pending timer instance that is still left on the timer object that is being closed, we set/clear a bit flag SNDRV_TIMER_IFLG_CALLBACK around the call of callbacks. This can be simplified by replace with the list_empty() call for ti->ack_list. This covers the existence more comprehensively and safely.
A gratis bonus is that we can get rid of SNDRV_TIMER_IFLG_CALLBACK bit flag definition as well.
Signed-off-by: Takashi Iwai tiwai@suse.de --- include/sound/timer.h | 1 - sound/core/timer.c | 10 ++++------ 2 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/include/sound/timer.h b/include/sound/timer.h index 7ae226ab6990..bcfee20ea226 100644 --- a/include/sound/timer.h +++ b/include/sound/timer.h @@ -43,7 +43,6 @@ #define SNDRV_TIMER_IFLG_START 0x00000004 #define SNDRV_TIMER_IFLG_AUTO 0x00000008 /* auto restart */ #define SNDRV_TIMER_IFLG_FAST 0x00000010 /* fast callback (do not use tasklet) */ -#define SNDRV_TIMER_IFLG_CALLBACK 0x00000020 /* timer callback is active */ #define SNDRV_TIMER_IFLG_EXCLUSIVE 0x00000040 /* exclusive owner - no more instances */ #define SNDRV_TIMER_IFLG_EARLY_EVENT 0x00000080 /* write early event to the poll queue */
diff --git a/sound/core/timer.c b/sound/core/timer.c index 107d8ebeeb2e..e343de0e4f9e 100644 --- a/sound/core/timer.c +++ b/sound/core/timer.c @@ -366,7 +366,7 @@ static int snd_timer_close_locked(struct snd_timer_instance *timeri) timer->num_instances--; /* wait, until the active callback is finished */ spin_lock_irq(&timer->lock); - while (timeri->flags & SNDRV_TIMER_IFLG_CALLBACK) { + while (!list_empty(&timeri->ack_list)) { spin_unlock_irq(&timer->lock); udelay(10); spin_lock_irq(&timer->lock); @@ -731,19 +731,17 @@ static void snd_timer_process_callbacks(struct snd_timer *timer, ti = list_first_entry(head, struct snd_timer_instance, ack_list);
- /* remove from ack_list and make empty */ - list_del_init(&ti->ack_list); - ticks = ti->pticks; ti->pticks = 0; resolution = ti->resolution;
- ti->flags |= SNDRV_TIMER_IFLG_CALLBACK; spin_unlock(&timer->lock); if (ti->callback) ti->callback(ti, resolution, ticks); spin_lock(&timer->lock); - ti->flags &= ~SNDRV_TIMER_IFLG_CALLBACK; + + /* remove from ack_list and make empty */ + list_del_init(&ti->ack_list); } }
snd_timer_close() is supposed to close the timer instance and sync with the deactivation of pending actions. However, there are still some overlooked cases:
- It calls snd_timer_stop() at the beginning, but some other might re-trigger the timer right after that.
- snd_timer_stop() calls del_timer_sync() only when all belonging instances are closed. If multiple instances were assigned to a timer object and one is closed, the timer is still running. Then the pending action assigned to this timer might be left.
Actually either of the above is the likely cause of the reported syzkaller UAF.
This patch plug these holes by introducing SNDRV_TIMER_IFLG_DEAD flag. This is set at the beginning of snd_timer_close(), and the flag is checked at snd_timer_start*() and else, so that no longer new action is left after snd_timer_close().
Reported-by: syzbot+d5136d4d3240cbe45a2a@syzkaller.appspotmail.com Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/timer.c | 45 +++++++++++++++++++++++++++++++++------------ 1 file changed, 33 insertions(+), 12 deletions(-)
diff --git a/sound/core/timer.c b/sound/core/timer.c index e343de0e4f9e..bb7e90ab90f8 100644 --- a/sound/core/timer.c +++ b/sound/core/timer.c @@ -38,6 +38,7 @@
/* internal flags */ #define SNDRV_TIMER_IFLG_PAUSED 0x00010000 +#define SNDRV_TIMER_IFLG_DEAD 0x00020000
#if IS_ENABLED(CONFIG_SND_HRTIMER) #define DEFAULT_TIMER_LIMIT 4 @@ -353,15 +354,20 @@ EXPORT_SYMBOL(snd_timer_open); */ static int snd_timer_close_locked(struct snd_timer_instance *timeri) { - struct snd_timer *timer = NULL; + struct snd_timer *timer = timeri->timer; struct snd_timer_instance *slave, *tmp;
+ if (timer) { + spin_lock_irq(&timer->lock); + timeri->flags |= SNDRV_TIMER_IFLG_DEAD; + spin_unlock_irq(&timer->lock); + } + list_del(&timeri->open_list);
/* force to stop the timer */ snd_timer_stop(timeri);
- timer = timeri->timer; if (timer) { timer->num_instances--; /* wait, until the active callback is finished */ @@ -497,6 +503,10 @@ static int snd_timer_start1(struct snd_timer_instance *timeri, return -EINVAL;
spin_lock_irqsave(&timer->lock, flags); + if (timeri->flags & SNDRV_TIMER_IFLG_DEAD) { + result = -EINVAL; + goto unlock; + } if (timer->card && timer->card->shutdown) { result = -ENODEV; goto unlock; @@ -541,11 +551,16 @@ static int snd_timer_start_slave(struct snd_timer_instance *timeri, bool start) { unsigned long flags; + int err;
spin_lock_irqsave(&slave_active_lock, flags); + if (timeri->flags & SNDRV_TIMER_IFLG_DEAD) { + err = -EINVAL; + goto unlock; + } if (timeri->flags & SNDRV_TIMER_IFLG_RUNNING) { - spin_unlock_irqrestore(&slave_active_lock, flags); - return -EBUSY; + err = -EBUSY; + goto unlock; } timeri->flags |= SNDRV_TIMER_IFLG_RUNNING; if (timeri->master && timeri->timer) { @@ -556,8 +571,10 @@ static int snd_timer_start_slave(struct snd_timer_instance *timeri, SNDRV_TIMER_EVENT_CONTINUE); spin_unlock(&timeri->timer->lock); } + err = 1; /* delayed start */ + unlock: spin_unlock_irqrestore(&slave_active_lock, flags); - return 1; /* delayed start */ + return err; }
/* stop/pause a master timer */ @@ -731,14 +748,16 @@ static void snd_timer_process_callbacks(struct snd_timer *timer, ti = list_first_entry(head, struct snd_timer_instance, ack_list);
- ticks = ti->pticks; - ti->pticks = 0; - resolution = ti->resolution; + if (!(ti->flags & SNDRV_TIMER_IFLG_DEAD)) { + ticks = ti->pticks; + ti->pticks = 0; + resolution = ti->resolution;
- spin_unlock(&timer->lock); - if (ti->callback) - ti->callback(ti, resolution, ticks); - spin_lock(&timer->lock); + spin_unlock(&timer->lock); + if (ti->callback) + ti->callback(ti, resolution, ticks); + spin_lock(&timer->lock); + }
/* remove from ack_list and make empty */ list_del_init(&ti->ack_list); @@ -810,6 +829,8 @@ void snd_timer_interrupt(struct snd_timer * timer, unsigned long ticks_left) */ list_for_each_entry_safe(ti, tmp, &timer->active_list_head, active_list) { + if (ti->flags & SNDRV_TIMER_IFLG_DEAD) + continue; if (!(ti->flags & SNDRV_TIMER_IFLG_RUNNING)) continue; ti->pticks += ticks_left;
participants (1)
-
Takashi Iwai