On 02/21/14 19:44, Andrew Fish wrote: > > On Feb 21, 2014, at 10:17 AM, Laszlo Ersek <[email protected] > <mailto:[email protected]>> wrote: > >> On 02/21/14 16:12, Ryan Harkin wrote: >>> Hi Olivier, >>> >>> I've just noticed that the upstream EDK2 repository for the FVP AEMv8 >>> model is broken when built with Linaro GCC 13.12 onwards. >>> >>> The error I see is: >>> >>> UEFI firmware (version built at 14:54:24 on Feb 21 2014) >>> add-symbol-file >>> /linaro/uefi/master/upstream/edk2.git/Build/ArmVExpress-FVP-AArch64/DEBUG_ARMGCC/AARCH64/ArmPlatformPkg/PrePi/PeiMPCore/DEBUG/ArmPlatformPrePiMPCore.dll >>> 0x88000780 >>> Decompress Failed - Not Found >>> >>> ASSERT_EFI_ERROR (Status = Not Found) >>> ASSERT >>> /linaro/uefi/master/upstream/edk2.git/ArmPlatformPkg/PrePi/PrePi.c(194): >>> !EFI_ERROR (Status) >>> >>> I've tracked the bug as far as function "FfsProcessSection" [1] where >>> at line 373, it calls into function "ExtractGuidedSectionDecode" [2] >>> which then calls into "SavedData->ExtractDecodeHandlerTable [Index]". >>> At that point, I can't work out where it goes. >>> >>> I can "fix" the problem if I re-org the variables at the top of >>> FfsProcessSection so that DstBuffer is at the start of the >>> declarations. That is obviously not a fix. But it will probably hint >>> at why the subsequent code is broken. >>> >>> Cheers, >>> Ryan >>> >>> [1] EmbeddedPkg/Library/PrePiLib/FwVol.c, line 285 >>> [2] >>> EmbeddedPkg/Library/PrePiExtractGuidedSectionLib/PrePiExtractGuidedSectionLib.c:166 >> >> Here's a random shot (could be completely unrelated): >> >> >> EFI_STATUS >> FfsProcessSection ( >> IN EFI_SECTION_TYPE SectionType, >> IN EFI_COMMON_SECTION_HEADER *Section, >> IN UINTN SectionSize, >> OUT VOID **OutputBuffer >> ) >> { >> /* ... */ >> UINTN DstBufferSize; >> /* ... */ >> >> You're on AArch64, so UINTN means UINT64. >> >> Note that this "auto" variable is not initialized, hence its contents >> are indeterminate. Fast forward to the next use: >> >> Status = UefiDecompressGetInfo ( >> (UINT8 *) ((EFI_COMPRESSION_SECTION *) Section + 1), >> (UINT32) SectionLength - sizeof >> (EFI_COMPRESSION_SECTION), >> (UINT32 *) &DstBufferSize, >> &ScratchBufferSize >> ); >> } else if (Section->Type == EFI_SECTION_GUID_DEFINED) { >> Status = ExtractGuidedSectionGetInfo ( >> Section, >> (UINT32 *) &DstBufferSize, >> &ScratchBufferSize, >> &SectionAttribute >> ); >> >> Whichever of these functions is invoked, it fills in only four bytes >> (UINT32). Then, >> >> DstBuffer = (VOID *)(UINTN)AllocatePages (EFI_SIZE_TO_PAGES >> (DstBufferSize) + 1); >> >> etc etc etc. >> >> By reordering the local variables, you could be limiting the nonzero >> garbage in "DstBufferSize" to those four bytes that *are* ultimately >> overwritten. >> >> I guess initing DstBufferSize to zero is easy enough to try... :) >> > > Why not just make it UINT32 and remove the casts. A real fix!
I considered that, but didn't propose it, because it could have made a difference somewhere down the line. For example, if DstBufferSize were multiplied by an UINT32, then the product's type (== range) would be affected (on 64-bit): UINTN * UINT32 --> UINTN UINT32 * UINT32 --> UINT32 and I didn't check if something depended on the type (== range) of the product. Admittedly, multiplying a buffer size with anything doesn't make much sense in this instance, but I'm generally paranoid about type changes without auditing all uses. Thanks! Laszlo ------------------------------------------------------------------------------ Managing the Performance of Cloud-Based Applications Take advantage of what the Cloud has to offer - Avoid Common Pitfalls. Read the Whitepaper. http://pubads.g.doubleclick.net/gampad/clk?id=121054471&iu=/4140/ostg.clktrk _______________________________________________ edk2-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/edk2-devel
