> --- 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?
>
> + 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.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#92462): https://edk2.groups.io/g/devel/message/92462
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]
-=-=-=-=-=-=-=-=-=-=-=-
