[edk2] MdeModulePkg/BootGraphicsResourceTableDxe: don't allocate below 4 GB

Message ID 1489943988-15378-1-git-send-email-ard.biesheuvel@linaro.org
State Accepted
Commit 09da11081915dea25d2d1bba87cdf460102e52bb
Headers show

Commit Message

Ard Biesheuvel March 19, 2017, 5:19 p.m.
The BGRT table has an 8 byte field for the memory address of the image
data, and yet the driver explicitly allocates below 4 GB. This results
in an ASSERT() on systems that do not have any memory below 4 GB to begin
with.

Since neither the PI, the UEFI or the ACPI spec contain any mention of
why this data should reside below 4 GB, replace the allocation call
with an ordinary AllocatePages() call.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
 MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.c | 41 ++------------------
 1 file changed, 3 insertions(+), 38 deletions(-)

-- 
2.7.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Zeng, Star March 20, 2017, 1:09 a.m. | #1
I am ok with this patch.

Jiewen and Liming, do you have any comments?

Thanks,
Star
-----Original Message-----
From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] 

Sent: Monday, March 20, 2017 1:20 AM
To: edk2-devel@lists.01.org; Zeng, Star <star.zeng@intel.com>; Tian, Feng <feng.tian@intel.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Subject: [PATCH] MdeModulePkg/BootGraphicsResourceTableDxe: don't allocate below 4 GB

The BGRT table has an 8 byte field for the memory address of the image data, and yet the driver explicitly allocates below 4 GB. This results in an ASSERT() on systems that do not have any memory below 4 GB to begin with.

Since neither the PI, the UEFI or the ACPI spec contain any mention of why this data should reside below 4 GB, replace the allocation call with an ordinary AllocatePages() call.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
 MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.c | 41 ++------------------
 1 file changed, 3 insertions(+), 38 deletions(-)

diff --git a/MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.c b/MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.c
index 804ffa5a6bb6..6a7165a95489 100644
--- a/MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.c
+++ b/MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraph
+++ icsResourceTableDxe.c
@@ -227,43 +227,6 @@ BgrtAcpiTableChecksum (  }
 
 /**
-  Allocate EfiBootServicesData below 4G memory address.
-
-  This function allocates EfiBootServicesData below 4G memory address.
-
-  @param[in]  Size   Size of memory to allocate.
-
-  @return Allocated address for output.
-
-**/
-VOID *
-BgrtAllocateBsDataMemoryBelow4G (
-  IN UINTN       Size
-  )
-{
-  UINTN                 Pages;
-  EFI_PHYSICAL_ADDRESS  Address;
-  EFI_STATUS            Status;
-  VOID                  *Buffer;
-
-  Pages   = EFI_SIZE_TO_PAGES (Size);
-  Address = 0xffffffff;
-
-  Status = gBS->AllocatePages (
-                  AllocateMaxAddress,
-                  EfiBootServicesData,
-                  Pages,
-                  &Address
-                  );
-  ASSERT_EFI_ERROR (Status);
-
-  Buffer = (VOID *) (UINTN) Address;
-  ZeroMem (Buffer, Size);
-
-  return Buffer;
-}
-
-/**
   Install Boot Graphics Resource Table to ACPI table.
 
   @return Status code.
@@ -358,11 +321,13 @@ InstallBootGraphicsResourceTable (
     // The image should be stored in EfiBootServicesData, allowing the system to reclaim the memory
     //
     BmpSize = (mLogoWidth * 3 + PaddingSize) * mLogoHeight + sizeof (BMP_IMAGE_HEADER);
-    ImageBuffer = BgrtAllocateBsDataMemoryBelow4G (BmpSize);
+    ImageBuffer = AllocatePages (EFI_SIZE_TO_PAGES (BmpSize));
     if (ImageBuffer == NULL) {
       return EFI_OUT_OF_RESOURCES;
     }
 
+    ZeroMem (ImageBuffer, BmpSize);
+
     mBmpImageHeaderTemplate.Size = (UINT32) BmpSize;
     mBmpImageHeaderTemplate.ImageSize = (UINT32) BmpSize - sizeof (BMP_IMAGE_HEADER);
     mBmpImageHeaderTemplate.PixelWidth = (UINT32) mLogoWidth;
--
2.7.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Gao, Liming March 21, 2017, 2:37 a.m. | #2
Yes. Spec has no such limitation. I agree this change. 

Reviewed-by: Liming Gao <liming.gao@intel.com>


Thanks
Liming
>-----Original Message-----

>From: Zeng, Star

>Sent: Monday, March 20, 2017 9:10 AM

>To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; edk2-devel@lists.01.org;

>Tian, Feng <feng.tian@intel.com>

>Cc: Yao, Jiewen <jiewen.yao@intel.com>; Gao, Liming

><liming.gao@intel.com>; Zeng, Star <star.zeng@intel.com>

>Subject: RE: [PATCH] MdeModulePkg/BootGraphicsResourceTableDxe: don't

>allocate below 4 GB

>

>I am ok with this patch.

>

>Jiewen and Liming, do you have any comments?

>

>Thanks,

>Star

>-----Original Message-----

>From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]

>Sent: Monday, March 20, 2017 1:20 AM

>To: edk2-devel@lists.01.org; Zeng, Star <star.zeng@intel.com>; Tian, Feng

><feng.tian@intel.com>

>Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>Subject: [PATCH] MdeModulePkg/BootGraphicsResourceTableDxe: don't

>allocate below 4 GB

>

>The BGRT table has an 8 byte field for the memory address of the image data,

>and yet the driver explicitly allocates below 4 GB. This results in an ASSERT()

>on systems that do not have any memory below 4 GB to begin with.

>

>Since neither the PI, the UEFI or the ACPI spec contain any mention of why

>this data should reside below 4 GB, replace the allocation call with an ordinary

>AllocatePages() call.

>

>Contributed-under: TianoCore Contribution Agreement 1.0

>Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>---

>

>MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphi

>csResourceTableDxe.c | 41 ++------------------

> 1 file changed, 3 insertions(+), 38 deletions(-)

>

>diff --git

>a/MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGrap

>hicsResourceTableDxe.c

>b/MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGrap

>hicsResourceTableDxe.c

>index 804ffa5a6bb6..6a7165a95489 100644

>---

>a/MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGrap

>hicsResourceTableDxe.c

>+++

>b/MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGrap

>h

>+++ icsResourceTableDxe.c

>@@ -227,43 +227,6 @@ BgrtAcpiTableChecksum (  }

>

> /**

>-  Allocate EfiBootServicesData below 4G memory address.

>-

>-  This function allocates EfiBootServicesData below 4G memory address.

>-

>-  @param[in]  Size   Size of memory to allocate.

>-

>-  @return Allocated address for output.

>-

>-**/

>-VOID *

>-BgrtAllocateBsDataMemoryBelow4G (

>-  IN UINTN       Size

>-  )

>-{

>-  UINTN                 Pages;

>-  EFI_PHYSICAL_ADDRESS  Address;

>-  EFI_STATUS            Status;

>-  VOID                  *Buffer;

>-

>-  Pages   = EFI_SIZE_TO_PAGES (Size);

>-  Address = 0xffffffff;

>-

>-  Status = gBS->AllocatePages (

>-                  AllocateMaxAddress,

>-                  EfiBootServicesData,

>-                  Pages,

>-                  &Address

>-                  );

>-  ASSERT_EFI_ERROR (Status);

>-

>-  Buffer = (VOID *) (UINTN) Address;

>-  ZeroMem (Buffer, Size);

>-

>-  return Buffer;

>-}

>-

>-/**

>   Install Boot Graphics Resource Table to ACPI table.

>

>   @return Status code.

>@@ -358,11 +321,13 @@ InstallBootGraphicsResourceTable (

>     // The image should be stored in EfiBootServicesData, allowing the system

>to reclaim the memory

>     //

>     BmpSize = (mLogoWidth * 3 + PaddingSize) * mLogoHeight + sizeof

>(BMP_IMAGE_HEADER);

>-    ImageBuffer = BgrtAllocateBsDataMemoryBelow4G (BmpSize);

>+    ImageBuffer = AllocatePages (EFI_SIZE_TO_PAGES (BmpSize));

>     if (ImageBuffer == NULL) {

>       return EFI_OUT_OF_RESOURCES;

>     }

>

>+    ZeroMem (ImageBuffer, BmpSize);

>+

>     mBmpImageHeaderTemplate.Size = (UINT32) BmpSize;

>     mBmpImageHeaderTemplate.ImageSize = (UINT32) BmpSize - sizeof

>(BMP_IMAGE_HEADER);

>     mBmpImageHeaderTemplate.PixelWidth = (UINT32) mLogoWidth;

>--

>2.7.4


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel March 21, 2017, 7:11 a.m. | #3
On 21 March 2017 at 02:37, Gao, Liming <liming.gao@intel.com> wrote:
> Yes. Spec has no such limitation. I agree this change.

>

> Reviewed-by: Liming Gao <liming.gao@intel.com>

>


Pushed as 09da11081915, thanks.

> Thanks

> Liming

>>-----Original Message-----

>>From: Zeng, Star

>>Sent: Monday, March 20, 2017 9:10 AM

>>To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; edk2-devel@lists.01.org;

>>Tian, Feng <feng.tian@intel.com>

>>Cc: Yao, Jiewen <jiewen.yao@intel.com>; Gao, Liming

>><liming.gao@intel.com>; Zeng, Star <star.zeng@intel.com>

>>Subject: RE: [PATCH] MdeModulePkg/BootGraphicsResourceTableDxe: don't

>>allocate below 4 GB

>>

>>I am ok with this patch.

>>

>>Jiewen and Liming, do you have any comments?

>>

>>Thanks,

>>Star

>>-----Original Message-----

>>From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]

>>Sent: Monday, March 20, 2017 1:20 AM

>>To: edk2-devel@lists.01.org; Zeng, Star <star.zeng@intel.com>; Tian, Feng

>><feng.tian@intel.com>

>>Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>>Subject: [PATCH] MdeModulePkg/BootGraphicsResourceTableDxe: don't

>>allocate below 4 GB

>>

>>The BGRT table has an 8 byte field for the memory address of the image data,

>>and yet the driver explicitly allocates below 4 GB. This results in an ASSERT()

>>on systems that do not have any memory below 4 GB to begin with.

>>

>>Since neither the PI, the UEFI or the ACPI spec contain any mention of why

>>this data should reside below 4 GB, replace the allocation call with an ordinary

>>AllocatePages() call.

>>

>>Contributed-under: TianoCore Contribution Agreement 1.0

>>Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>>---

>>

>>MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphi

>>csResourceTableDxe.c | 41 ++------------------

>> 1 file changed, 3 insertions(+), 38 deletions(-)

>>

>>diff --git

>>a/MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGrap

>>hicsResourceTableDxe.c

>>b/MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGrap

>>hicsResourceTableDxe.c

>>index 804ffa5a6bb6..6a7165a95489 100644

>>---

>>a/MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGrap

>>hicsResourceTableDxe.c

>>+++

>>b/MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGrap

>>h

>>+++ icsResourceTableDxe.c

>>@@ -227,43 +227,6 @@ BgrtAcpiTableChecksum (  }

>>

>> /**

>>-  Allocate EfiBootServicesData below 4G memory address.

>>-

>>-  This function allocates EfiBootServicesData below 4G memory address.

>>-

>>-  @param[in]  Size   Size of memory to allocate.

>>-

>>-  @return Allocated address for output.

>>-

>>-**/

>>-VOID *

>>-BgrtAllocateBsDataMemoryBelow4G (

>>-  IN UINTN       Size

>>-  )

>>-{

>>-  UINTN                 Pages;

>>-  EFI_PHYSICAL_ADDRESS  Address;

>>-  EFI_STATUS            Status;

>>-  VOID                  *Buffer;

>>-

>>-  Pages   = EFI_SIZE_TO_PAGES (Size);

>>-  Address = 0xffffffff;

>>-

>>-  Status = gBS->AllocatePages (

>>-                  AllocateMaxAddress,

>>-                  EfiBootServicesData,

>>-                  Pages,

>>-                  &Address

>>-                  );

>>-  ASSERT_EFI_ERROR (Status);

>>-

>>-  Buffer = (VOID *) (UINTN) Address;

>>-  ZeroMem (Buffer, Size);

>>-

>>-  return Buffer;

>>-}

>>-

>>-/**

>>   Install Boot Graphics Resource Table to ACPI table.

>>

>>   @return Status code.

>>@@ -358,11 +321,13 @@ InstallBootGraphicsResourceTable (

>>     // The image should be stored in EfiBootServicesData, allowing the system

>>to reclaim the memory

>>     //

>>     BmpSize = (mLogoWidth * 3 + PaddingSize) * mLogoHeight + sizeof

>>(BMP_IMAGE_HEADER);

>>-    ImageBuffer = BgrtAllocateBsDataMemoryBelow4G (BmpSize);

>>+    ImageBuffer = AllocatePages (EFI_SIZE_TO_PAGES (BmpSize));

>>     if (ImageBuffer == NULL) {

>>       return EFI_OUT_OF_RESOURCES;

>>     }

>>

>>+    ZeroMem (ImageBuffer, BmpSize);

>>+

>>     mBmpImageHeaderTemplate.Size = (UINT32) BmpSize;

>>     mBmpImageHeaderTemplate.ImageSize = (UINT32) BmpSize - sizeof

>>(BMP_IMAGE_HEADER);

>>     mBmpImageHeaderTemplate.PixelWidth = (UINT32) mLogoWidth;

>>--

>>2.7.4

>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patch

diff --git a/MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.c b/MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.c
index 804ffa5a6bb6..6a7165a95489 100644
--- a/MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.c
+++ b/MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.c
@@ -227,43 +227,6 @@  BgrtAcpiTableChecksum (
 }
 
 /**
-  Allocate EfiBootServicesData below 4G memory address.
-
-  This function allocates EfiBootServicesData below 4G memory address.
-
-  @param[in]  Size   Size of memory to allocate.
-
-  @return Allocated address for output.
-
-**/
-VOID *
-BgrtAllocateBsDataMemoryBelow4G (
-  IN UINTN       Size
-  )
-{
-  UINTN                 Pages;
-  EFI_PHYSICAL_ADDRESS  Address;
-  EFI_STATUS            Status;
-  VOID                  *Buffer;
-
-  Pages   = EFI_SIZE_TO_PAGES (Size);
-  Address = 0xffffffff;
-
-  Status = gBS->AllocatePages (
-                  AllocateMaxAddress,
-                  EfiBootServicesData,
-                  Pages,
-                  &Address
-                  );
-  ASSERT_EFI_ERROR (Status);
-
-  Buffer = (VOID *) (UINTN) Address;
-  ZeroMem (Buffer, Size);
-
-  return Buffer;
-}
-
-/**
   Install Boot Graphics Resource Table to ACPI table.
 
   @return Status code.
@@ -358,11 +321,13 @@  InstallBootGraphicsResourceTable (
     // The image should be stored in EfiBootServicesData, allowing the system to reclaim the memory
     //
     BmpSize = (mLogoWidth * 3 + PaddingSize) * mLogoHeight + sizeof (BMP_IMAGE_HEADER);
-    ImageBuffer = BgrtAllocateBsDataMemoryBelow4G (BmpSize);
+    ImageBuffer = AllocatePages (EFI_SIZE_TO_PAGES (BmpSize));
     if (ImageBuffer == NULL) {
       return EFI_OUT_OF_RESOURCES;
     }
 
+    ZeroMem (ImageBuffer, BmpSize);
+
     mBmpImageHeaderTemplate.Size = (UINT32) BmpSize;
     mBmpImageHeaderTemplate.ImageSize = (UINT32) BmpSize - sizeof (BMP_IMAGE_HEADER);
     mBmpImageHeaderTemplate.PixelWidth = (UINT32) mLogoWidth;