From ae16157ee5c96e36e5d1ec558f875e6b89188770 Mon Sep 17 00:00:00 2001 From: Laszlo Ersek Date: Tue, 27 Apr 2021 10:10:42 +0200 Subject: [PATCH 05/10] NetworkPkg/IScsiDxe: fix potential integer overflow in IScsiBinToHex() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RH-Author: Laszlo Ersek RH-MergeRequest: 3: NetworkPkg/IScsiDxe: fix IScsiHexToBin() security and functionality bugs [rhel-8.4.0.z] RH-Commit: [5/10] 96bbb794ca2355c2d9e83d79d385582daf8e4aa4 RH-Bugzilla: 1956676 RH-Acked-by: Philippe Mathieu-Daudé 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 Cc: Maciej Rabeda Cc: Philippe Mathieu-Daudé Cc: Siyuan Fu Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3356 Signed-off-by: Laszlo Ersek Reviewed-by: Maciej Rabeda Reviewed-by: Philippe Mathieu-Daudé Upstream: https://bugzilla.tianocore.org/show_bug.cgi?id=3356, c#17...c#22 --- NetworkPkg/IScsiDxe/IScsiDxe.inf | 1 + NetworkPkg/IScsiDxe/IScsiImpl.h | 1 + NetworkPkg/IScsiDxe/IScsiMisc.c | 19 +++++++++++++++---- NetworkPkg/IScsiDxe/IScsiMisc.h | 1 + 4 files changed, 18 insertions(+), 4 deletions(-) diff --git a/NetworkPkg/IScsiDxe/IScsiDxe.inf b/NetworkPkg/IScsiDxe/IScsiDxe.inf index 543c408302..1dde56d00c 100644 --- a/NetworkPkg/IScsiDxe/IScsiDxe.inf +++ b/NetworkPkg/IScsiDxe/IScsiDxe.inf @@ -74,6 +74,7 @@ MemoryAllocationLib NetLib PrintLib + SafeIntLib TcpIoLib UefiBootServicesTableLib UefiDriverEntryPoint diff --git a/NetworkPkg/IScsiDxe/IScsiImpl.h b/NetworkPkg/IScsiDxe/IScsiImpl.h index d895c7feb9..ac3a25730e 100644 --- a/NetworkPkg/IScsiDxe/IScsiImpl.h +++ b/NetworkPkg/IScsiDxe/IScsiImpl.h @@ -44,6 +44,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent #include #include #include +#include #include #include #include diff --git a/NetworkPkg/IScsiDxe/IScsiMisc.c b/NetworkPkg/IScsiDxe/IScsiMisc.c index b8fef3ff6f..42988e15cb 100644 --- a/NetworkPkg/IScsiDxe/IScsiMisc.c +++ b/NetworkPkg/IScsiDxe/IScsiMisc.c @@ -316,6 +316,7 @@ IScsiMacAddrToStr ( @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. **/ @@ -327,18 +328,28 @@ IScsiBinToHex ( 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. // diff --git a/NetworkPkg/IScsiDxe/IScsiMisc.h b/NetworkPkg/IScsiDxe/IScsiMisc.h index 46c725aab3..231413993b 100644 --- a/NetworkPkg/IScsiDxe/IScsiMisc.h +++ b/NetworkPkg/IScsiDxe/IScsiMisc.h @@ -150,6 +150,7 @@ IScsiAsciiStrToIp ( @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. **/ -- 2.27.0