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 ?
