USB Device buffer overflow

USB Device buffer overflow

[Szymon Heidrich]

[-- Attachment #1: Type: text/plain, Size: 3134 bytes --]

Hello,

Similar to CVE-2021-39685 affecting the Linux kernel U-Boot is vulnerable to a buffer overflow
present in the USB Gadget stack. Handling of a control transfer request with wLength larger than
USB_BUFSIZ (4096) may result in a buffer overflow.

The buffer for USB control endpoint is allocated in the composite_bind function implemented in
drivers/usb/gadget/composite.c. The buffer size is set to USB_BUFSIZ (4096) bytes.

>	/* preallocate control response and buffer */
>	cdev->req = usb_ep_alloc_request(gadget->ep0, GFP_KERNEL);
>	if (!cdev->req)
>		goto fail;
>	cdev->req->buf = memalign(CONFIG_SYS_CACHELINE_SIZE, USB_BUFSIZ);
>	if (!cdev->req->buf)
>		goto fail;
>	cdev->req->complete = composite_setup_complete;
>	gadget->ep0->driver_data = cdev;

In the composite_setup function data transfer phase is set up to the length of "value" bytes
which in multiple cases may be controlled by an attacker (is set to wLength).

>	if (value >= 0) {
>		req->length = value;
>		req->zero = value < w_length;
>		value = usb_ep_queue(gadget->ep0, req, GFP_KERNEL);
>		if (value < 0) {
>			debug("ep_queue --> %d\n", value);
>			req->status = 0;
>			composite_setup_complete(gadget->ep0, req);
>		}
>	}

In example the OS descriptor handler may be forced to set value to w_length.

> } else {
>     /* "extended compatibility ID"s */
>     count = count_ext_compat(os_desc_cfg);
>     buf[8] = count;
>     count *= 24; /* 24 B/ext compat desc */
>     count += 16; /* header */
>     put_unaligned_le32(count, buf);
>     buf += 16;
>     fill_ext_compat(os_desc_cfg, buf);
>     value = w_length;
> }

Execution of this code path for wLength set to a value larger then USB_BUFSIZ will result
in a buffer overflow. Since wLength is a double byte value it may have values up to 0xffff.

Besides the common OS descriptor handler this issue may be exploited for some of the available
gadgets e.g. f_dfu, f_sdp where "value" is derived from wLength.

drivers/usb/gadget/f_dfu.c
> static int handle_dnload(struct usb_gadget *gadget, u16 len)
> {
> 	struct usb_composite_dev *cdev = get_gadget_data(gadget);
> 	struct usb_request *req = cdev->req;
> 	struct f_dfu *f_dfu = req->context;
> 
> 	if (len == 0)
> 		f_dfu->dfu_state = DFU_STATE_dfuMANIFEST_SYNC;
> 
> 	req->complete = dnload_request_complete;
> 
> 	return len;
> }

drivers/usb/gadget/f_sdp.c
> if (req_type == USB_TYPE_CLASS) {
>    int report = w_value & HID_REPORT_ID_MASK;
> 
>     /* HID (SDP) request */
>     switch (ctrl->bRequest) {
>     case HID_REQ_SET_REPORT:
>         switch (report) {
>         case 1:
>             value = SDP_COMMAND_LEN + 1;
>             req->complete = sdp_rx_command_complete;
>             sdp_func->ep_int_enable = false;
>             break;
>         case 2:
>             value = len;
>             req->complete = sdp_rx_data_complete;
>             sdp_func->state = SDP_STATE_RX_FILE_DATA_BUSY;
>             break;
>         }
>     }
> }

Please find attached a patch addressing this issue.
Depending on request direction wLength larger than USB_BUFSIZ will result in either
endpoint stall or value trim.

Best regards,
Szymon

[-- Attachment #2: 0001-Prevent-buffer-overflow-on-USB-control-endpoint.patch --]
[-- Type: text/plain, Size: 1316 bytes --]

From 1723d976ab0387be0d91b177011559cb07a66e03 Mon Sep 17 00:00:00 2001
From: Szymon Heidrich <szymon.heidrich@gmail.com>
Date: Wed, 9 Nov 2022 17:55:34 +0100
Subject: [PATCH] Prevent buffer overflow on USB control endpoint

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@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 2a309e62..cb89f6dc 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;
+		} else {
+			goto done;
+		}
+	}
+
 	/*
 	 * partial re-init of the response message; the function or the
 	 * gadget might need to intercept e.g. a control-OUT completion
-- 
2.38.1

Re: USB Device buffer overflow

[Sultan Khan]

Hello Szymon,

Looks like a generalization of CVE-2022-2347 I found earlier. While both I and Venkatesh Yadav Abbarapu of AMD made patches for that CVE localized to DFU, given the presence of the same problematic pattern elsewhere, the bounds check aspect of that CVE fix would perhaps be better in a centralized location like what you did here.

Sultan

> On Nov 16, 2022, at 6:56 PM, Szymon Heidrich <szymon.heidrich@gmail.com> wrote:
> Hello,
> 
> Similar to CVE-2021-39685 affecting the Linux kernel U-Boot is vulnerable to a buffer overflow
> present in the USB Gadget stack. Handling of a control transfer request with wLength larger than
> USB_BUFSIZ (4096) may result in a buffer overflow.
> 
> The buffer for USB control endpoint is allocated in the composite_bind function implemented in
> drivers/usb/gadget/composite.c. The buffer size is set to USB_BUFSIZ (4096) bytes.
> 
>>    /* preallocate control response and buffer */
>>    cdev->req = usb_ep_alloc_request(gadget->ep0, GFP_KERNEL);
>>    if (!cdev->req)
>>        goto fail;
>>    cdev->req->buf = memalign(CONFIG_SYS_CACHELINE_SIZE, USB_BUFSIZ);
>>    if (!cdev->req->buf)
>>        goto fail;
>>    cdev->req->complete = composite_setup_complete;
>>    gadget->ep0->driver_data = cdev;
> 
> In the composite_setup function data transfer phase is set up to the length of "value" bytes
> which in multiple cases may be controlled by an attacker (is set to wLength).
> 
>>    if (value >= 0) {
>>        req->length = value;
>>        req->zero = value < w_length;
>>        value = usb_ep_queue(gadget->ep0, req, GFP_KERNEL);
>>        if (value < 0) {
>>            debug("ep_queue --> %d\n", value);
>>            req->status = 0;
>>            composite_setup_complete(gadget->ep0, req);
>>        }
>>    }
> 
> In example the OS descriptor handler may be forced to set value to w_length.
> 
>> } else {
>>    /* "extended compatibility ID"s */
>>    count = count_ext_compat(os_desc_cfg);
>>    buf[8] = count;
>>    count *= 24; /* 24 B/ext compat desc */
>>    count += 16; /* header */
>>    put_unaligned_le32(count, buf);
>>    buf += 16;
>>    fill_ext_compat(os_desc_cfg, buf);
>>    value = w_length;
>> }
> 
> Execution of this code path for wLength set to a value larger then USB_BUFSIZ will result
> in a buffer overflow. Since wLength is a double byte value it may have values up to 0xffff.
> 
> Besides the common OS descriptor handler this issue may be exploited for some of the available
> gadgets e.g. f_dfu, f_sdp where "value" is derived from wLength.
> 
> drivers/usb/gadget/f_dfu.c
>> static int handle_dnload(struct usb_gadget *gadget, u16 len)
>> {
>>    struct usb_composite_dev *cdev = get_gadget_data(gadget);
>>    struct usb_request *req = cdev->req;
>>    struct f_dfu *f_dfu = req->context;
>> 
>>    if (len == 0)
>>        f_dfu->dfu_state = DFU_STATE_dfuMANIFEST_SYNC;
>> 
>>    req->complete = dnload_request_complete;
>> 
>>    return len;
>> }
> 
> drivers/usb/gadget/f_sdp.c
>> if (req_type == USB_TYPE_CLASS) {
>>   int report = w_value & HID_REPORT_ID_MASK;
>> 
>>    /* HID (SDP) request */
>>    switch (ctrl->bRequest) {
>>    case HID_REQ_SET_REPORT:
>>        switch (report) {
>>        case 1:
>>            value = SDP_COMMAND_LEN + 1;
>>            req->complete = sdp_rx_command_complete;
>>            sdp_func->ep_int_enable = false;
>>            break;
>>        case 2:
>>            value = len;
>>            req->complete = sdp_rx_data_complete;
>>            sdp_func->state = SDP_STATE_RX_FILE_DATA_BUSY;
>>            break;
>>        }
>>    }
>> }
> 
> Please find attached a patch addressing this issue.
> Depending on request direction wLength larger than USB_BUFSIZ will result in either
> endpoint stall or value trim.
> 
> Best regards,
> Szymon
> <0001-Prevent-buffer-overflow-on-USB-control-endpoint.patch>

Re: USB Device buffer overflow

[Fabio Estevam]

Hi Szymon,

[Adding Marek]

On Wed, Nov 16, 2022 at 8:56 PM Szymon Heidrich
<szymon.heidrich@gmail.com> wrote:
>
> Hello,
>
> Similar to CVE-2021-39685 affecting the Linux kernel U-Boot is vulnerable to a buffer overflow
> present in the USB Gadget stack. Handling of a control transfer request with wLength larger than
> USB_BUFSIZ (4096) may result in a buffer overflow.
>
> The buffer for USB control endpoint is allocated in the composite_bind function implemented in
> drivers/usb/gadget/composite.c. The buffer size is set to USB_BUFSIZ (4096) bytes.
>
> >       /* preallocate control response and buffer */
> >       cdev->req = usb_ep_alloc_request(gadget->ep0, GFP_KERNEL);
> >       if (!cdev->req)
> >               goto fail;
> >       cdev->req->buf = memalign(CONFIG_SYS_CACHELINE_SIZE, USB_BUFSIZ);
> >       if (!cdev->req->buf)
> >               goto fail;
> >       cdev->req->complete = composite_setup_complete;
> >       gadget->ep0->driver_data = cdev;
>
> In the composite_setup function data transfer phase is set up to the length of "value" bytes
> which in multiple cases may be controlled by an attacker (is set to wLength).
>
> >       if (value >= 0) {
> >               req->length = value;
> >               req->zero = value < w_length;
> >               value = usb_ep_queue(gadget->ep0, req, GFP_KERNEL);
> >               if (value < 0) {
> >                       debug("ep_queue --> %d\n", value);
> >                       req->status = 0;
> >                       composite_setup_complete(gadget->ep0, req);
> >               }
> >       }
>
> In example the OS descriptor handler may be forced to set value to w_length.
>
> > } else {
> >     /* "extended compatibility ID"s */
> >     count = count_ext_compat(os_desc_cfg);
> >     buf[8] = count;
> >     count *= 24; /* 24 B/ext compat desc */
> >     count += 16; /* header */
> >     put_unaligned_le32(count, buf);
> >     buf += 16;
> >     fill_ext_compat(os_desc_cfg, buf);
> >     value = w_length;
> > }
>
> Execution of this code path for wLength set to a value larger then USB_BUFSIZ will result
> in a buffer overflow. Since wLength is a double byte value it may have values up to 0xffff.
>
> Besides the common OS descriptor handler this issue may be exploited for some of the available
> gadgets e.g. f_dfu, f_sdp where "value" is derived from wLength.
>
> drivers/usb/gadget/f_dfu.c
> > static int handle_dnload(struct usb_gadget *gadget, u16 len)
> > {
> >       struct usb_composite_dev *cdev = get_gadget_data(gadget);
> >       struct usb_request *req = cdev->req;
> >       struct f_dfu *f_dfu = req->context;
> >
> >       if (len == 0)
> >               f_dfu->dfu_state = DFU_STATE_dfuMANIFEST_SYNC;
> >
> >       req->complete = dnload_request_complete;
> >
> >       return len;
> > }
>
> drivers/usb/gadget/f_sdp.c
> > if (req_type == USB_TYPE_CLASS) {
> >    int report = w_value & HID_REPORT_ID_MASK;
> >
> >     /* HID (SDP) request */
> >     switch (ctrl->bRequest) {
> >     case HID_REQ_SET_REPORT:
> >         switch (report) {
> >         case 1:
> >             value = SDP_COMMAND_LEN + 1;
> >             req->complete = sdp_rx_command_complete;
> >             sdp_func->ep_int_enable = false;
> >             break;
> >         case 2:
> >             value = len;
> >             req->complete = sdp_rx_data_complete;
> >             sdp_func->state = SDP_STATE_RX_FILE_DATA_BUSY;
> >             break;
> >         }
> >     }
> > }
>
> Please find attached a patch addressing this issue.
> Depending on request direction wLength larger than USB_BUFSIZ will result in either
> endpoint stall or value trim.

Thanks for the patch. Please submit it via git send-email instead of attachment.

[PATCH] Prevent buffer overflow on USB control endpoint

[Szymon Heidrich]

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@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;
+		} else {
+			goto done;
+		}
+	}
+
 	/*
 	 * partial re-init of the response message; the function or the
 	 * gadget might need to intercept e.g. a control-OUT completion
-- 
2.38.1

Re: [PATCH] Prevent buffer overflow on USB control endpoint

[Fabio Estevam]

[Adding Lukasz and Marek]

On Thu, Nov 17, 2022 at 6:50 AM Szymon Heidrich
<szymon.heidrich@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@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;
> +               } else {
> +                       goto done;
> +               }
> +       }
> +
>         /*
>          * partial re-init of the response message; the function or the
>          * gadget might need to intercept e.g. a control-OUT completion
> --
> 2.38.1
>

Re: [PATCH] Prevent buffer overflow on USB control endpoint

[Marek Vasut]

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@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@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 ?

Re: [PATCH] Prevent buffer overflow on USB control endpoint

[Szymon Heidrich]

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@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@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.

Re: [PATCH] Prevent buffer overflow on USB control endpoint

[Marek Vasut]

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@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@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);

?

Re: [PATCH] Prevent buffer overflow on USB control endpoint

[Szymon Heidrich]

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@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@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.

Re: [PATCH] Prevent buffer overflow on USB control endpoint

[Marek Vasut]

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@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@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 ?

Re: [PATCH] Prevent buffer overflow on USB control endpoint

[Szymon Heidrich]

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@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@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?

Re: [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@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@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.

Re: [PATCH] Prevent buffer overflow on USB control endpoint

[Szymon Heidrich]

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.heidrich@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@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?

Re: [PATCH] Prevent buffer overflow on USB control endpoint

[Marek Vasut]

On 11/28/22 10:21, Szymon Heidrich wrote:
> 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.heidrich@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@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?

Sorry for the delay, I am still processing outstanding email.

Re: [PATCH] Prevent buffer overflow on USB control endpoint

[Szymon Heidrich]

On 28/11/2022 10:27, Marek Vasut wrote:
> On 11/28/22 10:21, Szymon Heidrich wrote:
>> 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.heidrich@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@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?
> 
> Sorry for the delay, I am still processing outstanding email.

Could you please review this patch?

Re: [PATCH] Prevent buffer overflow on USB control endpoint

[Marek Vasut]

On 12/12/22 10:55, Szymon Heidrich wrote:
> On 28/11/2022 10:27, Marek Vasut wrote:
>> On 11/28/22 10:21, Szymon Heidrich wrote:
>>> 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.heidrich@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@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?
>>
>> Sorry for the delay, I am still processing outstanding email.
> 
> Could you please review this patch?

I have it (or rather, one outstanding reply) in the pipeline, sorry for 
the delay.