OcMiscLib: Fix potential overflow in ApplyPatch()

Changelog.md

@@ -4,6 +4,7 @@ OpenCore Changelog
 - Fixed ocvalidate return code to be non-zero when issues are found
 - Added `OEM` values to `PlatformInfo` in `Automatic` mode
 - Improved CPU frequency calculation on Haswell and earlier
+- Fixed issues when applying certain patches
 
 #### v0.6.6
 - Added keyboard and pointer entry scroll support in OpenCanopy

Include/Acidanthera/Library/OcMiscLib.h

@@ -28,14 +28,14 @@
 **/
 #define SECONDS_TO_MICROSECONDS(x) ((x)*1000000)
 
-INT32
+BOOLEAN
 FindPattern (
   IN CONST UINT8   *Pattern,
   IN CONST UINT8   *PatternMask OPTIONAL,
   IN CONST UINT32  PatternSize,
   IN CONST UINT8   *Data,
   IN UINT32        DataSize,
-  IN INT32         DataOff
+  IN UINT32        *DataOff
   );
 
 UINT32

Library/OcAfterBootCompatLib/CustomSlide.c

@@ -660,6 +660,7 @@ AppleSlideUnlockForSafeMode (
   UINTN       SearchSeqNewSize;
   BOOLEAN     NewWay;
   BOOLEAN     IsSur;
+  UINT32      SurOffset;
 
   StartOff = ImageBase;
   EndOff   = StartOff + ImageSize - sizeof (SearchSeq) - MaxDist;
@@ -667,14 +668,15 @@ AppleSlideUnlockForSafeMode (
   //
   // Rebranding started with macOS 11. All the ones before had Mac OS X or none.
   //
+  SurOffset = 0;
   IsSur = FindPattern (
     (CONST UINT8 *) "macOS ",
     NULL,
     L_STR_LEN ("macOS "),
     ImageBase,
     (UINT32) ImageSize,
-    0
-    ) >= 0;
+    &SurOffset
+    );
 
   if (IsSur) {
     for (FirstOff = 0; StartOff + FirstOff <= EndOff; ++FirstOff) {

Library/OcAppleKernelLib/KernelVersion.c

@@ -118,22 +118,23 @@ OcKernelReadDarwinVersion (
   IN  UINT32        KernelSize
   )
 {
-  INT32   Offset;
+  BOOLEAN Exists;
+  UINT32  Offset;
   UINT32  Index;
   CHAR8   DarwinVersion[32];
   UINT32  DarwinVersionInteger;
 
-
-  Offset = FindPattern (
+  Offset = 0;
+  Exists = FindPattern (
     (CONST UINT8 *) "Darwin Kernel Version ",
     NULL,
     L_STR_LEN ("Darwin Kernel Version "),
     Kernel,
     KernelSize,
-    0
+    &Offset
     );
 
-  if (Offset < 0) {
+  if (!Exists) {
     DEBUG ((DEBUG_WARN, "OCAK: Failed to determine kernel version\n"));
     return 0;
   }
@@ -141,7 +142,7 @@ OcKernelReadDarwinVersion (
   Offset += L_STR_LEN ("Darwin Kernel Version ");
 
   for (Index = 0; Index < ARRAY_SIZE (DarwinVersion) - 1; ++Index, ++Offset) {
-    if ((UINT32) Offset >= KernelSize || Kernel[Offset] == ':') {
+    if (Offset >= KernelSize || Kernel[Offset] == ':') {
       break;
     }
     DarwinVersion[Index] = (CHAR8) Kernel[Offset];

Library/OcBootManagementLib/ImageLoader.c

@@ -613,19 +613,21 @@ DetectCapabilities (
   IN  UINT32                       SourceSize
   )
 {
-  INT32  Result;
+  BOOLEAN Exists;
+  UINT32  Result;
 
   //
   // Find Mac OS X version pattern.
   // This pattern started to appear with 10.7.
   //
-  Result = FindPattern (
+  Result = 0;
+  Exists = FindPattern (
     (CONST UINT8 *)"Mac OS X 10.",
     NULL,
     L_STR_LEN ("Mac OS X 10."),
     SourceBuffer,
     SourceSize - sizeof (UINT32),
-    0
+    &Result
     );
 
 #ifdef MDE_CPU_IA32
@@ -635,7 +637,7 @@ DetectCapabilities (
   // developer preview 10.8 images, so simply decide on Mac OS X
   // version pattern presence.
   //
-  if (Result >= 0) {
+  if (Exists) {
     return OC_KERN_CAPABILITY_K32_U64;
   }
   return OC_KERN_CAPABILITY_K32_U32 | OC_KERN_CAPABILITY_K32_U64;
@@ -644,7 +646,7 @@ DetectCapabilities (
   // For X64 mode, when the pattern is found, this can be 10.7 or 10.8+.
   // 10.7 supports K32_64 and K64, while newer versions have only K64.
   //
-  if (Result >= 0) {
+  if (Exists) {
     if (((UINT8 *)SourceBuffer)[Result + L_STR_LEN ("Mac OS X 10.")] == '7') {
       return OC_KERN_CAPABILITY_K32_U64 | OC_KERN_CAPABILITY_K64_U64;
     }
@@ -656,15 +658,16 @@ DetectCapabilities (
   // 10.6 supports K32 and K64, while older versions have only K32.
   // Detect 10.6 by x86_64 pattern presence.
   //
-  Result = FindPattern (
+  Result = SourceSize / 2;
+  Exists = FindPattern (
     (CONST UINT8 *)"x86_64",
     NULL,
     L_STR_SIZE ("x86_64"),
     SourceBuffer,
     SourceSize - sizeof (UINT32),
-    (INT32) (SourceSize / 2)
+    &Result
     );
-  if (Result >= 0) {
+  if (Exists) {
     return OC_KERN_CAPABILITY_K32_U32 | OC_KERN_CAPABILITY_K32_U64 | OC_KERN_CAPABILITY_K64_U64;
   }
   return OC_KERN_CAPABILITY_K32_U32 | OC_KERN_CAPABILITY_K32_U64;

Library/OcMiscLib/DataPatcher.c

@@ -15,55 +15,88 @@
 #include <Library/BaseLib.h>
 #include <Library/BaseMemoryLib.h>
 #include <Library/DebugLib.h>
+#include <Library/OcGuardLib.h>
 #include <Library/OcMiscLib.h>
 
-INT32
-FindPattern (
+STATIC
+BOOLEAN
+InternalFindPattern (
   IN CONST UINT8   *Pattern,
   IN CONST UINT8   *PatternMask OPTIONAL,
   IN CONST UINT32  PatternSize,
   IN CONST UINT8   *Data,
   IN UINT32        DataSize,
-  IN INT32         DataOff
+  IN UINT32        *DataOff
   )
 {
   UINT32   Index;
+  UINT32   LastOffset;
+  UINT32   CurrentOffset;
 
-  ASSERT (DataOff >= 0);
+  ASSERT (DataSize >= PatternSize);
 
-  if (PatternSize == 0 || DataSize == 0 || (DataOff < 0) || (UINT32)DataOff >= DataSize || DataSize - DataOff < PatternSize) {
-    return -1;
+  if (PatternSize == 0) {
+    return FALSE;
   }
 
+  CurrentOffset = *DataOff;
+  LastOffset = DataSize - PatternSize;
+
   if (PatternMask == NULL) {
-    while (DataOff + PatternSize <= DataSize) {
+    while (CurrentOffset <= LastOffset) {
       for (Index = 0; Index < PatternSize; ++Index) {
-        if (Data[DataOff + Index] != Pattern[Index]) {
+        if (Data[CurrentOffset + Index] != Pattern[Index]) {
           break;
         }
       }
 
       if (Index == PatternSize) {
-        return DataOff;
+        *DataOff = CurrentOffset;
+        return TRUE;
       }
-      ++DataOff;
+      ++CurrentOffset;
     }
   } else {
-    while (DataOff + PatternSize <= DataSize) {
+    while (CurrentOffset <= LastOffset) {
       for (Index = 0; Index < PatternSize; ++Index) {
-        if ((Data[DataOff + Index] & PatternMask[Index]) != Pattern[Index]) {
+        if ((Data[CurrentOffset + Index] & PatternMask[Index]) != Pattern[Index]) {
           break;
         }
       }
 
       if (Index == PatternSize) {
-        return DataOff;
+        *DataOff = CurrentOffset;
+        return TRUE;
       }
-      ++DataOff;
+      ++CurrentOffset;
     }
   }
 
-  return -1;
+  return FALSE;
+}
+
+BOOLEAN
+FindPattern (
+  IN CONST UINT8   *Pattern,
+  IN CONST UINT8   *PatternMask OPTIONAL,
+  IN CONST UINT32  PatternSize,
+  IN CONST UINT8   *Data,
+  IN UINT32        DataSize,
+  IN UINT32        *DataOff
+  )
+{
+  if (DataSize < PatternSize) {
+    return FALSE;
+  }
+
+  return InternalFindPattern (
+    Pattern,
+    PatternMask,
+    PatternSize,
+    Data,
+    DataSize,
+    DataOff
+    );
 }
 
 UINT32
@@ -80,49 +113,68 @@ ApplyPatch (
   )
 {
   UINT32  ReplaceCount;
-  INT32   DataOff;
+  UINT32  DataOff;
+  BOOLEAN Found;
+
+  if (DataSize < PatternSize) {
+    return 0;
+  }
 
   ReplaceCount = 0;
   DataOff = 0;
 
-  do {
-    DataOff = FindPattern (Pattern, PatternMask, PatternSize, Data, DataSize, DataOff);
-
-    if (DataOff >= 0) {
-      //
-      // Skip this finding if requested.
-      //
-      if (Skip > 0) {
-        --Skip;
-        DataOff += PatternSize;
-        continue;
-      }
+  while (TRUE) {
+    Found = InternalFindPattern (
+      Pattern,
+      PatternMask,
+      PatternSize,
+      Data,
+      DataSize,
+      &DataOff
+      );
+
+    if (!Found) {
+      break;
+    }
 
-      //
-      // Perform replacement.
-      //
-      if (ReplaceMask == NULL) {
-        CopyMem (&Data[DataOff], Replace, PatternSize);
-      } else {
-        for (UINTN Index = 0; Index < PatternSize; ++Index) {
-          Data[DataOff + Index] = (Data[DataOff + Index] & ~ReplaceMask[Index]) | (Replace[Index] & ReplaceMask[Index]);
-        }
-      }
-      ++ReplaceCount;
+    //
+    // DataOff + PatternSize - 1 is guaranteed to be a valid offset here. As
+    // DataSize can at most be MAX_UINT32, the maximum valid offset is
+    // MAX_UINT32 - 1. In consequence, DataOff + PatternSize cannot wrap around.
+    //
+
+    //
+    // Skip this finding if requested.
+    //
+    if (Skip > 0) {
+      --Skip;
       DataOff += PatternSize;
+      continue;
+    }
 
-      //
-      // Check replace count if requested.
-      //
-      if (Count > 0) {
-        --Count;
-        if (Count == 0) {
-          break;
-        }
+    //
+    // Perform replacement.
+    //
+    if (ReplaceMask == NULL) {
+      CopyMem (&Data[DataOff], Replace, PatternSize);
+    } else {
+      for (UINTN Index = 0; Index < PatternSize; ++Index) {
+        Data[DataOff + Index] = (Data[DataOff + Index] & ~ReplaceMask[Index]) | (Replace[Index] & ReplaceMask[Index]);
       }
     }
-
-  } while (DataOff >= 0);
+    ++ReplaceCount;
+    DataOff += PatternSize;
+
+    //
+    // Check replace count if requested.
+    //
+    if (Count > 0) {
+      --Count;
+      if (Count == 0) {
+        break;
+      }
+    }
+  }
 
   return ReplaceCount;
 }