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.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?
>
> Sorry for the delay, I am still processing outstanding email.
Could you please review this patch?
