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

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

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
>