Changeset 87161 in vbox for trunk/src/VBox

Timestamp:

Jan 4, 2021 12:45:18 PM (4 years ago)

Author:

vboxsync

Message:

USB/IP: Be more strict about stuff we accept from the remote end, bugref:9861

File:

• trunk/src/VBox/Devices/USB/usbip/USBProxyDevice-usbip.cpp (modified) (11 diffs)

trunk/src/VBox/Devices/USB/usbip/USBProxyDevice-usbip.cpp

@@ -820 +820 @@
 static int usbProxyUsbIpDisconnect(PUSBPROXYDEVUSBIP pProxyDevUsbIp)
 {
-    int rc = VINF_SUCCESS;
+    int rc = RTPollSetRemove(pProxyDevUsbIp->hPollSet, USBIP_POLL_ID_SOCKET);
+    Assert(RT_SUCCESS(rc) || rc == VERR_POLL_HANDLE_ID_NOT_FOUND);
 
     rc = RTTcpClientCloseEx(pProxyDevUsbIp->hSocket, false /*fGracefulShutdown*/);
@@ -964 +965 @@
                 case USBPROXYUSBIPRECVSTATE_HDR_RESIDUAL:
                 {
-                    /** @todo Verify that the directions match, verify that the length doesn't exceed the buffer. */
-
                     switch (RT_N2H_U32(pProxyDevUsbIp->BufRet.Hdr.u32ReqRet))
                     {
@@ -980 +979 @@
                                 {
                                     uint8_t *pbData = NULL;
+                                    size_t cbRet = 0;
 
                                     AssertPtr(pProxyDevUsbIp->pUrbUsbIp->pVUsbUrb);
@@ -986 +986 @@
                                         /* Preserve the setup request. */
                                         pbData = &pProxyDevUsbIp->pUrbUsbIp->pVUsbUrb->abData[sizeof(VUSBSETUP)];
-                                        pProxyDevUsbIp->pUrbUsbIp->pVUsbUrb->cbData = pProxyDevUsbIp->BufRet.RetSubmit.u32ActualLength + sizeof(VUSBSETUP);
+                                        cbRet = pProxyDevUsbIp->BufRet.RetSubmit.u32ActualLength + sizeof(VUSBSETUP);
                                     }
                                     else
                                     {
                                         pbData = &pProxyDevUsbIp->pUrbUsbIp->pVUsbUrb->abData[0];
-                                        pProxyDevUsbIp->pUrbUsbIp->pVUsbUrb->cbData = pProxyDevUsbIp->BufRet.RetSubmit.u32ActualLength;
+                                        cbRet = pProxyDevUsbIp->BufRet.RetSubmit.u32ActualLength;
                                     }
 
                                     if (pProxyDevUsbIp->BufRet.RetSubmit.u32ActualLength)
-                                        usbProxyUsbIpRecvStateAdvance(pProxyDevUsbIp, USBPROXYUSBIPRECVSTATE_URB_BUFFER,
-                                                                      pbData, pProxyDevUsbIp->BufRet.RetSubmit.u32ActualLength);
+                                    {
+                                        if (RT_LIKELY(pProxyDevUsbIp->pUrbUsbIp->pVUsbUrb->cbData >= cbRet))
+                                        {
+                                            pProxyDevUsbIp->pUrbUsbIp->pVUsbUrb->cbData = cbRet;
+                                            usbProxyUsbIpRecvStateAdvance(pProxyDevUsbIp, USBPROXYUSBIPRECVSTATE_URB_BUFFER,
+                                                                          pbData, pProxyDevUsbIp->BufRet.RetSubmit.u32ActualLength);
+                                        }
+                                        else
+                                        {
+                                            /*
+                                             * Bogus length returned from the USB/IP remote server.
+                                             * Error out because there is no way to find the end of the current
+                                             * URB and the beginning of the next one. The error will cause closing the
+                                             * connection to the rogue remote and all URBs get completed with an error.
+                                             */
+                                            LogRelMax(10, ("USB/IP: Received reply with sequence number %u contains invalid length %zu (max %zu)\n",
+                                                           pProxyDevUsbIp->BufRet.Hdr.u32SeqNum, cbRet,
+                                                           pProxyDevUsbIp->pUrbUsbIp->pVUsbUrb->cbData));
+                                            rc = VERR_NET_PROTOCOL_ERROR;
+                                        }
+                                    }
                                     else
                                     {
@@ -1015 +1034 @@
                                         RT_N2H_U32(pProxyDevUsbIp->BufRet.Hdr.u32SeqNum)));
                                 usbProxyUsbIpResetRecvState(pProxyDevUsbIp);
+                                rc = VERR_NET_PROTOCOL_ERROR;
                             }
                             break;
@@ -1049 +1069 @@
                     {
                         PVUSBURBISOCPTK pIsocPkt = &pProxyDevUsbIp->pUrbUsbIp->pVUsbUrb->aIsocPkts[i];
-                        usbProxyUsbIpIsocPktDescN2H(&pProxyDevUsbIp->aIsocPktDesc[i]);
-                        pIsocPkt->enmStatus = usbProxyUsbIpVUsbStatusConvertFromStatus(pProxyDevUsbIp->aIsocPktDesc[i].i32Status);
-                        pIsocPkt->off       = pProxyDevUsbIp->aIsocPktDesc[i].u32Offset;
-                        pIsocPkt->cb        = pProxyDevUsbIp->aIsocPktDesc[i].u32ActualLength;
+                        PUsbIpIsocPktDesc pIsocPktUsbIp = &pProxyDevUsbIp->aIsocPktDesc[i];
+
+                        usbProxyUsbIpIsocPktDescN2H(pIsocPktUsbIp);
+                        pIsocPkt->enmStatus = usbProxyUsbIpVUsbStatusConvertFromStatus(pIsocPktUsbIp->i32Status);
+
+                        if (RT_LIKELY(   pIsocPktUsbIp->u32Offset < pProxyDevUsbIp->pUrbUsbIp->pVUsbUrb->cbData
+                                      && pProxyDevUsbIp->pUrbUsbIp->pVUsbUrb->cbData - pIsocPktUsbIp->u32Offset >= pIsocPktUsbIp->u32ActualLength))
+                        {
+                            pIsocPkt->off = pIsocPktUsbIp->u32Offset;
+                            pIsocPkt->cb  = pIsocPktUsbIp->u32ActualLength;
+                        }
+                        else
+                        {
+                            /*
+                             * The offset and length value in the isoc packet descriptor are bogus and would cause a buffer overflow later on, leave an
+                             * error message and disconnect from the rogue remote end.
+                             */
+                            LogRelMax(10, ("USB/IP: Received reply with sequence number %u contains invalid isoc packet descriptor %u (offset=%u length=%u)\n",
+                                           pProxyDevUsbIp->BufRet.Hdr.u32SeqNum, i,
+                                           pIsocPktUsbIp->u32Offset, pIsocPktUsbIp->u32ActualLength));
+                            rc = VERR_NET_PROTOCOL_ERROR;
+                            break;
+                        }
                     }
 
@@ -1063 +1102 @@
         }
     }
-    else
-    {
-        /** @todo Complete all URBs with DNR error and mark device as unplugged. */
-#if 0
-        pUrbUsbIp = pProxyDevUsbIp->pUrbUsbIp;
-        pUrbUsbIp->pVUsbUrb->enmStatus = VUSBSTATUS_DNR;
-        usbProxyUsbIpResetRecvState(pProxyDevUsbIp);
-#endif
-    }
 
     if (RT_SUCCESS(rc))
         *ppUrbUsbIp = pUrbUsbIp;
+    else
+    {
+        /* Complete all URBs with DNR error and mark device as unplugged, the current one is still in the in flight list. */
+        pProxyDevUsbIp->pUrbUsbIp = NULL;
+        usbProxyUsbIpResetRecvState(pProxyDevUsbIp);
+        usbProxyUsbIpDisconnect(pProxyDevUsbIp);
+
+        rc = RTSemFastMutexRequest(pProxyDevUsbIp->hMtxLists);
+        AssertRC(rc);
+        PUSBPROXYURBUSBIP pIt;
+        PUSBPROXYURBUSBIP pItNext;
+        RTListForEachSafe(&pProxyDevUsbIp->ListUrbsInFlight, pIt, pItNext, USBPROXYURBUSBIP, NodeList)
+        {
+            if (pIt->pVUsbUrb) /* can be NULL for requests created by usbProxyUsbIpCtrlUrbExchangeSync(). */
+                pIt->pVUsbUrb->enmStatus = VUSBSTATUS_CRC;
+            RTListNodeRemove(&pIt->NodeList);
+            RTListAppend(&pProxyDevUsbIp->ListUrbsLanded, &pIt->NodeList);
+        }
+        RTSemFastMutexRelease(pProxyDevUsbIp->hMtxLists);
+    }
 
     return rc;
@@ -1171 +1221 @@
             break;
         default:
-            usbProxyUsbIpUrbFree(pProxyDevUsbIp, pUrbUsbIp);
             return VERR_INVALID_PARAMETER; /** @todo better status code. */
     }
@@ -1216 +1265 @@
         if (RT_FAILURE(rc))
         {
-            /** @todo  Complete the URB with an error. */
-            usbProxyUsbIpUrbFree(pProxyDevUsbIp, pIter);
+            /* Complete URB with an error and place into landed list. */
+            pIter->pVUsbUrb->enmStatus = VUSBSTATUS_DNR;
+            usbProxyUsbIpLinkUrb(pProxyDevUsbIp, &pProxyDevUsbIp->ListUrbsLanded, pIter);
         }
     }
@@ -1475 +1525 @@
 
     PUSBPROXYDEVUSBIP pDevUsbIp = USBPROXYDEV_2_DATA(pProxyDev, PUSBPROXYDEVUSBIP);
+    if (pDevUsbIp->hSocket != NIL_RTSOCKET)
+        usbProxyUsbIpDisconnect(pDevUsbIp);
+
     /* Destroy the pipe and pollset if necessary. */
     if (pDevUsbIp->hPollSet != NIL_RTPOLLSET)
     {
-        if (pDevUsbIp->hSocket != NIL_RTSOCKET)
-        {
-            rc = RTPollSetRemove(pDevUsbIp->hPollSet, USBIP_POLL_ID_SOCKET);
-            Assert(RT_SUCCESS(rc) || rc == VERR_POLL_HANDLE_ID_NOT_FOUND);
-        }
         rc = RTPollSetRemove(pDevUsbIp->hPollSet, USBIP_POLL_ID_PIPE);
         AssertRC(rc);
@@ -1493 +1541 @@
     }
 
-    if (pDevUsbIp->hSocket != NIL_RTSOCKET)
-        usbProxyUsbIpDisconnect(pDevUsbIp);
     if (pDevUsbIp->pszHost)
         RTStrFree(pDevUsbIp->pszHost);