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@xxxxxxxxxx>
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
>
