| Message ID | 4bf3de35-6976-2380-df2f-41ab5c0c5f3b@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Marek Vasut |
| Headers | show |
| Series | USB Device buffer overflow | expand |
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>
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.
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