* [RFC PATCH 0/1] MdeModulePkg/PiDxeS3BootScriptLib: Use SafeIntLib to avoid truncation @ 2020-02-13 18:29 Philippe Mathieu-Daudé 2020-02-13 18:29 ` [RFC PATCH 1/1] " Philippe Mathieu-Daudé 0 siblings, 1 reply; 7+ messages in thread From: Philippe Mathieu-Daudé @ 2020-02-13 18:29 UTC (permalink / raw) To: devel Cc: Philippe Mathieu-Daude, Jian J Wang, Hao A Wu, Eric Dong, Laszlo Ersek Commit 322ac05f8bbc added truncation checks to fix CVE-2019-14563. I found the 'a * b > d - c' reverse notation not obvious to review, and suggested to write 'a * b + c > d'. Laszlo explained me this is the EDK2 standard pattern to check against each overflow, but pointed out the SafeIntLib which have pretty readable calls. This is my try at using it. Regards, Phil. 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> Cc: Laszlo Ersek <lersek@redhat.com> Philippe Mathieu-Daudé (1): MdeModulePkg/PiDxeS3BootScriptLib: Use SafeIntLib to avoid truncation .../DxeS3BootScriptLib.inf | 1 + .../InternalBootScriptLib.h | 1 + .../PiDxeS3BootScriptLib/BootScriptSave.c | 114 +++++++++++------- 3 files changed, 73 insertions(+), 43 deletions(-) -- 2.21.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [RFC PATCH 1/1] MdeModulePkg/PiDxeS3BootScriptLib: Use SafeIntLib to avoid truncation 2020-02-13 18:29 [RFC PATCH 0/1] MdeModulePkg/PiDxeS3BootScriptLib: Use SafeIntLib to avoid truncation Philippe Mathieu-Daudé @ 2020-02-13 18:29 ` Philippe Mathieu-Daudé 2020-02-13 18:33 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 7+ messages in thread From: Philippe Mathieu-Daudé @ 2020-02-13 18:29 UTC (permalink / raw) To: devel Cc: Philippe Mathieu-Daude, Jian J Wang, Hao A Wu, Eric Dong, Laszlo Ersek 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; } /** Adds a record for an I/O modify operation into a S3 boot script table @param Width The width of the I/O operations.Enumerated in S3_BOOT_SCRIPT_LIB_WIDTH. @param Address The base address of the I/O operations. @param Data A pointer to the data to be OR-ed. @param DataMask A pointer to the data mask to be AND-ed with the data read from the register @retval RETURN_OUT_OF_RESOURCES Not enough memory for the table do operation. @retval RETURN_SUCCESS Opcode is added. **/ @@ -1100,54 +1105,59 @@ EFIAPI S3BootScriptSaveMemWrite ( 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_MEM_WRITE ScriptMemWrite; - 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_MEM_WRITE))) { + Status = SafeUint8Add (Length, sizeof (EFI_BOOT_SCRIPT_MEM_WRITE), &Length); + if (EFI_ERROR (Status)) { return RETURN_OUT_OF_RESOURCES; } - Length = (UINT8)(sizeof (EFI_BOOT_SCRIPT_MEM_WRITE) + (WidthInByte * Count)); Script = S3BootScriptGetEntryAddAddress (Length); if (Script == NULL) { return RETURN_OUT_OF_RESOURCES; } // // Build script data // ScriptMemWrite.OpCode = EFI_BOOT_SCRIPT_MEM_WRITE_OPCODE; ScriptMemWrite.Length = Length; ScriptMemWrite.Width = Width; ScriptMemWrite.Address = Address; ScriptMemWrite.Count = (UINT32) Count; CopyMem ((VOID*)Script, (VOID*)&ScriptMemWrite, sizeof(EFI_BOOT_SCRIPT_MEM_WRITE)); CopyMem ((VOID*)(Script + sizeof (EFI_BOOT_SCRIPT_MEM_WRITE)), Buffer, WidthInByte * Count); SyncBootScript (Script); return RETURN_SUCCESS; } /** Adds a record for a memory modify operation into a specified boot script table. @param Width The width of the I/O operations.Enumerated in S3_BOOT_SCRIPT_LIB_WIDTH. @param Address The base address of the memory operations. Address needs alignment if required @param Data A pointer to the data to be OR-ed. @param DataMask A pointer to the data mask to be AND-ed with the data read from the register. @retval RETURN_OUT_OF_RESOURCES Not enough memory for the table do operation. @retval RETURN_SUCCESS Opcode is added. **/ @@ -1206,62 +1216,67 @@ EFIAPI S3BootScriptSavePciCfgWrite ( 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_PCI_CONFIG_WRITE ScriptPciWrite; if (Width == S3BootScriptWidthUint64 || Width == S3BootScriptWidthFifoUint64 || Width == S3BootScriptWidthFillUint64) { return EFI_INVALID_PARAMETER; } - 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_PCI_CONFIG_WRITE))) { + Status = SafeUint8Add (Length, sizeof (EFI_BOOT_SCRIPT_PCI_CONFIG_WRITE), &Length); + if (EFI_ERROR (Status)) { return RETURN_OUT_OF_RESOURCES; } - Length = (UINT8)(sizeof (EFI_BOOT_SCRIPT_PCI_CONFIG_WRITE) + (WidthInByte * Count)); Script = S3BootScriptGetEntryAddAddress (Length); if (Script == NULL) { return RETURN_OUT_OF_RESOURCES; } // // Build script data // ScriptPciWrite.OpCode = EFI_BOOT_SCRIPT_PCI_CONFIG_WRITE_OPCODE; ScriptPciWrite.Length = Length; ScriptPciWrite.Width = Width; ScriptPciWrite.Address = Address; ScriptPciWrite.Count = (UINT32) Count; CopyMem ((VOID*)Script, (VOID*)&ScriptPciWrite, sizeof (EFI_BOOT_SCRIPT_PCI_CONFIG_WRITE)); CopyMem ((VOID*)(Script + sizeof (EFI_BOOT_SCRIPT_PCI_CONFIG_WRITE)), Buffer, WidthInByte * Count); SyncBootScript (Script); return RETURN_SUCCESS; } /** Adds a record for a PCI configuration space modify operation into a specified boot script table. @param Width The width of the I/O operations.Enumerated in S3_BOOT_SCRIPT_LIB_WIDTH. @param Address The address within the PCI configuration space. @param Data A pointer to the data to be OR-ed.The size depends on Width. @param DataMask A pointer to the data mask to be AND-ed. @retval RETURN_OUT_OF_RESOURCES Not enough memory for the table do operation. @retval RETURN__SUCCESS Opcode is added. @note A known Limitations in the implementation which is 64bits operations are not supported. **/ @@ -1331,65 +1346,70 @@ EFIAPI S3BootScriptSavePciCfg2Write ( IN S3_BOOT_SCRIPT_LIB_WIDTH Width, IN UINT16 Segment, IN UINT64 Address, IN UINTN Count, IN VOID *Buffer ) { + EFI_STATUS Status; UINT8 Length; UINT8 *Script; UINT8 WidthInByte; EFI_BOOT_SCRIPT_PCI_CONFIG2_WRITE ScriptPciWrite2; if (Width == S3BootScriptWidthUint64 || Width == S3BootScriptWidthFifoUint64 || Width == S3BootScriptWidthFillUint64) { return EFI_INVALID_PARAMETER; } - 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_PCI_CONFIG2_WRITE))) { + Status = SafeUint8Add (Length, sizeof (EFI_BOOT_SCRIPT_PCI_CONFIG2_WRITE), &Length); + if (EFI_ERROR (Status)) { return RETURN_OUT_OF_RESOURCES; } - Length = (UINT8)(sizeof (EFI_BOOT_SCRIPT_PCI_CONFIG2_WRITE) + (WidthInByte * Count)); Script = S3BootScriptGetEntryAddAddress (Length); if (Script == NULL) { return RETURN_OUT_OF_RESOURCES; } // // Build script data // ScriptPciWrite2.OpCode = EFI_BOOT_SCRIPT_PCI_CONFIG2_WRITE_OPCODE; ScriptPciWrite2.Length = Length; ScriptPciWrite2.Width = Width; ScriptPciWrite2.Address = Address; ScriptPciWrite2.Segment = Segment; ScriptPciWrite2.Count = (UINT32)Count; CopyMem ((VOID*)Script, (VOID*)&ScriptPciWrite2, sizeof (EFI_BOOT_SCRIPT_PCI_CONFIG2_WRITE)); CopyMem ((VOID*)(Script + sizeof (EFI_BOOT_SCRIPT_PCI_CONFIG2_WRITE)), Buffer, WidthInByte * Count); SyncBootScript (Script); return RETURN_SUCCESS; } /** Adds a record for a PCI configuration 2 space modify operation into a specified boot script table. @param Width The width of the I/O operations.Enumerated in S3_BOOT_SCRIPT_LIB_WIDTH. @param Segment The PCI segment number for Address. @param Address The address within the PCI configuration space. @param Data A pointer to the data to be OR-ed. The size depends on Width. @param DataMask A pointer to the data mask to be AND-ed. @retval RETURN_OUT_OF_RESOURCES Not enough memory for the table do operation. @retval RETURN_SUCCESS Opcode is added. @note A known Limitations in the implementation which is 64bits operations are not supported. **/ @@ -1560,64 +1580,66 @@ EFIAPI S3BootScriptSaveSmbusExecute ( IN UINTN SmBusAddress, IN EFI_SMBUS_OPERATION Operation, IN UINTN *Length, IN VOID *Buffer ) { EFI_STATUS Status; UINTN BufferLength; UINT8 DataSize; UINT8 *Script; EFI_BOOT_SCRIPT_SMBUS_EXECUTE ScriptSmbusExecute; if (Length == NULL) { BufferLength = 0; } else { BufferLength = *Length; } Status = CheckParameters (SmBusAddress, Operation, &BufferLength, Buffer); if (EFI_ERROR (Status)) { return Status; } - // - // Truncation check - // - if (BufferLength > MAX_UINT8 - sizeof (EFI_BOOT_SCRIPT_SMBUS_EXECUTE)) { + Status = SafeUintnToUint8 (BufferLength, &DataSize); + if (EFI_ERROR (Status)) { + return RETURN_OUT_OF_RESOURCES; + } + + Status = SafeUint8Add (DataSize, sizeof (EFI_BOOT_SCRIPT_SMBUS_EXECUTE), &DataSize); + if (EFI_ERROR (Status)) { return RETURN_OUT_OF_RESOURCES; } - DataSize = (UINT8)(sizeof (EFI_BOOT_SCRIPT_SMBUS_EXECUTE) + BufferLength); Script = S3BootScriptGetEntryAddAddress (DataSize); if (Script == NULL) { return RETURN_OUT_OF_RESOURCES; } // // Build script data // ScriptSmbusExecute.OpCode = EFI_BOOT_SCRIPT_SMBUS_EXECUTE_OPCODE; ScriptSmbusExecute.Length = DataSize; ScriptSmbusExecute.SmBusAddress = (UINT64) SmBusAddress; ScriptSmbusExecute.Operation = Operation; ScriptSmbusExecute.DataSize = (UINT32) BufferLength; CopyMem ((VOID*)Script, (VOID*)&ScriptSmbusExecute, sizeof (EFI_BOOT_SCRIPT_SMBUS_EXECUTE)); CopyMem ( (VOID*)(Script + sizeof (EFI_BOOT_SCRIPT_SMBUS_EXECUTE)), Buffer, BufferLength ); SyncBootScript (Script); return RETURN_SUCCESS; } /** Adds a record for an execution stall on the processor into a specified boot script table. @param Duration Duration in microseconds of the stall @retval RETURN_OUT_OF_RESOURCES Not enough memory for the table do operation. @retval RETURN_SUCCESS Opcode is added. **/ @@ -1768,48 +1790,51 @@ EFIAPI S3BootScriptSaveInformation ( IN UINT32 InformationLength, IN VOID *Information ) { + EFI_STATUS Status; UINT8 Length; UINT8 *Script; EFI_BOOT_SCRIPT_INFORMATION ScriptInformation; - // - // Truncation check - // - if (InformationLength > MAX_UINT8 - sizeof (EFI_BOOT_SCRIPT_INFORMATION)) { + Status = SafeUint32ToUint8 (InformationLength, &Length); + if (EFI_ERROR (Status)) { + return RETURN_OUT_OF_RESOURCES; + } + + Status = SafeUint8Add (Length, sizeof (EFI_BOOT_SCRIPT_INFORMATION), &Length); + if (EFI_ERROR (Status)) { return RETURN_OUT_OF_RESOURCES; } - Length = (UINT8)(sizeof (EFI_BOOT_SCRIPT_INFORMATION) + InformationLength); Script = S3BootScriptGetEntryAddAddress (Length); if (Script == NULL) { return RETURN_OUT_OF_RESOURCES; } // // Build script data // ScriptInformation.OpCode = EFI_BOOT_SCRIPT_INFORMATION_OPCODE; ScriptInformation.Length = Length; ScriptInformation.InformationLength = InformationLength; CopyMem ((VOID*)Script, (VOID*)&ScriptInformation, sizeof (EFI_BOOT_SCRIPT_INFORMATION)); CopyMem ((VOID*)(Script + sizeof (EFI_BOOT_SCRIPT_INFORMATION)), (VOID *) Information, (UINTN) InformationLength); SyncBootScript (Script); return RETURN_SUCCESS; } /** Store a string in the boot script table. This opcode is a no-op on dispatch and is only used for debugging script issues. @param String The string to save to boot script table @retval RETURN_OUT_OF_RESOURCES Not enough memory for the table do operation. @retval RETURN_SUCCESS Opcode is added. **/ @@ -2231,62 +2256,65 @@ EFIAPI S3BootScriptLabelInternal ( IN BOOLEAN BeforeOrAfter, IN OUT VOID **Position OPTIONAL, IN UINT32 InformationLength, IN CONST CHAR8 *Information ) { + EFI_STATUS Status; UINT8 Length; UINT8 *Script; EFI_BOOT_SCRIPT_INFORMATION ScriptInformation; - // - // Truncation check - // - if (InformationLength > MAX_UINT8 - sizeof (EFI_BOOT_SCRIPT_INFORMATION)) { + Status = SafeUint32ToUint8 (InformationLength, &Length); + if (EFI_ERROR (Status)) { + return RETURN_OUT_OF_RESOURCES; + } + + Status = SafeUint8Add (Length, sizeof (EFI_BOOT_SCRIPT_INFORMATION), &Length); + if (EFI_ERROR (Status)) { return RETURN_OUT_OF_RESOURCES; } - Length = (UINT8)(sizeof (EFI_BOOT_SCRIPT_INFORMATION) + InformationLength); Script = S3BootScriptGetEntryAddAddress (Length); if (Script == NULL) { return RETURN_OUT_OF_RESOURCES; } // // Build script data // ScriptInformation.OpCode = S3_BOOT_SCRIPT_LIB_LABEL_OPCODE; ScriptInformation.Length = Length; ScriptInformation.InformationLength = InformationLength; CopyMem ((VOID*)Script, (VOID*)&ScriptInformation, sizeof (EFI_BOOT_SCRIPT_INFORMATION)); CopyMem ((VOID*)(Script + sizeof (EFI_BOOT_SCRIPT_INFORMATION)), (VOID *) Information, (UINTN) InformationLength); SyncBootScript (Script); return S3BootScriptMoveLastOpcode (BeforeOrAfter, Position); } /** Find a label within the boot script table and, if not present, optionally create it. @param BeforeOrAfter Specifies whether the opcode is stored before (TRUE) or after (FALSE) the position in the boot script table specified by Position. @param CreateIfNotFound Specifies whether the label will be created if the label does not exists (TRUE) or not (FALSE). @param Position On entry, specifies the position in the boot script table where the opcode will be inserted, either before or after, depending on BeforeOrAfter. On exit, specifies the position of the inserted opcode in the boot script table. @param Label Points to the label which will be inserted in the boot script table. @retval EFI_SUCCESS The operation succeeded. A record was added into the specified script table. @retval EFI_INVALID_PARAMETER The parameter is illegal or the given boot script is not supported. If the opcode is unknow or not supported because of the PCD Feature Flags. @retval EFI_OUT_OF_RESOURCES There is insufficient memory to store the boot script. **/ -- 2.21.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [RFC PATCH 1/1] MdeModulePkg/PiDxeS3BootScriptLib: Use SafeIntLib to avoid truncation 2020-02-13 18:29 ` [RFC PATCH 1/1] " Philippe Mathieu-Daudé @ 2020-02-13 18:33 ` Philippe Mathieu-Daudé 2020-02-17 9:32 ` [edk2-devel] " Laszlo Ersek 0 siblings, 1 reply; 7+ messages in thread From: Philippe Mathieu-Daudé @ 2020-02-13 18:33 UTC (permalink / raw) To: devel; +Cc: Jian J Wang, Hao A Wu, Eric Dong, Laszlo Ersek 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). ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [RFC PATCH 1/1] MdeModulePkg/PiDxeS3BootScriptLib: Use SafeIntLib to avoid truncation 2020-02-13 18:33 ` Philippe Mathieu-Daudé @ 2020-02-17 9:32 ` Laszlo Ersek 2020-02-18 6:51 ` GuoMinJ ` (2 more replies) 0 siblings, 3 replies; 7+ messages in thread From: Laszlo Ersek @ 2020-02-17 9:32 UTC (permalink / raw) To: devel, philmd; +Cc: Jian J Wang, Hao A Wu, Eric Dong 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [RFC PATCH 1/1] MdeModulePkg/PiDxeS3BootScriptLib: Use SafeIntLib to avoid truncation 2020-02-17 9:32 ` [edk2-devel] " Laszlo Ersek @ 2020-02-18 6:51 ` GuoMinJ 2020-02-18 7:04 ` GuoMinJ 2020-02-18 7:10 ` GuoMinJ 2 siblings, 0 replies; 7+ messages in thread From: GuoMinJ @ 2020-02-18 6:51 UTC (permalink / raw) To: Laszlo Ersek, devel [-- Attachment #1: Type: text/plain, Size: 47 bytes --] I agree with this comment. Thanks. Guomin [-- Attachment #2: Type: text/html, Size: 59 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [RFC PATCH 1/1] MdeModulePkg/PiDxeS3BootScriptLib: Use SafeIntLib to avoid truncation 2020-02-17 9:32 ` [edk2-devel] " Laszlo Ersek 2020-02-18 6:51 ` GuoMinJ @ 2020-02-18 7:04 ` GuoMinJ 2020-02-18 7:10 ` GuoMinJ 2 siblings, 0 replies; 7+ messages in thread From: GuoMinJ @ 2020-02-18 7:04 UTC (permalink / raw) To: Laszlo Ersek, devel [-- Attachment #1: Type: text/plain, Size: 28 bytes --] I agree with this comment. [-- Attachment #2: Type: text/html, Size: 28 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [RFC PATCH 1/1] MdeModulePkg/PiDxeS3BootScriptLib: Use SafeIntLib to avoid truncation 2020-02-17 9:32 ` [edk2-devel] " Laszlo Ersek 2020-02-18 6:51 ` GuoMinJ 2020-02-18 7:04 ` GuoMinJ @ 2020-02-18 7:10 ` GuoMinJ 2 siblings, 0 replies; 7+ messages in thread From: GuoMinJ @ 2020-02-18 7:10 UTC (permalink / raw) To: devel, lersek; +Cc: philmd, Jian J Wang, Hao A Wu, Eric Dong [-- 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 --] ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-02-18 7:10 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-02-13 18:29 [RFC PATCH 0/1] MdeModulePkg/PiDxeS3BootScriptLib: Use SafeIntLib to avoid truncation Philippe Mathieu-Daudé 2020-02-13 18:29 ` [RFC PATCH 1/1] " Philippe Mathieu-Daudé 2020-02-13 18:33 ` Philippe Mathieu-Daudé 2020-02-17 9:32 ` [edk2-devel] " Laszlo Ersek 2020-02-18 6:51 ` GuoMinJ 2020-02-18 7:04 ` GuoMinJ 2020-02-18 7:10 ` GuoMinJ
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox