[edk2-devel] [PATCH EDK2 v1 1/1] MdeModulePkg/PiSmmCore:Avoid overflow risk

[wenyi,xie via groups.io]

As the CommunicationBuffer plus BufferSize may overflow, check the
value first before using.

Cc: Jian J Wang <jian.j.w...@intel.com>
Cc: Liming Gao <gaolim...@byosoft.com.cn>
Cc: Eric Dong <eric.d...@intel.com>
Cc: Ray Ni <ray...@intel.com>
Signed-off-by: Wenyi Xie <xiewen...@huawei.com>
---
 MdeModulePkg/Core/PiSmmCore/PiSmmCore.c | 22 +++++++++++++-------
 MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c  |  5 +++++
 2 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c 
b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
index 9e5c6cbe33dd..fcf8c61d7f1b 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
@@ -613,23 +613,28 @@ SmmEndOfS3ResumeHandler (
   @retval FALSE     Buffer doesn't overlap.
 
 **/
-BOOLEAN
+EFI_STATUS
 InternalIsBufferOverlapped (
   IN UINT8  *Buff1,
   IN UINTN  Size1,
   IN UINT8  *Buff2,
-  IN UINTN  Size2
+  IN UINTN  Size2,
+  IN BOOLEAN *IsOverlapped
   )
 {
+  *IsOverlapped = TRUE;
+  if (((UINTN)Buff1 > MAX_UINTN - Size1) || ((UINTN)Buff2 > MAX_UINTN - 
Size2)) {
+    return EFI_INVALID_PARAMETER;
+  }
   //
   // 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))) {
-    return FALSE;
+    *IsOverlapped = FALSE;
   }
 
-  return TRUE;
+  return EFI_SUCCESS;
 }
 
 /**
@@ -693,17 +698,18 @@ SmmEntryPoint (
       //
       // Synchronous SMI for SMM Core or request from Communicate protocol
       //
-      IsOverlapped = InternalIsBufferOverlapped (
+      Status = InternalIsBufferOverlapped (
                        (UINT8 *)CommunicationBuffer,
                        BufferSize,
                        (UINT8 *)gSmmCorePrivate,
-                       sizeof (*gSmmCorePrivate)
+                       sizeof (*gSmmCorePrivate),
+                       &IsOverlapped
                        );
-      if (!SmmIsBufferOutsideSmmValid ((UINTN)CommunicationBuffer, BufferSize) 
|| IsOverlapped) {
+      if (!SmmIsBufferOutsideSmmValid ((UINTN)CommunicationBuffer, BufferSize) 
|| EFI_ERROR(Status) || IsOverlapped) {
         //
         // If CommunicationBuffer is not in valid address scope,
         // or there is overlap between gSmmCorePrivate and CommunicationBuffer,
-        // return EFI_INVALID_PARAMETER
+        // return EFI_ACCESS_DENIED
         //
         gSmmCorePrivate->CommunicationBuffer = NULL;
         gSmmCorePrivate->ReturnStatus        = EFI_ACCESS_DENIED;
diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c 
b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
index 4f00cebaf5ed..bd13cf97ec93 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
@@ -525,6 +525,11 @@ SmmCommunicationCommunicate (
 
   CommunicateHeader = (EFI_SMM_COMMUNICATE_HEADER *)CommBuffer;
 
+  if (CommunicateHeader->MessageLength > MAX_UINTN - OFFSET_OF 
(EFI_SMM_COMMUNICATE_HEADER, Data)) {
+    DEBUG ((DEBUG_ERROR, "MessageLength is invalid!\n"));
+    return EFI_INVALID_PARAMETER;
+  }
+
   if (CommSize == NULL) {
     TempCommSize = OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data) + 
CommunicateHeader->MessageLength;
   } else {
-- 
2.20.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#92157): https://edk2.groups.io/g/devel/message/92157
Mute This Topic: https://groups.io/mt/92830802/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-