[reactos] 01/01: [FSTUB] Fix out of bounds access in IoReadDiskSignature

[Victor Perevertkin]

https://git.reactos.org/?p=reactos.git;a=commitdiff;h=8d2fe54188616fa586946…

commit 8d2fe54188616fa5869468ecf108bf4af9a8624f
Author:     Victor Perevertkin <victor.perevertkin(a)reactos.org>
AuthorDate: Mon Dec 7 14:43:34 2020 +0300
Commit:     Victor Perevertkin <victor.perevertkin(a)reactos.org>
CommitDate: Mon Dec 7 14:43:34 2020 +0300

    [FSTUB] Fix out of bounds access in IoReadDiskSignature
    
    - Convert PARTITION_TABLE_OFFSET to the number of bytes instead of
      (number of bytes) / 2. This avoids many confusing casts
    - Use a cache aligned buffer for MBR
---
 ntoskrnl/fstub/disksup.c        | 31 ++++++++++++-------------------
 ntoskrnl/fstub/fstubex.c        | 12 ++++++------
 ntoskrnl/include/internal/hal.h |  7 ++++---
 3 files changed, 22 insertions(+), 28 deletions(-)

diff --git a/ntoskrnl/fstub/disksup.c b/ntoskrnl/fstub/disksup.c
index 71d51873f94..a685393b143 100644
--- a/ntoskrnl/fstub/disksup.c
+++ b/ntoskrnl/fstub/disksup.c
@@ -1712,8 +1712,7 @@ xHalExamineMBR(IN PDEVICE_OBJECT DeviceObject,
         }
 
         /* Get the partition entry */
-        PartitionDescriptor = (PPARTITION_DESCRIPTOR)
-                               &(((PUSHORT)Buffer)[PARTITION_TABLE_OFFSET]);
+        PartitionDescriptor =
(PPARTITION_DESCRIPTOR)&Buffer[PARTITION_TABLE_OFFSET];
 
         /* Make sure it's what the caller wanted */
         if (PartitionDescriptor->PartitionType != MbrTypeIdentifier)
@@ -1831,7 +1830,7 @@ xHalIoReadPartitionTable(IN PDEVICE_OBJECT DeviceObject,
             DiskGeometryEx.DiskSize, MaxSector);
 
     /* Allocate our buffer */
-    Buffer = ExAllocatePoolWithTag(NonPagedPool, InputSize, TAG_FILE_SYSTEM);
+    Buffer = ExAllocatePoolWithTag(NonPagedPoolCacheAligned, InputSize,
TAG_FILE_SYSTEM);
     if (!Buffer)
     {
         /* Fail, free the input buffer */
@@ -1901,12 +1900,11 @@ xHalIoReadPartitionTable(IN PDEVICE_OBJECT DeviceObject,
         if (!Offset.QuadPart)
         {
             /* Then read the signature off the disk */
-            (*PartitionBuffer)->Signature = ((PULONG)Buffer)[PARTITION_TABLE_OFFSET /
2 - 1];
+            (*PartitionBuffer)->Signature =
*(PUINT32)&Buffer[DISK_SIGNATURE_OFFSET];
         }
 
         /* Get the partition descriptor array */
-        PartitionDescriptor = (PPARTITION_DESCRIPTOR)
-                               &(((PUSHORT)Buffer)[PARTITION_TABLE_OFFSET]);
+        PartitionDescriptor =
(PPARTITION_DESCRIPTOR)&Buffer[PARTITION_TABLE_OFFSET];
 
         /* Start looping partitions */
         j++;
@@ -2091,8 +2089,7 @@ xHalIoReadPartitionTable(IN PDEVICE_OBJECT DeviceObject,
         Offset.QuadPart = 0;
 
         /* Go back to the descriptor array and loop it */
-        PartitionDescriptor = (PPARTITION_DESCRIPTOR)
-                               &(((PUSHORT)Buffer)[PARTITION_TABLE_OFFSET]);
+        PartitionDescriptor =
(PPARTITION_DESCRIPTOR)&Buffer[PARTITION_TABLE_OFFSET];
         for (Entry = 1; Entry <= NUM_PARTITION_TABLE_ENTRIES; Entry++,
PartitionDescriptor++)
         {
             /* Check if this is a container partition, since we skipped them */
@@ -2237,7 +2234,7 @@ xHalIoSetPartitionInformation(IN PDEVICE_OBJECT DeviceObject,
     }
 
     /* Allocate our partition buffer */
-    Buffer = ExAllocatePoolWithTag(NonPagedPool, PAGE_SIZE, TAG_FILE_SYSTEM);
+    Buffer = ExAllocatePoolWithTag(NonPagedPoolCacheAligned, PAGE_SIZE,
TAG_FILE_SYSTEM);
     if (!Buffer) return STATUS_INSUFFICIENT_RESOURCES;
 
     /* Initialize the event we'll use and loop partitions */
@@ -2290,8 +2287,7 @@ xHalIoSetPartitionInformation(IN PDEVICE_OBJECT DeviceObject,
         }
 
         /* Get the partition descriptors and loop them */
-        PartitionDescriptor = (PPARTITION_DESCRIPTOR)
-                              &(((PUSHORT)Buffer)[PARTITION_TABLE_OFFSET]);
+        PartitionDescriptor =
(PPARTITION_DESCRIPTOR)&Buffer[PARTITION_TABLE_OFFSET];
         for (Entry = 1; Entry <= NUM_PARTITION_TABLE_ENTRIES; Entry++,
PartitionDescriptor++)
         {
             /* Check if it's unused or a container partition */
@@ -2352,8 +2348,7 @@ xHalIoSetPartitionInformation(IN PDEVICE_OBJECT DeviceObject,
         if (Entry <= NUM_PARTITION_TABLE_ENTRIES) break;
 
         /* Nothing found yet, get the partition array again */
-        PartitionDescriptor = (PPARTITION_DESCRIPTOR)
-                               &(((PUSHORT)Buffer)[PARTITION_TABLE_OFFSET]);
+        PartitionDescriptor =
(PPARTITION_DESCRIPTOR)&Buffer[PARTITION_TABLE_OFFSET];
         for (Entry = 1; Entry <= NUM_PARTITION_TABLE_ENTRIES; Entry++,
PartitionDescriptor++)
         {
             /* Check if this was a container partition (we skipped these) */
@@ -2469,7 +2464,7 @@ xHalIoWritePartitionTable(IN PDEVICE_OBJECT DeviceObject,
     DiskLayout->TableCount = (PartitionBuffer->PartitionCount +
NUM_PARTITION_TABLE_ENTRIES - 1) / NUM_PARTITION_TABLE_ENTRIES;
 
     /* Allocate our partition buffer */
-    Buffer = ExAllocatePoolWithTag(NonPagedPool, PAGE_SIZE, TAG_FILE_SYSTEM);
+    Buffer = ExAllocatePoolWithTag(NonPagedPoolCacheAligned, PAGE_SIZE,
TAG_FILE_SYSTEM);
     if (!Buffer) return STATUS_INSUFFICIENT_RESOURCES;
 
     /* Loop the entries */
@@ -2520,7 +2515,7 @@ xHalIoWritePartitionTable(IN PDEVICE_OBJECT DeviceObject,
         if (!IsSuperFloppy)
         {
             /* Set the boot record signature */
-            Buffer[BOOT_SIGNATURE_OFFSET] = BOOT_RECORD_SIGNATURE;
+            ((PUSHORT)Buffer)[BOOT_SIGNATURE_OFFSET] = BOOT_RECORD_SIGNATURE;
 
             /* By default, don't require a rewrite */
             DoRewrite = FALSE;
@@ -2529,12 +2524,10 @@ xHalIoWritePartitionTable(IN PDEVICE_OBJECT DeviceObject,
             if (!Offset.QuadPart)
             {
                 /* Check if the signature doesn't match */
-                if (((PULONG)Buffer)[PARTITION_TABLE_OFFSET / 2 - 1] !=
-                    PartitionBuffer->Signature)
+                if (*(PUINT32)&Buffer[PARTITION_TABLE_OFFSET] !=
PartitionBuffer->Signature)
                 {
                     /* Then write the signature and now we need a rewrite */
-                    ((PULONG)Buffer)[PARTITION_TABLE_OFFSET / 2 - 1] =
-                        PartitionBuffer->Signature;
+                    *(PUINT32)&Buffer[PARTITION_TABLE_OFFSET] =
PartitionBuffer->Signature;
                     DoRewrite = TRUE;
                 }
             }
diff --git a/ntoskrnl/fstub/fstubex.c b/ntoskrnl/fstub/fstubex.c
index c1ef4cdd47c..5e3a819ae7c 100644
--- a/ntoskrnl/fstub/fstubex.c
+++ b/ntoskrnl/fstub/fstubex.c
@@ -674,7 +674,7 @@ FstubDetectPartitionStyle(IN PDISK_INFORMATION Disk,
 
     /* Get the partition descriptor array */
     PartitionDescriptor = (PPARTITION_DESCRIPTOR)
-                          &(Disk->Buffer[PARTITION_TABLE_OFFSET]);
+                          &(Disk->Buffer[PARTITION_TABLE_OFFSET /
sizeof(Disk->Buffer[0])]);
     /* If we have not the 0xAA55 then it's raw partition */
     if (Disk->Buffer[BOOT_SIGNATURE_OFFSET] != BOOT_RECORD_SIGNATURE)
     {
@@ -2140,7 +2140,7 @@ IoReadDiskSignature(IN PDEVICE_OBJECT DeviceObject,
                     IN ULONG BytesPerSector,
                     OUT PDISK_SIGNATURE Signature)
 {
-    PULONG Buffer;
+    PUCHAR Buffer;
     NTSTATUS Status;
     ULONG HeaderCRC32, i, CheckSum;
     PEFI_PARTITION_HEADER EFIHeader;
@@ -2207,7 +2207,7 @@ IoReadDiskSignature(IN PDEVICE_OBJECT DeviceObject,
         /* Then zero the one in EFI header. This is needed to compute header checksum */
         EFIHeader->HeaderCRC32 = 0;
         /* Compute header checksum and compare with the one present in partition table
*/
-        if (RtlComputeCrc32(0, (PUCHAR)Buffer, sizeof(EFI_PARTITION_HEADER)) !=
HeaderCRC32)
+        if (RtlComputeCrc32(0, Buffer, sizeof(EFI_PARTITION_HEADER)) != HeaderCRC32)
         {
             Status = STATUS_DISK_CORRUPT_ERROR;
             goto Cleanup;
@@ -2220,14 +2220,14 @@ IoReadDiskSignature(IN PDEVICE_OBJECT DeviceObject,
     else
     {
         /* Compute MBR checksum */
-        for (i = 0, CheckSum = 0; i < 512 / sizeof(ULONG) ; i++)
+        for (i = 0, CheckSum = 0; i < BytesPerSector / sizeof(UINT32); i++)
         {
-            CheckSum += Buffer[i];
+            CheckSum += *(PUINT32)&Buffer[i];
         }
 
         /* Set partition table style to MBR and return signature (offset 440) and
checksum */
         Signature->PartitionStyle = PARTITION_STYLE_MBR;
-        Signature->Mbr.Signature = Buffer[PARTITION_TABLE_OFFSET / 2 - 1];
+        Signature->Mbr.Signature = *(PUINT32)&Buffer[DISK_SIGNATURE_OFFSET];
         Signature->Mbr.CheckSum = CheckSum;
     }
 
diff --git a/ntoskrnl/include/internal/hal.h b/ntoskrnl/include/internal/hal.h
index 1e61d138eac..b02b7575a60 100644
--- a/ntoskrnl/include/internal/hal.h
+++ b/ntoskrnl/include/internal/hal.h
@@ -240,9 +240,10 @@ xKdUnmapVirtualAddress(
 //
 // Various offsets in the boot record
 //
-#define PARTITION_TABLE_OFFSET                      (0x1BE / 2)
-#define BOOT_SIGNATURE_OFFSET                       ((0x200 / 2) - 1)
-#define BOOT_RECORD_RESERVED                        0x1BC
+#define DISK_SIGNATURE_OFFSET                       0x1B8
+#define PARTITION_TABLE_OFFSET                      0x1BE
+#define BOOT_SIGNATURE_OFFSET                       ((0x200 / sizeof(INT16)) - 1)
+
 #define BOOT_RECORD_SIGNATURE                       0xAA55
 #define NUM_PARTITION_TABLE_ENTRIES                 4