[PATCH v2 0/2] Fix bugs in HiiDatabase driver

[PATCH v2 0/2] Fix bugs in HiiDatabase driver

[Ray Ni]

v2: put the CVE number in patch title.

Ray Ni (2):
  MdeModulePkg/HiiDatabase: Fix potential integer overflow
    (CVE-2018-12181)
  MdeModulePkg/HiiImage: Fix stack overflow when corrupted BMP is parsed
    (CVE-2018-12181)

 MdeModulePkg/Universal/HiiDatabaseDxe/Image.c | 130 ++++++++++++++----
 1 file changed, 105 insertions(+), 25 deletions(-)

-- 
2.20.1.windows.1

[PATCH v2 1/2] MdeModulePkg/HiiDatabase: Fix potential integer overflow (CVE-2018-12181)

[Ray Ni]

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>
---
 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 71ebc559c0..80a4ec1114 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
@@ -651,8 +652,16 @@ HiiNewImage (
 
   EfiAcquireLock (&mHiiDatabaseLock);
 
-  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,
@@ -671,6 +680,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) {
       EfiReleaseLock (&mHiiDatabaseLock);
@@ -701,6 +722,13 @@ HiiNewImage (
     PackageListNode->PackageListHdr.PackageLength += NewBlockSize;
 
   } 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.
@@ -902,8 +930,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;
@@ -952,9 +983,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;
@@ -1124,8 +1159,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.
   //
@@ -1219,8 +1269,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;
@@ -1280,6 +1330,13 @@ HiiDrawImage (
   // Otherwise a new bitmap will be allocated to hold this image.
   //
   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)
     //
@@ -1287,15 +1344,23 @@ HiiDrawImage (
     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;
     }
@@ -1358,11 +1423,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;
     }
@@ -1372,8 +1452,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;
 
     //
@@ -1387,7 +1467,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);
-- 
2.20.1.windows.1

Re: [PATCH v2 1/2] MdeModulePkg/HiiDatabase: Fix potential integer overflow (CVE-2018-12181)

[Wu, Hao A]

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

Best Regards,
Hao Wu


> -----Original Message-----
> From: Ni, Ray
> Sent: Friday, March 08, 2019 10:35 AM
> To: edk2-devel@lists.01.org
> Cc: Bi, Dandan; Wu, Hao A
> Subject: [PATCH v2 1/2] MdeModulePkg/HiiDatabase: Fix potential integer
> overflow (CVE-2018-12181)
> 
> 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>
> ---
>  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 71ebc559c0..80a4ec1114 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
> @@ -651,8 +652,16 @@ HiiNewImage (
> 
>    EfiAcquireLock (&mHiiDatabaseLock);
> 
> -  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,
> @@ -671,6 +680,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) {
>        EfiReleaseLock (&mHiiDatabaseLock);
> @@ -701,6 +722,13 @@ HiiNewImage (
>      PackageListNode->PackageListHdr.PackageLength += NewBlockSize;
> 
>    } 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.
> @@ -902,8 +930,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;
> @@ -952,9 +983,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;
> @@ -1124,8 +1159,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.
>    //
> @@ -1219,8 +1269,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;
> @@ -1280,6 +1330,13 @@ HiiDrawImage (
>    // Otherwise a new bitmap will be allocated to hold this image.
>    //
>    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)
>      //
> @@ -1287,15 +1344,23 @@ HiiDrawImage (
>      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;
>      }
> @@ -1358,11 +1423,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;
>      }
> @@ -1372,8 +1452,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;
> 
>      //
> @@ -1387,7 +1467,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);
> --
> 2.20.1.windows.1

[PATCH v2 2/2] MdeModulePkg/HiiImage: Fix stack overflow when corrupted BMP is parsed (CVE-2018-12181)

[Ray Ni]

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

For 4bit BMP, there are only 2^4 = 16 colors in the palette.
But when a corrupted BMP contains more than 16 colors in the palette,
today's implementation wrongly copies all colors to the local
PaletteValue[16] array which causes stack overflow.

The similar issue also exists in the logic to handle 8bit BMP.

The patch fixes the issue by only copies the first 16 or 256 colors
in the palette depending on the BMP type.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ray Ni <ray.ni@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
---
 MdeModulePkg/Universal/HiiDatabaseDxe/Image.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Universal/HiiDatabaseDxe/Image.c b/MdeModulePkg/Universal/HiiDatabaseDxe/Image.c
index 80a4ec1114..8532f272eb 100644
--- a/MdeModulePkg/Universal/HiiDatabaseDxe/Image.c
+++ b/MdeModulePkg/Universal/HiiDatabaseDxe/Image.c
@@ -370,7 +370,7 @@ Output4bitPixel (
   PaletteNum = (UINT16)(Palette->PaletteSize / sizeof (EFI_HII_RGB_PIXEL));
 
   ZeroMem (PaletteValue, sizeof (PaletteValue));
-  CopyRgbToGopPixel (PaletteValue, Palette->PaletteValue, PaletteNum);
+  CopyRgbToGopPixel (PaletteValue, Palette->PaletteValue, MIN (PaletteNum, ARRAY_SIZE (PaletteValue)));
   FreePool (Palette);
 
   //
@@ -447,7 +447,7 @@ Output8bitPixel (
   CopyMem (Palette, PaletteInfo, PaletteSize);
   PaletteNum = (UINT16)(Palette->PaletteSize / sizeof (EFI_HII_RGB_PIXEL));
   ZeroMem (PaletteValue, sizeof (PaletteValue));
-  CopyRgbToGopPixel (PaletteValue, Palette->PaletteValue, PaletteNum);
+  CopyRgbToGopPixel (PaletteValue, Palette->PaletteValue, MIN (PaletteNum, ARRAY_SIZE (PaletteValue)));
   FreePool (Palette);
 
   //
-- 
2.20.1.windows.1

Re: [PATCH v2 0/2] Fix bugs in HiiDatabase driver

[Wang, Jian J]

Hi all,

This is a very important fix for this issue. If no objection, I'd like the patch be part of this stable tag.


As to this patch series, 

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


> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ray Ni
> Sent: Friday, March 08, 2019 10:35 AM
> To: edk2-devel@lists.01.org
> Subject: [edk2] [PATCH v2 0/2] Fix bugs in HiiDatabase driver
> 
> v2: put the CVE number in patch title.
> 
> Ray Ni (2):
>   MdeModulePkg/HiiDatabase: Fix potential integer overflow
>     (CVE-2018-12181)
>   MdeModulePkg/HiiImage: Fix stack overflow when corrupted BMP is parsed
>     (CVE-2018-12181)
> 
>  MdeModulePkg/Universal/HiiDatabaseDxe/Image.c | 130 ++++++++++++++----
>  1 file changed, 105 insertions(+), 25 deletions(-)
> 
> --
> 2.20.1.windows.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH v2 0/2] Fix bugs in HiiDatabase driver

[Gao, Liming]

This is to fix the security issue. I agree it is an import bug fix. I am OK to push it for edk2-stable201903 tag

Thanks
Liming
> -----Original Message-----
> From: Wang, Jian J
> Sent: Thursday, March 7, 2019 7:17 PM
> To: Ni, Ray <ray.ni@intel.com>; edk2-devel@lists.01.org
> Cc: Cetola, Stephano <stephano.cetola@intel.com>; Gao, Liming <liming.gao@intel.com>
> Subject: RE: [edk2] [PATCH v2 0/2] Fix bugs in HiiDatabase driver
> 
> Hi all,
> 
> This is a very important fix for this issue. If no objection, I'd like the patch be part of this stable tag.
> 
> 
> As to this patch series,
> 
>     Reviewed-by: Jian J Wang <jian.j.wang@intel.com>
> 
> 
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ray Ni
> > Sent: Friday, March 08, 2019 10:35 AM
> > To: edk2-devel@lists.01.org
> > Subject: [edk2] [PATCH v2 0/2] Fix bugs in HiiDatabase driver
> >
> > v2: put the CVE number in patch title.
> >
> > Ray Ni (2):
> >   MdeModulePkg/HiiDatabase: Fix potential integer overflow
> >     (CVE-2018-12181)
> >   MdeModulePkg/HiiImage: Fix stack overflow when corrupted BMP is parsed
> >     (CVE-2018-12181)
> >
> >  MdeModulePkg/Universal/HiiDatabaseDxe/Image.c | 130 ++++++++++++++----
> >  1 file changed, 105 insertions(+), 25 deletions(-)
> >
> > --
> > 2.20.1.windows.1
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH v2 0/2] Fix bugs in HiiDatabase driver

[Laszlo Ersek]

On 03/08/19 07:41, Gao, Liming wrote:
> This is to fix the security issue. I agree it is an import bug fix. I am OK to push it for edk2-stable201903 tag

Me too.

If we had stable *branches* (as opposed to just stable tags), then we
wouldn't have to delay the stable tag (the release) -- we'd just apply
the CVE fix to both the master branch (*after* the stable tag) and on
the stable branch too. But our development workflow isn't there yet, so
I guess we can delay the stable tag a bit more. I suggest updating the
date in
<https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning#edk2-stable201903-tag-planning>.

Thanks!
Laszlo

>> -----Original Message-----
>> From: Wang, Jian J
>> Sent: Thursday, March 7, 2019 7:17 PM
>> To: Ni, Ray <ray.ni@intel.com>; edk2-devel@lists.01.org
>> Cc: Cetola, Stephano <stephano.cetola@intel.com>; Gao, Liming <liming.gao@intel.com>
>> Subject: RE: [edk2] [PATCH v2 0/2] Fix bugs in HiiDatabase driver
>>
>> Hi all,
>>
>> This is a very important fix for this issue. If no objection, I'd like the patch be part of this stable tag.
>>
>>
>> As to this patch series,
>>
>>     Reviewed-by: Jian J Wang <jian.j.wang@intel.com>
>>
>>
>>> -----Original Message-----
>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ray Ni
>>> Sent: Friday, March 08, 2019 10:35 AM
>>> To: edk2-devel@lists.01.org
>>> Subject: [edk2] [PATCH v2 0/2] Fix bugs in HiiDatabase driver
>>>
>>> v2: put the CVE number in patch title.
>>>
>>> Ray Ni (2):
>>>   MdeModulePkg/HiiDatabase: Fix potential integer overflow
>>>     (CVE-2018-12181)
>>>   MdeModulePkg/HiiImage: Fix stack overflow when corrupted BMP is parsed
>>>     (CVE-2018-12181)
>>>
>>>  MdeModulePkg/Universal/HiiDatabaseDxe/Image.c | 130 ++++++++++++++----
>>>  1 file changed, 105 insertions(+), 25 deletions(-)
>>>
>>> --
>>> 2.20.1.windows.1
>>>
>>> _______________________________________________
>>> edk2-devel mailing list
>>> edk2-devel@lists.01.org
>>> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH v2 0/2] Fix bugs in HiiDatabase driver

[Laszlo Ersek]

On 03/08/19 09:52, Laszlo Ersek wrote:
> On 03/08/19 07:41, Gao, Liming wrote:
>> This is to fix the security issue. I agree it is an import bug fix. I am OK to push it for edk2-stable201903 tag
> 
> Me too.
> 
> If we had stable *branches* (as opposed to just stable tags), then we
> wouldn't have to delay the stable tag (the release) -- we'd just apply
> the CVE fix to both the master branch (*after* the stable tag) and on
> the stable branch too. But our development workflow isn't there yet, so
> I guess we can delay the stable tag a bit more. I suggest updating the
> date in
> <https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning#edk2-stable201903-tag-planning>.

Is there anything else blocking the tag, at the moment?

Thanks
Laszlo

Re: [PATCH v2 0/2] Fix bugs in HiiDatabase driver

[Gao, Liming]

Laszlo:
  Thanks for your suggestion. There is no other. I have update Wiki page to delay 12 hour for this stable tag. I have pushed this CVE fix. I will create the stable tag quickly and send another announcement. 

Thanks
Liming
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Friday, March 8, 2019 4:51 AM
> To: Gao, Liming <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Ni, Ray <ray.ni@intel.com>; edk2-devel@lists.01.org;
> Kinney, Michael D <michael.d.kinney@intel.com>; afish@apple.com; Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Cetola, Stephano <stephano.cetola@intel.com>
> Subject: Re: [edk2] [PATCH v2 0/2] Fix bugs in HiiDatabase driver
> 
> On 03/08/19 09:52, Laszlo Ersek wrote:
> > On 03/08/19 07:41, Gao, Liming wrote:
> >> This is to fix the security issue. I agree it is an import bug fix. I am OK to push it for edk2-stable201903 tag
> >
> > Me too.
> >
> > If we had stable *branches* (as opposed to just stable tags), then we
> > wouldn't have to delay the stable tag (the release) -- we'd just apply
> > the CVE fix to both the master branch (*after* the stable tag) and on
> > the stable branch too. But our development workflow isn't there yet, so
> > I guess we can delay the stable tag a bit more. I suggest updating the
> > date in
> > <https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning#edk2-stable201903-tag-planning>.
> 
> Is there anything else blocking the tag, at the moment?
> 
> Thanks
> Laszlo