Skip to content

Commit

Permalink
ALSA: timer: Fix link corruption due to double start or stop
Browse files Browse the repository at this point in the history
Although ALSA timer code got hardening for races, it still causes
use-after-free error.  This is however rather a corrupted linked list,
not actually the concurrent accesses.  Namely, when timer start is
triggered twice, list_add_tail() is called twice, too.  This ends
up with the link corruption and triggers KASAN error.

The simplest fix would be replacing list_add_tail() with
list_move_tail(), but fundamentally it's the problem that we don't
check the double start/stop correctly.  So, the right fix here is to
add the proper checks to snd_timer_start() and snd_timer_stop() (and
their variants).

BugLink: http://lkml.kernel.org/r/CACT4Y+ZyPRoMQjmawbvmCEDrkBD2BQuH7R09=eOkf5ESK8kJAw@mail.gmail.com
Reported-by: Dmitry Vyukov <dvyukov@google.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
  • Loading branch information
Takashi Iwai committed Feb 1, 2016
1 parent 2cdc7b6 commit f784beb
Showing 1 changed file with 28 additions and 2 deletions.
30 changes: 28 additions & 2 deletions sound/core/timer.c
Original file line number Diff line number Diff line change
Expand Up @@ -451,6 +451,10 @@ static int snd_timer_start_slave(struct snd_timer_instance *timeri)
unsigned long flags;

spin_lock_irqsave(&slave_active_lock, flags);
if (timeri->flags & SNDRV_TIMER_IFLG_RUNNING) {
spin_unlock_irqrestore(&slave_active_lock, flags);
return -EBUSY;
}
timeri->flags |= SNDRV_TIMER_IFLG_RUNNING;
if (timeri->master && timeri->timer) {
spin_lock(&timeri->timer->lock);
Expand All @@ -475,7 +479,8 @@ int snd_timer_start(struct snd_timer_instance *timeri, unsigned int ticks)
return -EINVAL;
if (timeri->flags & SNDRV_TIMER_IFLG_SLAVE) {
result = snd_timer_start_slave(timeri);
snd_timer_notify1(timeri, SNDRV_TIMER_EVENT_START);
if (result >= 0)
snd_timer_notify1(timeri, SNDRV_TIMER_EVENT_START);
return result;
}
timer = timeri->timer;
Expand All @@ -484,11 +489,18 @@ int snd_timer_start(struct snd_timer_instance *timeri, unsigned int ticks)
if (timer->card && timer->card->shutdown)
return -ENODEV;
spin_lock_irqsave(&timer->lock, flags);
if (timeri->flags & (SNDRV_TIMER_IFLG_RUNNING |
SNDRV_TIMER_IFLG_START)) {
result = -EBUSY;
goto unlock;
}
timeri->ticks = timeri->cticks = ticks;
timeri->pticks = 0;
result = snd_timer_start1(timer, timeri, ticks);
unlock:
spin_unlock_irqrestore(&timer->lock, flags);
snd_timer_notify1(timeri, SNDRV_TIMER_EVENT_START);
if (result >= 0)
snd_timer_notify1(timeri, SNDRV_TIMER_EVENT_START);
return result;
}

Expand All @@ -502,6 +514,10 @@ static int _snd_timer_stop(struct snd_timer_instance *timeri, int event)

if (timeri->flags & SNDRV_TIMER_IFLG_SLAVE) {
spin_lock_irqsave(&slave_active_lock, flags);
if (!(timeri->flags & SNDRV_TIMER_IFLG_RUNNING)) {
spin_unlock_irqrestore(&slave_active_lock, flags);
return -EBUSY;
}
timeri->flags &= ~SNDRV_TIMER_IFLG_RUNNING;
list_del_init(&timeri->ack_list);
list_del_init(&timeri->active_list);
Expand All @@ -512,6 +528,11 @@ static int _snd_timer_stop(struct snd_timer_instance *timeri, int event)
if (!timer)
return -EINVAL;
spin_lock_irqsave(&timer->lock, flags);
if (!(timeri->flags & (SNDRV_TIMER_IFLG_RUNNING |
SNDRV_TIMER_IFLG_START))) {
spin_unlock_irqrestore(&timer->lock, flags);
return -EBUSY;
}
list_del_init(&timeri->ack_list);
list_del_init(&timeri->active_list);
if (timer->card && timer->card->shutdown) {
Expand Down Expand Up @@ -581,10 +602,15 @@ int snd_timer_continue(struct snd_timer_instance *timeri)
if (timer->card && timer->card->shutdown)
return -ENODEV;
spin_lock_irqsave(&timer->lock, flags);
if (timeri->flags & SNDRV_TIMER_IFLG_RUNNING) {
result = -EBUSY;
goto unlock;
}
if (!timeri->cticks)
timeri->cticks = 1;
timeri->pticks = 0;
result = snd_timer_start1(timer, timeri, timer->sticks);
unlock:
spin_unlock_irqrestore(&timer->lock, flags);
snd_timer_notify1(timeri, SNDRV_TIMER_EVENT_CONTINUE);
return result;
Expand Down

0 comments on commit f784beb

Please sign in to comment.