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.
