From 9e68568e34bef0037bb16b3cbe361e559b8da369 Mon Sep 17 00:00:00 2001 From: Laszlo Ersek 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é Acked-by: Vitaly Kuznetsov From: Ray Ni --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 . 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 , 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 Cc: Dandan Bi Cc: Hao A Wu Reviewed-by: Hao Wu Reviewed-by: Jian J Wang (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