Re: sound: out-of-bounds write in snd_rawmidi_kernel_write1

[Takashi Iwai]

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