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