On 20/11/2022 16:29, Szymon Heidrich wrote: > On 20/11/2022 15:43, Marek Vasut wrote: >> On 11/17/22 12:50, Fabio Estevam wrote: >>> [Adding Lukasz and Marek] >>> >>> On Thu, Nov 17, 2022 at 6:50 AM Szymon Heidrich >>> <szymon.heidr...@gmail.com> wrote: >>>> >>>> Assure that the control endpoint buffer of size USB_BUFSIZ (4096) >>>> can not be overflown during handling of USB control transfer >>>> requests with wLength greater than USB_BUFSIZ. >>>> >>>> Signed-off-by: Szymon Heidrich <szymon.heidr...@gmail.com> >>>> --- >>>> drivers/usb/gadget/composite.c | 11 +++++++++++ >>>> 1 file changed, 11 insertions(+) >>>> >>>> diff --git a/drivers/usb/gadget/composite.c >>>> b/drivers/usb/gadget/composite.c >>>> index 2a309e624e..cb89f6dca9 100644 >>>> --- a/drivers/usb/gadget/composite.c >>>> +++ b/drivers/usb/gadget/composite.c >>>> @@ -1019,6 +1019,17 @@ composite_setup(struct usb_gadget *gadget, const >>>> struct usb_ctrlrequest *ctrl) >>>> u8 endp; >>>> struct usb_configuration *c; >>>> >>>> + if (w_length > USB_BUFSIZ) { >>>> + if (ctrl->bRequestType & USB_DIR_IN) { >>>> + /* Cast away the const, we are going to overwrite >>>> on purpose. */ >>>> + __le16 *temp = (__le16 *)&ctrl->wLength; >>>> + *temp = cpu_to_le16(USB_BUFSIZ); >>>> + w_length = USB_BUFSIZ; >> >> Won't this end up sending corrupted packets in case they are longer than >> USB_BUFSIZ ? >> >> Where do such long packets come from ? >> >> What is the test-case ? > > The USB host will not attempt to retrieve more than wLenght bytes during > transfer phase. > If the device would erroneously attempt to provide more data it would result > in an unexpected state. > In case of most implementations the buffer for endpoint 0 along with max > control transfer is limited to 4096 bytes (USB_BUFSIZ for U-Boot and Linux > kernel). > Still according to the USB specification wLength is two bytes an the device > may receive requests with wLength larger than 4096 bytes e.g. in case of a > custom/malicious USB host. > For example one may build libusb with MAX_CTRL_BUFFER_LENGTH altered to > 0xffff and this will allow the host to send requests with wLength up to > 0xffff. > In this case the original implementation may result in buffer overflows as in > multiple locations a value directly derived from wLength is set as the > transfer phase length. > With the change applied IN requests with wLength larger than USB_BUFSIZ will > be trimmed to USB_BUFSIZ, otherwise the host would read wLength-USB_BUFSIZ > past cdev->req->buf. > I am not aware of any cases where more than USB_BUFSIZ would be provided from > a buffer other than cdev->req->buf. In case I missed such case please let me > know.
Is there anything additional required from my side?