[edk2-devel] [RFC PATCH 1/1] MdeModulePkg/PiDxeS3BootScriptLib: Use SafeIntLib to avoid truncation
Laszlo Ersek
lersek at redhat.com
Mon Feb 17 09:32:10 UTC 2020
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 at intel.com>
>> Cc: Hao A Wu <hao.a.wu at intel.com>
>> Cc: Eric Dong <eric.dong at intel.com>
>> Suggested-by: Laszlo Ersek <lersek at redhat.com>
>> Signed-off-by: Philippe Mathieu-Daude <philmd at 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 (#54547): https://edk2.groups.io/g/devel/message/54547
Mute This Topic: https://groups.io/mt/71248587/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list