MdePkg: PE loader should zero out dest buffer on allocation

[edk2-devel] [PATCH] MdePkg: PE loader should zero out dest buffer on allocation

Posted by Zhiguang Liu 4 years, 3 months ago

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

When PE loader loads image to memory, the first section of image may
not locate right next to the image header, which causes some memory
space remaining uninitialized. This is a security issue.
This patch compares the ending address of image header and the beginning
address of the first section. If there is a gap, zero out this gap.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
 MdePkg/Library/BasePeCoffLib/BasePeCoff.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
index 07bb62f860..2cdfb4a082 100644
--- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
+++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
@@ -1306,6 +1306,14 @@ PeCoffLoaderLoadImage (
   // Load each section of the image
   //
   Section = FirstSection;
+  //
+  // Zero out the memory space between image header and the first section
+  //
+  End  = (CHAR8 *)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders);
+  Base = PeCoffLoaderImageAddress (ImageContext, Section->VirtualAddress, TeStrippedOffset);
+  if (End < Base) {
+    ZeroMem (End, Base - End);
+  }
   for (Index = 0; Index < NumberOfSections; Index++) {
     //
     // Read the section
-- 
2.16.2.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53185): https://edk2.groups.io/g/devel/message/53185
Mute This Topic: https://groups.io/mt/69664451/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] MdePkg: PE loader should zero out dest buffer on allocation

Posted by Marvin Häuser 4 years, 3 months ago

Good day,

Please see my comment in the related BZ: 
https://bugzilla.tianocore.org/show_bug.cgi?id=1999#c5

Best regards,
Marvin

Am 13.01.2020 um 09:18 schrieb Zhiguang Liu:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1999
> 
> When PE loader loads image to memory, the first section of image may
> not locate right next to the image header, which causes some memory
> space remaining uninitialized. This is a security issue.
> This patch compares the ending address of image header and the beginning
> address of the first section. If there is a gap, zero out this gap.
> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> ---
>   MdePkg/Library/BasePeCoffLib/BasePeCoff.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> index 07bb62f860..2cdfb4a082 100644
> --- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> +++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> @@ -1306,6 +1306,14 @@ PeCoffLoaderLoadImage (
>     // Load each section of the image
>     //
>     Section = FirstSection;
> +  //
> +  // Zero out the memory space between image header and the first section
> +  //
> +  End  = (CHAR8 *)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders);
> +  Base = PeCoffLoaderImageAddress (ImageContext, Section->VirtualAddress, TeStrippedOffset);
> +  if (End < Base) {
> +    ZeroMem (End, Base - End);
> +  }
>     for (Index = 0; Index < NumberOfSections; Index++) {
>       //
>       // Read the section
> 

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53186): https://edk2.groups.io/g/devel/message/53186
Mute This Topic: https://groups.io/mt/69664451/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] MdePkg: PE loader should zero out dest buffer on allocation

Posted by Laszlo Ersek 4 years, 3 months ago

On 01/13/20 09:18, Zhiguang Liu wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1999
> 
> When PE loader loads image to memory, the first section of image may
> not locate right next to the image header, which causes some memory
> space remaining uninitialized. This is a security issue.
> This patch compares the ending address of image header and the beginning
> address of the first section. If there is a gap, zero out this gap.
> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> ---
>  MdePkg/Library/BasePeCoffLib/BasePeCoff.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> index 07bb62f860..2cdfb4a082 100644
> --- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> +++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> @@ -1306,6 +1306,14 @@ PeCoffLoaderLoadImage (
>    // Load each section of the image
>    //
>    Section = FirstSection;
> +  //
> +  // Zero out the memory space between image header and the first section
> +  //
> +  End  = (CHAR8 *)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders);
> +  Base = PeCoffLoaderImageAddress (ImageContext, Section->VirtualAddress, TeStrippedOffset);
> +  if (End < Base) {
> +    ZeroMem (End, Base - End);
> +  }
>    for (Index = 0; Index < NumberOfSections; Index++) {
>      //
>      // Read the section
> 

CC'ing Marvin

Thanks
Laszlo


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53196): https://edk2.groups.io/g/devel/message/53196
Mute This Topic: https://groups.io/mt/69664451/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-