CTRL: fix buffer overflow possibility (#28) [1/3]

IntergatedCircuits · Jun 17, 2021 · c88233a · c88233a
1 parent ddf28bb
commit c88233a
Show file tree

Hide file tree

Showing 6 changed files with 42 additions and 17 deletions.
diff --git a/Class/CDC/usbd_cdc.c b/Class/CDC/usbd_cdc.c
@@ -349,7 +349,8 @@ static USBD_ReturnType cdc_setupStage(USBD_CDC_IfHandleType *itf)
                     /* Reset the data interface */
                     cdc_deinit(itf);
 
-                    retval = USBD_CtrlReceiveData(dev, &itf->LineCoding);
+                    retval = USBD_CtrlReceiveData(dev,
+                            &itf->LineCoding, sizeof(itf->LineCoding));
                     break;
 
                 case CDC_REQ_GET_LINE_CODING:

diff --git a/Class/CDC/usbd_ncm.c b/Class/CDC/usbd_ncm.c
@@ -38,6 +38,8 @@
 
 #define NCM_MAX_SEGMENT_SIZE    1514
 
+#define NCM_NET_ADDRESS_SIZE    6
+
 #define NCM_APP(ITF)    ((USBD_NCM_AppType*)((ITF)->App))
 
 typedef enum
@@ -125,6 +127,13 @@ typedef PACKED(struct)
 }USBD_NCM_ParametersType;
 
 
+/* NTB Parameters */
+typedef PACKED(struct)
+{
+    uint32_t size;
+}USBD_NCM_NTB_InputSize;
+
+
 typedef PACKED(struct)
 {
     /* Interface Association Descriptor */
@@ -459,32 +468,34 @@ static USBD_ReturnType ncm_setupStage(USBD_NCM_IfHandleType *itf)
 
             case CDC_REQ_GET_NTB_INPUT_SIZE:
             {
-                PACKED(struct) {
-                    uint32_t size;
-                }*insize = (void*)dev->CtrlData;
+                USBD_NCM_NTB_InputSize *insize = (void*)dev->CtrlData;
                 insize->size = itf->In.MaxSize;
                 retval = USBD_CtrlSendData(dev, dev->CtrlData, sizeof(*insize));
                 break;
             }
 
             case CDC_REQ_SET_NTB_INPUT_SIZE:
             {
-                retval = USBD_CtrlReceiveData(dev, dev->CtrlData);
+                retval = USBD_CtrlReceiveData(dev, dev->CtrlData, sizeof(USBD_NCM_NTB_InputSize));
                 break;
             }
 
 #if 0
             /* Optional requests */
             case CDC_REQ_GET_NET_ADDRESS:
-                /* TODO */
+                if (dev->Setup.Length == NCM_NET_ADDRESS_SIZE)
+                {
+                    /* TODO */
+                }
                 break;
 
             case CDC_REQ_SET_NET_ADDRESS:
                 /* Only accepted while data interface alt sel = 0
                  * doesn't change iMACAddress value */
-                if (itf->Base.AltSelector == 0)
+                if ((dev->Setup.Length == NCM_NET_ADDRESS_SIZE) &&
+                    (itf->Base.AltSelector == 0))
                 {
-                    retval = USBD_CtrlReceiveData(dev, dev->CtrlData);
+                    retval = USBD_CtrlReceiveData(dev, dev->CtrlData, NCM_NET_ADDRESS_SIZE);
                 }
                 break;
 
@@ -521,9 +532,7 @@ static void ncm_dataStage(USBD_NCM_IfHandleType *itf)
         {
             case CDC_REQ_SET_NTB_INPUT_SIZE:
             {
-                PACKED(struct) {
-                    uint32_t size;
-                }*insize = (void*)dev->CtrlData;
+                USBD_NCM_NTB_InputSize *insize = (void*)dev->CtrlData;
                 /* Sanity check */
                 if (insize->size > (sizeof(((USBD_NCM_TransferHeaderType*)0)->V16) +
                                     sizeof(((USBD_NCM_DatagramPointerTableType*)0)->V16)))

diff --git a/Class/DFU/usbd_dfu.c b/Class/DFU/usbd_dfu.c
@@ -562,7 +562,7 @@ static USBD_ReturnType dfu_download(USBD_DFU_IfHandleType *itf)
                 itf->DevStatus.State = DFU_STATE_DNLOAD_SYNC;
 
                 /* Prepare the reception of the buffer over EP0 */
-                retval = USBD_CtrlReceiveData(dev, dev->CtrlData);
+                retval = USBD_CtrlReceiveData(dev, dev->CtrlData, itf->BlockLength);
             }
         }
     }

diff --git a/Class/HID/usbd_hid.c b/Class/HID/usbd_hid.c
@@ -380,6 +380,7 @@ static USBD_ReturnType hid_setupStage(USBD_HID_IfHandleType *itf)
         case USB_REQ_TYPE_CLASS:
         {
             uint8_t reportId = (uint8_t)dev->Setup.Value;
+            USBD_HID_ReportType reportType = dev->Setup.Value >> 8;
 
             switch (dev->Setup.Request)
             {
@@ -388,7 +389,7 @@ static USBD_ReturnType hid_setupStage(USBD_HID_IfHandleType *itf)
                 {
                     /* Set flag, invoke callback which should provide data
                      * via USBD_HID_ReportIn() */
-                    itf->Request = dev->Setup.Value >> 8;
+                    itf->Request = reportType;
                     USBD_SAFE_CALLBACK(HID_APP(itf)->GetReport,
                             itf, itf->Request, reportId);
 
@@ -401,7 +402,16 @@ static USBD_ReturnType hid_setupStage(USBD_HID_IfHandleType *itf)
                 /* HID report OUT */
                 case HID_REQ_SET_REPORT:
                 {
-                    retval = USBD_CtrlReceiveData(dev, dev->CtrlData);
+                    uint16_t max_len;
+                    if (reportType == HID_REPORT_OUTPUT)
+                    {
+                        max_len = HID_APP(itf)->Report->Output.MaxSize;
+                    }
+                    else
+                    {
+                        max_len = HID_APP(itf)->Report->Feature.MaxSize;
+                    }
+                    retval = USBD_CtrlReceiveData(dev, dev->CtrlData, max_len);
                     break;
                 }
 

diff --git a/Device/usbd_ctrl.c b/Device/usbd_ctrl.c
@@ -164,16 +164,21 @@ USBD_ReturnType USBD_CtrlSendData(USBD_HandleType *dev, void *data, uint16_t len
  * @brief This function receives control data according to the setup request.
  * @param dev: USB Device handle reference
  * @param data: pointer to the target buffer to receive to
+ * @param len: maximum allowed length of the data
  * @return OK if called from the right context, ERROR otherwise
  */
-USBD_ReturnType USBD_CtrlReceiveData(USBD_HandleType *dev, void *data)
+USBD_ReturnType USBD_CtrlReceiveData(USBD_HandleType *dev, void *data, uint16_t len)
 {
     USBD_ReturnType retval = USBD_E_ERROR;
 
     /* Sanity check */
     if (dev->EP.OUT[0].State == USB_EP_STATE_SETUP)
     {
-        uint16_t len = dev->Setup.Length;
+        /* Don't receive more bytes than requested */
+        if (dev->Setup.Length < len)
+        {
+            len = dev->Setup.Length;
+        }
 
         dev->EP.OUT[0].State = USB_EP_STATE_DATA;
         USBD_PD_EpReceive(dev, 0x00, (uint8_t*)data, len);

diff --git a/Include/private/usbd_internal.h b/Include/private/usbd_internal.h
@@ -44,7 +44,7 @@ USBD_ReturnType USBD_CtrlSendData       (USBD_HandleType *dev,
                                          uint16_t len);
 
 USBD_ReturnType USBD_CtrlReceiveData    (USBD_HandleType *dev,
-                                         void *data);
+                                         void *data, uint16_t len);
 
 uint16_t        USBD_EpDesc             (USBD_HandleType *dev,
                                          uint8_t epAddr,