Thanks Eric,
gBS->AllocatePages() already returns an 4KB-aligned address.
I still think the fix you have done should go in BasePeCoffLib (in
PeCoffLoaderGetImageInfo() and PeCoffLoaderLoadImage()).
What I think is not correct is to copy the EFI_TE_IMAGE_HEADER just before
the sections, you should probably add the 'StrippedSize' padding between the
EFI_TE_IMAGE_HEADER and the first section. Potentially in your case,
EFI_TE_IMAGE_HEADER could not be aligned ...
But if you think your patch is the right thing to do then I am happy with
that!
And at least the allocated memory is now correct :-)
Regards,
Olivier
From: Dong, Eric [mailto:eric.d...@intel.com]
Sent: 19 February 2013 07:56
To: edk2-devel@lists.sourceforge.net; Tian, Feng; Kinney, Michael D
Cc: Harry Liebel
Subject: Re: [edk2] [PATCH] MdeModulePkg/Core/Pei: Critical buffer overflow
when loading TE Images
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.
Thanks,
Eric
From: Olivier Martin [mailto:olivier.mar...@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/4e844595f27ba8031b1b72fb3e3ab16bcf
246ebc)
# 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
