[PATCH 1/2] ALSA: timer: Simplify timer hw resolution calls

[PATCH 1/2] ALSA: timer: Simplify timer hw resolution calls

[Takashi Iwai]

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(&register_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);
-- 
2.16.3

[PATCH 2/2] ALSA: timer: Assure timer resolution access always locked

[Takashi Iwai]

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/seq/seq_timer.c |  4 +---
 sound/core/timer.c         | 21 ++++++++++++++-------
 2 files changed, 15 insertions(+), 10 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)
diff --git a/sound/core/timer.c b/sound/core/timer.c
index 23b941df9c43..753a9b9a0074 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);
+	if (timer) {
+		spin_lock_irqsave(&timer->lock, flags);
+		ret = snd_timer_hw_resolution(timer);
+		spin_unlock_irqrestore(&timer->lock, flags);
+	}
 	return 0;
 }
 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(&register_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;
 	}
-- 
2.16.3

Re: [PATCH 2/2] ALSA: timer: Assure timer resolution access always locked

[Takashi Iwai]

On Thu, 17 May 2018 00:10:48 +0200,
Takashi Iwai wrote:
> 
> @@ -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);
> +	if (timer) {
> +		spin_lock_irqsave(&timer->lock, flags);
> +		ret = snd_timer_hw_resolution(timer);
> +		spin_unlock_irqrestore(&timer->lock, flags);
> +	}
>  	return 0;

This must be return ret, of course.
Will resend v2 series addressing this.


Takashi