MdeModulePkg/PiSmmCore: SmmEntryPoint underflow (CVE-2021-38578)

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3387

Added use of SafeIntLib to validate values are not causing overflows or
underflows in user controlled values when calculating buffer sizes.

Signed-off-by: Miki Demeter <miki.demeter@intel.com>
Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>

diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
index 9e5c6cbe33dd46c7c204ab2add81f2b6b2f4bba5..875c7c0258b7b27accfb114213cbc933709c617f 100644 (file)
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
@@ -610,6 +610,7 @@ SmmEndOfS3ResumeHandler (
   @param[in] Size2  Size of Buff2\r
 \r
   @retval TRUE      Buffers overlap in memory.\r
+  @retval TRUE      Math error.     Prevents potential math over and underflows.\r
   @retval FALSE     Buffer doesn't overlap.\r
 \r
 **/\r
@@ -621,11 +622,24 @@ InternalIsBufferOverlapped (
   IN UINTN  Size2\r
   )\r
 {\r
+  UINTN    End1;\r
+  UINTN    End2;\r
+  BOOLEAN  IsOverUnderflow1;\r
+  BOOLEAN  IsOverUnderflow2;\r
+\r
+  // Check for over or underflow\r
+  IsOverUnderflow1 = EFI_ERROR (SafeUintnAdd ((UINTN)Buff1, Size1, &End1));\r
+  IsOverUnderflow2 = EFI_ERROR (SafeUintnAdd ((UINTN)Buff2, Size2, &End2));\r
+\r
+  if (IsOverUnderflow1 || IsOverUnderflow2) {\r
+    return TRUE;\r
+  }\r
+\r
   //\r
   // If buff1's end is less than the start of buff2, then it's ok.\r
   // Also, if buff1's start is beyond buff2's end, then it's ok.\r
   //\r
-  if (((Buff1 + Size1) <= Buff2) || (Buff1 >= (Buff2 + Size2))) {\r
+  if ((End1 <= (UINTN)Buff2) || ((UINTN)Buff1 >= End2)) {\r
     return FALSE;\r
   }\r
 \r
@@ -651,6 +665,7 @@ SmmEntryPoint (
   EFI_SMM_COMMUNICATE_HEADER  *CommunicateHeader;\r
   BOOLEAN                     InLegacyBoot;\r
   BOOLEAN                     IsOverlapped;\r
+  BOOLEAN                     IsOverUnderflow;\r
   VOID                        *CommunicationBuffer;\r
   UINTN                       BufferSize;\r
 \r
@@ -699,23 +714,31 @@ SmmEntryPoint (
                        (UINT8 *)gSmmCorePrivate,\r
                        sizeof (*gSmmCorePrivate)\r
                        );\r
-      if (!SmmIsBufferOutsideSmmValid ((UINTN)CommunicationBuffer, BufferSize) || IsOverlapped) {\r
+      //\r
+      // Check for over or underflows\r
+      //\r
+      IsOverUnderflow = EFI_ERROR (SafeUintnSub (BufferSize, OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data), &BufferSize));\r
+\r
+      if (!SmmIsBufferOutsideSmmValid ((UINTN)CommunicationBuffer, BufferSize) ||\r
+          IsOverlapped || IsOverUnderflow)\r
+      {\r
         //\r
         // If CommunicationBuffer is not in valid address scope,\r
         // or there is overlap between gSmmCorePrivate and CommunicationBuffer,\r
+        // or there is over or underflow,\r
         // return EFI_INVALID_PARAMETER\r
         //\r
         gSmmCorePrivate->CommunicationBuffer = NULL;\r
         gSmmCorePrivate->ReturnStatus        = EFI_ACCESS_DENIED;\r
       } else {\r
         CommunicateHeader = (EFI_SMM_COMMUNICATE_HEADER *)CommunicationBuffer;\r
-        BufferSize       -= OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data);\r
-        Status            = SmiManage (\r
-                              &CommunicateHeader->HeaderGuid,\r
-                              NULL,\r
-                              CommunicateHeader->Data,\r
-                              &BufferSize\r
-                              );\r
+        // BufferSize was updated by the SafeUintnSub() call above.\r
+        Status = SmiManage (\r
+                   &CommunicateHeader->HeaderGuid,\r
+                   NULL,\r
+                   CommunicateHeader->Data,\r
+                   &BufferSize\r
+                   );\r
         //\r
         // Update CommunicationBuffer, BufferSize and ReturnStatus\r
         // Communicate service finished, reset the pointer to CommBuffer to NULL\r

diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h
index 71422b9dfcdfe889e570304c81feaf64154a34bf..b8a490a8c3b599f1bc955f2e9ecfed45734ff7ab 100644 (file)
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h
@@ -54,6 +54,7 @@
 #include <Library/PerformanceLib.h>\r
 #include <Library/HobLib.h>\r
 #include <Library/SmmMemLib.h>\r
+#include <Library/SafeIntLib.h>\r
 \r
 #include "PiSmmCorePrivateData.h"\r
 #include "HeapGuard.h"\r

diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
index c8bfae3860fc0b564bdddb17b1d30fb8e85a7cdf..3df44b38f13cc0500fa49a55148a6f8c6f41017f 100644 (file)
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
@@ -60,6 +60,7 @@
   PerformanceLib\r
   HobLib\r
   SmmMemLib\r
+  SafeIntLib\r
 \r
 [Protocols]\r
   gEfiDxeSmmReadyToLockProtocolGuid             ## UNDEFINED # SmiHandlerRegister\r

diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
index 4f00cebaf5ed7aceccbb7031d1096ced10751aab..fbba868fd0b1644353623eead119b42c3d16c9a7 100644 (file)
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
@@ -34,8 +34,8 @@
 #include <Library/UefiRuntimeLib.h>\r
 #include <Library/PcdLib.h>\r
 #include <Library/ReportStatusCodeLib.h>\r
-\r
 #include "PiSmmCorePrivateData.h"\r
+#include <Library/SafeIntLib.h>\r
 \r
 #define SMRAM_CAPABILITIES  (EFI_MEMORY_WB | EFI_MEMORY_UC)\r
 \r
@@ -1354,6 +1354,7 @@ SmmSplitSmramEntry (
   @param[in] ReservedRangeToCompare     Pointer to EFI_SMM_RESERVED_SMRAM_REGION to compare.\r
 \r
   @retval TRUE  There is overlap.\r
+  @retval TRUE  Math error.\r
   @retval FALSE There is no overlap.\r
 \r
 **/\r
@@ -1363,11 +1364,29 @@ SmmIsSmramOverlap (
   IN EFI_SMM_RESERVED_SMRAM_REGION  *ReservedRangeToCompare\r
   )\r
 {\r
-  UINT64  RangeToCompareEnd;\r
-  UINT64  ReservedRangeToCompareEnd;\r
-\r
-  RangeToCompareEnd         = RangeToCompare->CpuStart + RangeToCompare->PhysicalSize;\r
-  ReservedRangeToCompareEnd = ReservedRangeToCompare->SmramReservedStart + ReservedRangeToCompare->SmramReservedSize;\r
+  UINT64   RangeToCompareEnd;\r
+  UINT64   ReservedRangeToCompareEnd;\r
+  BOOLEAN  IsOverUnderflow1;\r
+  BOOLEAN  IsOverUnderflow2;\r
+\r
+  // Check for over or underflow.\r
+  IsOverUnderflow1 = EFI_ERROR (\r
+                       SafeUint64Add (\r
+                         (UINT64)RangeToCompare->CpuStart,\r
+                         RangeToCompare->PhysicalSize,\r
+                         &RangeToCompareEnd\r
+                         )\r
+                       );\r
+  IsOverUnderflow2 = EFI_ERROR (\r
+                       SafeUint64Add (\r
+                         (UINT64)ReservedRangeToCompare->SmramReservedStart,\r
+                         ReservedRangeToCompare->SmramReservedSize,\r
+                         &ReservedRangeToCompareEnd\r
+                         )\r
+                       );\r
+  if (IsOverUnderflow1 || IsOverUnderflow2) {\r
+    return TRUE;\r
+  }\r
 \r
   if ((RangeToCompare->CpuStart >= ReservedRangeToCompare->SmramReservedStart) &&\r
       (RangeToCompare->CpuStart < ReservedRangeToCompareEnd))\r

diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf
index 6109d6b5449c080337cba4cfea3146d206e90a98..ddeb39cee266ac14800c00828be813d8d8820ade 100644 (file)
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf
@@ -46,6 +46,7 @@
   DxeServicesLib\r
   PcdLib\r
   ReportStatusCodeLib\r
+  SafeIntLib\r
 \r
 [Protocols]\r
   gEfiSmmBase2ProtocolGuid                      ## PRODUCES\r