Patch "USB: devio: Don't corrupt user memory" has been added to the 4.4-stable tree

PATCH [email protected] 3 participants 4 messages Oct 9, 2017 – Oct 17, 2017

Subsystems (2)

Stable branch USB

lore

Share & tools

lore

[email protected]

author maintainer Applied

2017-10-09 11:32 UTC #0 lore raw

This is a note to let you know that I've just added the patch titled

USB: devio: Don't corrupt user memory

to the 4.4-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary

The filename of the patch is:
usb-devio-don-t-corrupt-user-memory.patch
and it can be found in the queue-4.4 subdirectory.

If you, or anyone else, feels it should not be added to the stable tree, please let <[email protected]> know about it.

Quoted 1 line

From fa1ed74eb1c233be6131ec92df21ab46499a15b6 Mon Sep 17 00:00:00 2001

From: Dan Carpenter <[email protected]>
Date: Fri, 22 Sep 2017 23:43:46 +0300
Subject: USB: devio: Don't corrupt user memory

From: Dan Carpenter <[email protected]>

commit fa1ed74eb1c233be6131ec92df21ab46499a15b6 upstream.

The user buffer has "uurb->buffer_length" bytes. If the kernel has more information than that, we should truncate it instead of writing past the end of the user's buffer. I added a WARN_ONCE() to help the user debug the issue.

Reported-by Alan Stern <[email protected]>

Signed-off-by Dan Carpenter <[email protected]>

Acked-by Alan Stern <[email protected]>

Signed-off-by Greg Kroah-Hartman <[email protected]>

---
 drivers/usb/core/devio.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

drivers/usb/core/devio.c +5 -1

@@ -1417,7 +1417,11 @@ static int proc_do_submiturb(struct usb_
 			totlen += isopkt[u].length;
 		}
 		u *= sizeof(struct usb_iso_packet_descriptor);
-		uurb->buffer_length = totlen;
+		if (totlen <= uurb->buffer_length)
+			uurb->buffer_length = totlen;
+		else
+			WARN_ONCE(1, "uurb->buffer_length is too short %d vs %d",
+				  totlen, uurb->buffer_length);
 		break;
 
 	default:

Patches currently in stable-queue which might be from [email protected] are

queue-4.4/usb-devio-don-t-corrupt-user-memory.patch

Ben Hutchings

2017-10-17 14:36 UTC #1 lore raw

On Mon, 2017-10-09 at 13:31 +0200, [email protected] wrote:
[...]

Quoted 8 lines

From: Dan Carpenter <[email protected]>
commit fa1ed74eb1c233be6131ec92df21ab46499a15b6 upstream.
The user buffer has "uurb->buffer_length" bytes. If the kernel has more information than that, we should truncate it instead of writing past the end of the user's buffer. I added a WARN_ONCE() to help the user debug the issue.

[...]

Users should not be able to provoke a WARN_ON at will, that's a DoS (log spam, possible panic).

And this truncated user buffer length is also used for allocation of the kernel buffer. Are you totally sure that this can't result in a kernel buffer overrun (or leak)?

This fix seems worse than continuing to allow userspace to shoot itself in the foot.

Ben.

Signature · 2 lines

Ben Hutchings
Software Developer, Codethink Ltd.

Dan Carpenter

2017-10-17 14:48 UTC #2 lore raw

Quoted 15 lines · Ben Hutchings

On Mon, 2017-10-09 at 13:31 +0200, [email protected] wrote:
[...]
Quoted 8 lines
From: Dan Carpenter <[email protected]>
commit fa1ed74eb1c233be6131ec92df21ab46499a15b6 upstream.
The user buffer has "uurb->buffer_length" bytes.ï¿½ï¿½If the kernel has more information than that, we should truncate it instead of writing past the end of the user's buffer.ï¿½ï¿½I added a WARN_ONCE() to help the user debug the issue.
[...]
Users should not be able to provoke a WARN_ON at will, that's a DoS (log spam, possible panic).

Quoted 7 lines

And this truncated user buffer length is also used for allocation of the kernel buffer. Are you totally sure that this can't result in a kernel buffer overrun (or leak)?
This fix seems worse than continuing to allow userspace to shoot itself in the foot.

We don't want to add this because it breaks API and does actually lead to a leak. But it was a WARN_ONCE() not, a WARN_ON() so that part was ok. Probably it helped find the bug in my code.

regards,
dan carpenter

Greg Kroah-Hartman

author maintainer

2017-10-17 14:58 UTC #3 lore raw

In reply to Ben Hutchings

Quoted 15 lines · Ben Hutchings

On Mon, 2017-10-09 at 13:31 +0200, [email protected] wrote:
[...]
Quoted 8 lines
From: Dan Carpenter <[email protected]>
commit fa1ed74eb1c233be6131ec92df21ab46499a15b6 upstream.
The user buffer has "uurb->buffer_length" bytes.ï¿½ï¿½If the kernel has more information than that, we should truncate it instead of writing past the end of the user's buffer.ï¿½ï¿½I added a WARN_ONCE() to help the user debug the issue.
[...]
Users should not be able to provoke a WARN_ON at will, that's a DoS (log spam, possible panic).

Quoted 6 lines

And this truncated user buffer length is also used for allocation of the kernel buffer. Are you totally sure that this can't result in a kernel buffer overrun (or leak)?
This fix seems worse than continuing to allow userspace to shoot itself in the foot.

A fix reverting this has gone into my tree right now, and will get to Linus real-soon, to fix this issue.

thanks,

greg k-h