* Re: [PATCH 0/2] Fix bugs in HiiDatabase driver
2019-03-08 2:21 [PATCH 0/2] Fix bugs in HiiDatabase driver Ray Ni
@ 2019-03-08 2:20 ` Gao, Liming
2019-03-08 2:21 ` [PATCH 1/2] MdeModulePkg/HiiDatabase: Fix potential integer overflow Ray Ni
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Gao, Liming @ 2019-03-08 2:20 UTC (permalink / raw)
To: Ni, Ray, edk2-devel@lists.01.org
Please follow CVE format in https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-Format
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ray Ni
> Sent: Thursday, March 7, 2019 6:21 PM
> To: edk2-devel@lists.01.org
> Subject: [edk2] [PATCH 0/2] Fix bugs in HiiDatabase driver
>
> Ray Ni (2):
> MdeModulePkg/HiiDatabase: Fix potential integer overflow
> MdeModulePkg/HiiImage: Fix stack overflow when corrupted BMP is parsed
>
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 0/2] Fix bugs in HiiDatabase driver
@ 2019-03-08 2:21 Ray Ni
2019-03-08 2:20 ` Gao, Liming
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: Ray Ni @ 2019-03-08 2:21 UTC (permalink / raw)
To: edk2-devel
Ray Ni (2):
MdeModulePkg/HiiDatabase: Fix potential integer overflow
MdeModulePkg/HiiImage: Fix stack overflow when corrupted BMP is parsed
MdeModulePkg/Universal/HiiDatabaseDxe/Image.c | 130 ++++++++++++++----
1 file changed, 105 insertions(+), 25 deletions(-)
--
2.20.1.windows.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] MdeModulePkg/HiiDatabase: Fix potential integer overflow
2019-03-08 2:21 [PATCH 0/2] Fix bugs in HiiDatabase driver Ray Ni
2019-03-08 2:20 ` Gao, Liming
@ 2019-03-08 2:21 ` Ray Ni
2019-03-08 2:21 ` [PATCH 2/2] MdeModulePkg/HiiImage: Fix stack overflow when corrupted BMP is parsed Ray Ni
2019-03-08 2:21 ` [PATCH 0/2] Fix bugs in HiiDatabase driver Wu, Hao A
3 siblings, 0 replies; 6+ messages in thread
From: Ray Ni @ 2019-03-08 2:21 UTC (permalink / raw)
To: edk2-devel; +Cc: Dandan Bi, Hao A Wu
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1135
CVE number: CVE-2018-12181
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] MdeModulePkg/HiiImage: Fix stack overflow when corrupted BMP is parsed
2019-03-08 2:21 [PATCH 0/2] Fix bugs in HiiDatabase driver Ray Ni
2019-03-08 2:20 ` Gao, Liming
2019-03-08 2:21 ` [PATCH 1/2] MdeModulePkg/HiiDatabase: Fix potential integer overflow Ray Ni
@ 2019-03-08 2:21 ` Ray Ni
2019-03-08 2:21 ` [PATCH 0/2] Fix bugs in HiiDatabase driver Wu, Hao A
3 siblings, 0 replies; 6+ messages in thread
From: Ray Ni @ 2019-03-08 2:21 UTC (permalink / raw)
To: edk2-devel; +Cc: Liming Gao, Jiewen Yao
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1135
CVE number: CVE-2018-12181
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 0/2] Fix bugs in HiiDatabase driver
2019-03-08 2:21 [PATCH 0/2] Fix bugs in HiiDatabase driver Ray Ni
` (2 preceding siblings ...)
2019-03-08 2:21 ` [PATCH 2/2] MdeModulePkg/HiiImage: Fix stack overflow when corrupted BMP is parsed Ray Ni
@ 2019-03-08 2:21 ` Wu, Hao A
2019-03-08 2:33 ` Ni, Ray
3 siblings, 1 reply; 6+ messages in thread
From: Wu, Hao A @ 2019-03-08 2:21 UTC (permalink / raw)
To: Ni, Ray, edk2-devel@lists.01.org
Quick comment, please add the CVE number in the patch subject.
Liming has already documented the new rule for this kind of fix:
https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-Format
Best Regards,
Hao Wu
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Ray Ni
> Sent: Friday, March 08, 2019 10:21 AM
> To: edk2-devel@lists.01.org
> Subject: [edk2] [PATCH 0/2] Fix bugs in HiiDatabase driver
>
> Ray Ni (2):
> MdeModulePkg/HiiDatabase: Fix potential integer overflow
> MdeModulePkg/HiiImage: Fix stack overflow when corrupted BMP is
> parsed
>
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/2] Fix bugs in HiiDatabase driver
2019-03-08 2:21 ` [PATCH 0/2] Fix bugs in HiiDatabase driver Wu, Hao A
@ 2019-03-08 2:33 ` Ni, Ray
0 siblings, 0 replies; 6+ messages in thread
From: Ni, Ray @ 2019-03-08 2:33 UTC (permalink / raw)
To: Wu, Hao A, edk2-devel@lists.01.org
Thanks for the comments.
Sent out V2 with correct patch subject.
> -----Original Message-----
> From: Wu, Hao A
> Sent: Friday, March 8, 2019 10:22 AM
> To: Ni, Ray <ray.ni@intel.com>; edk2-devel@lists.01.org
> Subject: RE: [edk2] [PATCH 0/2] Fix bugs in HiiDatabase driver
>
> Quick comment, please add the CVE number in the patch subject.
>
> Liming has already documented the new rule for this kind of fix:
> https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-
> Format
>
> Best Regards,
> Hao Wu
>
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> > Ray Ni
> > Sent: Friday, March 08, 2019 10:21 AM
> > To: edk2-devel@lists.01.org
> > Subject: [edk2] [PATCH 0/2] Fix bugs in HiiDatabase driver
> >
> > Ray Ni (2):
> > MdeModulePkg/HiiDatabase: Fix potential integer overflow
> > MdeModulePkg/HiiImage: Fix stack overflow when corrupted BMP is
> > parsed
> >
> > 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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-03-08 2:33 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-03-08 2:21 [PATCH 0/2] Fix bugs in HiiDatabase driver Ray Ni
2019-03-08 2:20 ` Gao, Liming
2019-03-08 2:21 ` [PATCH 1/2] MdeModulePkg/HiiDatabase: Fix potential integer overflow Ray Ni
2019-03-08 2:21 ` [PATCH 2/2] MdeModulePkg/HiiImage: Fix stack overflow when corrupted BMP is parsed Ray Ni
2019-03-08 2:21 ` [PATCH 0/2] Fix bugs in HiiDatabase driver Wu, Hao A
2019-03-08 2:33 ` Ni, Ray
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox