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
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#92504): https://edk2.groups.io/g/devel/message/92504
Mute This Topic: https://groups.io/mt/93034134/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
