public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [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