[PATCH] Prevent buffer overflow on USB control endpoint

[Marek Vasut]

On 12/19/22 19:52, Szymon Heidrich wrote:
> 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.heidrich at 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.heidrich at 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-marex@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?

Please review/test the aforementioned patch and let me know if it works 
for you, the patch can still be tweaked one way or the other.