On 16/12/2022 04:22, Marek Vasut wrote: > On 11/20/22 18:42, Szymon Heidrich wrote: >> On 20/11/2022 18:25, Marek Vasut wrote: >>> On 11/20/22 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. >>> >>> Why shouldn't the patch do simple >>> >>> w_length = min(w_length, USB_BUFSIZ); >>> >>> ? >> >> The composite_setup function in composite.c uses w_length but function >> specific setup functions are passed the struct usb_ctrlrequest *ctrl so both >> must be updated. >> If only w_length is updated using min(w_length, USB_BUFSIZ) than function >> specific setup will still receive the original request including unaltered >> wLength so it >> may again return a number of bytes greater than buffer size resulting in an >> overflow. > > Can you fill this sanitization into the setup wrapper here ? > > https://patchwork.ozlabs.org/project/uboot/patch/20221216032047.536441-1-ma...@denx.de/ > > That way, it would apply for all the gadget drivers and you won't have to > deal with the const overwrite , right ?
Sure, that should be fine. I'll provide a patch when when this is merged, OK?