Messages in this thread | | | Subject | Re: [PATCH -next] ALSA: timer: Fix use-after-free problem | From | "wangwensheng (C)" <> | Date | Wed, 3 Nov 2021 11:27:58 +0800 |
| |
在 2021/11/2 22:10, Takashi Iwai 写道: > 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: >
Thanks for your suggestions. It looks much more efficient and easier and is also effective. I will take this as a second version.
> --- 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
| |