Re: sound: use-after-free in snd_timer_interrupt

From: Dmitry Vyukov
Date: Mon Jan 18 2016 - 08:30:39 EST


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
>