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; }