On 18 February 2013 16:56, Olivier Martin <olivier.mar...@arm.com> 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: “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).
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 <ryan.har...@linaro.org> 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.net/ > _______________________________________________ > 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 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.net/ _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
