| Message ID | 2fd29934-03ac-b67c-a644-07aebbfe8aff@cisco.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Rosie, Anirudh, On Fri, Jul 29, 2016 at 08:48:01PM -0400, Rosie Hall wrote: > 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 ... > --- 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. > > -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. > { > 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? > + > if (data_len > 0xff) > data_len -= 0x100; > if (x_len > 0xff) Thanks.
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 <dmitry.torokhov@gmail.com> wrote: > Hi Rosie, Anirudh, > > On Fri, Jul 29, 2016 at 08:48:01PM -0400, Rosie Hall wrote: >> 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 > > ... > > >> --- 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. > > >> >> -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. > >> { >> 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? > >> + >> if (data_len > 0xff) >> data_len -= 0x100; >> if (x_len > 0xff) > > Thanks. > > -- > Dmitry
--- 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;