I agree with this comment.
Laszlo Ersek <ler...@redhat.com> 于2020年2月17日周一 下午5:32写道：
> 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.w...@intel.com>
> >> Cc: Hao A Wu <hao.a...@intel.com>
> >> Cc: Eric Dong <eric.d...@intel.com>
> >> Suggested-by: Laszlo Ersek <ler...@redhat.com>
> >> Signed-off-by: Philippe Mathieu-Daude <phi...@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
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#54573): https://edk2.groups.io/g/devel/message/54573
Mute This Topic: https://groups.io/mt/71248587/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
