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

["wenyi,xie" <xiewenyi2@huawei.com>]

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 <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
>>
>>
>>
>> 
>>
> 
> 
> 
> .
>