cherry-embedded / CherryUSB

CherryUSB is a tiny and portable USB Stack (device & host) for embedded system with USB IP
https://cherryusb.readthedocs.io/
Apache License 2.0
1.14k stars 245 forks source link

If setup->wLength is too long, it may cause the global structure variable to overflow. #194

Closed Licae closed 2 months ago

Licae commented 2 months ago

Dear developer! When I was auditing the code, I found problems with potential spillover risks. In function usbd_event_ep0_setup_complete_handler, if the length of setup->wLength is too long and setup->bmRequestType is USB_REQUEST_DIR_IN, the following if judgment may be invalid.

    if (setup->wLength > CONFIG_USBDEV_REQUEST_BUFFER_LEN) {
        if ((setup->bmRequestType & USB_REQUEST_DIR_MASK) == USB_REQUEST_DIR_OUT) {
            USB_LOG_ERR("Request buffer too small\r\n");
            usbd_ep_set_stall(busid, USB_CONTROL_IN_EP0);
            return;
        }
    }

setup->wLength may also affect the following variables, because g_usbd_core is a global structure, and there may be the risk of overflow in the future.

  g_usbd_core[busid].ep0_data_buf = g_usbd_core[busid].req_data;
  g_usbd_core[busid].ep0_data_buf_residue = setup->wLength;
  g_usbd_core[busid].ep0_data_buf_len = setup->wLength;
  g_usbd_core[busid].zlp_flag = false;

I think the judgment logic here should be rewritten to ensure that the subsequent g_usbd_core global structure is safer.

sakumisu commented 2 months ago

fixed