From 9e68568e34bef0037bb16b3cbe361e559b8da369 Mon Sep 17 00:00:00 2001

From: Laszlo Ersek <lersek@redhat.com>

Date: Fri, 22 Mar 2019 17:39:35 +0100

Subject: [PATCH 1/8] MdeModulePkg/HiiDatabase: Fix potential integer overflow

 (CVE-2018-12181)

MIME-Version: 1.0

Content-Type: text/plain; charset=UTF-8

Content-Transfer-Encoding: 8bit

Message-id: <20190322163936.10835-2-lersek@redhat.com>

Patchwork-id: 85124

O-Subject:  [RHEL-7.7 ovmf PATCH 1/2] MdeModulePkg/HiiDatabase: Fix potential

	integer overflow (CVE-2018-12181)

Bugzilla: 1691479

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

Acked-by: Vitaly Kuznetsov <vkuznets@redhat.com>

From: Ray Ni <ray.ni@intel.com>

--v-- RHEL7 note --v--

A contextual conflict had to be resolved manually because we don't have

upstream commit 979b7d802c31 ("MdeModulePkg/HiiDB: Make sure database

update behaviors are atomic", 2018-10-26), which was written for upstream

BZ <https://bugzilla.tianocore.org/show_bug.cgi?id=1235>. More

specifically, the context to which upstream ffe5f7a6b4e9 (i.e. the patch

being backported) applies includes EfiAcquireLock() added in 979b7d802c31,

and our downstream context lacks that.

While reviewing this, I noticed that some of the new error paths

introduced by the more rigorous checking in upstream ffe5f7a6b4e9 fail to

release the lock. For upstream I reported a new BZ about this

<https://bugzilla.tianocore.org/show_bug.cgi?id=1652>, but down-stream, we

don't have the EfiAcquireLock() in the first place, so there is no leak.

--^-- RHEL7 note --^--

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1135

Contributed-under: TianoCore Contribution Agreement 1.1

Signed-off-by: Ray Ni <ray.ni@intel.com>

Cc: Dandan Bi <dandan.bi@intel.com>

Cc: Hao A Wu <hao.a.wu@intel.com>

Reviewed-by: Hao Wu <hao.a.wu@intel.com>

Reviewed-by: Jian J Wang <jian.j.wang@intel.com>

(cherry picked from commit ffe5f7a6b4e978dffbe1df228963adc914451106)

---

 MdeModulePkg/Universal/HiiDatabaseDxe/Image.c | 126 +++++++++++++++++++++-----

 1 file changed, 103 insertions(+), 23 deletions(-)

diff --git a/MdeModulePkg/Universal/HiiDatabaseDxe/Image.c b/MdeModulePkg/Universal/HiiDatabaseDxe/Image.c

index 431a5b8..dc9566b 100644

--- a/MdeModulePkg/Universal/HiiDatabaseDxe/Image.c

+++ b/MdeModulePkg/Universal/HiiDatabaseDxe/Image.c

@@ -16,6 +16,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.

 #include "HiiDatabase.h"

+#define MAX_UINT24    0xFFFFFF

 /**

   Get the imageid of last image block: EFI_HII_IIBT_END_BLOCK when input

@@ -649,8 +650,16 @@ HiiNewImage (

     return EFI_NOT_FOUND;

   }

-  NewBlockSize = sizeof (EFI_HII_IIBT_IMAGE_24BIT_BLOCK) - sizeof (EFI_HII_RGB_PIXEL) +

-                 BITMAP_LEN_24_BIT ((UINT32) Image->Width, Image->Height);

+  //

+  // Calcuate the size of new image.

+  // Make sure the size doesn't overflow UINT32.

+  // Note: 24Bit BMP occpuies 3 bytes per pixel.

+  //

+  NewBlockSize = (UINT32)Image->Width * Image->Height;

+  if (NewBlockSize > (MAX_UINT32 - (sizeof (EFI_HII_IIBT_IMAGE_24BIT_BLOCK) - sizeof (EFI_HII_RGB_PIXEL))) / 3) {

+    return EFI_OUT_OF_RESOURCES;

+  }

+  NewBlockSize = NewBlockSize * 3 + (sizeof (EFI_HII_IIBT_IMAGE_24BIT_BLOCK) - sizeof (EFI_HII_RGB_PIXEL));

   //

   // Get the image package in the package list,

@@ -669,6 +678,18 @@ HiiNewImage (

     //

     // Update the package's image block by appending the new block to the end.

     //

+

+    //

+    // Make sure the final package length doesn't overflow.

+    // Length of the package header is represented using 24 bits. So MAX length is MAX_UINT24.

+    //

+    if (NewBlockSize > MAX_UINT24 - ImagePackage->ImagePkgHdr.Header.Length) {

+      return EFI_OUT_OF_RESOURCES;

+    }

+    //

+    // Because ImagePackage->ImageBlockSize < ImagePackage->ImagePkgHdr.Header.Length,

+    // So (ImagePackage->ImageBlockSize + NewBlockSize) <= MAX_UINT24

+    //

     ImageBlocks = AllocatePool (ImagePackage->ImageBlockSize + NewBlockSize);

     if (ImageBlocks == NULL) {

       return EFI_OUT_OF_RESOURCES;

@@ -699,6 +720,13 @@ HiiNewImage (

   } else {

     //

+    // Make sure the final package length doesn't overflow.

+    // Length of the package header is represented using 24 bits. So MAX length is MAX_UINT24.

+    //

+    if (NewBlockSize > MAX_UINT24 - (sizeof (EFI_HII_IMAGE_PACKAGE_HDR) + sizeof (EFI_HII_IIBT_END_BLOCK))) {

+      return EFI_OUT_OF_RESOURCES;

+    }

+    //

     // The specified package list does not contain image package.

     // Create one to add this image block.

     //

@@ -895,8 +923,11 @@ IGetImage (

     // Use the common block code since the definition of these structures is the same.

     //

     CopyMem (&Iibt1bit, CurrentImageBlock, sizeof (EFI_HII_IIBT_IMAGE_1BIT_BLOCK));

-    ImageLength = sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL) *

-                  ((UINT32) Iibt1bit.Bitmap.Width * Iibt1bit.Bitmap.Height);

+    ImageLength = (UINTN) Iibt1bit.Bitmap.Width * Iibt1bit.Bitmap.Height;

+    if (ImageLength > MAX_UINTN / sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL)) {

+      return EFI_OUT_OF_RESOURCES;

+    }

+    ImageLength  *= sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL);

     Image->Bitmap = AllocateZeroPool (ImageLength);

     if (Image->Bitmap == NULL) {

       return EFI_OUT_OF_RESOURCES;

@@ -945,9 +976,13 @@ IGetImage (

     // fall through

     //

   case EFI_HII_IIBT_IMAGE_24BIT:

-    Width = ReadUnaligned16 ((VOID *) &((EFI_HII_IIBT_IMAGE_24BIT_BLOCK *) CurrentImageBlock)->Bitmap.Width);

+    Width  = ReadUnaligned16 ((VOID *) &((EFI_HII_IIBT_IMAGE_24BIT_BLOCK *) CurrentImageBlock)->Bitmap.Width);

     Height = ReadUnaligned16 ((VOID *) &((EFI_HII_IIBT_IMAGE_24BIT_BLOCK *) CurrentImageBlock)->Bitmap.Height);

-    ImageLength = sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL) * ((UINT32) Width * Height);

+    ImageLength = (UINTN)Width * Height;

+    if (ImageLength > MAX_UINTN / sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL)) {

+      return EFI_OUT_OF_RESOURCES;

+    }

+    ImageLength  *= sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL);

     Image->Bitmap = AllocateZeroPool (ImageLength);

     if (Image->Bitmap == NULL) {

       return EFI_OUT_OF_RESOURCES;

@@ -1114,8 +1149,23 @@ HiiSetImage (

   //

   // Create the new image block according to input image.

   //

-  NewBlockSize = sizeof (EFI_HII_IIBT_IMAGE_24BIT_BLOCK) - sizeof (EFI_HII_RGB_PIXEL) +

-                 BITMAP_LEN_24_BIT ((UINT32) Image->Width, Image->Height);

+

+  //

+  // Make sure the final package length doesn't overflow.

+  // Length of the package header is represented using 24 bits. So MAX length is MAX_UINT24.

+  // 24Bit BMP occpuies 3 bytes per pixel.

+  //

+  NewBlockSize = (UINT32)Image->Width * Image->Height;

+  if (NewBlockSize > (MAX_UINT32 - (sizeof (EFI_HII_IIBT_IMAGE_24BIT_BLOCK) - sizeof (EFI_HII_RGB_PIXEL))) / 3) {

+    return EFI_OUT_OF_RESOURCES;

+  }

+  NewBlockSize = NewBlockSize * 3 + (sizeof (EFI_HII_IIBT_IMAGE_24BIT_BLOCK) - sizeof (EFI_HII_RGB_PIXEL));

+  if ((NewBlockSize > OldBlockSize) &&

+      (NewBlockSize - OldBlockSize > MAX_UINT24 - ImagePackage->ImagePkgHdr.Header.Length)

+      ) {

+    return EFI_OUT_OF_RESOURCES;

+  }

+

   //

   // Adjust the image package to remove the original block firstly then add the new block.

   //

@@ -1207,8 +1257,8 @@ HiiDrawImage (

   EFI_IMAGE_OUTPUT                    *ImageOut;

   EFI_GRAPHICS_OUTPUT_BLT_PIXEL       *BltBuffer;

   UINTN                               BufferLen;

-  UINTN                               Width;

-  UINTN                               Height;

+  UINT16                              Width;

+  UINT16                              Height;

   UINTN                               Xpos;

   UINTN                               Ypos;

   UINTN                               OffsetY1;

@@ -1269,21 +1319,36 @@ HiiDrawImage (

   //

   if (*Blt != NULL) {

     //

+    // Make sure the BltX and BltY is inside the Blt area.

+    //

+    if ((BltX >= (*Blt)->Width) || (BltY >= (*Blt)->Height)) {

+      return EFI_INVALID_PARAMETER;

+    }

+

+    //

     // Clip the image by (Width, Height)

     //

     Width  = Image->Width;

     Height = Image->Height;

-    if (Width > (*Blt)->Width - BltX) {

-      Width = (*Blt)->Width - BltX;

+    if (Width > (*Blt)->Width - (UINT16)BltX) {

+      Width = (*Blt)->Width - (UINT16)BltX;

     }

-    if (Height > (*Blt)->Height - BltY) {

-      Height = (*Blt)->Height - BltY;

+    if (Height > (*Blt)->Height - (UINT16)BltY) {

+      Height = (*Blt)->Height - (UINT16)BltY;

     }

-    BufferLen = Width * Height * sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL);

-    BltBuffer = (EFI_GRAPHICS_OUTPUT_BLT_PIXEL *) AllocateZeroPool (BufferLen);

+    //

+    // Prepare the buffer for the temporary image.

+    // Make sure the buffer size doesn't overflow UINTN.

+    //

+    BufferLen = Width * Height;

+    if (BufferLen > MAX_UINTN / sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL)) {

+      return EFI_OUT_OF_RESOURCES;

+    }

+    BufferLen *= sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL);

+    BltBuffer  = AllocateZeroPool (BufferLen);

     if (BltBuffer == NULL) {

       return EFI_OUT_OF_RESOURCES;

     }

@@ -1346,11 +1411,26 @@ HiiDrawImage (

     //

     // Allocate a new bitmap to hold the incoming image.

     //

-    Width  = Image->Width  + BltX;

-    Height = Image->Height + BltY;

-    BufferLen = Width * Height * sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL);

-    BltBuffer = (EFI_GRAPHICS_OUTPUT_BLT_PIXEL *) AllocateZeroPool (BufferLen);

+    //

+    // Make sure the final width and height doesn't overflow UINT16.

+    //

+    if ((BltX > (UINTN)MAX_UINT16 - Image->Width) || (BltY > (UINTN)MAX_UINT16 - Image->Height)) {

+      return EFI_INVALID_PARAMETER;

+    }

+

+    Width  = Image->Width  + (UINT16)BltX;

+    Height = Image->Height + (UINT16)BltY;

+

+    //

+    // Make sure the output image size doesn't overflow UINTN.

+    //

+    BufferLen = Width * Height;

+    if (BufferLen > MAX_UINTN / sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL)) {

+      return EFI_OUT_OF_RESOURCES;

+    }

+    BufferLen *= sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL);

+    BltBuffer  = AllocateZeroPool (BufferLen);

     if (BltBuffer == NULL) {

       return EFI_OUT_OF_RESOURCES;

     }

@@ -1360,8 +1440,8 @@ HiiDrawImage (

       FreePool (BltBuffer);

       return EFI_OUT_OF_RESOURCES;

     }

-    ImageOut->Width        = (UINT16) Width;

-    ImageOut->Height       = (UINT16) Height;

+    ImageOut->Width        = Width;

+    ImageOut->Height       = Height;

     ImageOut->Image.Bitmap = BltBuffer;

     //

@@ -1375,7 +1455,7 @@ HiiDrawImage (

       return Status;

     }

     ASSERT (FontInfo != NULL);

-    for (Index = 0; Index < Width * Height; Index++) {

+    for (Index = 0; Index < (UINTN)Width * Height; Index++) {

       BltBuffer[Index] = FontInfo->BackgroundColor;

     }

     FreePool (FontInfo);

-- 

1.8.3.1