Re: [edk2] [PATCH] MdeModulePkg/Core/Pei: Critical buffer overflow when loading TE Images

Subject: Re: [edk2] [PATCH] MdeModulePkg/Core/Pei: Critical buffer overflow when loading TE Images

From: "Kinney, Michael D" <michael.d.kinney@intel.com>

To: Ryan Harkin <ryan.harkin@linaro.org>, "edk2-devel@lists.sourceforge.net" <edk2-devel@lists.sourceforge.net>

Date: 2013-02-20 03:27:48

Olivier,

Thanks for finding this issue and providing the patch.  We are reviewing and testing is now and will let you know the results soon.

Mike

-----Original Message-----
From: Ryan Harkin [mailto:ryan.harkin@linaro.org] 
Sent: Tuesday, February 19, 2013 12:31 AM
To: edk2-devel@lists.sourceforge.net
Cc: Tian, Feng; Kinney, Michael D; Harry Liebel
Subject: Re: [edk2] [PATCH] MdeModulePkg/Core/Pei: Critical buffer overflow when loading TE Images

Hi Dong,

On 19 February 2013 07:56, Dong, Eric  wrote:
> Oliver,
>
>
>
> In red mark code is used to fix the alignment issue raised in IPF platform,
> we can't remove it. For TE image, it also needs to make sure the section
> data is section alignment. So in my patch, i just expand the memory size to
> the PE image size for TE image. Attach my patch; please help to verify this
> patch.
>

Your patch also resolved my issue.  Again, I haven't tried to
understand the code, but I tested that it works, so you can have my
tag for what it's worth:

Tested-by: Ryan Harkin 

>
>
> Thanks,
>
> Eric
>
> From: Olivier Martin [mailto:olivier.martin@arm.com]
> Sent: Tuesday, February 19, 2013 12:57 AM
> To: Tian, Feng; Kinney, Michael D
> Cc: edk2-devel@lists.sourceforge.net; Harry Liebel
> Subject: [edk2] [PATCH] MdeModulePkg/Core/Pei: Critical buffer overflow when
> loading TE Images
>
>
>
> Dear MdePkg and MdeModulePkg maintainers,
>
>
>
> We found a buffer overflow in the TE image loading. This issue is
> architecture independent and could potentially crash the platform.
>
> The issue has been introduced by this commit: "Fix alignment requirement
> when Load IPF TeImage into memory"
> (https://github.com/tianocore/edk2/commit/4e844595f27ba8031b1b72fb3e3ab16bcf246ebc)
>
>
>
> # TE images are loaded by PEI Core during the PI phase with the function
> LoadAndRelocatePeCoffImage() (in MdeModulePkg/Core/Pei/Image/Image.c).
>
>
>
> LoadAndRelocatePeCoffImage() {
>
> // Get the Image information (including the total size of the TE image)
>
> PeCoffLoaderGetImageInfo (&ImageContext);
>
>
>
> // Allocate the memory for the image:
>
> ImageContext.ImageAddress = (EFI_PHYSICAL_ADDRESS)(UINTN) AllocatePages
> (EFI_SIZE_TO_PAGES ((UINT32) ImageContext.ImageSize));
>
>
>
> // Skip the reserved space for the stripped PeHeader when load TeImage into
> memory.
>
> if (ImageContext.IsTeImage) {
>
>   ImageContext.ImageAddress = ImageContext.ImageAddress +
>
>                               ((EFI_TE_IMAGE_HEADER *)
> Pe32Data)->StrippedSize -
>
>                               sizeof (EFI_TE_IMAGE_HEADER);
>
> }
>
>
>
> // Load the image to our new buffer
>
> Status = PeCoffLoaderLoadImage (&ImageContext);
>
> }
>
>
>
> # PeCoffLoaderGetImageInfo() (in MdePkg/Library/BasePeCoffLib) calculates
> the size of the not-yet loaded image including the size of the header
> (=sizeof(EFI_TE_IMAGE_HEADER))
>
>
>
> PeCoffLoaderGetImageInfo() {
>
> // Image Base Address
>
> TeStrippedOffset = (UINT32)Hdr.Te->StrippedSize - sizeof
> (EFI_TE_IMAGE_HEADER);
>
> ImageContext->ImageAddress = (PHYSICAL_ADDRESS)(Hdr.Te->ImageBase +
> TeStrippedOffset);
>
>
>
> // Calculate the size
>
> SectionHeaderOffset = (UINTN)(sizeof (EFI_TE_IMAGE_HEADER));
>
> for (Index = 0; Index < Hdr.Te->NumberOfSections;) {
>
>   (...)
>
>
>
>
>
>   //
>
>   // In Te image header there is not a field to describe the ImageSize.
>
>   // Actually, the ImageSize equals the RVA plus the VirtualSize of
>
>   // the last section mapped into memory (Must be rounded up to
>
>   // a multiple of Section Alignment). Per the PE/COFF specification, the
>
>   // section headers in the Section Table must appear in order of the RVA
>
>   // values for the corresponding sections. So the ImageSize can be
> determined
>
>   // by the RVA and the VirtualSize of the last section header in the
>
>   // Section Table.
>
>   //
>
>   if ((++Index) == (UINTN)Hdr.Te->NumberOfSections) {
>
>     ImageContext->ImageSize = (SectionHeader.VirtualAddress +
> SectionHeader.Misc.VirtualSize) - TeStrippedOffset;
>
>   }
>
> }
>
> }
>
>
>
> # And finally the TE image is loaded with the function
> PeCoffLoaderLoadImage() (in MdePkg/Library/BasePeCoffLib):
>
>
>
> PeCoffLoaderLoadImage() {
>
> BaseAddress = ImageContext->ImageAddress;
>
> // Read the TE header
>
> Status = ImageContext->ImageRead (
>
>                         ImageContext->Handle,
>
>                         0,
>
>                         &ImageContext->SizeOfHeaders,
>
>                         (void *)(UINTN)ImageContext->ImageAddress
>
>                         );
>
>
>
> FirstSection = (EFI_IMAGE_SECTION_HEADER *) (
>
>                       (UINTN)ImageContext->ImageAddress +
>
>                       sizeof(EFI_TE_IMAGE_HEADER)
>
>                       );
>
>
>
> Section = FirstSection;
>
> for (Index = 0; Index < NumberOfSections; Index++) {
>
>   Status = ImageContext->ImageRead (
>
>                             ImageContext->Handle,
>
>                             Section->PointerToRawData - TeStrippedOffset,
>
>                             &Size,
>
>                             Base
>
>                             );
>
> }
>
> }
>
>
>
> The faulty code is in RED. If we suppose the size which is calculated by
> PeCoffLoaderGetImageInfo() is correct and includes the TE image header then
> changing the base address of the (not-yet) loaded image will potentially
> create an overflow (as we load the TE image header from this new base
> address).
>
>
>
> At the least the code in RED must be removed. But some additional code might
> be needed to fix the alignment of the code section and the image size
> calculation to take in account this alignment.
>
> But these fixes would go into MdePkg/Library/BasePeCoffLib.
>
>
>
> The reason why it generally works is because the allocated region is rounded
> to a EFI_PAGE_SIZE granularity. So, we could have some spare bytes for the
> overflow.
>
> The PI module that was causing the overflow for us is PEI Core. With our
> toolchain, PEI core is almost exactly 122880Bytes = (EFI_PAGE_SIZE * 30).
>
>
>
> Regards,
>
> Olivier
>
>
> ------------------------------------------------------------------------------
> Everyone hates slow websites. So do we.
> Make your web apps faster with AppDynamics
> Download AppDynamics Lite for free today:
> http://p.sf.net/sfu/appdyn_d2d_feb
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/edk2-devel
>

------------------------------------------------------------------------------
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
Download AppDynamics Lite for free today:
http://p.sf.net/sfu/appdyn_d2d_feb
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel