Re: [edk2-devel] [RFC PATCH 1/1] MdeModulePkg/PiDxeS3BootScriptLib: Use SafeIntLib to avoid truncation

["GuoMinJ" <newexplorerj@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 6996 bytes --]

I agree with this comment.

Laszlo Ersek <lersek@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.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
>
>
> 
>
>

[-- Attachment #2: Type: text/html, Size: 9763 bytes --]