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>

5 files changed, 60 insertions, 15 deletions

diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
index 9e5c6cbe33..875c7c0258 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
@@ -610,6 +610,7 @@ SmmEndOfS3ResumeHandler (
   @param[in] Size2  Size of Buff2

 

   @retval TRUE      Buffers overlap in memory.

+  @retval TRUE      Math error.     Prevents potential math over and underflows.

   @retval FALSE     Buffer doesn't overlap.

 

 **/

@@ -621,11 +622,24 @@ InternalIsBufferOverlapped (
   IN UINTN  Size2

   )

 {

+  UINTN    End1;

+  UINTN    End2;

+  BOOLEAN  IsOverUnderflow1;

+  BOOLEAN  IsOverUnderflow2;

+

+  // Check for over or underflow

+  IsOverUnderflow1 = EFI_ERROR (SafeUintnAdd ((UINTN)Buff1, Size1, &End1));

+  IsOverUnderflow2 = EFI_ERROR (SafeUintnAdd ((UINTN)Buff2, Size2, &End2));

+

+  if (IsOverUnderflow1 || IsOverUnderflow2) {

+    return TRUE;

+  }

+

   //

   // If buff1's end is less than the start of buff2, then it's ok.

   // Also, if buff1's start is beyond buff2's end, then it's ok.

   //

-  if (((Buff1 + Size1) <= Buff2) || (Buff1 >= (Buff2 + Size2))) {

+  if ((End1 <= (UINTN)Buff2) || ((UINTN)Buff1 >= End2)) {

     return FALSE;

   }

 

@@ -651,6 +665,7 @@ SmmEntryPoint (
   EFI_SMM_COMMUNICATE_HEADER  *CommunicateHeader;

   BOOLEAN                     InLegacyBoot;

   BOOLEAN                     IsOverlapped;

+  BOOLEAN                     IsOverUnderflow;

   VOID                        *CommunicationBuffer;

   UINTN                       BufferSize;

 

@@ -699,23 +714,31 @@ SmmEntryPoint (
                        (UINT8 *)gSmmCorePrivate,

                        sizeof (*gSmmCorePrivate)

                        );

-      if (!SmmIsBufferOutsideSmmValid ((UINTN)CommunicationBuffer, BufferSize) || IsOverlapped) {

+      //

+      // Check for over or underflows

+      //

+      IsOverUnderflow = EFI_ERROR (SafeUintnSub (BufferSize, OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data), &BufferSize));

+

+      if (!SmmIsBufferOutsideSmmValid ((UINTN)CommunicationBuffer, BufferSize) ||

+          IsOverlapped || IsOverUnderflow)

+      {

         //

         // If CommunicationBuffer is not in valid address scope,

         // or there is overlap between gSmmCorePrivate and CommunicationBuffer,

+        // or there is over or underflow,

         // return EFI_INVALID_PARAMETER

         //

         gSmmCorePrivate->CommunicationBuffer = NULL;

         gSmmCorePrivate->ReturnStatus        = EFI_ACCESS_DENIED;

       } else {

         CommunicateHeader = (EFI_SMM_COMMUNICATE_HEADER *)CommunicationBuffer;

-        BufferSize       -= OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data);

-        Status            = SmiManage (

-                              &CommunicateHeader->HeaderGuid,

-                              NULL,

-                              CommunicateHeader->Data,

-                              &BufferSize

-                              );

+        // BufferSize was updated by the SafeUintnSub() call above.

+        Status = SmiManage (

+                   &CommunicateHeader->HeaderGuid,

+                   NULL,

+                   CommunicateHeader->Data,

+                   &BufferSize

+                   );

         //

         // Update CommunicationBuffer, BufferSize and ReturnStatus

         // Communicate service finished, reset the pointer to CommBuffer to NULL

diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h
index 71422b9dfc..b8a490a8c3 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h
@@ -54,6 +54,7 @@
 #include <Library/PerformanceLib.h>

 #include <Library/HobLib.h>

 #include <Library/SmmMemLib.h>

+#include <Library/SafeIntLib.h>

 

 #include "PiSmmCorePrivateData.h"

 #include "HeapGuard.h"

diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
index c8bfae3860..3df44b38f1 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
@@ -60,6 +60,7 @@
   PerformanceLib

   HobLib

   SmmMemLib

+  SafeIntLib

 

 [Protocols]

   gEfiDxeSmmReadyToLockProtocolGuid             ## UNDEFINED # SmiHandlerRegister

diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
index 4f00cebaf5..fbba868fd0 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
@@ -34,8 +34,8 @@
 #include <Library/UefiRuntimeLib.h>

 #include <Library/PcdLib.h>

 #include <Library/ReportStatusCodeLib.h>

-

 #include "PiSmmCorePrivateData.h"

+#include <Library/SafeIntLib.h>

 

 #define SMRAM_CAPABILITIES  (EFI_MEMORY_WB | EFI_MEMORY_UC)

 

@@ -1354,6 +1354,7 @@ SmmSplitSmramEntry (
   @param[in] ReservedRangeToCompare     Pointer to EFI_SMM_RESERVED_SMRAM_REGION to compare.

 

   @retval TRUE  There is overlap.

+  @retval TRUE  Math error.

   @retval FALSE There is no overlap.

 

 **/

@@ -1363,11 +1364,29 @@ SmmIsSmramOverlap (
   IN EFI_SMM_RESERVED_SMRAM_REGION  *ReservedRangeToCompare

   )

 {

-  UINT64  RangeToCompareEnd;

-  UINT64  ReservedRangeToCompareEnd;

-

-  RangeToCompareEnd         = RangeToCompare->CpuStart + RangeToCompare->PhysicalSize;

-  ReservedRangeToCompareEnd = ReservedRangeToCompare->SmramReservedStart + ReservedRangeToCompare->SmramReservedSize;

+  UINT64   RangeToCompareEnd;

+  UINT64   ReservedRangeToCompareEnd;

+  BOOLEAN  IsOverUnderflow1;

+  BOOLEAN  IsOverUnderflow2;

+

+  // Check for over or underflow.

+  IsOverUnderflow1 = EFI_ERROR (

+                       SafeUint64Add (

+                         (UINT64)RangeToCompare->CpuStart,

+                         RangeToCompare->PhysicalSize,

+                         &RangeToCompareEnd

+                         )

+                       );

+  IsOverUnderflow2 = EFI_ERROR (

+                       SafeUint64Add (

+                         (UINT64)ReservedRangeToCompare->SmramReservedStart,

+                         ReservedRangeToCompare->SmramReservedSize,

+                         &ReservedRangeToCompareEnd

+                         )

+                       );

+  if (IsOverUnderflow1 || IsOverUnderflow2) {

+    return TRUE;

+  }

 

   if ((RangeToCompare->CpuStart >= ReservedRangeToCompare->SmramReservedStart) &&

       (RangeToCompare->CpuStart < ReservedRangeToCompareEnd))

diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf
index 6109d6b544..ddeb39cee2 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf
@@ -46,6 +46,7 @@
   DxeServicesLib

   PcdLib

   ReportStatusCodeLib

+  SafeIntLib

 

 [Protocols]

   gEfiSmmBase2ProtocolGuid                      ## PRODUCES