[alsa-devel] [PATCH v2 0/3] ALSA: possible race fixes for timer resolution callback
Hi,
this is a revised patchset for fixing the possible races among timer resolution callback and other operations. The first two are code cleanups and the last one actually covers it.
v1->v2: - Fix bogus return value from snd_timer_resolution() - Split the change in seq_timer.c to an individual patch
Takashi
===
Takashi Iwai (3): ALSA: timer: Simplify timer hw resolution calls ALSA: seq: Avoid open-code for getting timer resolution ALSA: timer: Assure timer resolution access always locked
sound/core/seq/seq_timer.c | 4 +--- sound/core/timer.c | 48 ++++++++++++++++++++++++---------------------- 2 files changed, 26 insertions(+), 26 deletions(-)
There multiple open-codes to get the hardware timer resolution. Make a local helper function snd_timer_hw_resolution() and call it from all relevant places.
There is no functional change by this, just a preliminary work for the following timer resolution hardening patch.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/timer.c | 33 ++++++++++++++------------------- 1 file changed, 14 insertions(+), 19 deletions(-)
diff --git a/sound/core/timer.c b/sound/core/timer.c index dc87728c5b74..23b941df9c43 100644 --- a/sound/core/timer.c +++ b/sound/core/timer.c @@ -427,6 +427,14 @@ int snd_timer_close(struct snd_timer_instance *timeri) } EXPORT_SYMBOL(snd_timer_close);
+static unsigned long snd_timer_hw_resolution(struct snd_timer *timer) +{ + if (timer->hw.c_resolution) + return timer->hw.c_resolution(timer); + else + return timer->hw.resolution; +} + unsigned long snd_timer_resolution(struct snd_timer_instance *timeri) { struct snd_timer * timer; @@ -434,11 +442,8 @@ unsigned long snd_timer_resolution(struct snd_timer_instance *timeri) if (timeri == NULL) return 0; timer = timeri->timer; - if (timer) { - if (timer->hw.c_resolution) - return timer->hw.c_resolution(timer); - return timer->hw.resolution; - } + if (timer) + return snd_timer_hw_resolution(timer); return 0; } EXPORT_SYMBOL(snd_timer_resolution); @@ -771,10 +776,7 @@ void snd_timer_interrupt(struct snd_timer * timer, unsigned long ticks_left) spin_lock_irqsave(&timer->lock, flags);
/* remember the current resolution */ - if (timer->hw.c_resolution) - resolution = timer->hw.c_resolution(timer); - else - resolution = timer->hw.resolution; + resolution = snd_timer_hw_resolution(timer);
/* loop for all active instances * Here we cannot use list_for_each_entry because the active_list of a @@ -1014,12 +1016,8 @@ void snd_timer_notify(struct snd_timer *timer, int event, struct timespec *tstam spin_lock_irqsave(&timer->lock, flags); if (event == SNDRV_TIMER_EVENT_MSTART || event == SNDRV_TIMER_EVENT_MCONTINUE || - event == SNDRV_TIMER_EVENT_MRESUME) { - if (timer->hw.c_resolution) - resolution = timer->hw.c_resolution(timer); - else - resolution = timer->hw.resolution; - } + event == SNDRV_TIMER_EVENT_MRESUME) + resolution = snd_timer_hw_resolution(timer); list_for_each_entry(ti, &timer->active_list_head, active_list) { if (ti->ccallback) ti->ccallback(ti, event, tstamp, resolution); @@ -1656,10 +1654,7 @@ static int snd_timer_user_gstatus(struct file *file, mutex_lock(®ister_mutex); t = snd_timer_find(&tid); if (t != NULL) { - if (t->hw.c_resolution) - gstatus.resolution = t->hw.c_resolution(t); - else - gstatus.resolution = t->hw.resolution; + gstatus.resolution = snd_timer_hw_resolution(t); if (t->hw.precise_resolution) { t->hw.precise_resolution(t, &gstatus.resolution_num, &gstatus.resolution_den);
Instead of open-coding for getting the timer resolution, use the standard snd_timer_resolution() helper.
The original code falls back to the callback function when the resolution is zero, but it must be always so when the callback function is defined. So this should be no functional change.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/seq/seq_timer.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/sound/core/seq/seq_timer.c b/sound/core/seq/seq_timer.c index 23167578231f..f587d0e27476 100644 --- a/sound/core/seq/seq_timer.c +++ b/sound/core/seq/seq_timer.c @@ -371,9 +371,7 @@ static int initialize_timer(struct snd_seq_timer *tmr)
tmr->ticks = 1; if (!(t->hw.flags & SNDRV_TIMER_HW_SLAVE)) { - unsigned long r = t->hw.resolution; - if (! r && t->hw.c_resolution) - r = t->hw.c_resolution(t); + unsigned long r = snd_timer_resolution(tmr->timeri); if (r) { tmr->ticks = (unsigned int)(1000000000uL / (r * freq)); if (! tmr->ticks)
On Thu, 2018-05-17 at 10:49 +0200, Takashi Iwai wrote:
Instead of open-coding for getting the timer resolution, use the standard snd_timer_resolution() helper.
The original code falls back to the callback function when the resolution is zero, but it must be always so when the callback function is defined. So this should be no functional change.
Maybe it *should* be so, but I can see three drivers where it isn't:
sound/isa/ad1816a/ad1816a_lib.c sound/isa/wss/wss_lib.c sound/sparc/cs4231.c
For ad1816a_lib.c, the c_resolution implementation always returns the same value that's in resolution, so this really doesn't make a functional change.
However, for the other two, the c_resolution implementation can return several different values.
Ben.
Signed-off-by: Takashi Iwai tiwai@suse.de
sound/core/seq/seq_timer.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/sound/core/seq/seq_timer.c b/sound/core/seq/seq_timer.c index 23167578231f..f587d0e27476 100644 --- a/sound/core/seq/seq_timer.c +++ b/sound/core/seq/seq_timer.c @@ -371,9 +371,7 @@ static int initialize_timer(struct snd_seq_timer *tmr) tmr->ticks = 1; if (!(t->hw.flags & SNDRV_TIMER_HW_SLAVE)) {
unsigned long r = t->hw.resolution;if (! r && t->hw.c_resolution)r = t->hw.c_resolution(t);
unsigned long r = snd_timer_resolution(tmr->timeri);if (r) { tmr->ticks = (unsigned int)(1000000000uL / (r * freq)); if (! tmr->ticks)
On Thu, 31 May 2018 19:18:53 +0200, Ben Hutchings wrote:
On Thu, 2018-05-17 at 10:49 +0200, Takashi Iwai wrote:
Instead of open-coding for getting the timer resolution, use the standard snd_timer_resolution() helper.
The original code falls back to the callback function when the resolution is zero, but it must be always so when the callback function is defined. So this should be no functional change.
Maybe it *should* be so, but I can see three drivers where it isn't:
sound/isa/ad1816a/ad1816a_lib.c sound/isa/wss/wss_lib.c sound/sparc/cs4231.c
For ad1816a_lib.c, the c_resolution implementation always returns the same value that's in resolution, so this really doesn't make a functional change.
However, for the other two, the c_resolution implementation can return several different values.
A good point. This implies that the patch fixes a long-standing issue :)
The ALSA timer API is very rarely used, however, maybe the only real application is the alsa-lib dmix/dsnoop. And the h/w resolution doesn't matter in that case (as it's deploying only the PCM slave timer), I don't think we'd hit any issues, practically seen.
thanks,
Takashi
Ben.
Signed-off-by: Takashi Iwai tiwai@suse.de
sound/core/seq/seq_timer.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/sound/core/seq/seq_timer.c b/sound/core/seq/seq_timer.c index 23167578231f..f587d0e27476 100644 --- a/sound/core/seq/seq_timer.c +++ b/sound/core/seq/seq_timer.c @@ -371,9 +371,7 @@ static int initialize_timer(struct snd_seq_timer *tmr) tmr->ticks = 1; if (!(t->hw.flags & SNDRV_TIMER_HW_SLAVE)) {
unsigned long r = t->hw.resolution;if (! r && t->hw.c_resolution)r = t->hw.c_resolution(t);
unsigned long r = snd_timer_resolution(tmr->timeri);if (r) { tmr->ticks = (unsigned int)(1000000000uL / (r * freq)); if (! tmr->ticks)
-- Ben Hutchings, Software Developer Codethink Ltd https://www.codethink.co.uk/ Dale House, 35 Dale Street Manchester, M1 2HF, United Kingdom
There are still many places calling the timer's hw.c_resolution callback without lock, and this may lead to some races, as we faced in the commit a820ccbe21e8 ("ALSA: pcm: Fix UAF at PCM release via PCM timer access").
This patch changes snd_timer_resolution() to take the timer->lock for avoiding the races. A place calling this function already inside the lock (from the notifier) is replaced with the snd_timer_hw_resolution() accordingly, as well as wrapping with the lock around another place calling snd_timer_hw_resolution(), too.
Reported-by: Ben Hutchings ben.hutchings@codethink.co.uk Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/timer.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-)
diff --git a/sound/core/timer.c b/sound/core/timer.c index 23b941df9c43..6b4cec0f3224 100644 --- a/sound/core/timer.c +++ b/sound/core/timer.c @@ -438,19 +438,24 @@ 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; + unsigned long flags;
if (timeri == NULL) return 0; timer = timeri->timer; - if (timer) - return snd_timer_hw_resolution(timer); - return 0; + if (timer) { + spin_lock_irqsave(&timer->lock, flags); + ret = snd_timer_hw_resolution(timer); + spin_unlock_irqrestore(&timer->lock, flags); + } + return ret; } EXPORT_SYMBOL(snd_timer_resolution);
static void snd_timer_notify1(struct snd_timer_instance *ti, int event) { - struct snd_timer *timer; + struct snd_timer *timer = ti->timer; unsigned long resolution = 0; struct snd_timer_instance *ts; struct timespec tstamp; @@ -462,14 +467,14 @@ static void snd_timer_notify1(struct snd_timer_instance *ti, int event) if (snd_BUG_ON(event < SNDRV_TIMER_EVENT_START || event > SNDRV_TIMER_EVENT_PAUSE)) return; - if (event == SNDRV_TIMER_EVENT_START || - event == SNDRV_TIMER_EVENT_CONTINUE) - resolution = snd_timer_resolution(ti); + if (timer && + (event == SNDRV_TIMER_EVENT_START || + event == SNDRV_TIMER_EVENT_CONTINUE)) + resolution = snd_timer_hw_resolution(timer); if (ti->ccallback) ti->ccallback(ti, event, &tstamp, resolution); if (ti->flags & SNDRV_TIMER_IFLG_SLAVE) return; - timer = ti->timer; if (timer == NULL) return; if (timer->hw.flags & SNDRV_TIMER_HW_SLAVE) @@ -1654,6 +1659,7 @@ static int snd_timer_user_gstatus(struct file *file, mutex_lock(®ister_mutex); t = snd_timer_find(&tid); if (t != NULL) { + spin_lock_irq(&t->lock); gstatus.resolution = snd_timer_hw_resolution(t); if (t->hw.precise_resolution) { t->hw.precise_resolution(t, &gstatus.resolution_num, @@ -1662,6 +1668,7 @@ static int snd_timer_user_gstatus(struct file *file, gstatus.resolution_num = gstatus.resolution; gstatus.resolution_den = 1000000000uL; } + spin_unlock_irq(&t->lock); } else { err = -ENODEV; }
participants (2)
-
Ben Hutchings -
Takashi Iwai