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

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

2022-08-14 Thread wenyi,xie via groups.io




On 2022/8/15 9:12, gaoliming wrote:
> Wenyi:
>   I add my comments below. 

Thank you for your comments. I agree with you that overflow can be treated as 
overlap. I will update the patch and send it later.

Thanks
Wenyi
> 
>> -邮件原件-
>> 发件人: devel@edk2.groups.io  代表 wenyi,xie via
>> groups.io
>> 发送时间: 2022年8月5日 15:21
>> 收件人: devel@edk2.groups.io; jian.j.w...@intel.com;
>> gaolim...@byosoft.com.cn; eric.d...@intel.com; ray...@intel.com
>> 抄送: songdongku...@huawei.com; xiewen...@huawei.com
>> 主题: [edk2-devel] [PATCH EDK2 v1 1/1] MdeModulePkg/PiSmmCore:Avoid
>> overflow risk
>>
>> As the CommunicationBuffer plus BufferSize may overflow, check the
>> value first before using.
>>
>> Cc: Jian J Wang 
>> Cc: Liming Gao 
>> Cc: Eric Dong 
>> Cc: Ray Ni 
>> Signed-off-by: Wenyi Xie 
>> ---
>>  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;
>>  }
>>
> If Buff1 + Size1 overflows MAX_UINTN, it can be also regarded as overlap,
> return TRUE. 
> 
> If you think overflow is not overlap, please also update function
> InternalIsBufferOverlapped name and description to avoid confuse. 
> 
> Thanks
> Liming
> 
>>  /**
>> @@ -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 (#92407): https://edk2.groups.io/g/devel/message/92407
Mute This Topic: https://groups.io/mt/93027797/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

2022-08-14 Thread gaoliming via groups.io

Wenyi:
  I add my comments below. 

> -邮件原件-
> 发件人: devel@edk2.groups.io  代表 wenyi,xie via
> groups.io
> 发送时间: 2022年8月5日 15:21
> 收件人: devel@edk2.groups.io; jian.j.w...@intel.com;
> gaolim...@byosoft.com.cn; eric.d...@intel.com; ray...@intel.com
> 抄送: songdongku...@huawei.com; xiewen...@huawei.com
> 主题: [edk2-devel] [PATCH EDK2 v1 1/1] MdeModulePkg/PiSmmCore:Avoid
> overflow risk
> 
> As the CommunicationBuffer plus BufferSize may overflow, check the
> value first before using.
> 
> Cc: Jian J Wang 
> Cc: Liming Gao 
> Cc: Eric Dong 
> Cc: Ray Ni 
> Signed-off-by: Wenyi Xie 
> ---
>  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;
>  }
> 
If Buff1 + Size1 overflows MAX_UINTN, it can be also regarded as overlap,
return TRUE. 

If you think overflow is not overlap, please also update function
InternalIsBufferOverlapped name and description to avoid confuse. 

Thanks
Liming

>  /**
> @@ -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 (#92406): https://edk2.groups.io/g/devel/message/92406
Mute This Topic: https://groups.io/mt/93027797/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

2022-08-05 Thread wenyi,xie via groups.io

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

Cc: Jian J Wang 
Cc: Liming Gao 
Cc: Eric Dong 
Cc: Ray Ni 
Signed-off-by: Wenyi Xie 
---
 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]
-=-=-=-=-=-=-=-=-=-=-=-

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

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

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

3 matches

Site Navigation

Mail list logo

Footer information