For people following this critical issue, a commit has been submitted this morning (UTC time) into EDK2 (svn rev14145).
-----Original Message----- From: Kinney, Michael D [mailto:michael.d.kin...@intel.com] Sent: 19 February 2013 19:28 To: ryan.har...@linaro.org; edk2-devel@lists.sourceforge.net Cc: Harry Liebel Subject: Re: [edk2] [PATCH] MdeModulePkg/Core/Pei: Critical buffer overflow when loading TE Images Olivier, Thanks for finding this issue and providing the patch. We are reviewing and testing is now and will let you know the results soon. Mike -----Original Message----- From: Ryan Harkin [mailto:ryan.har...@linaro.org] Sent: Tuesday, February 19, 2013 12:31 AM To: edk2-devel@lists.sourceforge.net Cc: Tian, Feng; Kinney, Michael D; Harry Liebel Subject: Re: [edk2] [PATCH] MdeModulePkg/Core/Pei: Critical buffer overflow when loading TE Images 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/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 > ---------------------------------------------------------------------------- -- 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
