Re: sound: out-of-bounds write in snd_rawmidi_kernel_write1
From: Takashi Iwai
Date: Wed Feb 03 2016 - 09:26:51 EST
On Wed, 03 Feb 2016 14:37:17 +0100,
Dmitry Vyukov wrote:
>
> On Wed, Feb 3, 2016 at 1:02 PM, Takashi Iwai <tiwai@xxxxxxx> wrote:
> > 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.
>
>
> Yes, it fixes the crash for me. Thanks!
Great, I'll queue the fix. FWIW, below is the final form I'm going to
push.
thanks,
Takashi
-- 8< --
From: Takashi Iwai <tiwai@xxxxxxx>
Subject: [PATCH] ALSA: rawmidi: Fix race at copying & updating the position
The rawmidi read and write functions manage runtime stream status
such as runtime->appl_ptr and runtime->avail. These point where to
copy the new data and how many bytes have been copied (or to be
read). The problem is that rawmidi read/write call copy_from_user()
or copy_to_user(), and the runtime spinlock is temporarily unlocked
and relocked while copying user-space. Since the current code
advances and updates the runtime status after the spin unlock/relock,
the copy and the update may be asynchronous, and eventually
runtime->avail might go to a negative value when many concurrent
accesses are done. This may lead to memory corruption in the end.
For fixing this race, in this patch, the status update code is
performed in the same lock before the temporary unlock. Also, the
spinlock is now taken more widely in snd_rawmidi_kernel_read1() for
protecting more properly during the whole operation.
BugLink: http://lkml.kernel.org/r/CACT4Y+b-dCmNf1GpgPKfDO0ih+uZCL2JV4__j-r1kdhPLSgQCQ@xxxxxxxxxxxxxx
Reported-by: Dmitry Vyukov <dvyukov@xxxxxxxxxx>
Tested-by: Dmitry Vyukov <dvyukov@xxxxxxxxxx>
Cc: <stable@xxxxxxxxxxxxxxx>
Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>
---
sound/core/rawmidi.c | 34 ++++++++++++++++++++++------------
1 file changed, 22 insertions(+), 12 deletions(-)
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;
}
--
2.7.0