Olivier, For section alignment, it only requires the section data begin at the Section Alignment address, not require TE image header also section alignment. When generate PE image, the compiler has make sure the section data is section alignment to the PE file's base address when load into memory. So if we make sure the base address of the PE image is section alignment, all the section data are section alignment. For TE image, we just make sure the PE image that this TE image converts from is section alignment; all the section data will section alignment.
Based on the above analysis, I update the patch; please help to verify the new patch. Thanks, Eric From: Olivier Martin [mailto:olivier.mar...@arm.com] Sent: Tuesday, February 19, 2013 5:49 PM 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 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]<mailto:[mailto:eric.d...@intel.com]> Sent: 19 February 2013 07:56 To: edk2-devel@lists.sourceforge.net<mailto: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<mailto: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
Image.c.patch
Description: Image.c.patch
------------------------------------------------------------------------------ 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
