Changeset 97923 in vbox for trunk/src/VBox/Additions

Timestamp:

Dec 30, 2022 10:24:20 PM (2 years ago)

Author:

vboxsync

Message:

Add/VBoxGuestR0LibPhysHeap.cpp: Made VbglR0PhysHeapFree and VbglR0PhysHeapGetPhysAddr more strict wrt input. Cleanups.

Location:

trunk/src/VBox/Additions/common/VBoxGuest/lib

Files:

• VBoxGuestR0LibPhysHeap.cpp (modified) (4 diffs)
• testcase/tstVbglR0PhysHeap-1.cpp (modified) (7 diffs)

trunk/src/VBox/Additions/common/VBoxGuest/lib/VBoxGuestR0LibPhysHeap.cpp

@@ -219 +219 @@
     if (pBlock)
         return pBlock + 1;
-    return NULL;
-}
-
-
-DECLINLINE(VBGLPHYSHEAPBLOCK *) vbglPhysHeapData2Block(void *pv)
-{
-    if (pv)
-    {
-        VBGLPHYSHEAPBLOCK *pBlock = (VBGLPHYSHEAPBLOCK *)pv - 1;
-        AssertMsgReturn(pBlock->u32Signature == VBGL_PH_BLOCKSIGNATURE,
-                        ("pBlock->u32Signature = %08X\n", pBlock->u32Signature),
-                        NULL);
-        return pBlock;
-    }
     return NULL;
 }
@@ -487 +473 @@
      */
 
+    /** @todo r=bird: Don't walk these lists for ever, use the block count
+     *        statistics to limit the walking to the first X or something. */
     pBlock = NULL;
     if (cbSize <= PAGE_SIZE / 4 * 3)
@@ -588 +576 @@
 }
 
+
 DECLR0VBGL(uint32_t) VbglR0PhysHeapGetPhysAddr(void *pv)
 {
-    uint32_t physAddr = 0;
-    VBGLPHYSHEAPBLOCK *pBlock = vbglPhysHeapData2Block(pv);
-
-    if (pBlock)
-    {
-        VBGL_PH_ASSERT_MSG(pBlock->fAllocated, ("pBlock = %p\n", pBlock));
-
-        if (pBlock->fAllocated)
-            physAddr = pBlock->pChunk->physAddr + (uint32_t)((uintptr_t)pv - (uintptr_t)pBlock->pChunk);
-    }
-
-    return physAddr;
+    /*
+     * Validate the incoming pointer.
+     */
+    if (pv != NULL)
+    {
+        VBGLPHYSHEAPBLOCK *pBlock = (VBGLPHYSHEAPBLOCK *)pv - 1;
+        if (   pBlock->u32Signature == VBGL_PH_BLOCKSIGNATURE
+            && pBlock->fAllocated)
+        {
+            /*
+             * Calculate and return its physical address.
+             */
+            return pBlock->pChunk->physAddr + (uint32_t)((uintptr_t)pv - (uintptr_t)pBlock->pChunk);
+        }
+
+        AssertMsgFailed(("Use after free or corrupt pointer variable: pv=%p pBlock=%p: u32Signature=%#x cb=%#x fAllocated=%d\n",
+                         pv, pBlock, pBlock->u32Signature, pBlock->cbDataSize, pBlock->fAllocated));
+    }
+    else
+        AssertMsgFailed(("Unexpected NULL pointer\n"));
+    return 0;
 }
 
@@ -607 +605 @@
 DECLR0VBGL(void) VbglR0PhysHeapFree(void *pv)
 {
-    VBGLPHYSHEAPBLOCK *pBlock;
-    VBGLPHYSHEAPBLOCK *pNeighbour;
-    VBGLPHYSHEAPCHUNK *pChunk;
-
-    int rc = vbglPhysHeapEnter();
-    if (RT_FAILURE(rc))
-        return;
-
-    dumpheap("pre free");
-
-    pBlock = vbglPhysHeapData2Block(pv);
-
-    if (!pBlock)
-    {
-        vbglPhysHeapLeave();
-        return;
-    }
-
-    VBGL_PH_ASSERT_MSG(pBlock->fAllocated, ("pBlock = %p\n", pBlock));
-
-    /* Exclude from allocated list */
-    vbglPhysHeapExcludeBlock(pBlock);
-
-    dumpheap("post exclude");
-
-    VBGL_PH_dprintf(("VbglR0PhysHeapFree %p size %x\n", pv, pBlock->cbDataSize));
-
-    /* Mark as free */
-    pBlock->fAllocated = false;
-
-    /* Insert to free list */
-    vbglPhysHeapInsertBlock(NULL, pBlock);
-
-    dumpheap("post insert");
-
-    pChunk = pBlock->pChunk;
-
-    /* Check if we can merge 2 free blocks. To simplify heap maintenance,
-     * we will look at block after the just freed one.
-     * This will not prevent us from detecting free memory chunks.
-     * Also in most cases blocks are deallocated in reverse allocation order
-     * and in that case the merging will work.
-     */
-    /** @todo r=bird: This simplistic approach is of course not working.
-     *        However, since the heap lists aren't sorted in any way, we cannot
-     *        cheaply determine where the block before us starts. */
-
-    pNeighbour = (VBGLPHYSHEAPBLOCK *)((uintptr_t)(pBlock + 1) + pBlock->cbDataSize);
-
-    if (   (uintptr_t)pNeighbour < (uintptr_t)pChunk + pChunk->cbSize
-        && !pNeighbour->fAllocated)
-    {
-        /* The next block is free as well. */
-
-        /* Adjust size of current memory block */
-        pBlock->cbDataSize += pNeighbour->cbDataSize + sizeof(VBGLPHYSHEAPBLOCK);
-
-        /* Exclude the next neighbour */
-        vbglPhysHeapExcludeBlock(pNeighbour);
-    }
-
-    dumpheap("post merge");
-
-    /* now check if there are 2 or more free (unused) chunks */
-    if (pChunk->acBlocks[1 /*allocated*/] == 0)
-    {
-        VBGLPHYSHEAPCHUNK *pCurChunk;
-
-        uint32_t cUnusedChunks = 0;
-
-        for (pCurChunk = g_vbgldata.pChunkHead; pCurChunk; pCurChunk = pCurChunk->pNext)
+    if (pv != NULL)
+    {
+        VBGLPHYSHEAPBLOCK *pBlock;
+
+        int rc = RTSemFastMutexRequest(g_vbgldata.mutexHeap);
+        AssertRCReturnVoid(rc);
+
+        dumpheap("pre free");
+
+        /*
+         * Validate the block header.
+         */
+        pBlock = (VBGLPHYSHEAPBLOCK *)pv - 1;
+        if (   pBlock->u32Signature == VBGL_PH_BLOCKSIGNATURE
+            && pBlock->fAllocated)
         {
-            Assert(pCurChunk->u32Signature == VBGL_PH_CHUNKSIGNATURE);
-            if (pCurChunk->acBlocks[1 /*allocated*/] == 0)
-                cUnusedChunks++;
+            VBGLPHYSHEAPCHUNK *pChunk;
+            VBGLPHYSHEAPBLOCK *pNeighbour;
+
+            /*
+             * Move the block from the allocated list to the free list.
+             */
+            VBGL_PH_dprintf(("VbglR0PhysHeapFree: %p size %#x\n", pv, pBlock->cbDataSize));
+            vbglPhysHeapExcludeBlock(pBlock);
+
+            dumpheap("post exclude");
+
+            pBlock->fAllocated = false;
+            vbglPhysHeapInsertBlock(NULL, pBlock);
+
+            dumpheap("post insert");
+
+            /*
+             * Check if the block after this one is also free and we can merge it into
+             * this one.
+             *
+             * Because the free list isn't sorted by address we cannot cheaply do the
+             * same for the block before us, so we have to hope for the best for now.
+             */
+            /** @todo When the free:used ration in chunk is too skewed, scan the whole
+             *        chunk and merge adjacent blocks that way every so often.  Always do so
+             *        when this is the last used one and we end up with more than 1 free
+             *        node afterwards. */
+            pChunk = pBlock->pChunk;
+            pNeighbour = (VBGLPHYSHEAPBLOCK *)((uintptr_t)(pBlock + 1) + pBlock->cbDataSize);
+            if (   (uintptr_t)pNeighbour <= (uintptr_t)pChunk + pChunk->cbSize - sizeof(*pNeighbour)
+                && !pNeighbour->fAllocated)
+            {
+                /* Adjust size of current memory block */
+                pBlock->cbDataSize += pNeighbour->cbDataSize + sizeof(VBGLPHYSHEAPBLOCK);
+
+                /* Exclude the next neighbour */
+                vbglPhysHeapExcludeBlock(pNeighbour);
+
+                dumpheap("post merge");
+            }
+
+            /*
+             * If this chunk is now completely unused, delete it if there are
+             * more completely free ones.
+             */
+            if (   pChunk->acBlocks[1 /*allocated*/] == 0
+                && (pChunk->pPrev || pChunk->pNext))
+            {
+                VBGLPHYSHEAPCHUNK *pCurChunk;
+                uint32_t           cUnusedChunks = 0;
+                for (pCurChunk = g_vbgldata.pChunkHead; pCurChunk; pCurChunk = pCurChunk->pNext)
+                {
+                    AssertBreak(pCurChunk->u32Signature == VBGL_PH_CHUNKSIGNATURE);
+                    if (pCurChunk->acBlocks[1 /*allocated*/] == 0)
+                    {
+                        cUnusedChunks++;
+                        if (cUnusedChunks > 1)
+                        {
+                            /* Delete current chunk, it will also exclude all free blocks
+                             * remaining in the chunk from the free list, so the pBlock
+                             * will also be invalid after this.
+                             */
+                            vbglPhysHeapChunkDelete(pChunk);
+                            pNeighbour = pBlock = NULL; /* invalid */
+                            break;
+                        }
+                    }
+                }
+            }
+
+            dumpheap("post free");
         }
-
-        if (cUnusedChunks > 1)
-        {
-            /* Delete current chunk, it will also exclude all free blocks
-             * remaining in the chunk from the free list, so the pBlock
-             * will also be invalid after this.
-             */
-            vbglPhysHeapChunkDelete(pChunk);
-        }
-    }
-
-    dumpheap("post free");
-
-    vbglPhysHeapLeave();
+        else
+            AssertMsgFailed(("pBlock: %p: u32Signature=%#x cb=%#x fAllocated=%d - double free?\n",
+                             pBlock, pBlock->u32Signature, pBlock->cbDataSize, pBlock->fAllocated));
+
+        rc = RTSemFastMutexRelease(g_vbgldata.mutexHeap);
+        AssertRC(rc);
+    }
 }
 

trunk/src/VBox/Additions/common/VBoxGuest/lib/testcase/tstVbglR0PhysHeap-1.cpp

@@ -87 +87 @@
             g_cChunks++;
             g_cbChunks += cb;
-            *pPhys = (uint32_t)(uintptr_t)pvRet ^ UINT32_C(0xf0f0f000);
+            *pPhys = (uint32_t)(uintptr_t)pvRet ^ (UINT32_C(0xf0f0f0f0) & ~(uint32_t)PAGE_OFFSET_MASK);
+
+            /* Avoid problematic values that won't happen in real life:  */
+            if (!*pPhys)
+                *pPhys = 4U << PAGE_SHIFT;
+            if (UINT32_MAX - *pPhys < cb)
+                *pPhys -= RT_ALIGN_32(cb, PAGE_SIZE);
+
             return pvRet;
         }
@@ -160 +167 @@
     if (RT_FAILURE(rc))
         return RTTestSummaryAndDestroy(hTest);
+
+#define CHECK_PHYS_ADDR(a_pv) do { \
+        uint32_t const uPhys = VbglR0PhysHeapGetPhysAddr(a_pv); \
+        if (uPhys == 0 || uPhys == UINT32_MAX || (uPhys & PAGE_OFFSET_MASK) != ((uintptr_t)(a_pv) & PAGE_OFFSET_MASK)) \
+            RTTestIFailed("line %u: %s=%p: uPhys=%#x\n", __LINE__, #a_pv, (a_pv), uPhys); \
+    } while (0)
 
     /*
@@ -210 +223 @@
         RTTESTI_CHECK_MSG(RT_ALIGN_P(s_aOps[i].pvAlloc, sizeof(void *)) == s_aOps[i].pvAlloc,
                           ("VbglR0PhysHeapAlloc(%#x) -> %p\n", s_aOps[i].cb, i));
-        if (!s_aOps[i].pvAlloc)
-            return RTTestSummaryAndDestroy(hTest);
+
+        CHECK_PHYS_ADDR(s_aOps[i].pvAlloc);
     }
 
@@ -230 +243 @@
         if (!pv)
             return RTTestSummaryAndDestroy(hTest);
+        CHECK_PHYS_ADDR(pv);
+
         //RTPrintf("debug: i=%d pv=%p cbReal=%#zx cbBeforeSub=%#zx cbAfterSubFree=%#zx cbAfterSubAlloc=%#zx \n", i, pv, RTHeapOffsetSize(Heap, pv),
         //         cbBeforeSub, cbAfterSubFree, VbglR0PhysHeapGetFreeSize());
@@ -283 +298 @@
             }
             if (s_aHistory[i].pv)
+            {
                 memset(s_aHistory[i].pv, 0xbb, s_aHistory[i].cb);
+                CHECK_PHYS_ADDR(s_aHistory[i].pv);
+            }
         }
         else
@@ -311 +329 @@
                     s_aHistory[i].pv = VbglR0PhysHeapAlloc(s_aHistory[i].cb);
                     if (s_aHistory[i].pv)
+                    {
                         memset(s_aHistory[i].pv, 0x55, s_aHistory[i].cb);
+                        CHECK_PHYS_ADDR(s_aHistory[i].pv);
+                    }
                 }
 
@@ -327 +348 @@
                         }
                         if (s_aHistory[i].pv)
+                        {
                             memset(s_aHistory[i].pv, 0x55, s_aHistory[i].cb);
+                            CHECK_PHYS_ADDR(s_aHistory[i].pv);
+                        }
 
                         cbFree = VbglR0PhysHeapGetFreeSize();