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

["gaoliming" <gaoliming@byosoft.com.cn>]

Wenyi:
  I add my comments below. 

> -----邮件原件-----
> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 wenyi,xie via
> groups.io
> 发送时间: 2022年8月5日 15:21
> 收件人: devel@edk2.groups.io; jian.j.wang@intel.com;
> gaoliming@byosoft.com.cn; eric.dong@intel.com; ray.ni@intel.com
> 抄送: songdongkuang@huawei.com; xiewenyi2@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 <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 | 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
> 
> 
> 
> 
>