[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 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 | 5 ++++-
 MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c  | 4 ++++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
index 9e5c6cbe33dd..a2a97a4056ee 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
@@ -621,6 +621,9 @@ InternalIsBufferOverlapped (
   IN UINTN  Size2
   )
 {
+  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 +706,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..fe3e6ba54281 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
@@ -525,6 +525,10 @@ SmmCommunicationCommunicate (
 
   CommunicateHeader = (EFI_SMM_COMMUNICATE_HEADER *)CommBuffer;
 
+  if (CommunicateHeader->MessageLength > MAX_UINTN - OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data)) {
+    return EFI_INVALID_PARAMETER;
+  }
+
   if (CommSize == NULL) {
     TempCommSize = OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data) + CommunicateHeader->MessageLength;
   } else {
-- 
2.20.1.windows.1

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

[Ni, Ray]

> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
> @@ -621,6 +621,9 @@ InternalIsBufferOverlapped (
>    IN UINTN  Size2
>    )
>  {
> +  if (((UINTN)Buff1 > MAX_UINTN - Size1) || ((UINTN)Buff2 > MAX_UINTN -
> Size2)) {
> +    return TRUE;
> +  }
1. The change looks good because it avoids integer overflow in below code that adds Size1 to Buff1 and
adds Size2 to Buff2.
Can you please add comments to explain the logic?


> 
> +  if (CommunicateHeader->MessageLength > MAX_UINTN - OFFSET_OF
> (EFI_SMM_COMMUNICATE_HEADER, Data)) {
> +    return EFI_INVALID_PARAMETER;
> +  }
2. Above check avoids integer overflow in below code that adds CommunicateHeader->MessageLength
to OFFSET_OF(EFI_SMM_COMMUNICATE_HEADER, Data).
Can it be moved to inside the below if-clause?


> +
>    if (CommSize == NULL) {
>      TempCommSize = OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data)
> + CommunicateHeader->MessageLength;
>    } else {
3.  I further reviewed the else-clause logic. When CommSize is not NULL, is that needed to make sure
that *CommSize >= OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data) + CommunicateHeader->MessageLength?
Or is the check already in the code somewhere?
If we think the check is needed, I agree with the change #2 to have a common logic to check integer overflow.

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

[wenyi,xie]

On 2022/8/16 9:49, Ni, Ray wrote:
>> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
>> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
>> @@ -621,6 +621,9 @@ InternalIsBufferOverlapped (
>>    IN UINTN  Size2
>>    )
>>  {
>> +  if (((UINTN)Buff1 > MAX_UINTN - Size1) || ((UINTN)Buff2 > MAX_UINTN -
>> Size2)) {
>> +    return TRUE;
>> +  }
> 1. The change looks good because it avoids integer overflow in below code that adds Size1 to Buff1 and
> adds Size2 to Buff2.
> Can you please add comments to explain the logic?
> 
OK, I will add comments.
> 
>>
>> +  if (CommunicateHeader->MessageLength > MAX_UINTN - OFFSET_OF
>> (EFI_SMM_COMMUNICATE_HEADER, Data)) {
>> +    return EFI_INVALID_PARAMETER;
>> +  }
> 2. Above check avoids integer overflow in below code that adds CommunicateHeader->MessageLength
> to OFFSET_OF(EFI_SMM_COMMUNICATE_HEADER, Data).
> Can it be moved to inside the below if-clause?
> 
> 
>> +
>>    if (CommSize == NULL) {
>>      TempCommSize = OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data)
>> + CommunicateHeader->MessageLength;
>>    } else {
> 3.  I further reviewed the else-clause logic. When CommSize is not NULL, is that needed to make sure
> that *CommSize >= OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data) + CommunicateHeader->MessageLength?
> Or is the check already in the code somewhere?
> If we think the check is needed, I agree with the change #2 to have a common logic to check integer overflow.
> .When CommSize is not NULL, it seems only CommSize is been used, CommunicateHeader->MessageLength is been ignored, and value of CommSize will been verified before used by SmiHandler.
So I think it's no need  make sure that *CommSize >= OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data) + CommunicateHeader->MessageLength.
I will move it to inside if-clause.

Thanks
Wenyi
>