[PATCH EDK2 v2 0/1] MdeModulePkg/PiSmmCore:Avoid overflow risk

[PATCH EDK2 v2 0/1] MdeModulePkg/PiSmmCore:Avoid overflow risk

[wenyi,xie]

Main Changes since v1 :
1.treate overflow as overlap

Wenyi Xie (1):
  MdeModulePkg/PiSmmCore:Avoid overflow risk

 MdeModulePkg/Core/PiSmmCore/PiSmmCore.c | 5 ++++-
 MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c  | 4 ++++
 2 files changed, 8 insertions(+), 1 deletion(-)

-- 
2.20.1.windows.1

[PATCH EDK2 v2 0/1] MdeModulePkg/PiSmmCore:Avoid overflow risk

[wenyi,xie]

Main Changes since v1 :
1.add comments for overflow check;
2.move condition check to if-clause.

Wenyi Xie (1):
  MdeModulePkg/PiSmmCore:Avoid overflow risk

 MdeModulePkg/Core/PiSmmCore/PiSmmCore.c | 10 +++++++++-
 MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c  |  4 ++++
 2 files changed, 13 insertions(+), 1 deletion(-)

-- 
2.20.1.windows.1

[PATCH EDK2 v2 1/1] MdeModulePkg/PiSmmCore:Avoid overflow risk

[wenyi,xie]

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

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Signed-off-by: Wenyi Xie <xiewenyi2@huawei.com>
---
 MdeModulePkg/Core/PiSmmCore/PiSmmCore.c | 10 +++++++++-
 MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c  |  4 ++++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
index 9e5c6cbe33dd..003db3b85802 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
@@ -621,6 +621,14 @@ InternalIsBufferOverlapped (
   IN UINTN  Size2
   )
 {
+  //
+  // If integer overflow when adding Buff1 to Size1, treat it as Overlap.
+  // Also, if integer overflow when adding Buff2 to Size2, treat it as Overlap.
+  //
+  if (((UINTN)Buff1 > MAX_UINTN - Size1) || ((UINTN)Buff2 > MAX_UINTN - Size2)) {
+    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.
@@ -703,7 +711,7 @@ SmmEntryPoint (
         //
         // 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..78df802fe748 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
@@ -526,6 +526,10 @@ SmmCommunicationCommunicate (
   CommunicateHeader = (EFI_SMM_COMMUNICATE_HEADER *)CommBuffer;
 
   if (CommSize == NULL) {
+    if (CommunicateHeader->MessageLength > MAX_UINTN - OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data)) {
+      return EFI_INVALID_PARAMETER;
+    }
+
     TempCommSize = OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data) + CommunicateHeader->MessageLength;
   } else {
     TempCommSize = *CommSize;
-- 
2.20.1.windows.1