Hi Dong, On 19 February 2013 07:56, Dong, Eric <eric.d...@intel.com> 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 <ryan.har...@linaro.org> > > > 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/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
