Re: sound: use-after-free in snd_timer_interrupt

[Dmitry Vyukov]

On Mon, Jan 18, 2016 at 2:06 PM, Takashi Iwai <tiwai@xxxxxxx> wrote:
>> No, unfortunately the hang still happens with the patch:
>
> Thanks for testing.  I think I understood the problem.  We faced a
> similar issue and moved hrtimer_cancel() in the past.  But this wasn't
> enough, as the start function may be called also in interrupt, too.
>
> How about the one below instead?


Yes, this fixes the hang. Thanks!

Tested-by: Dmitry Vyukov <dvyukov@xxxxxxxxxx>


> Takashi
>
> ---
> From: Takashi Iwai <tiwai@xxxxxxx>
> Subject: [PATCH] ALSA: hrtimer: Fix stall by hrtimer_cancel()
>
> hrtimer_cancel() waits for the completion from the callback, thus it
> must not be called inside the callback itself.  This was already a
> problem, and the early commit [fcfdebe70759: ALSA: hrtimer - Fix
> lock-up] tried to address it.
>
> However, the previous fix is still insufficient: it may still cause a
> lockup when the ALSA timer instance reprograms itself at its
> callback.  Then it invokes the start function even in
> snd_timer_interrupt() that is called in hrtimer callback itself,
> results in a CPU stall.  It's not a hypothetical problem, as actually
> triggered by syzkaller fuzzer.
>
> This patch tries to fix the issue again.  Now we call
> hrtimer_try_to_cancel() at both start and stop functions so that it
> won't fall into a deadlock, yet giving some chance to cancel the queue
> if the functions have been called outside the callback.  The proper
> hrtimer_cancel() is called in anyway at closing, so this should be
> enough.
>
> Reported-by: Dmitry Vyukov <dvyukov@xxxxxxxxxx>
> Cc: <stable@xxxxxxxxxxxxxxx>
> Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>
> ---
>  sound/core/hrtimer.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/sound/core/hrtimer.c b/sound/core/hrtimer.c
> index f845ecf7e172..656d9a9032dc 100644
> --- a/sound/core/hrtimer.c
> +++ b/sound/core/hrtimer.c
> @@ -90,7 +90,7 @@ static int snd_hrtimer_start(struct snd_timer *t)
>         struct snd_hrtimer *stime = t->private_data;
>
>         atomic_set(&stime->running, 0);
> -       hrtimer_cancel(&stime->hrt);
> +       hrtimer_try_to_cancel(&stime->hrt);
>         hrtimer_start(&stime->hrt, ns_to_ktime(t->sticks * resolution),
>                       HRTIMER_MODE_REL);
>         atomic_set(&stime->running, 1);
> @@ -101,6 +101,7 @@ static int snd_hrtimer_stop(struct snd_timer *t)
>  {
>         struct snd_hrtimer *stime = t->private_data;
>         atomic_set(&stime->running, 0);
> +       hrtimer_try_to_cancel(&stime->hrt);
>         return 0;
>  }
>
> --
> 2.7.0
>