[edk2-devel] [PUBLIC edk2 PATCH v2 05/10] NetworkPkg/IScsiDxe: fix potential integer overflow in IScsiBinToHex()
Laszlo Ersek
lersek at redhat.com
Tue Jun 8 12:12:54 UTC 2021
Considering IScsiBinToHex():
> if (((*HexLength) - 3) < BinLength * 2) {
> *HexLength = BinLength * 2 + 3;
> }
the following subexpressions are problematic:
(*HexLength) - 3
BinLength * 2
BinLength * 2 + 3
The first one may wrap under zero, the latter two may wrap over
MAX_UINT32.
Rewrite the calculation using SafeIntLib.
While at it, change the type of the "Index" variable from UINTN to UINT32.
The largest "Index"-based value that we calculate is
Index * 2 + 2 (with (Index == BinLength))
Because the patch makes
BinLength * 2 + 3
safe to calculate in UINT32, using UINT32 for
Index * 2 + 2 (with (Index == BinLength))
is safe too. Consistently using UINT32 improves readability.
This patch is best reviewed with "git show -W".
The integer overflows that this patch fixes are theoretical; a subsequent
patch in the series will audit the IScsiBinToHex() call sites, and show
that none of them can fail.
Cc: Jiaxin Wu <jiaxin.wu at intel.com>
Cc: Maciej Rabeda <maciej.rabeda at linux.intel.com>
Cc: Philippe Mathieu-Daudé <philmd at redhat.com>
Cc: Siyuan Fu <siyuan.fu at intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3356
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
Reviewed-by: Maciej Rabeda <maciej.rabeda at linux.intel.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd at redhat.com>
---
NetworkPkg/IScsiDxe/IScsiDxe.inf | 1 +
NetworkPkg/IScsiDxe/IScsiImpl.h | 1 +
NetworkPkg/IScsiDxe/IScsiMisc.h | 1 +
NetworkPkg/IScsiDxe/IScsiMisc.c | 19 +++++++++++++++----
4 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/NetworkPkg/IScsiDxe/IScsiDxe.inf b/NetworkPkg/IScsiDxe/IScsiDxe.inf
index 543c4083026a..1dde56d00ca2 100644
--- a/NetworkPkg/IScsiDxe/IScsiDxe.inf
+++ b/NetworkPkg/IScsiDxe/IScsiDxe.inf
@@ -58,38 +58,39 @@ [Sources]
IScsiProto.c
IScsiProto.h
[Packages]
MdePkg/MdePkg.dec
MdeModulePkg/MdeModulePkg.dec
CryptoPkg/CryptoPkg.dec
NetworkPkg/NetworkPkg.dec
[LibraryClasses]
BaseCryptLib
BaseLib
BaseMemoryLib
DebugLib
DevicePathLib
HiiLib
MemoryAllocationLib
NetLib
PrintLib
+ SafeIntLib
TcpIoLib
UefiBootServicesTableLib
UefiDriverEntryPoint
UefiHiiServicesLib
UefiLib
UefiRuntimeServicesTableLib
[Protocols]
gEfiAcpiTableProtocolGuid ## SOMETIMES_CONSUMES ## SystemTable
gEfiDriverBindingProtocolGuid ## SOMETIMES_PRODUCES
gEfiPciIoProtocolGuid ## SOMETIMES_CONSUMES
gEfiDhcp4ProtocolGuid ## SOMETIMES_CONSUMES
gEfiDhcp6ProtocolGuid ## SOMETIMES_CONSUMES
gEfiDhcp4ServiceBindingProtocolGuid ## SOMETIMES_CONSUMES
gEfiDhcp6ServiceBindingProtocolGuid ## SOMETIMES_CONSUMES
gEfiDns4ServiceBindingProtocolGuid ## SOMETIMES_CONSUMES
gEfiDns4ProtocolGuid ## SOMETIMES_CONSUMES
gEfiDns6ServiceBindingProtocolGuid ## SOMETIMES_CONSUMES
gEfiDns6ProtocolGuid ## SOMETIMES_CONSUMES
diff --git a/NetworkPkg/IScsiDxe/IScsiImpl.h b/NetworkPkg/IScsiDxe/IScsiImpl.h
index d895c7feb947..ac3a25730efb 100644
--- a/NetworkPkg/IScsiDxe/IScsiImpl.h
+++ b/NetworkPkg/IScsiDxe/IScsiImpl.h
@@ -28,38 +28,39 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include <Protocol/Tcp6.h>
#include <Protocol/Ip4Config2.h>
#include <Protocol/Ip6Config.h>
#include <Protocol/AuthenticationInfo.h>
#include <Protocol/IScsiInitiatorName.h>
#include <Protocol/ScsiPassThruExt.h>
#include <Protocol/AdapterInformation.h>
#include <Protocol/NetworkInterfaceIdentifier.h>
#include <Library/BaseCryptLib.h>
#include <Library/BaseLib.h>
#include <Library/BaseMemoryLib.h>
#include <Library/DebugLib.h>
#include <Library/DevicePathLib.h>
#include <Library/HiiLib.h>
#include <Library/MemoryAllocationLib.h>
#include <Library/NetLib.h>
#include <Library/PrintLib.h>
+#include <Library/SafeIntLib.h>
#include <Library/TcpIoLib.h>
#include <Library/UefiBootServicesTableLib.h>
#include <Library/UefiHiiServicesLib.h>
#include <Library/UefiLib.h>
#include <Library/UefiRuntimeServicesTableLib.h>
#include <Guid/MdeModuleHii.h>
#include <Guid/EventGroup.h>
#include <Guid/Acpi.h>
#include "IScsiConfigNVDataStruc.h"
#include "IScsiDriver.h"
#include "IScsiProto.h"
#include "IScsiCHAP.h"
#include "IScsiDhcp.h"
#include "IScsiDhcp6.h"
#include "IScsiIbft.h"
#include "IScsiMisc.h"
diff --git a/NetworkPkg/IScsiDxe/IScsiMisc.h b/NetworkPkg/IScsiDxe/IScsiMisc.h
index 46c725aab3a4..231413993b08 100644
--- a/NetworkPkg/IScsiDxe/IScsiMisc.h
+++ b/NetworkPkg/IScsiDxe/IScsiMisc.h
@@ -134,38 +134,39 @@ IScsiMacAddrToStr (
**/
EFI_STATUS
IScsiAsciiStrToIp (
IN CHAR8 *Str,
IN UINT8 IpMode,
OUT EFI_IP_ADDRESS *Ip
);
/**
Convert the binary encoded buffer into a hexadecimal encoded string.
@param[in] BinBuffer The buffer containing the binary data.
@param[in] BinLength Length of the binary buffer.
@param[in, out] HexStr Pointer to the string.
@param[in, out] HexLength The length of the string.
@retval EFI_SUCCESS The binary data is converted to the hexadecimal string
and the length of the string is updated.
@retval EFI_BUFFER_TOO_SMALL The string is too small.
+ @retval EFI_BAD_BUFFER_SIZE BinLength is too large for hex encoding.
@retval EFI_INVALID_PARAMETER The IP string is malformatted.
**/
EFI_STATUS
IScsiBinToHex (
IN UINT8 *BinBuffer,
IN UINT32 BinLength,
IN OUT CHAR8 *HexStr,
IN OUT UINT32 *HexLength
);
/**
Convert the hexadecimal string into a binary encoded buffer.
@param[in, out] BinBuffer The binary buffer.
@param[in, out] BinLength Length of the binary buffer.
@param[in] HexStr The hexadecimal string.
@retval EFI_SUCCESS The hexadecimal string is converted into a binary
diff --git a/NetworkPkg/IScsiDxe/IScsiMisc.c b/NetworkPkg/IScsiDxe/IScsiMisc.c
index b8fef3ff6f5a..42988e15cb06 100644
--- a/NetworkPkg/IScsiDxe/IScsiMisc.c
+++ b/NetworkPkg/IScsiDxe/IScsiMisc.c
@@ -300,61 +300,72 @@ IScsiMacAddrToStr (
String = &Str[3 * Index - 1] ;
if (VlanId != 0) {
String += UnicodeSPrint (String, 6 * sizeof (CHAR16), L"\\%04x", (UINTN) VlanId);
}
*String = L'\0';
}
/**
Convert the binary encoded buffer into a hexadecimal encoded string.
@param[in] BinBuffer The buffer containing the binary data.
@param[in] BinLength Length of the binary buffer.
@param[in, out] HexStr Pointer to the string.
@param[in, out] HexLength The length of the string.
@retval EFI_SUCCESS The binary data is converted to the hexadecimal string
and the length of the string is updated.
@retval EFI_BUFFER_TOO_SMALL The string is too small.
+ @retval EFI_BAD_BUFFER_SIZE BinLength is too large for hex encoding.
@retval EFI_INVALID_PARAMETER The IP string is malformatted.
**/
EFI_STATUS
IScsiBinToHex (
IN UINT8 *BinBuffer,
IN UINT32 BinLength,
IN OUT CHAR8 *HexStr,
IN OUT UINT32 *HexLength
)
{
- UINTN Index;
+ UINT32 HexLengthMin;
+ UINT32 HexLengthProvided;
+ UINT32 Index;
if ((HexStr == NULL) || (BinBuffer == NULL) || (BinLength == 0)) {
return EFI_INVALID_PARAMETER;
}
- if (((*HexLength) - 3) < BinLength * 2) {
- *HexLength = BinLength * 2 + 3;
+ //
+ // Safely calculate: HexLengthMin := BinLength * 2 + 3.
+ //
+ if (RETURN_ERROR (SafeUint32Mult (BinLength, 2, &HexLengthMin)) ||
+ RETURN_ERROR (SafeUint32Add (HexLengthMin, 3, &HexLengthMin))) {
+ return EFI_BAD_BUFFER_SIZE;
+ }
+
+ HexLengthProvided = *HexLength;
+ *HexLength = HexLengthMin;
+ if (HexLengthProvided < HexLengthMin) {
return EFI_BUFFER_TOO_SMALL;
}
- *HexLength = BinLength * 2 + 3;
//
// Prefix for Hex String.
//
HexStr[0] = '0';
HexStr[1] = 'x';
for (Index = 0; Index < BinLength; Index++) {
HexStr[Index * 2 + 2] = IScsiHexString[BinBuffer[Index] >> 4];
HexStr[Index * 2 + 3] = IScsiHexString[BinBuffer[Index] & 0xf];
}
HexStr[Index * 2 + 2] = '\0';
return EFI_SUCCESS;
}
/**
Convert the hexadecimal string into a binary encoded buffer.
--
2.19.1.3.g30247aa5d201
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76202): https://edk2.groups.io/g/devel/message/76202
Mute This Topic: https://groups.io/mt/83394112/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list