Hi Szymon, [Adding Marek]
On Wed, Nov 16, 2022 at 8:56 PM Szymon Heidrich <szymon.heidr...@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.