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

Timestamp:

Jun 22, 2009 7:22:10 PM (15 years ago)

Author:

vboxsync

Message:

VHD: Fix access beyond the data block bitmap with ASMBitTest and ASMBitSet. Fix crash with snapshots

File:

• trunk/src/VBox/Devices/Storage/VHDHDDCore.cpp (modified) (10 diffs)

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

@@ -223 +223 @@
 
 /**
+ * Internal: Allocates the block bitmap rounding up to the next 32bit boundary.
+ *           Can be freed with RTMemFree. The memory is zeroed.
+ */
+DECLINLINE(uint8_t *)vhdBlockBitmapAllocate(PVHDIMAGE pImage)
+{
+    return (uint8_t *)RTMemAllocZ(RT_ALIGN_T(pImage->cbDataBlockBitmap, 4, uint32_t));
+}
+
+/**
  * Internal: Compute and update header checksum.
  */
@@ -373 +382 @@
         if (ddh.ParentLocatorEntry[i].u32Code != RT_H2BE_U32(VHD_PLATFORM_CODE_NONE))
         {
-            rc = vhdLocatorUpdate(pImage, &ddh.ParentLocatorEntry[i], pImage->pszParentFilename);
-            if (RT_FAILURE(rc))
-                goto out;
+            if (pImage->pszParentFilename)
+            {
+                rc = vhdLocatorUpdate(pImage, &ddh.ParentLocatorEntry[i], pImage->pszParentFilename);
+                if (RT_FAILURE(rc))
+                    goto out;
+            }
+            else
+            {
+                /* The parent was deleted. */
+                ddh.ParentLocatorEntry[i].u32Code = RT_H2BE_U32(VHD_PLATFORM_CODE_NONE);
+            }
         }
     }
@@ -546 +563 @@
     LogFlowFunc(("cbDataBlockBitmap=%u\n", pImage->cbDataBlockBitmap));
 
-    pImage->pu8Bitmap = (uint8_t *)RTMemAllocZ(pImage->cbDataBlockBitmap);
+    pImage->pu8Bitmap = vhdBlockBitmapAllocate(pImage);
     if (!pImage->pu8Bitmap)
         return VERR_NO_MEMORY;
@@ -889 +906 @@
 }
 
+/**
+ * Internal: Checks if a sector in the block bitmap is set
+ */
+DECLINLINE(bool) vhdBlockBitmapSectorContainsData(PVHDIMAGE pImage, uint32_t cBlockBitmapEntry)
+{
+    uint32_t iBitmap = (cBlockBitmapEntry / 8) & ~3; /* Byte in the block bitmap. */
+
+    /*
+     * The index of the bit in the byte of the data block bitmap.
+     * The most signifcant bit stands for a lower sector number.
+     */
+    uint8_t  iBitInByte = (32-1) - (cBlockBitmapEntry % 32);
+    uint8_t *puBitmap = pImage->pu8Bitmap + iBitmap;
+
+    AssertMsg(puBitmap < (pImage->pu8Bitmap + pImage->cbDataBlockBitmap),
+                ("VHD: Current bitmap position exceeds maximum size of the bitmap\n"));
+
+    return ASMBitTest(puBitmap, iBitInByte);
+}
+
+/**
+ * Internal: Sets the given sector in the sector bitmap.
+ */
+DECLINLINE(void) vhdBlockBitmapSectorSet(PVHDIMAGE pImage, uint32_t cBlockBitmapEntry)
+{
+    uint32_t iBitmap = (cBlockBitmapEntry / 8) & ~3; /* Byte in the block bitmap. */
+
+    /*
+     * The index of the bit in the byte of the data block bitmap.
+     * The most signifcant bit stands for a lower sector number.
+     */
+    uint8_t  iBitInByte = (32-1) - (cBlockBitmapEntry % 32);
+    uint8_t  *puBitmap  = pImage->pu8Bitmap + iBitmap;
+
+    AssertMsg(puBitmap < (pImage->pu8Bitmap + pImage->cbDataBlockBitmap),
+                ("VHD: Current bitmap position exceeds maximum size of the bitmap\n"));
+
+    ASMBitSet(puBitmap, iBitInByte);
+}
+
 static int vhdRead(void *pBackendData, uint64_t uOffset, void *pvBuf, size_t cbRead, size_t *pcbActuallyRead)
 {
@@ -939 +996 @@
         {
             uint32_t cSectors = 0;
-            uint32_t iBitmap = cBATEntryIndex / 8; /* Byte in the block bitmap. */
-            Assert(iBitmap < pImage->cbDataBlockBitmap);
-
-            /*
-             * The index of the bit in the byte of the data block bitmap.
-             * The most signifcant bit stands for a lower sector number.
-             *
-             * There are 8 bits in a byte but they go from 0 .. 7
-             * hence the (8 - 1) to get the index in the bitmap byte.
-             */
-            uint8_t  iBitInByte = (8 - 1) - (cBATEntryIndex % 8);
-            uint8_t *puBitmap   = pImage->pu8Bitmap + iBitmap;
-
-            if (ASMBitTest(puBitmap, iBitInByte))
+
+            if (vhdBlockBitmapSectorContainsData(pImage, cBATEntryIndex))
             {
-                uint32_t iBATEntryIndexCurr = cBATEntryIndex + 1;
-
+                cBATEntryIndex++;
                 cSectors = 1;
 
@@ -963 +1007 @@
                  * must be read from child.
                  */
-                while (cSectors < (cbRead / VHD_SECTOR_SIZE))
+                while (   (cSectors < (cbRead / VHD_SECTOR_SIZE))
+                       && vhdBlockBitmapSectorContainsData(pImage, cBATEntryIndex))
                 {
-
-                    iBitmap    = iBATEntryIndexCurr / 8; /* Byte in the block bitmap. */
-                    iBitInByte = (8 - 1) - (iBATEntryIndexCurr % 8);
-                    puBitmap  = pImage->pu8Bitmap + iBitmap;
-                    AssertMsg(puBitmap < (pImage->pu8Bitmap + pImage->cbDataBlockBitmap),
-                              ("VHD: Current bitmap position exceeds maximum size of the bitmap\n"));
-                    if (!ASMBitTest(puBitmap, iBitInByte))
-                        break;
-
-                    iBATEntryIndexCurr++;
+                    cBATEntryIndex++;
                     cSectors++;
                 }
@@ -985 +1021 @@
             else
             {
-                uint32_t iBATEntryIndexCurr = cBATEntryIndex + 1;
-
                 /*
                  * The first sector being read is marked clean, so we should read from
@@ -995 +1029 @@
                  * should be read from the parent.
                  */
-
+                cBATEntryIndex++;
                 cSectors = 1;
 
-                while (cSectors < (cbRead / VHD_SECTOR_SIZE))
+                while (   (cSectors < (cbRead / VHD_SECTOR_SIZE))
+                       && !vhdBlockBitmapSectorContainsData(pImage, cBATEntryIndex))
                 {
-                    iBitmap    = iBATEntryIndexCurr / 8; /* Byte in the block bitmap. */
-                    iBitInByte = (8 - 1) - (iBATEntryIndexCurr % 8);
-                    puBitmap  = pImage->pu8Bitmap + iBitmap;
-                    AssertMsg(puBitmap < (pImage->pu8Bitmap + pImage->cbDataBlockBitmap),
-                              ("VHD: Current bitmap position exceeds maximum size of the bitmap\n"));
-                    if (ASMBitTest(puBitmap, iBitInByte))
-                        break;
-
-                    iBATEntryIndexCurr++;
+                    cBATEntryIndex++;
                     cSectors++;
                 }
@@ -1120 +1147 @@
             for (uint32_t iSector = 0; iSector < (cbToWrite / VHD_SECTOR_SIZE); iSector++)
             {
-                uint32_t iBitmap    = cBATEntryIndex / 8; /* Byte in the block bitmap. */
-                uint8_t  iBitInByte = (8 - 1) - (cBATEntryIndex % 8);
-                uint8_t  *puBitmap  = pImage->pu8Bitmap + iBitmap;
-
-                ASMBitSet(puBitmap, iBitInByte);
+                vhdBlockBitmapSectorSet(pImage, cBATEntryIndex);
                 cBATEntryIndex++;
             }
@@ -1527 +1550 @@
     pImage->cbDataBlockBitmap       = pImage->cSectorsPerDataBlock / 8;
     pImage->cDataBlockBitmapSectors = pImage->cbDataBlockBitmap / VHD_SECTOR_SIZE;
-    pImage->pu8Bitmap               = (uint8_t *)RTMemAllocZ(pImage->cbDataBlockBitmap);
+    pImage->pu8Bitmap               = vhdBlockBitmapAllocate(pImage);
     if (!pImage->pu8Bitmap)
         return vhdError(pImage, VERR_NO_MEMORY, RT_SRC_POS, N_("VHD: cannot allocate memory for bitmap storage"));