Commit 417d58e0 authored by Takashi Iwai's avatar Takashi Iwai Committed by Luis Henriques

ALSA: rawmidi: Fix race at copying & updating the position

commit 81f57754 upstream.

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@mail.gmail.comReported-by: default avatarDmitry Vyukov <dvyukov@google.com>
Tested-by: default avatarDmitry Vyukov <dvyukov@google.com>
Signed-off-by: default avatarTakashi Iwai <tiwai@suse.de>
Signed-off-by: default avatarLuis Henriques <luis.henriques@canonical.com>
parent 530372de
...@@ -952,31 +952,36 @@ static long snd_rawmidi_kernel_read1(struct snd_rawmidi_substream *substream, ...@@ -952,31 +952,36 @@ static long snd_rawmidi_kernel_read1(struct snd_rawmidi_substream *substream,
unsigned long flags; unsigned long flags;
long result = 0, count1; long result = 0, count1;
struct snd_rawmidi_runtime *runtime = substream->runtime; struct snd_rawmidi_runtime *runtime = substream->runtime;
unsigned long appl_ptr;
spin_lock_irqsave(&runtime->lock, flags);
while (count > 0 && runtime->avail) { while (count > 0 && runtime->avail) {
count1 = runtime->buffer_size - runtime->appl_ptr; count1 = runtime->buffer_size - runtime->appl_ptr;
if (count1 > count) if (count1 > count)
count1 = count; count1 = count;
spin_lock_irqsave(&runtime->lock, flags);
if (count1 > (int)runtime->avail) if (count1 > (int)runtime->avail)
count1 = 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) if (kernelbuf)
memcpy(kernelbuf + result, runtime->buffer + runtime->appl_ptr, count1); memcpy(kernelbuf + result, runtime->buffer + appl_ptr, count1);
if (userbuf) { if (userbuf) {
spin_unlock_irqrestore(&runtime->lock, flags); spin_unlock_irqrestore(&runtime->lock, flags);
if (copy_to_user(userbuf + result, if (copy_to_user(userbuf + result,
runtime->buffer + runtime->appl_ptr, count1)) { runtime->buffer + appl_ptr, count1)) {
return result > 0 ? result : -EFAULT; return result > 0 ? result : -EFAULT;
} }
spin_lock_irqsave(&runtime->lock, flags); 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; result += count1;
count -= count1; count -= count1;
} }
spin_unlock_irqrestore(&runtime->lock, flags);
return result; return result;
} }
...@@ -1233,6 +1238,7 @@ static long snd_rawmidi_kernel_write1(struct snd_rawmidi_substream *substream, ...@@ -1233,6 +1238,7 @@ static long snd_rawmidi_kernel_write1(struct snd_rawmidi_substream *substream,
unsigned long flags; unsigned long flags;
long count1, result; long count1, result;
struct snd_rawmidi_runtime *runtime = substream->runtime; struct snd_rawmidi_runtime *runtime = substream->runtime;
unsigned long appl_ptr;
if (!kernelbuf && !userbuf) if (!kernelbuf && !userbuf)
return -EINVAL; return -EINVAL;
...@@ -1253,12 +1259,19 @@ static long snd_rawmidi_kernel_write1(struct snd_rawmidi_substream *substream, ...@@ -1253,12 +1259,19 @@ static long snd_rawmidi_kernel_write1(struct snd_rawmidi_substream *substream,
count1 = count; count1 = count;
if (count1 > (long)runtime->avail) if (count1 > (long)runtime->avail)
count1 = 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) if (kernelbuf)
memcpy(runtime->buffer + runtime->appl_ptr, memcpy(runtime->buffer + appl_ptr,
kernelbuf + result, count1); kernelbuf + result, count1);
else if (userbuf) { else if (userbuf) {
spin_unlock_irqrestore(&runtime->lock, flags); 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)) { userbuf + result, count1)) {
spin_lock_irqsave(&runtime->lock, flags); spin_lock_irqsave(&runtime->lock, flags);
result = result > 0 ? result : -EFAULT; result = result > 0 ? result : -EFAULT;
...@@ -1266,9 +1279,6 @@ static long snd_rawmidi_kernel_write1(struct snd_rawmidi_substream *substream, ...@@ -1266,9 +1279,6 @@ static long snd_rawmidi_kernel_write1(struct snd_rawmidi_substream *substream,
} }
spin_lock_irqsave(&runtime->lock, flags); spin_lock_irqsave(&runtime->lock, flags);
} }
runtime->appl_ptr += count1;
runtime->appl_ptr %= runtime->buffer_size;
runtime->avail -= count1;
result += count1; result += count1;
count -= count1; count -= count1;
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment