usb: usbip: fix isoc packet num validation in get_pipe

usb: usbip: fix isoc packet num validation in get_pipe

[Malte Leip]

Change the validation of number_of_packets in get_pipe to compare the
number of packets to a fixed maximum number of packets allowed, set to
be 1024. This number was chosen due to it being used by other drivers as
well, for example drivers/usb/host/uhci-q.c

Background/reason:
The get_pipe function in stub_rx.c validates the number of packets in
isochronous mode and aborts with an error if that number is too large,
in order to prevent malicious input from possibly triggering large
memory allocations. This was previously done by checking whether
pdu->u.cmd_submit.number_of_packets is bigger than the number of packets
that would be needed for pdu->u.cmd_submit.transfer_buffer_length bytes
if all except possibly the last packet had maximum length, given by
usb_endpoint_maxp(epd) *  usb_endpoint_maxp_mult(epd). This leads to an
error if URBs with packets shorter than the maximum possible length are
submitted, which is allowed according to
Documentation/driver-api/usb/URB.rst and occurs for example with the
snd-usb-audio driver.

Fixes: c6688ef9f297 ("usbip: fix stub_rx: harden CMD_SUBMIT path to handle malicious input")
Signed-off-by: Malte Leip <malte@leip.net>
---

Further background:

The headphones / USB soundcard
ID 046d:0a1f Logitech, Inc. G930
is currently (tested specifically with 4.19) not usable with the snd-usb-audio
driver over usbip.  (I also reported this problem a year ago at [1] where some
further information can be found.) The system log of the host shows the
following errors:
Mar 31 20:24:01.696845 sys-usb kernel: usbip-host 3-1: CMD_SUBMIT: isoc invalid num packets 6
Mar 31 20:24:01.696980 sys-usb kernel: usbip_core: unknown command
Mar 31 20:24:01.697588 sys-usb kernel: usbip-host 3-1: recv invalid request

I can provide more detailed debug output, try to test with other versions etc.
if required, but I think the cause is the problem described in the commit
message above, and this patch fixes the problem for me.

The snd-usb-audio driver submits URBs in isochronous mode with packet sizes
below the maximum, particularly in prepare_silent_urb in sound/usb/endpoint.c -
the size of the next packets is determined by snd_usb_endpoint_next_packet_size
in the same file and isn't always the maximum packet size. This then leads to a
problem as described in the commit message.

There are also other places in Linux where number_of_packets of an URB is
validated, for example in drivers/usb/host/uhci-q.c or
drivers/usb/core/devio.c.  Both have fixed absolute limits, the former of 1024,
the latter of 128.  Hence my proposal to replace the current check with
checking that number_of_packets is not larger than 1024.

Independently of the problem with below-maximum-size packets: Why does the
current check even do what it is intended to do? What would prevent an attacker
from not only setting number_of_packets to a very large number, but also
transfer_buffer_length, sufficient to pass the current check?

[1]: https://github.com/QubesOS/qubes-issues/issues/36284

 drivers/usb/usbip/stub_rx.c      | 12 +++---------
 drivers/usb/usbip/usbip_common.h |  7 +++++++
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/usbip/stub_rx.c b/drivers/usb/usbip/stub_rx.c
index 97b09a42a10c..dbfb2f24d71e 100644
--- a/drivers/usb/usbip/stub_rx.c
+++ b/drivers/usb/usbip/stub_rx.c
@@ -361,16 +361,10 @@ static int get_pipe(struct stub_device *sdev, struct usbip_header *pdu)
 	}
 
 	if (usb_endpoint_xfer_isoc(epd)) {
-		/* validate packet size and number of packets */
-		unsigned int maxp, packets, bytes;
-
-		maxp = usb_endpoint_maxp(epd);
-		maxp *= usb_endpoint_maxp_mult(epd);
-		bytes = pdu->u.cmd_submit.transfer_buffer_length;
-		packets = DIV_ROUND_UP(bytes, maxp);
-
+		/* validate number of packets */
 		if (pdu->u.cmd_submit.number_of_packets < 0 ||
-		    pdu->u.cmd_submit.number_of_packets > packets) {
+		    pdu->u.cmd_submit.number_of_packets >
+		    USBIP_MAX_ISO_PACKETS) {
 			dev_err(&sdev->udev->dev,
 				"CMD_SUBMIT: isoc invalid num packets %d\n",
 				pdu->u.cmd_submit.number_of_packets);
diff --git a/drivers/usb/usbip/usbip_common.h b/drivers/usb/usbip/usbip_common.h
index bf8afe9b5883..8be857a4fa13 100644
--- a/drivers/usb/usbip/usbip_common.h
+++ b/drivers/usb/usbip/usbip_common.h
@@ -121,6 +121,13 @@ extern struct device_attribute dev_attr_usbip_debug;
 #define USBIP_DIR_OUT	0x00
 #define USBIP_DIR_IN	0x01
 
+/*
+ * Arbitrary limit for the maximum number of isochronous packets in an URB,
+ * compare for example the uhci_submit_isochronous function in
+ * drivers/usb/host/uhci-q.c
+ */
+#define USBIP_MAX_ISO_PACKETS 1024
+
 /**
  * struct usbip_header_basic - data pertinent to every request
  * @command: the usbip request type

[PATCH] usb: usbip: fix isoc packet num validation in get_pipe

[Malte Leip]

Change the validation of number_of_packets in get_pipe to compare the
number of packets to a fixed maximum number of packets allowed, set to
be 1024. This number was chosen due to it being used by other drivers as
well, for example drivers/usb/host/uhci-q.c

Background/reason:
The get_pipe function in stub_rx.c validates the number of packets in
isochronous mode and aborts with an error if that number is too large,
in order to prevent malicious input from possibly triggering large
memory allocations. This was previously done by checking whether
pdu->u.cmd_submit.number_of_packets is bigger than the number of packets
that would be needed for pdu->u.cmd_submit.transfer_buffer_length bytes
if all except possibly the last packet had maximum length, given by
usb_endpoint_maxp(epd) *  usb_endpoint_maxp_mult(epd). This leads to an
error if URBs with packets shorter than the maximum possible length are
submitted, which is allowed according to
Documentation/driver-api/usb/URB.rst and occurs for example with the
snd-usb-audio driver.

Fixes: c6688ef9f297 ("usbip: fix stub_rx: harden CMD_SUBMIT path to handle malicious input")
Signed-off-by: Malte Leip <malte@leip.net>
---

Further background:

The headphones / USB soundcard
ID 046d:0a1f Logitech, Inc. G930
is currently (tested specifically with 4.19) not usable with the snd-usb-audio
driver over usbip.  (I also reported this problem a year ago at [1] where some
further information can be found.) The system log of the host shows the
following errors:
Mar 31 20:24:01.696845 sys-usb kernel: usbip-host 3-1: CMD_SUBMIT: isoc invalid num packets 6
Mar 31 20:24:01.696980 sys-usb kernel: usbip_core: unknown command
Mar 31 20:24:01.697588 sys-usb kernel: usbip-host 3-1: recv invalid request

I can provide more detailed debug output, try to test with other versions etc.
if required, but I think the cause is the problem described in the commit
message above, and this patch fixes the problem for me.

The snd-usb-audio driver submits URBs in isochronous mode with packet sizes
below the maximum, particularly in prepare_silent_urb in sound/usb/endpoint.c -
the size of the next packets is determined by snd_usb_endpoint_next_packet_size
in the same file and isn't always the maximum packet size. This then leads to a
problem as described in the commit message.

There are also other places in Linux where number_of_packets of an URB is
validated, for example in drivers/usb/host/uhci-q.c or
drivers/usb/core/devio.c.  Both have fixed absolute limits, the former of 1024,
the latter of 128.  Hence my proposal to replace the current check with
checking that number_of_packets is not larger than 1024.

Independently of the problem with below-maximum-size packets: Why does the
current check even do what it is intended to do? What would prevent an attacker
from not only setting number_of_packets to a very large number, but also
transfer_buffer_length, sufficient to pass the current check?

[1]: https://github.com/QubesOS/qubes-issues/issues/36284

 drivers/usb/usbip/stub_rx.c      | 12 +++---------
 drivers/usb/usbip/usbip_common.h |  7 +++++++
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/usbip/stub_rx.c b/drivers/usb/usbip/stub_rx.c
index 97b09a42a10c..dbfb2f24d71e 100644
--- a/drivers/usb/usbip/stub_rx.c
+++ b/drivers/usb/usbip/stub_rx.c
@@ -361,16 +361,10 @@ static int get_pipe(struct stub_device *sdev, struct usbip_header *pdu)
 	}
 
 	if (usb_endpoint_xfer_isoc(epd)) {
-		/* validate packet size and number of packets */
-		unsigned int maxp, packets, bytes;
-
-		maxp = usb_endpoint_maxp(epd);
-		maxp *= usb_endpoint_maxp_mult(epd);
-		bytes = pdu->u.cmd_submit.transfer_buffer_length;
-		packets = DIV_ROUND_UP(bytes, maxp);
-
+		/* validate number of packets */
 		if (pdu->u.cmd_submit.number_of_packets < 0 ||
-		    pdu->u.cmd_submit.number_of_packets > packets) {
+		    pdu->u.cmd_submit.number_of_packets >
+		    USBIP_MAX_ISO_PACKETS) {
 			dev_err(&sdev->udev->dev,
 				"CMD_SUBMIT: isoc invalid num packets %d\n",
 				pdu->u.cmd_submit.number_of_packets);
diff --git a/drivers/usb/usbip/usbip_common.h b/drivers/usb/usbip/usbip_common.h
index bf8afe9b5883..8be857a4fa13 100644
--- a/drivers/usb/usbip/usbip_common.h
+++ b/drivers/usb/usbip/usbip_common.h
@@ -121,6 +121,13 @@ extern struct device_attribute dev_attr_usbip_debug;
 #define USBIP_DIR_OUT	0x00
 #define USBIP_DIR_IN	0x01
 
+/*
+ * Arbitrary limit for the maximum number of isochronous packets in an URB,
+ * compare for example the uhci_submit_isochronous function in
+ * drivers/usb/host/uhci-q.c
+ */
+#define USBIP_MAX_ISO_PACKETS 1024
+
 /**
  * struct usbip_header_basic - data pertinent to every request
  * @command: the usbip request type
-- 
2.11.0

usb: usbip: fix isoc packet num validation in get_pipe

[Shuah Khan]

On 4/14/19 4:00 AM, Malte Leip wrote:
> Change the validation of number_of_packets in get_pipe to compare the
> number of packets to a fixed maximum number of packets allowed, set to
> be 1024. This number was chosen due to it being used by other drivers as
> well, for example drivers/usb/host/uhci-q.c
> 
> Background/reason:
> The get_pipe function in stub_rx.c validates the number of packets in
> isochronous mode and aborts with an error if that number is too large,
> in order to prevent malicious input from possibly triggering large
> memory allocations. This was previously done by checking whether
> pdu->u.cmd_submit.number_of_packets is bigger than the number of packets
> that would be needed for pdu->u.cmd_submit.transfer_buffer_length bytes
> if all except possibly the last packet had maximum length, given by
> usb_endpoint_maxp(epd) *  usb_endpoint_maxp_mult(epd). This leads to an
> error if URBs with packets shorter than the maximum possible length are
> submitted, which is allowed according to
> Documentation/driver-api/usb/URB.rst and occurs for example with the
> snd-usb-audio driver.
> 
> Fixes: c6688ef9f297 ("usbip: fix stub_rx: harden CMD_SUBMIT path to handle malicious input")
> Signed-off-by: Malte Leip <malte@leip.net>

Looks good to me.

Acked-by: Shuah Khan <skhan@linuxfoundation.org>

thanks,
-- Shuah

Re: [PATCH] usb: usbip: fix isoc packet num validation in get_pipe

[shuah]

On 4/14/19 4:00 AM, Malte Leip wrote:
> Change the validation of number_of_packets in get_pipe to compare the
> number of packets to a fixed maximum number of packets allowed, set to
> be 1024. This number was chosen due to it being used by other drivers as
> well, for example drivers/usb/host/uhci-q.c
> 
> Background/reason:
> The get_pipe function in stub_rx.c validates the number of packets in
> isochronous mode and aborts with an error if that number is too large,
> in order to prevent malicious input from possibly triggering large
> memory allocations. This was previously done by checking whether
> pdu->u.cmd_submit.number_of_packets is bigger than the number of packets
> that would be needed for pdu->u.cmd_submit.transfer_buffer_length bytes
> if all except possibly the last packet had maximum length, given by
> usb_endpoint_maxp(epd) *  usb_endpoint_maxp_mult(epd). This leads to an
> error if URBs with packets shorter than the maximum possible length are
> submitted, which is allowed according to
> Documentation/driver-api/usb/URB.rst and occurs for example with the
> snd-usb-audio driver.
> 
> Fixes: c6688ef9f297 ("usbip: fix stub_rx: harden CMD_SUBMIT path to handle malicious input")
> Signed-off-by: Malte Leip <malte@leip.net>

Looks good to me.

Acked-by: Shuah Khan <skhan@linuxfoundation.org>

thanks,
-- Shuah