From ae16157ee5c96e36e5d1ec558f875e6b89188770 Mon Sep 17 00:00:00 2001

From: Laszlo Ersek <lersek@redhat.com>

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 <lersek@redhat.com>

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é <philmd@redhat.com>

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@intel.com>

Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>

Cc: Philippe Mathieu-Daudé <philmd@redhat.com>

Cc: Siyuan Fu <siyuan.fu@intel.com>

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3356

Signed-off-by: Laszlo Ersek <lersek@redhat.com>

Reviewed-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

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

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