Skip to contentSkip to conversation

usb: core: devio: add defensive bounds check for len2 in do_proc_bulk()

PATCH In review rivaldihormat-debug 3 participants 3 messages Sep 4 USB

lore
Share & tools
lore

rivaldihormat-debug

author
#0 lore raw

PATCH 1/1 usb: core: devio: add defensive bounds check for len2 in do_proc_bulk()

Although the USB host controller hardware prevents len2 from exceeding len1 in normal operation via babble error handling, add an explicit kernel-side check to improve robustness. This helps prevent unexpected behavior in virtualized environments where emulation bugs might break this guarantee.

Clamp len2 to len1 if it exceeds the allocated buffer size and log a warning message.

Signed-off-by rivaldihormat-debug <[email protected]>
---
 drivers/usb/core/devio.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/usb/core/devio.c +4 -0
index 101cb9425480..9d8930853e8f 100644
@@ -1343,6 +1343,10 @@ static int do_proc_bulk(struct usb_dev_state *ps,
 		snoop_urb(dev, NULL, pipe, len2, i, COMPLETE, tbuf, len2);
 
 		if (!i && len2) {
+                        if (len2 > len1) {
+                                dev_warn(&dev->dev, "USB returned more data than expected\n");
+                                len2 = len1;
+                        }
 			if (copy_to_user(bulk->data, tbuf, len2)) {
 				ret = -EFAULT;
 				goto done;
Signature · 1 line
2.53.0
maintainer
#1 lore raw
Quoted 5 lines · rivaldihormat-debug

Although the USB host controller hardware prevents len2 from exceeding len1 in normal operation via babble error handling, add an explicit kernel-side check to improve robustness. This helps prevent unexpected behavior in virtualized environments where emulation bugs might break this guarantee.

Then that emulation environment is very very broken, please fix that.

Quoted 4 lines

Clamp len2 to len1 if it exceeds the allocated buffer size and log a warning message.

Signed-off-by rivaldihormat-debug <[email protected]>

Not a valid name to use here :(

Quoted 9 lines
---
 drivers/usb/core/devio.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/usb/core/devio.c +0 -0
index 101cb9425480..9d8930853e8f 100644

drivers/usb/core/devio.c static int do_proc_bulk(struct usb_dev_state *ps, · line 1348

Quoted 6 lines
drivers/usb/core/devio.c +3 -0
@@ -1343,6 +1343,10 @@ static int do_proc_bulk(struct usb_dev_state *ps,
 		snoop_urb(dev, NULL, pipe, len2, i, COMPLETE, tbuf, len2);
 
 		if (!i && len2) {
+                        if (len2 > len1) {
+                                dev_warn(&dev->dev, "USB returned more data than expected\n");
+                                len2 = len1;

This isn't even a good way to handle this error if it happened :(

#2 lore raw
In reply to rivaldihormat-debug
Quoted 5 lines · rivaldihormat-debug

Although the USB host controller hardware prevents len2 from exceeding len1 in normal operation via babble error handling, add an explicit kernel-side check to improve robustness. This helps prevent unexpected behavior in virtualized environments where emulation bugs might break this guarantee.

In the kernel, we do not try to protect against all possible hardware or software errors. For one thing, it's impossible. For another, it's a waste of time and space. Especially if the error has never been observed to happen with real hardware.

We also don't try to protect against problems in emulators. Instead, we fix the emulator.

Alan Stern