USB Device buffer overflow

[Szymon Heidrich]

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
-------------- next part --------------
From 1723d976ab0387be0d91b177011559cb07a66e03 Mon Sep 17 00:00:00 2001
From: Szymon Heidrich <szymon.heidrich at 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 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 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