Re: USB vulnerabilities

Hi Dmitry,

Thanks for the input. I have added my sign off to the iforce patch.
I'm sending it as an attachment since I don't have my email client
properly setup yet for inline patches. Let me know if there is
anything else that needs to be done.

For the touchscreen patch, I'd forgotten about NEXIO_BUFSIZE, using
that is a good alternative to changing all the reader functions.
However there is still the chance that the packet byte array is
smaller than NEXIO_BUFSIZE so there can still be a buffer overflow,
though much less significant than before, and still relatively
harmless. I will update my patch to use NEXIO_BUFSIZE if you think
that is preferrable. AS for the other two adjustments, I'm not
actually sure what purpose they serve, so I thought I should leave
them alone, but I'll try to make it more concise. I will send the
updated patch soon once I get everything setup to test the patch on my
computer.

Thanks,
Anirudh Bagde
 --
Anirudh Bagde


On Sat, Jul 30, 2016 at 9:14 PM, Dmitry Torokhov
[off-list ref] wrote:

Hi Rosie, Anirudh,

On Fri, Jul 29, 2016 at 08:48:01PM -0400, Rosie Hall wrote:

quoted

Dmitry,

Attached are the patches Anirudh created.  Also, I have added him to the
thread if you have any questions or comments for him.

Rosie

...

quoted

--- a/drivers/input/joystick/iforce/iforce-usb.c      2016-07-29 15:02:47.602630504 -0400
+++ b/drivers/input/joystick/iforce/iforce-usb.c      2016-07-29 15:02:32.946812336 -0400

@@ -135,12 +135,23 @@
 {
      struct usb_device *dev = interface_to_usbdev(intf);
      struct usb_host_interface *interface;
-     struct usb_endpoint_descriptor *epirq, *epout;
+     struct usb_endpoint_descriptor *epirq = NULL, *epout = NULL;
      struct iforce *iforce;
-     int err = -ENOMEM;
+     int i, err = -ENOMEM;

      interface = intf->cur_altsetting;

+     for (i = 0; i < interface->desc.bNumEndpoints; i++) {
+             if (!epirq &&
+                 usb_endpoint_dir_in(&interface->endpoint[i].desc))
+                     epirq = &interface->endpoint[i].desc;
+             if (!epout &&
+                 usb_endpoint_dir_out(&interface->endpoint[i].desc))
+                     epout = &interface->endpoint[i].desc;
+     }
+     if (!epirq || !epout)
+             return -ENODEV;
+
      epirq = &interface->endpoint[0].desc;
      epout = &interface->endpoint[1].desc;

The iforce patch looks good, but I need "Signed-off-by" from Anirudh for
me to apply it. Please see Documentation/SubmittingPatches.

quoted

-static int nexio_read_data(struct usbtouch_usb *usbtouch, unsigned char *pkt)
+static int nexio_read_data(struct usbtouch_usb *usbtouch, unsigned char *pkt,
+                        unsigned int len)

I do not think we need to pass the length to the readers: we know what
protocol we are dealing with and we can simply use NEXIO_BUFSIZE.

quoted

 {
      struct nexio_touch_packet *packet = (void *) pkt;
      struct nexio_priv *priv = usbtouch->priv;

@@ -977,6 +996,11 @@
      if ((pkt[0] & 0xe0) != 0xe0)
              return 0;

+     if (data_len > len)
+             data_len = len;
+     if (x_len + y_len > data_len)
+             return 0;

We have more adjustments to x_len and data_len below, so maybe we
should move these new checks there as well?

quoted

+
      if (data_len > 0xff)
              data_len -= 0x100;
      if (x_len > 0xff)

Thanks.

--
Dmitry