I agree with this comment.
On 02/13/20 19:33, Philippe Mathieu-Daudé wrote:
> On 2/13/20 7:29 PM, Philippe Mathieu-Daude wrote:
>> Math expressions written in terms of SafeIntLib function calls
>> are easily readable, making review trivial. Convert the truncation
>> checks added by commit 322ac05f8 to SafeIntLib calls.
>>
>> Cc: Jian J Wang <jian.j.wang@intel.com>
>> Cc: Hao A Wu <hao.a.wu@intel.com>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Suggested-by: Laszlo Ersek <lersek@redhat.com>
>> Signed-off-by: Philippe Mathieu-Daude <philmd@redhat.com>
>> ---
>> .../DxeS3BootScriptLib.inf | 1 +
>> .../InternalBootScriptLib.h | 1 +
>> .../PiDxeS3BootScriptLib/BootScriptSave.c | 114 +++++++++++-------
>> 3 files changed, 73 insertions(+), 43 deletions(-)
>>
>> diff --git
>> a/MdeModulePkg/Library/PiDxeS3BootScriptLib/DxeS3BootScriptLib.inf
>> b/MdeModulePkg/Library/PiDxeS3BootScriptLib/DxeS3BootScriptLib.inf
>> index 2b894c99da55..698039fe8e69 100644
>> --- a/MdeModulePkg/Library/PiDxeS3BootScriptLib/DxeS3BootScriptLib.inf
>> +++ b/MdeModulePkg/Library/PiDxeS3BootScriptLib/DxeS3BootScriptLib.inf
>> @@ -40,15 +40,16 @@ [Packages]
>> [LibraryClasses]
>> UefiBootServicesTableLib
>> BaseLib
>> BaseMemoryLib
>> TimerLib
>> DebugLib
>> PcdLib
>> UefiLib
>> SmbusLib
>> PciSegmentLib
>> IoLib
>> LockBoxLib
>> + SafeIntLib
>> [Protocols]
>> gEfiSmmBase2ProtocolGuid ## SOMETIMES_CONSUMES
>> diff --git
>> a/MdeModulePkg/Library/PiDxeS3BootScriptLib/InternalBootScriptLib.h
>> b/MdeModulePkg/Library/PiDxeS3BootScriptLib/InternalBootScriptLib.h
>> index 9485994087d0..7513220c15ac 100644
>> --- a/MdeModulePkg/Library/PiDxeS3BootScriptLib/InternalBootScriptLib.h
>> +++ b/MdeModulePkg/Library/PiDxeS3BootScriptLib/InternalBootScriptLib.h
>> @@ -1,49 +1,50 @@
>> /** @file
>> Support for S3 boot script lib. This file defined some internal
>> macro and internal
>> data structure
>> Copyright (c) 2006 - 2016, Intel Corporation. All rights
>> reserved.<BR>
>> SPDX-License-Identifier: BSD-2-Clause-Patent
>> **/
>> #ifndef __INTERNAL_BOOT_SCRIPT_LIB__
>> #define __INTERNAL_BOOT_SCRIPT_LIB__
>> #include <PiDxe.h>
>> #include <Guid/EventGroup.h>
>> #include <Protocol/SmmBase2.h>
>> #include <Protocol/DxeSmmReadyToLock.h>
>> #include <Protocol/SmmReadyToLock.h>
>> #include <Protocol/SmmExitBootServices.h>
>> #include <Protocol/SmmLegacyBoot.h>
>> #include <Library/S3BootScriptLib.h>
>> #include <Library/UefiBootServicesTableLib.h>
>> #include <Library/BaseLib.h>
>> #include <Library/PcdLib.h>
>> #include <Library/SmbusLib.h>
>> #include <Library/IoLib.h>
>> #include <Library/PciSegmentLib.h>
>> #include <Library/DebugLib.h>
>> #include <Library/BaseMemoryLib.h>
>> #include <Library/TimerLib.h>
>> #include <Library/UefiLib.h>
>> #include <Library/LockBoxLib.h>
>> +#include <Library/SafeIntLib.h>
>> #include "BootScriptInternalFormat.h"
>> #define MAX_IO_ADDRESS 0xFFFF
>> //
>> // Macro to convert a UEFI PCI address + segment to a PCI Segment
>> Library PCI address
>> //
>> #define PCI_ADDRESS_ENCODE(S, A) PCI_SEGMENT_LIB_ADDRESS( \
>> S, \
>> ((((UINTN)(A)) & 0xff000000) >>
>> 24), \
>> ((((UINTN)(A)) & 0x00ff0000) >>
>> 16), \
>> ((((UINTN)(A)) & 0xff00) >> 8), \
>> ((RShiftU64 ((A), 32) & 0xfff) |
>> ((A) & 0xff)) \
>> )
>> diff --git
>> a/MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c
>> b/MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c
>> index 9315fc9f0188..d229263638fc 100644
>> --- a/MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c
>> +++ b/MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c
>> @@ -995,55 +995,60 @@ EFIAPI
>> S3BootScriptSaveIoWrite (
>> IN S3_BOOT_SCRIPT_LIB_WIDTH Width,
>> IN UINT64 Address,
>> IN UINTN Count,
>> IN VOID *Buffer
>> )
>> {
>> + EFI_STATUS Status;
>> UINT8 Length;
>> UINT8 *Script;
>> UINT8 WidthInByte;
>> EFI_BOOT_SCRIPT_IO_WRITE ScriptIoWrite;
>> - WidthInByte = (UINT8) (0x01 << (Width & 0x03));
>> + Status = SafeUintnToUint8 (Count, &Length);
>> + if (EFI_ERROR (Status)) {
>> + return RETURN_OUT_OF_RESOURCES;
>> + }
>> +
>> + Status = SafeUint8Mult (Length, 0x01 << (Width & 0x03), &Length);
>> + if (EFI_ERROR (Status)) {
>> + return RETURN_OUT_OF_RESOURCES;
>> + }
>> - //
>> - // Truncation check
>> - //
>> - if ((Count > MAX_UINT8) ||
>> - (WidthInByte * Count > MAX_UINT8 - sizeof
>> (EFI_BOOT_SCRIPT_IO_WRITE))) {
>> + Status = SafeUint8Add (Length, sizeof (EFI_BOOT_SCRIPT_IO_WRITE),
>> &Length);
>> + if (EFI_ERROR (Status)) {
>> return RETURN_OUT_OF_RESOURCES;
>> }
>> - Length = (UINT8)(sizeof (EFI_BOOT_SCRIPT_IO_WRITE) + (WidthInByte *
>> Count));
>> Script = S3BootScriptGetEntryAddAddress (Length);
>> if (Script == NULL) {
>> return RETURN_OUT_OF_RESOURCES;
>> }
>> //
>> // save script data
>> //
>> ScriptIoWrite.OpCode = EFI_BOOT_SCRIPT_IO_WRITE_OPCODE;
>> ScriptIoWrite.Length = Length;
>> ScriptIoWrite.Width = Width;
>> ScriptIoWrite.Address = Address;
>> ScriptIoWrite.Count = (UINT32) Count;
>> CopyMem ((VOID*)Script, (VOID*)&ScriptIoWrite,
>> sizeof(EFI_BOOT_SCRIPT_IO_WRITE));
>> CopyMem ((VOID*)(Script + sizeof (EFI_BOOT_SCRIPT_IO_WRITE)),
>> Buffer, WidthInByte * Count);
>> SyncBootScript (Script);
>> return RETURN_SUCCESS;
>> }
>
> Oops wrong version (WidthInByte is uninitialized).
Also -- if the MdeModulePkg maintainers like the appraoch in general --,
you could eliminate the "WidthInByte * Count" multiplication from the
CopyMem() call altogether. Instead, you could save the SafeUint8Mult()
result in an intermediate variable, and reuse it here.
Just an idea (again, only for the case when the MdeModulePkg actually
like this refactoring).
Thanks!
Laszlo