* [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