From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756031AbcBCMCi (ORCPT ); Wed, 3 Feb 2016 07:02:38 -0500 Received: from mx2.suse.de ([195.135.220.15]:59944 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751308AbcBCMCh (ORCPT ); Wed, 3 Feb 2016 07:02:37 -0500 Date: Wed, 03 Feb 2016 13:02:35 +0100 Message-ID: From: Takashi Iwai To: "Dmitry Vyukov" Cc: , "Jaroslav Kysela" , "LKML" , "Alexander Potapenko" , "Kostya Serebryany" , "syzkaller" , "Sasha Levin" Subject: Re: sound: out-of-bounds write in snd_rawmidi_kernel_write1 In-Reply-To: References: User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/24.5 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 03 Feb 2016 12:39:31 +0100, Takashi Iwai wrote: > > On Wed, 03 Feb 2016 10:41:14 +0100, > Takashi Iwai wrote: > > > > On Wed, 03 Feb 2016 10:35:14 +0100, > > Takashi Iwai wrote: > > > > > > On Wed, 03 Feb 2016 09:57:50 +0100, > > > Dmitry Vyukov wrote: > > > > > > > > Hello, > > > > > > > > The following program triggers an out-of-bounds write in > > > > snd_rawmidi_kernel_write1 (run in parallel loop). It seems to try to > > > > copy -1 bytes (aka 4GB) from user space into kernel smashing all on > > > > its way. > > > > > > What card is /dev/midi3? Please check /proc/asound/cards. > > > Is it MTPAV? > > > > In anyway the patch below should paper over it. But it's still > > strange that it gets a negative value there. Could you put > > > > WARN_ON(count1 < 0) > > > > before the newly added check? > > > > I tried it locally with virmidi but it didn't appear, so far. Maybe > > my setup is too slow and has fewer CPUs than yours. > > Scratch my previous patch, I could reproduce the issue on a faster > machine in my office now :) Will work on it. This turned out to be a race in updates of runtime->appl_ptr & co. We do temporary spin unlock and relock while copying the user-space data, and then update these values. Meanwhile these values are referred as the position to copy, and the concurrent accesses may lead to the negative value. Below is a quick fix for that, just updating these before the temporary unlock. The patch also fixes the read size where it has more race problems... It seems working on my machine. Let me know if this works for you, too. Then I'll cook up the official patch. thanks, Takashi --- diff --git a/sound/core/rawmidi.c b/sound/core/rawmidi.c index 26ca02248885..795437b10082 100644 --- a/sound/core/rawmidi.c +++ b/sound/core/rawmidi.c @@ -942,31 +942,36 @@ static long snd_rawmidi_kernel_read1(struct snd_rawmidi_substream *substream, unsigned long flags; long result = 0, count1; struct snd_rawmidi_runtime *runtime = substream->runtime; + unsigned long appl_ptr; + spin_lock_irqsave(&runtime->lock, flags); while (count > 0 && runtime->avail) { count1 = runtime->buffer_size - runtime->appl_ptr; if (count1 > count) count1 = count; - spin_lock_irqsave(&runtime->lock, flags); if (count1 > (int)runtime->avail) count1 = runtime->avail; + + /* update runtime->appl_ptr before unlocking for userbuf */ + appl_ptr = runtime->appl_ptr; + runtime->appl_ptr += count1; + runtime->appl_ptr %= runtime->buffer_size; + runtime->avail -= count1; + if (kernelbuf) - memcpy(kernelbuf + result, runtime->buffer + runtime->appl_ptr, count1); + memcpy(kernelbuf + result, runtime->buffer + appl_ptr, count1); if (userbuf) { spin_unlock_irqrestore(&runtime->lock, flags); if (copy_to_user(userbuf + result, - runtime->buffer + runtime->appl_ptr, count1)) { + runtime->buffer + appl_ptr, count1)) { return result > 0 ? result : -EFAULT; } spin_lock_irqsave(&runtime->lock, flags); } - runtime->appl_ptr += count1; - runtime->appl_ptr %= runtime->buffer_size; - runtime->avail -= count1; - spin_unlock_irqrestore(&runtime->lock, flags); result += count1; count -= count1; } + spin_unlock_irqrestore(&runtime->lock, flags); return result; } @@ -1223,6 +1228,7 @@ static long snd_rawmidi_kernel_write1(struct snd_rawmidi_substream *substream, unsigned long flags; long count1, result; struct snd_rawmidi_runtime *runtime = substream->runtime; + unsigned long appl_ptr; if (!kernelbuf && !userbuf) return -EINVAL; @@ -1243,12 +1249,19 @@ static long snd_rawmidi_kernel_write1(struct snd_rawmidi_substream *substream, count1 = count; if (count1 > (long)runtime->avail) count1 = runtime->avail; + + /* update runtime->appl_ptr before unlocking for userbuf */ + appl_ptr = runtime->appl_ptr; + runtime->appl_ptr += count1; + runtime->appl_ptr %= runtime->buffer_size; + runtime->avail -= count1; + if (kernelbuf) - memcpy(runtime->buffer + runtime->appl_ptr, + memcpy(runtime->buffer + appl_ptr, kernelbuf + result, count1); else if (userbuf) { spin_unlock_irqrestore(&runtime->lock, flags); - if (copy_from_user(runtime->buffer + runtime->appl_ptr, + if (copy_from_user(runtime->buffer + appl_ptr, userbuf + result, count1)) { spin_lock_irqsave(&runtime->lock, flags); result = result > 0 ? result : -EFAULT; @@ -1256,9 +1269,6 @@ static long snd_rawmidi_kernel_write1(struct snd_rawmidi_substream *substream, } spin_lock_irqsave(&runtime->lock, flags); } - runtime->appl_ptr += count1; - runtime->appl_ptr %= runtime->buffer_size; - runtime->avail -= count1; result += count1; count -= count1; } From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: sound: out-of-bounds write in snd_rawmidi_kernel_write1 Date: Wed, 03 Feb 2016 13:02:35 +0100 Message-ID: References: Mime-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Return-path: In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org To: Dmitry Vyukov Cc: alsa-devel@alsa-project.org, Jaroslav Kysela , LKML , Alexander Potapenko , Kostya Serebryany , syzkaller , Sasha Levin List-Id: alsa-devel@alsa-project.org On Wed, 03 Feb 2016 12:39:31 +0100, Takashi Iwai wrote: > > On Wed, 03 Feb 2016 10:41:14 +0100, > Takashi Iwai wrote: > > > > On Wed, 03 Feb 2016 10:35:14 +0100, > > Takashi Iwai wrote: > > > > > > On Wed, 03 Feb 2016 09:57:50 +0100, > > > Dmitry Vyukov wrote: > > > > > > > > Hello, > > > > > > > > The following program triggers an out-of-bounds write in > > > > snd_rawmidi_kernel_write1 (run in parallel loop). It seems to try to > > > > copy -1 bytes (aka 4GB) from user space into kernel smashing all on > > > > its way. > > > > > > What card is /dev/midi3? Please check /proc/asound/cards. > > > Is it MTPAV? > > > > In anyway the patch below should paper over it. But it's still > > strange that it gets a negative value there. Could you put > > > > WARN_ON(count1 < 0) > > > > before the newly added check? > > > > I tried it locally with virmidi but it didn't appear, so far. Maybe > > my setup is too slow and has fewer CPUs than yours. > > Scratch my previous patch, I could reproduce the issue on a faster > machine in my office now :) Will work on it. This turned out to be a race in updates of runtime->appl_ptr & co. We do temporary spin unlock and relock while copying the user-space data, and then update these values. Meanwhile these values are referred as the position to copy, and the concurrent accesses may lead to the negative value. Below is a quick fix for that, just updating these before the temporary unlock. The patch also fixes the read size where it has more race problems... It seems working on my machine. Let me know if this works for you, too. Then I'll cook up the official patch. thanks, Takashi --- diff --git a/sound/core/rawmidi.c b/sound/core/rawmidi.c index 26ca02248885..795437b10082 100644 --- a/sound/core/rawmidi.c +++ b/sound/core/rawmidi.c @@ -942,31 +942,36 @@ static long snd_rawmidi_kernel_read1(struct snd_rawmidi_substream *substream, unsigned long flags; long result = 0, count1; struct snd_rawmidi_runtime *runtime = substream->runtime; + unsigned long appl_ptr; + spin_lock_irqsave(&runtime->lock, flags); while (count > 0 && runtime->avail) { count1 = runtime->buffer_size - runtime->appl_ptr; if (count1 > count) count1 = count; - spin_lock_irqsave(&runtime->lock, flags); if (count1 > (int)runtime->avail) count1 = runtime->avail; + + /* update runtime->appl_ptr before unlocking for userbuf */ + appl_ptr = runtime->appl_ptr; + runtime->appl_ptr += count1; + runtime->appl_ptr %= runtime->buffer_size; + runtime->avail -= count1; + if (kernelbuf) - memcpy(kernelbuf + result, runtime->buffer + runtime->appl_ptr, count1); + memcpy(kernelbuf + result, runtime->buffer + appl_ptr, count1); if (userbuf) { spin_unlock_irqrestore(&runtime->lock, flags); if (copy_to_user(userbuf + result, - runtime->buffer + runtime->appl_ptr, count1)) { + runtime->buffer + appl_ptr, count1)) { return result > 0 ? result : -EFAULT; } spin_lock_irqsave(&runtime->lock, flags); } - runtime->appl_ptr += count1; - runtime->appl_ptr %= runtime->buffer_size; - runtime->avail -= count1; - spin_unlock_irqrestore(&runtime->lock, flags); result += count1; count -= count1; } + spin_unlock_irqrestore(&runtime->lock, flags); return result; } @@ -1223,6 +1228,7 @@ static long snd_rawmidi_kernel_write1(struct snd_rawmidi_substream *substream, unsigned long flags; long count1, result; struct snd_rawmidi_runtime *runtime = substream->runtime; + unsigned long appl_ptr; if (!kernelbuf && !userbuf) return -EINVAL; @@ -1243,12 +1249,19 @@ static long snd_rawmidi_kernel_write1(struct snd_rawmidi_substream *substream, count1 = count; if (count1 > (long)runtime->avail) count1 = runtime->avail; + + /* update runtime->appl_ptr before unlocking for userbuf */ + appl_ptr = runtime->appl_ptr; + runtime->appl_ptr += count1; + runtime->appl_ptr %= runtime->buffer_size; + runtime->avail -= count1; + if (kernelbuf) - memcpy(runtime->buffer + runtime->appl_ptr, + memcpy(runtime->buffer + appl_ptr, kernelbuf + result, count1); else if (userbuf) { spin_unlock_irqrestore(&runtime->lock, flags); - if (copy_from_user(runtime->buffer + runtime->appl_ptr, + if (copy_from_user(runtime->buffer + appl_ptr, userbuf + result, count1)) { spin_lock_irqsave(&runtime->lock, flags); result = result > 0 ? result : -EFAULT; @@ -1256,9 +1269,6 @@ static long snd_rawmidi_kernel_write1(struct snd_rawmidi_substream *substream, } spin_lock_irqsave(&runtime->lock, flags); } - runtime->appl_ptr += count1; - runtime->appl_ptr %= runtime->buffer_size; - runtime->avail -= count1; result += count1; count -= count1; }