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: Ryan Harkin <ryan.harkin@linaro.org>

To: edk2-devel@lists.sourceforge.net

Date: 2013-02-19 01:50:59

On 18 February 2013 16:56, Olivier Martin  wrote:
> 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: =93Fix alignment requirement
> when Load IPF TeImage into memory=94
> (https://github.com/tianocore/edk2/commit/4e844595f27ba8031b1b72fb3e3ab16=
bcf246ebc)
>
>
>
> # 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 =3D (EFI_PHYSICAL_ADDRESS)(UINTN) AllocatePages
> (EFI_SIZE_TO_PAGES ((UINT32) ImageContext.ImageSize));
>
>
>
> // Skip the reserved space for the stripped PeHeader when load TeImage in=
to
> memory.
>
> if (ImageContext.IsTeImage) {
>
>   ImageContext.ImageAddress =3D ImageContext.ImageAddress +
>
>                               ((EFI_TE_IMAGE_HEADER *)
> Pe32Data)->StrippedSize -
>
>                               sizeof (EFI_TE_IMAGE_HEADER);
>
> }
>
>
>
> // Load the image to our new buffer
>
> Status =3D PeCoffLoaderLoadImage (&ImageContext);
>
> }
>
>
>
> # PeCoffLoaderGetImageInfo() (in MdePkg/Library/BasePeCoffLib) calculates
> the size of the not-yet loaded image including the size of the header
> (=3Dsizeof(EFI_TE_IMAGE_HEADER))
>
>
>
> PeCoffLoaderGetImageInfo() {
>
> // Image Base Address
>
> TeStrippedOffset =3D (UINT32)Hdr.Te->StrippedSize - sizeof
> (EFI_TE_IMAGE_HEADER);
>
> ImageContext->ImageAddress =3D (PHYSICAL_ADDRESS)(Hdr.Te->ImageBase +
> TeStrippedOffset);
>
>
>
> // Calculate the size
>
> SectionHeaderOffset =3D (UINTN)(sizeof (EFI_TE_IMAGE_HEADER));
>
> for (Index =3D 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) =3D=3D (UINTN)Hdr.Te->NumberOfSections) {
>
>     ImageContext->ImageSize =3D (SectionHeader.VirtualAddress +
> SectionHeader.Misc.VirtualSize) - TeStrippedOffset;
>
>   }
>
> }
>
> }
>
>
>
> # And finally the TE image is loaded with the function
> PeCoffLoaderLoadImage() (in MdePkg/Library/BasePeCoffLib):
>
>
>
> PeCoffLoaderLoadImage() {
>
> BaseAddress =3D ImageContext->ImageAddress;
>
> // Read the TE header
>
> Status =3D ImageContext->ImageRead (
>
>                         ImageContext->Handle,
>
>                         0,
>
>                         &ImageContext->SizeOfHeaders,
>
>                         (void *)(UINTN)ImageContext->ImageAddress
>
>                         );
>
>
>
> FirstSection =3D (EFI_IMAGE_SECTION_HEADER *) (
>
>                       (UINTN)ImageContext->ImageAddress +
>
>                       sizeof(EFI_TE_IMAGE_HEADER)
>
>                       );
>
>
>
> Section =3D FirstSection;
>
> for (Index =3D 0; Index < NumberOfSections; Index++) {
>
>   Status =3D 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 th=
en
> 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 mi=
ght
> 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 roun=
ded
> 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 =3D (EFI_PAGE_SIZE * 30=
).

I have also been suffering from this problem

https://bugs.launchpad.net/linaro-uefi/+bug/1116451

... and the attached patch fixes it.

>
>
>
> Regards,
>
> Olivier
>

I'll add my tag:

Tested-by: Ryan Harkin 


My only comment is that the title field in the attached patch is not a
nice as the subject line in this email.  I haven't attempted to
understand the code or the fix.


>
> -------------------------------------------------------------------------=
-----
> The Go Parallel Website, sponsored by Intel - in partnership with Geeknet,
> is your hub for all things parallel software development, from weekly
> thought
> leadership blogs to news, videos, case studies, tutorials, tech docs,
> whitepapers, evaluation guides, and opinion stories. Check out the most
> recent posts - join the conversation now. http://goparallel.sourceforge.n=
et/
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/edk2-devel
>

---------------------------------------------------------------------------=
---
The Go Parallel Website, sponsored by Intel - in partnership with Geeknet, =

is your hub for all things parallel software development, from weekly thoug=
ht =

leadership blogs to news, videos, case studies, tutorials, tech docs, =

whitepapers, evaluation guides, and opinion stories. Check out the most =

recent posts - join the conversation now. http://goparallel.sourceforge.net/
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel