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

["Ni, Ray" <ray.ni@intel.com>]

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