Changeset 78073 in vbox for trunk/src/VBox/Devices/USB

Timestamp:

Apr 10, 2019 10:09:05 AM (6 years ago)

Author:

vboxsync

svn:sync-xref-src-repo-rev:

129929

Message:

Devices/USB/DevOHCI.cpp: Remove a few todos and raise unrecoverable errors when the HC encounters an invalid state bugref:9429

File:

• trunk/src/VBox/Devices/USB/DevOHCI.cpp (modified) (20 diffs)

trunk/src/VBox/Devices/USB/DevOHCI.cpp

@@ -906 +906 @@
  */
 # define ohciR3SetInterrupt(ohci, intr) ohciR3SetInterruptInt(ohci, VERR_IGNORED, intr, #intr)
+
+
+/**
+ * Sets the HC in the unrecoverable error state and raises the appropriate interrupt.
+ *
+ * @returns nothing.
+ * @param   pThis               The OHCI instance.
+ */
+DECLINLINE(void) ohciR3RaiseUnrecoverableError(POHCI pThis)
+{
+    ohciR3SetInterrupt(pThis, OHCI_INTR_UNRECOVERABLE_ERROR);
+}
 
 
@@ -2081 +2093 @@
  * Sets up a OHCI transport buffer.
  *
+ * @returns Flags whether the buffer could be initialised successfully.
  * @param   pBuf    Ohci buffer.
  * @param   cbp     Current buffer pointer. 32-bit physical address.
  * @param   be      Last byte in buffer (BufferEnd). 32-bit physical address.
  */
-static void ohciR3BufInit(POHCIBUF pBuf, uint32_t cbp, uint32_t be)
-{
+static bool ohciR3BufInit(POHCIBUF pBuf, uint32_t cbp, uint32_t be)
+{
+    if (RT_UNLIKELY(be < cbp))
+        return false;
+
     if (!cbp || !be)
     {
@@ -2111 +2127 @@
         Log2(("ohci: cbp=%#010x be=%#010x cbTotal=%u PAGE FLIP\n", cbp, be, pBuf->cbTotal));
     }
+
+    return true;
 }
 
@@ -2660 +2678 @@
          */
         OHCIBUF Buf;
-        ohciR3BufInit(&Buf, pTd->cbp, pTd->be);
+        if (!ohciR3BufInit(&Buf, pTd->cbp, pTd->be))
+        {
+            ohciR3RaiseUnrecoverableError(pThis);
+            return;
+        }
         uint32_t NewCbp;
         if (cbLeft >= Buf.cbTotal)
@@ -2683 +2705 @@
         {
             Assert(Buf.cVecs > 0);
+
+            /* Be paranoid */
+            if (   Buf.aVecs[0].cb > cbLeft
+                || (   Buf.cVecs > 1
+                    && Buf.aVecs[1].cb > (cbLeft - Buf.aVecs[0].cb)))
+            {
+                ohciR3RaiseUnrecoverableError(pThis);
+                return;
+            }
+
             ohciR3PhysWrite(pThis, Buf.aVecs[0].Addr, pb, Buf.aVecs[0].cb);
             if (Buf.cVecs > 1)
@@ -2790 +2822 @@
 
     ohciR3Lock(pThis);
-    pThis->fIdle = false;   /* Mark as active */
-
-    /* get the current end point descriptor. */
-    OHCIED Ed;
-    ohciR3ReadEd(pThis, pUrb->pHci->EdAddr, &Ed);
-
-    /*
-     * Check that the URB hasn't been canceled and then try unlink the TDs.
-     *
-     * We drop the URB if the ED is marked halted/skip ASSUMING that this
-     * means the HCD has canceled the URB.
-     *
-     * If we succeed here (i.e. not dropping the URB), the TdCopy members will
-     * be updated but not yet written. We will delay the writing till we're done
-     * with the data copying, buffer pointer advancing and error handling.
-     */
-    int cFmAge = ohciR3InFlightRemoveUrb(pThis, pUrb);
-    if (pUrb->enmStatus == VUSBSTATUS_UNDO)
-    {
-        /* Leave the TD alone - the HCD doesn't want us talking to the device. */
-        Log(("%s: ohciR3RhXferCompletion: CANCELED {ED=%#010x cTds=%d TD0=%#010x age %d}\n",
-             pUrb->pszDesc, pUrb->pHci->EdAddr, pUrb->pHci->cTds, pUrb->paTds[0].TdAddr, cFmAge));
-        STAM_COUNTER_INC(&pThis->StatDroppedUrbs);
-        ohciR3Unlock(pThis);
-        return;
-    }
-    bool fHasBeenCanceled = false;
-    if (    (Ed.HeadP & ED_HEAD_HALTED)
-        ||  (Ed.hwinfo & ED_HWINFO_SKIP)
-        ||  cFmAge < 0
-        ||  (fHasBeenCanceled = ohciR3HasUrbBeenCanceled(pThis, pUrb, &Ed))
-        ||  !ohciR3UnlinkTds(pThis, pUrb, &Ed)
-       )
-    {
-        Log(("%s: ohciR3RhXferCompletion: DROPPED {ED=%#010x cTds=%d TD0=%#010x age %d} because:%s%s%s%s%s!!!\n",
-             pUrb->pszDesc, pUrb->pHci->EdAddr, pUrb->pHci->cTds, pUrb->paTds[0].TdAddr, cFmAge,
-             (Ed.HeadP & ED_HEAD_HALTED)                            ? " ep halted" : "",
-             (Ed.hwinfo & ED_HWINFO_SKIP)                           ? " ep skip" : "",
-             (Ed.HeadP & ED_PTR_MASK) != pUrb->paTds[0].TdAddr      ? " ep head-changed" : "",
-             cFmAge < 0                                             ? " td not-in-flight" : "",
-             fHasBeenCanceled                                       ? " td canceled" : ""));
-        NOREF(fHasBeenCanceled);
-        STAM_COUNTER_INC(&pThis->StatDroppedUrbs);
-        ohciR3Unlock(pThis);
-        return;
-    }
-
-    /*
-     * Complete the TD updating and write the back.
-     * When appropriate also copy data back to the guest memory.
-     */
-    if (pUrb->enmType == VUSBXFERTYPE_ISOC)
-        ohciR3RhXferCompleteIsochronousURB(pThis, pUrb /*, &Ed , cFmAge*/);
-    else
-        ohciR3RhXferCompleteGeneralURB(pThis, pUrb, &Ed, cFmAge);
-
-    /* finally write back the endpoint descriptor. */
-    ohciR3WriteEd(pThis, pUrb->pHci->EdAddr, &Ed);
+
+    /* Do nothing if the HC encountered an unrecoverable error. */
+    if (!(pThis->intr_status & OHCI_INTR_UNRECOVERABLE_ERROR))
+    {
+        pThis->fIdle = false;   /* Mark as active */
+
+        /* get the current end point descriptor. */
+        OHCIED Ed;
+        ohciR3ReadEd(pThis, pUrb->pHci->EdAddr, &Ed);
+
+        /*
+         * Check that the URB hasn't been canceled and then try unlink the TDs.
+         *
+         * We drop the URB if the ED is marked halted/skip ASSUMING that this
+         * means the HCD has canceled the URB.
+         *
+         * If we succeed here (i.e. not dropping the URB), the TdCopy members will
+         * be updated but not yet written. We will delay the writing till we're done
+         * with the data copying, buffer pointer advancing and error handling.
+         */
+        int cFmAge = ohciR3InFlightRemoveUrb(pThis, pUrb);
+        if (pUrb->enmStatus == VUSBSTATUS_UNDO)
+        {
+            /* Leave the TD alone - the HCD doesn't want us talking to the device. */
+            Log(("%s: ohciR3RhXferCompletion: CANCELED {ED=%#010x cTds=%d TD0=%#010x age %d}\n",
+                 pUrb->pszDesc, pUrb->pHci->EdAddr, pUrb->pHci->cTds, pUrb->paTds[0].TdAddr, cFmAge));
+            STAM_COUNTER_INC(&pThis->StatDroppedUrbs);
+            ohciR3Unlock(pThis);
+            return;
+        }
+        bool fHasBeenCanceled = false;
+        if (    (Ed.HeadP & ED_HEAD_HALTED)
+            ||  (Ed.hwinfo & ED_HWINFO_SKIP)
+            ||  cFmAge < 0
+            ||  (fHasBeenCanceled = ohciR3HasUrbBeenCanceled(pThis, pUrb, &Ed))
+            ||  !ohciR3UnlinkTds(pThis, pUrb, &Ed)
+           )
+        {
+            Log(("%s: ohciR3RhXferCompletion: DROPPED {ED=%#010x cTds=%d TD0=%#010x age %d} because:%s%s%s%s%s!!!\n",
+                 pUrb->pszDesc, pUrb->pHci->EdAddr, pUrb->pHci->cTds, pUrb->paTds[0].TdAddr, cFmAge,
+                 (Ed.HeadP & ED_HEAD_HALTED)                            ? " ep halted" : "",
+                 (Ed.hwinfo & ED_HWINFO_SKIP)                           ? " ep skip" : "",
+                 (Ed.HeadP & ED_PTR_MASK) != pUrb->paTds[0].TdAddr      ? " ep head-changed" : "",
+                 cFmAge < 0                                             ? " td not-in-flight" : "",
+                 fHasBeenCanceled                                       ? " td canceled" : ""));
+            NOREF(fHasBeenCanceled);
+            STAM_COUNTER_INC(&pThis->StatDroppedUrbs);
+            ohciR3Unlock(pThis);
+            return;
+        }
+
+        /*
+         * Complete the TD updating and write the back.
+         * When appropriate also copy data back to the guest memory.
+         */
+        if (pUrb->enmType == VUSBXFERTYPE_ISOC)
+            ohciR3RhXferCompleteIsochronousURB(pThis, pUrb /*, &Ed , cFmAge*/);
+        else
+            ohciR3RhXferCompleteGeneralURB(pThis, pUrb, &Ed, cFmAge);
+
+        /* finally write back the endpoint descriptor. */
+        ohciR3WriteEd(pThis, pUrb->pHci->EdAddr, &Ed);
+    }
+
     ohciR3Unlock(pThis);
 }
@@ -2935 +2973 @@
     ohciR3ReadTd(pThis, TdAddr, &Td);
     OHCIBUF Buf;
-    ohciR3BufInit(&Buf, Td.cbp, Td.be);
+    if (!ohciR3BufInit(&Buf, Td.cbp, Td.be))
+    {
+        ohciR3RaiseUnrecoverableError(pThis);
+        return false;
+    }
 
     *pNextTdAddr = Td.NextTD & ED_PTR_MASK;
@@ -2955 +2997 @@
                 default:
                     Log(("ohciR3ServiceTd: Invalid direction!!!! Td.hwinfo=%#x Ed.hwdinfo=%#x\n", Td.hwinfo, pEd->hwinfo));
-                    /** @todo Do the correct thing here */
+                    ohciR3RaiseUnrecoverableError(pThis);
                     return false;
             }
@@ -2988 +3030 @@
         &&  enmDir != VUSBDIRECTION_IN)
     {
+        /* Be paranoid. */
+        if (   Buf.aVecs[0].cb > pUrb->cbData
+            || (   Buf.cVecs > 1
+                && Buf.aVecs[1].cb > (pUrb->cbData - Buf.aVecs[0].cb)))
+        {
+            ohciR3RaiseUnrecoverableError(pThis);
+            VUSBIRhFreeUrb(pThis->RootHub.pIRhConn, pUrb);
+            return false;
+        }
+
         ohciR3PhysRead(pThis, Buf.aVecs[0].Addr, pUrb->abData, Buf.aVecs[0].cb);
         if (Buf.cVecs > 1)
@@ -3009 +3061 @@
     Log(("ohciR3ServiceTd: failed submitting TdAddr=%#010x EdAddr=%#010x pUrb=%p!!\n",
          TdAddr, EdAddr, pUrb));
+    VUSBIRhFreeUrb(pThis->RootHub.pIRhConn, pUrb);
     ohciR3InFlightRemove(pThis, TdAddr);
     return false;
@@ -3061 +3114 @@
     /* read the head */
     ohciR3ReadTd(pThis, TdAddr, &Head.Td);
-    ohciR3BufInit(&Head.Buf, Head.Td.cbp, Head.Td.be);
+    if (!ohciR3BufInit(&Head.Buf, Head.Td.cbp, Head.Td.be))
+    {
+        ohciR3RaiseUnrecoverableError(pThis);
+        return false;
+    }
     Head.TdAddr = TdAddr;
     Head.pNext = NULL;
@@ -3079 +3136 @@
         pCur->TdAddr = pTail->Td.NextTD & ED_PTR_MASK;
         ohciR3ReadTd(pThis, pCur->TdAddr, &pCur->Td);
-        ohciR3BufInit(&pCur->Buf, pCur->Td.cbp, pCur->Td.be);
+        if (!ohciR3BufInit(&pCur->Buf, pCur->Td.cbp, pCur->Td.be))
+        {
+            ohciR3RaiseUnrecoverableError(pThis);
+            return false;
+        }
 
         /* Don't combine if the direction doesn't match up. There can't actually be
@@ -3113 +3174 @@
                 default:
                     Log(("ohciR3ServiceTdMultiple: Invalid direction!!!! Head.Td.hwinfo=%#x Ed.hwdinfo=%#x\n", Head.Td.hwinfo, pEd->hwinfo));
-                    /** @todo Do the correct thing here */
+                    ohciR3RaiseUnrecoverableError(pThis);
                     return false;
             }
@@ -3180 +3241 @@
     for (struct OHCITDENTRY *pCur = &Head; pCur; pCur = pCur->pNext, iTd++)
         ohciR3InFlightRemove(pThis, pCur->TdAddr);
+    VUSBIRhFreeUrb(pThis->RootHub.pIRhConn, pUrb);
     return false;
 }
@@ -3291 +3353 @@
         default:
             Log(("ohciR3ServiceIsochronousTd: Invalid direction!!!! Ed.hwdinfo=%#x\n", pEd->hwinfo));
-            /* Should probably raise an unrecoverable HC error here */
+            ohciR3RaiseUnrecoverableError(pThis);
             return false;
     }
@@ -3307 +3369 @@
     uint32_t cbTotal = 0;
     if (((uint32_t)pITd->aPSW[R] >> ITD_PSW_CC_SHIFT) < (OHCI_CC_NOT_ACCESSED_0 >> TD_HWINFO_CC_SHIFT))
+    {
         Log(("ITdAddr=%RX32 PSW%d.CC=%#x < 'Not Accessed'!\n", ITdAddr, R, pITd->aPSW[R] >> ITD_PSW_CC_SHIFT)); /* => Unrecoverable Error*/
+        pThis->intr_status |= OHCI_INTR_UNRECOVERABLE_ERROR;
+        return false;
+    }
     uint16_t offPrev = aPkts[0].off = (pITd->aPSW[R] & ITD_PSW_OFFSET);
 
@@ -3318 +3384 @@
         cbTotal += aPkts[iR - R - 1].cb = off - offPrev;
         if (off < offPrev)
+        {
             Log(("ITdAddr=%RX32 PSW%d.offset=%#x < offPrev=%#x!\n", ITdAddr, iR, off, offPrev)); /* => Unrecoverable Error*/
+            ohciR3RaiseUnrecoverableError(pThis);
+            return false;
+        }
         if (((uint32_t)PSW >> ITD_PSW_CC_SHIFT) < (OHCI_CC_NOT_ACCESSED_0 >> TD_HWINFO_CC_SHIFT))
+        {
             Log(("ITdAddr=%RX32 PSW%d.CC=%#x < 'Not Accessed'!\n", ITdAddr, iR, PSW >> ITD_PSW_CC_SHIFT)); /* => Unrecoverable Error*/
+            ohciR3RaiseUnrecoverableError(pThis);
+            return false;
+        }
         offPrev = off;
     }
@@ -3329 +3403 @@
                           + 1 /* BE is inclusive */;
     if (offEnd < offPrev)
+    {
         Log(("ITdAddr=%RX32 offEnd=%#x < offPrev=%#x!\n", ITdAddr, offEnd, offPrev)); /* => Unrecoverable Error*/
+        ohciR3RaiseUnrecoverableError(pThis);
+        return false;
+    }
     cbTotal += aPkts[cFrames - 1 - R].cb = offEnd - offPrev;
     Assert(cbTotal <= 0x2000);
@@ -3406 +3484 @@
          pUrb, cbTotal, EdAddr, 1, ITdAddr, rc));
     ohciR3InFlightRemove(pThis, ITdAddr);
+    VUSBIRhFreeUrb(pThis->RootHub.pIRhConn, pUrb);
     return false;
 }
@@ -4145 +4224 @@
 # endif
 
-    /* Frame boundary, so do EOF stuff here. */
-    ohciR3BumpFrameNumber(pThis);
-    if ( (pThis->dqic != 0x7) && (pThis->dqic != 0))
-        pThis->dqic--;
-
-    /* Clean up any URBs that have been removed. */
-    ohciR3CancelOrphanedURBs(pThis);
-
-    /* Start the next frame. */
-    ohciR3StartOfFrame(pThis);
+    if (!(pThis->intr_status & OHCI_INTR_UNRECOVERABLE_ERROR))
+    {
+        /* Frame boundary, so do EOF stuff here. */
+        ohciR3BumpFrameNumber(pThis);
+        if ( (pThis->dqic != 0x7) && (pThis->dqic != 0))
+            pThis->dqic--;
+
+        /* Clean up any URBs that have been removed. */
+        ohciR3CancelOrphanedURBs(pThis);
+
+        /* Start the next frame. */
+        ohciR3StartOfFrame(pThis);
+    }
 
 # ifdef VBOX_WITH_OHCI_PHYS_READ_STATS