linux-kernel - Re: sound: divide by 0 in snd_hrtimer

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: <s5hfupblpq8.wl-tiwai@suse.de>
Date:   Wed, 07 Sep 2016 17:00:15 +0200
From:   Takashi Iwai <tiwai@...e.de>
To:     Dmitry Vyukov <dvyukov@...gle.com>
Cc:     Vegard Nossum <vegard.nossum@...il.com>,
        Vegard Nossum <vegard.nossum@...cle.com>,
        Jaroslav Kysela <perex@...ex.cz>,
        Kangjie Lu <kangjielu@...il.com>, alsa-devel@...a-project.org,
        LKML <linux-kernel@...r.kernel.org>,
        syzkaller <syzkaller@...glegroups.com>
Subject: Re: sound: divide by 0 in snd_hrtimer_callback (or hang)

On Tue, 06 Sep 2016 18:10:30 +0200,
Takashi Iwai wrote:
> 
> On Tue, 06 Sep 2016 16:06:08 +0200,
> Dmitry Vyukov wrote:
> > 
> > > Do you really see the zero-division error with sticks=1 fix?  That's
> > > unexpected.
> > 
> > Yes, just reproduced it again.
> > I am on 0693c28cfc8e25f18c25b65a8942c026f1854a3c of linux-next now, so
> > I already have:
> >     commit 6b760bb2c63a9e322c0e4a0b5daf335ad93d5a33
> >     ALSA: timer: fix division by zero after SNDRV_TIMER_IOCTL_CONTINUE
> > 
> > Compiled the program verbatim and run it using stress utility (http
> > s://godoc.org/golang.org/x/tools/cmd/stress namely just runs the
> > program in parallel loop) and instantly get the following crash.
> 
> OK, I'll take a deeper look.

The fix patch below seems working on my system.  Please give it a
try.


Takashi

-- 8< --
From: Takashi Iwai <tiwai@...e.de>
Subject: [PATCH] ALSA: timer: Fix zero-division by continue of uninitialized
 instance

When a user timer instance is continued without the explicit start
beforehand, the system gets eventually zero-division error like:

  divide error: 0000 [#1] SMP DEBUG_PAGEALLOC KASAN
  CPU: 1 PID: 27320 Comm: syz-executor Not tainted 4.8.0-rc3-next-20160825+ #8
  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
   task: ffff88003c9b2280 task.stack: ffff880027280000
   RIP: 0010:[<ffffffff858e1a6c>]  [<     inline     >] ktime_divns include/linux/ktime.h:195
   RIP: 0010:[<ffffffff858e1a6c>]  [<ffffffff858e1a6c>] snd_hrtimer_callback+0x1bc/0x3c0 sound/core/hrtimer.c:62
  Call Trace:
   <IRQ>
   [<     inline     >] __run_hrtimer kernel/time/hrtimer.c:1238
   [<ffffffff81504335>] __hrtimer_run_queues+0x325/0xe70 kernel/time/hrtimer.c:1302
   [<ffffffff81506ceb>] hrtimer_interrupt+0x18b/0x420 kernel/time/hrtimer.c:1336
   [<ffffffff8126d8df>] local_apic_timer_interrupt+0x6f/0xe0 arch/x86/kernel/apic/apic.c:933
   [<ffffffff86e13056>] smp_apic_timer_interrupt+0x76/0xa0 arch/x86/kernel/apic/apic.c:957
   [<ffffffff86e1210c>] apic_timer_interrupt+0x8c/0xa0 arch/x86/entry/entry_64.S:487
   <EOI>
   .....

Although a similar issue was spotted and a fix patch was merged in
commit [6b760bb2c63a: ALSA: timer: fix division by zero after
SNDRV_TIMER_IOCTL_CONTINUE], it seems covering only a part of
iceberg.

In this patch, we fix the issue a bit more drastically.  Basically the
continue of an uninitialized timer is supposed to be a fresh start, so
we do it for user timers.  For the direct snd_timer_continue() call,
there is no way to pass the initial tick value, so we kick out for the
uninitialized case.

Reported-by: Reported-by: Dmitry Vyukov <dvyukov@...gle.com>
Cc: <stable@...r.kernel.org>
Signed-off-by: Takashi Iwai <tiwai@...e.de>
---
 sound/core/timer.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/sound/core/timer.c b/sound/core/timer.c
index 2706061fc1ea..fc144f43faa6 100644
--- a/sound/core/timer.c
+++ b/sound/core/timer.c
@@ -35,6 +35,9 @@
 #include <sound/initval.h>
 #include <linux/kmod.h>
 
+/* internal flags */
+#define SNDRV_TIMER_IFLG_PAUSED		0x00010000
+
 #if IS_ENABLED(CONFIG_SND_HRTIMER)
 #define DEFAULT_TIMER_LIMIT 4
 #else
@@ -539,6 +542,10 @@ static int snd_timer_stop1(struct snd_timer_instance *timeri, bool stop)
 		}
 	}
 	timeri->flags &= ~(SNDRV_TIMER_IFLG_RUNNING | SNDRV_TIMER_IFLG_START);
+	if (stop)
+		timeri->flags &= ~SNDRV_TIMER_IFLG_PAUSED;
+	else
+		timeri->flags |= SNDRV_TIMER_IFLG_PAUSED;
 	snd_timer_notify1(timeri, stop ? SNDRV_TIMER_EVENT_STOP :
 			  SNDRV_TIMER_EVENT_CONTINUE);
  unlock:
@@ -600,6 +607,10 @@ int snd_timer_stop(struct snd_timer_instance *timeri)
  */
 int snd_timer_continue(struct snd_timer_instance *timeri)
 {
+	/* timer can continue only after pause */
+	if (!(timeri->flags & SNDRV_TIMER_IFLG_PAUSED))
+		return -EINVAL;
+
 	if (timeri->flags & SNDRV_TIMER_IFLG_SLAVE)
 		return snd_timer_start_slave(timeri, false);
 	else
@@ -1831,6 +1842,9 @@ static int snd_timer_user_continue(struct file *file)
 	tu = file->private_data;
 	if (!tu->timeri)
 		return -EBADFD;
+	/* start timer instead of continue if it's not used before */
+	if (!(tu->timeri->flags & SNDRV_TIMER_IFLG_PAUSED))
+		return snd_timer_user_start(file);
 	tu->timeri->lost = 0;
 	return (err = snd_timer_continue(tu->timeri)) < 0 ? err : 0;
 }
-- 
2.10.0