linux-kernel - Re: sound: use-after-free in snd_timer

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]

Message-ID: <CACT4Y+bVVxUeqZnwAbLwT3dCGcpRMG9B8WZMv3jk3SHjwMWW2w@mail.gmail.com>
Date:	Mon, 18 Jan 2016 14:30:13 +0100
From:	Dmitry Vyukov <dvyukov@...gle.com>
To:	Takashi Iwai <tiwai@...e.de>
Cc:	alsa-devel@...a-project.org, Jie Yang <yang.jie@...el.com>,
	Mark Brown <broonie@...nel.org>,
	Jaroslav Kysela <perex@...ex.cz>,
	LKML <linux-kernel@...r.kernel.org>,
	Eric Dumazet <edumazet@...gle.com>,
	Alexander Potapenko <glider@...gle.com>,
	Kostya Serebryany <kcc@...gle.com>,
	syzkaller <syzkaller@...glegroups.com>,
	Sasha Levin <sasha.levin@...cle.com>
Subject: Re: sound: use-after-free in snd_timer_interrupt

On Mon, Jan 18, 2016 at 2:06 PM, Takashi Iwai <tiwai@...e.de> 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@...gle.com>


> Takashi
>
> ---
> From: Takashi Iwai <tiwai@...e.de>
> 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@...gle.com>
> Cc: <stable@...r.kernel.org>
> Signed-off-by: Takashi Iwai <tiwai@...e.de>
> ---
>  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
>