| From b5a663aa426f4884c71cd8580adae73f33570f0d Mon Sep 17 00:00:00 2001 |
| From: Takashi Iwai <tiwai@suse.de> |
| Date: Thu, 14 Jan 2016 16:30:58 +0100 |
| Subject: ALSA: timer: Harden slave timer list handling |
| |
| From: Takashi Iwai <tiwai@suse.de> |
| |
| commit b5a663aa426f4884c71cd8580adae73f33570f0d upstream. |
| |
| A slave timer instance might be still accessible in a racy way while |
| operating the master instance as it lacks of locking. Since the |
| master operation is mostly protected with timer->lock, we should cope |
| with it while changing the slave instance, too. Also, some linked |
| lists (active_list and ack_list) of slave instances aren't unlinked |
| immediately at stopping or closing, and this may lead to unexpected |
| accesses. |
| |
| This patch tries to address these issues. It adds spin lock of |
| timer->lock (either from master or slave, which is equivalent) in a |
| few places. For avoiding a deadlock, we ensure that the global |
| slave_active_lock is always locked at first before each timer lock. |
| |
| Also, ack and active_list of slave instances are properly unlinked at |
| snd_timer_stop() and snd_timer_close(). |
| |
| Last but not least, remove the superfluous call of _snd_timer_stop() |
| at removing slave links. This is a noop, and calling it may confuse |
| readers wrt locking. Further cleanup will follow in a later patch. |
| |
| Actually we've got reports of use-after-free by syzkaller fuzzer, and |
| this hopefully fixes these issues. |
| |
| Reported-by: Dmitry Vyukov <dvyukov@google.com> |
| Signed-off-by: Takashi Iwai <tiwai@suse.de> |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| |
| --- |
| sound/core/timer.c | 18 ++++++++++++++---- |
| 1 file changed, 14 insertions(+), 4 deletions(-) |
| |
| --- a/sound/core/timer.c |
| +++ b/sound/core/timer.c |
| @@ -215,11 +215,13 @@ static void snd_timer_check_master(struc |
| slave->slave_id == master->slave_id) { |
| list_move_tail(&slave->open_list, &master->slave_list_head); |
| spin_lock_irq(&slave_active_lock); |
| + spin_lock(&master->timer->lock); |
| slave->master = master; |
| slave->timer = master->timer; |
| if (slave->flags & SNDRV_TIMER_IFLG_RUNNING) |
| list_add_tail(&slave->active_list, |
| &master->slave_active_head); |
| + spin_unlock(&master->timer->lock); |
| spin_unlock_irq(&slave_active_lock); |
| } |
| } |
| @@ -346,15 +348,18 @@ int snd_timer_close(struct snd_timer_ins |
| timer->hw.close) |
| timer->hw.close(timer); |
| /* remove slave links */ |
| + spin_lock_irq(&slave_active_lock); |
| + spin_lock(&timer->lock); |
| list_for_each_entry_safe(slave, tmp, &timeri->slave_list_head, |
| open_list) { |
| - spin_lock_irq(&slave_active_lock); |
| - _snd_timer_stop(slave, 1, SNDRV_TIMER_EVENT_RESOLUTION); |
| list_move_tail(&slave->open_list, &snd_timer_slave_list); |
| slave->master = NULL; |
| slave->timer = NULL; |
| - spin_unlock_irq(&slave_active_lock); |
| + list_del_init(&slave->ack_list); |
| + list_del_init(&slave->active_list); |
| } |
| + spin_unlock(&timer->lock); |
| + spin_unlock_irq(&slave_active_lock); |
| mutex_unlock(®ister_mutex); |
| } |
| out: |
| @@ -441,9 +446,12 @@ static int snd_timer_start_slave(struct |
| |
| spin_lock_irqsave(&slave_active_lock, flags); |
| timeri->flags |= SNDRV_TIMER_IFLG_RUNNING; |
| - if (timeri->master) |
| + if (timeri->master && timeri->timer) { |
| + spin_lock(&timeri->timer->lock); |
| list_add_tail(&timeri->active_list, |
| &timeri->master->slave_active_head); |
| + spin_unlock(&timeri->timer->lock); |
| + } |
| spin_unlock_irqrestore(&slave_active_lock, flags); |
| return 1; /* delayed start */ |
| } |
| @@ -489,6 +497,8 @@ static int _snd_timer_stop(struct snd_ti |
| if (!keep_flag) { |
| spin_lock_irqsave(&slave_active_lock, flags); |
| timeri->flags &= ~SNDRV_TIMER_IFLG_RUNNING; |
| + list_del_init(&timeri->ack_list); |
| + list_del_init(&timeri->active_list); |
| spin_unlock_irqrestore(&slave_active_lock, flags); |
| } |
| goto __end; |