Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 02 Nov 2021 15:10:48 +0100
Message-ID: <s5h5ytaps0n.wl-tiwai@suse.de>
From:   Takashi Iwai <tiwai@suse.de>
To:     Wang Wensheng <wangwensheng4@huawei.com>
Cc:     <perex@perex.cz>, <tiwai@suse.com>, <broonie@kernel.org>,
        <joe@perches.com>, <alsa-devel@alsa-project.org>,
        <linux-kernel@vger.kernel.org>, <rui.xiang@huawei.com>
Subject: Re: [PATCH -next] ALSA: timer: Fix use-after-free problem
In-Reply-To: <20211102134107.35126-1-wangwensheng4@huawei.com>
References: <20211102134107.35126-1-wangwensheng4@huawei.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka)
 FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.3
 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Tue, 02 Nov 2021 14:41:07 +0100,
Wang Wensheng wrote:
> 
> When the timer instance was add into ack_list but was not currently in
> process, the user could stop it via snd_timer_stop1() without delete it
> from the ack_list. Then the user could free the timer instance and when
> it was actually processed UAF occurred.
> 
> This issue could be reproduced via testcase snd_timer01 in ltp - running
> several instances of that testcase at the same time.
> 
> What I actually met was that the ack_list of the timer broken and the
> kernel went into deadloop with irqoff. That could be detected by
> hardlockup detector on board or when we run it on qemu, we could use gdb
> to dump the ack_list when the console has no response.
> 
> To fix this issue, we introduce a new flag SNDRV_TIMER_IFLG_ACKING to
> indicate the state where the timer instance is in ack_list but not
> currently processed and check against the new flag in snd_timer_stop1()
> and delete it from ack_list if the flag is set.
> 
> Signed-off-by: Wang Wensheng <wangwensheng4@huawei.com>

Thanks for the patch.  Just through a quick glance, I wonder whether
it'd be easier to do list_del_init(&timeri->ack_list) unconditionally
before the check of timeri->flags in snd_timer1_stop().  Ditto for
active_list.  So something like:

--- a/sound/core/timer.c
+++ b/sound/core/timer.c
@@ -624,13 +624,13 @@ static int snd_timer_stop1(struct snd_timer_instance *timeri, bool stop)
 	if (!timer)
 		return -EINVAL;
 	spin_lock_irqsave(&timer->lock, flags);
+	list_del_init(&timeri->ack_list);
+	list_del_init(&timeri->active_list);
 	if (!(timeri->flags & (SNDRV_TIMER_IFLG_RUNNING |
 			       SNDRV_TIMER_IFLG_START))) {
 		result = -EBUSY;
 		goto unlock;
 	}
-	list_del_init(&timeri->ack_list);
-	list_del_init(&timeri->active_list);
 	if (timer->card && timer->card->shutdown)
 		goto unlock;
 	if (stop) {


Takashi

> ---
>  include/sound/timer.h |  1 +
>  sound/core/timer.c    | 12 +++++++++---
>  2 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/include/sound/timer.h b/include/sound/timer.h
> index 760e132cc0cd..549288e94a39 100644
> --- a/include/sound/timer.h
> +++ b/include/sound/timer.h
> @@ -31,6 +31,7 @@
>  #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 */
> +#define SNDRV_TIMER_IFLG_ACKING    0x00000100	/* the timeri was added to ack_list */
>  
>  #define SNDRV_TIMER_FLG_CHANGE	0x00000001
>  #define SNDRV_TIMER_FLG_RESCHED	0x00000002	/* need reschedule */
> diff --git a/sound/core/timer.c b/sound/core/timer.c
> index 92b7008fcdb8..1d1e4274919c 100644
> --- a/sound/core/timer.c
> +++ b/sound/core/timer.c
> @@ -625,10 +625,12 @@ static int snd_timer_stop1(struct snd_timer_instance *timeri, bool stop)
>  		return -EINVAL;
>  	spin_lock_irqsave(&timer->lock, flags);
>  	if (!(timeri->flags & (SNDRV_TIMER_IFLG_RUNNING |
> -			       SNDRV_TIMER_IFLG_START))) {
> +			       SNDRV_TIMER_IFLG_START |
> +			       SNDRV_TIMER_IFLG_ACKING))) {
>  		result = -EBUSY;
>  		goto unlock;
>  	}
> +
>  	list_del_init(&timeri->ack_list);
>  	list_del_init(&timeri->active_list);
>  	if (timer->card && timer->card->shutdown)
> @@ -649,7 +651,8 @@ static int snd_timer_stop1(struct snd_timer_instance *timeri, bool stop)
>  			}
>  		}
>  	}
> -	timeri->flags &= ~(SNDRV_TIMER_IFLG_RUNNING | SNDRV_TIMER_IFLG_START);
> +	timeri->flags &= ~(SNDRV_TIMER_IFLG_RUNNING | SNDRV_TIMER_IFLG_START |
> +			   SNDRV_TIMER_IFLG_ACKING);
>  	if (stop)
>  		timeri->flags &= ~SNDRV_TIMER_IFLG_PAUSED;
>  	else
> @@ -786,6 +789,7 @@ static void snd_timer_process_callbacks(struct snd_timer *timer,
>  
>  		/* remove from ack_list and make empty */
>  		list_del_init(&ti->ack_list);
> +		ti->flags &= ~SNDRV_TIMER_IFLG_ACKING;
>  
>  		if (!(ti->flags & SNDRV_TIMER_IFLG_DEAD)) {
>  			ticks = ti->pticks;
> @@ -890,8 +894,10 @@ void snd_timer_interrupt(struct snd_timer * timer, unsigned long ticks_left)
>  			ack_list_head = &timer->ack_list_head;
>  		else
>  			ack_list_head = &timer->sack_list_head;
> -		if (list_empty(&ti->ack_list))
> +		if (list_empty(&ti->ack_list)) {
>  			list_add_tail(&ti->ack_list, ack_list_head);
> +			ti->flags |= SNDRV_TIMER_IFLG_ACKING;
> +		}
>  		list_for_each_entry(ts, &ti->slave_active_head, active_list) {
>  			ts->pticks = ti->pticks;
>  			ts->resolution = resolution;
> -- 
> 2.17.1
>