[PATCH 1/1] MdeModulePkg: Fix PiSmmCore integer over- and underflows.

[PATCH 1/1] MdeModulePkg: Fix PiSmmCore integer over- and underflows.

[Gerd Hoffmann]

Prevents potential math over and underflows when comparing buffers
for SMM validity.

Original patch uploaded to bugzilla by Bret Barkelew <bret@corthon.com>.
I've adapted the patch to latest master, uncristified, dropped MU
comments.

Ref: CVE-2021-38578
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3387
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf |  1 +
 MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf  |  1 +
 MdeModulePkg/Core/PiSmmCore/PiSmmCore.h   |  1 +
 MdeModulePkg/Core/PiSmmCore/PiSmmCore.c   | 32 ++++++++++++++++-------
 MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c    | 10 +++++--
 5 files changed, 34 insertions(+), 11 deletions(-)

diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
index c8bfae3860fc..3df44b38f13c 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
@@ -60,6 +60,7 @@ [LibraryClasses]
   PerformanceLib
   HobLib
   SmmMemLib
+  SafeIntLib
 
 [Protocols]
   gEfiDxeSmmReadyToLockProtocolGuid             ## UNDEFINED # SmiHandlerRegister
diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf
index 6109d6b5449c..ddeb39cee266 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf
@@ -46,6 +46,7 @@ [LibraryClasses]
   DxeServicesLib
   PcdLib
   ReportStatusCodeLib
+  SafeIntLib
 
 [Protocols]
   gEfiSmmBase2ProtocolGuid                      ## PRODUCES
diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h
index 71422b9dfcdf..b8a490a8c3b5 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.c b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
index 9e5c6cbe33dd..5ef8b31ee361 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.
   @retval FALSE     Buffer doesn't overlap.
 
 **/
@@ -621,11 +622,21 @@ InternalIsBufferOverlapped (
   IN UINTN  Size2
   )
 {
+  UINTN  End1;
+  UINTN  End2;
+
+  // Prevents potential math over and underflows.
+  if (EFI_ERROR (SafeUintnAdd ((UINTN)Buff1, Size1, &End1)) ||
+      EFI_ERROR (SafeUintnAdd ((UINTN)Buff2, Size2, &End2)))
+  {
+    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;
   }
 
@@ -699,7 +710,10 @@ SmmEntryPoint (
                        (UINT8 *)gSmmCorePrivate,
                        sizeof (*gSmmCorePrivate)
                        );
-      if (!SmmIsBufferOutsideSmmValid ((UINTN)CommunicationBuffer, BufferSize) || IsOverlapped) {
+      if (!SmmIsBufferOutsideSmmValid ((UINTN)CommunicationBuffer, BufferSize) ||
+          IsOverlapped ||
+          EFI_ERROR (SafeUintnSub (BufferSize, OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data), &BufferSize)))
+      {
         //
         // If CommunicationBuffer is not in valid address scope,
         // or there is overlap between gSmmCorePrivate and CommunicationBuffer,
@@ -709,13 +723,13 @@ SmmEntryPoint (
         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/PiSmmIpl.c b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
index 4f00cebaf5ed..f08ca55e26b7 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
@@ -34,6 +34,7 @@
 #include <Library/UefiRuntimeLib.h>
 #include <Library/PcdLib.h>
 #include <Library/ReportStatusCodeLib.h>
+#include <Library/SafeIntLib.h>
 
 #include "PiSmmCorePrivateData.h"
 
@@ -1354,6 +1355,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.
 
 **/
@@ -1366,8 +1368,12 @@ SmmIsSmramOverlap (
   UINT64  RangeToCompareEnd;
   UINT64  ReservedRangeToCompareEnd;
 
-  RangeToCompareEnd         = RangeToCompare->CpuStart + RangeToCompare->PhysicalSize;
-  ReservedRangeToCompareEnd = ReservedRangeToCompare->SmramReservedStart + ReservedRangeToCompare->SmramReservedSize;
+  // Prevents potential math over and underflows.
+  if (EFI_ERROR (SafeUint64Add ((UINT64)RangeToCompare->CpuStart, RangeToCompare->PhysicalSize, &RangeToCompareEnd)) ||
+      EFI_ERROR (SafeUint64Add ((UINT64)ReservedRangeToCompare->SmramReservedStart, ReservedRangeToCompare->SmramReservedSize, &ReservedRangeToCompareEnd)))
+  {
+    return TRUE;
+  }
 
   if ((RangeToCompare->CpuStart >= ReservedRangeToCompare->SmramReservedStart) &&
       (RangeToCompare->CpuStart < ReservedRangeToCompareEnd))
-- 
2.36.1