linux-kernel - Re: [PATCH -next] ALSA: timer: Fix use-after-free problem

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 02 Nov 2021 15:10:48 +0100
From:   Takashi Iwai <tiwai@...e.de>
To:     Wang Wensheng <wangwensheng4@...wei.com>
Cc:     <perex@...ex.cz>, <tiwai@...e.com>, <broonie@...nel.org>,
        <joe@...ches.com>, <alsa-devel@...a-project.org>,
        <linux-kernel@...r.kernel.org>, <rui.xiang@...wei.com>
Subject: Re: [PATCH -next] ALSA: timer: Fix use-after-free problem

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@...wei.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
>