Changeset 56658 in vbox for trunk/src/VBox/Devices

Timestamp:

Jun 26, 2015 12:51:57 PM (10 years ago)

Author:

vboxsync

Message:

Storage/AHCI: Fix possible use after free after an I/O error occurred. The first I/O request with an I/O error is saved for the error log page but is also put on the per port free requerst list. When the guest reads the error page the request is freed but is still on the free request list and could be used later

File:

• trunk/src/VBox/Devices/Storage/DevAHCI.cpp (modified) (7 diffs)

trunk/src/VBox/Devices/Storage/DevAHCI.cpp

@@ -5716 +5716 @@
     for (unsigned i = 0; i < RT_ELEMENTS(pAhciPort->aActiveTasks); i++)
     {
-        PAHCIREQ pAhciReq = (PAHCIREQ)ASMAtomicXchgPtr((void * volatile *)&pAhciPort->aActiveTasks[i], NULL);
-
-        if (   VALID_PTR(pAhciReq)
-            && pAhciReq != pAhciReqExcept)
-        {
-            bool fXchg = ASMAtomicCmpXchgU32((volatile uint32_t *)&pAhciReq->enmTxState, AHCITXSTATE_CANCELED, AHCITXSTATE_ACTIVE);
-            if (fXchg)
+        PAHCIREQ pAhciReq = ASMAtomicReadPtrT(&pAhciPort->aActiveTasks[i], PAHCIREQ);
+        if (pAhciReq != pAhciReqExcept)
+        {
+            pAhciReq = (PAHCIREQ)ASMAtomicXchgPtr((void * volatile *)&pAhciPort->aActiveTasks[i], NULL);
+
+            if (VALID_PTR(pAhciReq))
             {
-                /* Task is active and was canceled. */
-                AssertReleaseMsg(ASMAtomicReadU32(&pAhciPort->cTasksActive) > 0,
-                                 ("Task was canceled but none is active\n"));
-                ASMAtomicDecU32(&pAhciPort->cTasksActive);
-
-                /*
-                 * Clear the pointer in the cached array. The controller will allocate a
-                 * a new task structure for this tag.
-                 */
-                ASMAtomicWriteNullPtr(&pAhciPort->aActiveTasks[i]);
-                LogRel(("AHCI#%uP%u: Cancelled task %u\n", pAhciPort->CTX_SUFF(pDevIns)->iInstance,
-                        pAhciPort->iLUN, pAhciReq->uTag));
+                bool fXchg = ASMAtomicCmpXchgU32((volatile uint32_t *)&pAhciReq->enmTxState, AHCITXSTATE_CANCELED, AHCITXSTATE_ACTIVE);
+                if (fXchg)
+                {
+                    /* Task is active and was canceled. */
+                    AssertReleaseMsg(ASMAtomicReadU32(&pAhciPort->cTasksActive) > 0,
+                                     ("Task was canceled but none is active\n"));
+                    ASMAtomicDecU32(&pAhciPort->cTasksActive);
+
+                    /*
+                     * Clear the pointer in the cached array. The controller will allocate a
+                     * a new task structure for this tag.
+                     */
+                    ASMAtomicWriteNullPtr(&pAhciPort->aActiveTasks[i]);
+                    LogRel(("AHCI#%uP%u: Cancelled task %u\n", pAhciPort->CTX_SUFF(pDevIns)->iInstance,
+                            pAhciPort->iLUN, pAhciReq->uTag));
+                }
+                else
+                    AssertMsg(pAhciReq->enmTxState == AHCITXSTATE_FREE,
+                              ("Invalid task state, must be free!\n"));
             }
-            else
-                AssertMsg(pAhciReq->enmTxState == AHCITXSTATE_FREE,
-                          ("Invalid task state, must be free!\n"));
         }
     }
@@ -5960 +5963 @@
     pAhciReq = RTListGetFirst(pAhciPort->pListReqsFree, AHCIREQ, NodeList);
     if (pAhciReq)
+    {
+        AssertMsg(pAhciReq->enmTxState == AHCITXSTATE_FREE, ("Should be free!\n"));
         RTListNodeRemove(&pAhciReq->NodeList);
+    }
     RTCritSectLeave(&pAhciPort->CritSectReqsFree);
 
@@ -5978 +5984 @@
 static void ahciR3ReqFree(PAHCIPort pAhciPort, PAHCIREQ pAhciReq)
 {
+    AssertMsg(pAhciReq->enmTxState != AHCITXSTATE_FREE, ("Double free!\n"));
     pAhciReq->enmTxState = AHCITXSTATE_FREE;
 
@@ -6017 +6024 @@
     bool fPortReset = ASMAtomicReadBool(&pAhciPort->fPortReset);
     bool fXchg = ASMAtomicCmpXchgPtr(&pAhciPort->aActiveTasks[pAhciReq->uTag], NULL, pAhciReq);
+    bool fReqErrSaved = false;
 
     /*
@@ -6095 +6103 @@
                 pAhciReq->uATARegError    = ID_ERR;
                 pAhciReq->uATARegStatus   = ATA_STAT_READY | ATA_STAT_ERR;
-                ASMAtomicCmpXchgPtr(&pAhciPort->pTaskErr, pAhciReq, NULL);
+                fReqErrSaved = ASMAtomicCmpXchgPtr(&pAhciPort->pTaskErr, pAhciReq, NULL);
             }
             else
@@ -6207 +6215 @@
         PDMDevHlpAsyncNotificationCompleted(pAhciPort->pDevInsR3);
 
-    if (pAhciReq && !(pAhciReq->fFlags & AHCI_REQ_IS_ON_STACK))
+    /* Don't free the request yet when it is saved for the error log page. */
+    if (   pAhciReq
+        && !(pAhciReq->fFlags & AHCI_REQ_IS_ON_STACK)
+        && !fReqErrSaved)
         ahciR3ReqFree(pAhciPort, pAhciReq);
     return fCanceled;
@@ -6471 +6482 @@
                                 ahciTrimRangesDestroy(pTaskErr);
                             else if (pTaskErr->enmTxDir != AHCITXDIR_FLUSH)
-                                ahciIoBufFree(pAhciPort, pTaskErr, false /* fCopyToGuest */);
+                                ahciReqMemFree(pAhciPort, pTaskErr, true /* fForceFree */);
 
                             /* Finally free the error task state structure because it is completely unused now. */