From b5984a3d302d455b50f66190fcfc3fcdb5018e33 Mon Sep 17 00:00:00 2001 From: Mike Beaton Date: Mon, 20 Nov 2023 11:49:58 +0000 Subject: [PATCH] OcPeCoffExtLib: Add code path to OcPeCoffFixupInitializeContext which generates fixed image context without fixing image in memory - Required to allow AppleEfiSignTool to verify rare (but actually existing) Apple signed binaries with overlapping section errors --- Include/Acidanthera/Library/OcPeCoffExtLib.h | 14 +-- Library/OcPeCoffExtLib/OcPeCoffExtLib.c | 3 +- Library/OcPeCoffExtLib/OcPeCoffFixupInit.c | 90 ++++++++++++------- Utilities/AppleEfiSignTool/AppleEfiSignTool.c | 14 +-- 4 files changed, 73 insertions(+), 48 deletions(-) diff --git a/Include/Acidanthera/Library/OcPeCoffExtLib.h b/Include/Acidanthera/Library/OcPeCoffExtLib.h index 3a7e4947..7d43b458 100644 --- a/Include/Acidanthera/Library/OcPeCoffExtLib.h +++ b/Include/Acidanthera/Library/OcPeCoffExtLib.h @@ -107,15 +107,18 @@ OcPatchLegacyEfi ( Closely based on PeCoffInitializeContext from PeCoffLib2. The approach of modifying the image in memory is basically incompatible - with secure boot, athough: + with secure boot, although: a) Certain firmware may allow optionally registering the hash of any image which does not load, which would still work. b) It is fairly crazy anyway to want to apply secure boot to the old, insecure .efi files which need these fixups. - @param[out] Context The context describing the Image. - @param[in] FileBuffer The file data to parse as PE Image. - @param[in] FileSize The size, in Bytes, of FileBuffer. + @param[out] Context The context describing the Image. + @param[in] FileBuffer The file data to parse as PE Image. + @param[in] FileSize The size, in Bytes, of FileBuffer. + @param[in] InMemoryFixup If TRUE, fixes are made to image in memory. + If FALSE, Context is initialised as if fixes were + made, but no changes are made to loaded image. @retval RETURN_SUCCESS The Image context has been initialised successfully. @retval other The file data is malformed. @@ -124,7 +127,8 @@ RETURN_STATUS OcPeCoffFixupInitializeContext ( OUT PE_COFF_LOADER_IMAGE_CONTEXT *Context, IN CONST VOID *FileBuffer, - IN UINT32 FileSize + IN UINT32 FileSize, + IN BOOLEAN InMemoryFixup ); #endif // OC_PE_COFF_EXT_LIB_H diff --git a/Library/OcPeCoffExtLib/OcPeCoffExtLib.c b/Library/OcPeCoffExtLib/OcPeCoffExtLib.c index f217e332..2f92c195 100644 --- a/Library/OcPeCoffExtLib/OcPeCoffExtLib.c +++ b/Library/OcPeCoffExtLib/OcPeCoffExtLib.c @@ -583,7 +583,8 @@ OcPatchLegacyEfi ( ImageStatus = OcPeCoffFixupInitializeContext ( &ImageContext, DriverBuffer, - DriverSize + DriverSize, + TRUE ); if (EFI_ERROR (ImageStatus)) { DEBUG ((DEBUG_WARN, "OCPE: PeCoff legacy patch failure - %r\n", ImageStatus)); diff --git a/Library/OcPeCoffExtLib/OcPeCoffFixupInit.c b/Library/OcPeCoffExtLib/OcPeCoffFixupInit.c index 6212924d..7474de11 100644 --- a/Library/OcPeCoffExtLib/OcPeCoffFixupInit.c +++ b/Library/OcPeCoffExtLib/OcPeCoffFixupInit.c @@ -45,10 +45,13 @@ memory space space. The section data must be in bounds bounds of the file buffer. - @param[in,out] Context The context describing the Image. Must have been - initialised by PeCoffInitializeContext(). - @param[in] FileSize The size, in Bytes, of Context->FileBuffer. - @param[out] StartAddress On output, the RVA of the first Image section. + @param[in,out] Context The context describing the Image. Must have been + initialised by PeCoffInitializeContext(). + @param[in] FileSize The size, in Bytes, of Context->FileBuffer. + @param[out] StartAddress On output, the RVA of the first Image section. + @param[in] InMemoryFixup If TRUE, fixes are made to image in memory. + If FALSE, Context is initialised as if fixes were + made, but no changes are made to loaded image. @retval RETURN_SUCCESS The Image section Headers are well-formed. @retval other The Image section Headers are malformed. @@ -58,13 +61,17 @@ RETURN_STATUS InternalVerifySections ( IN OUT PE_COFF_LOADER_IMAGE_CONTEXT *Context, IN UINT32 FileSize, - OUT UINT32 *StartAddress + OUT UINT32 *StartAddress, + IN BOOLEAN InMemoryFixup ) { BOOLEAN Overflow; UINT32 NextSectRva; UINT32 FixupOffset; - UINT32 FixupVirtualSize; + UINT32 PreFixupVirtualAddress; + UINT32 PostFixupVirtualAddress; + UINT32 PreFixupVirtualSize; + UINT32 PostFixupVirtualSize; CHAR8 SectionName[EFI_IMAGE_SIZEOF_SHORT_NAME + 1]; UINT32 SectRawEnd; UINT16 SectionIndex; @@ -122,8 +129,11 @@ InternalVerifySections ( // Fix up W^X errors in memory. // if ((Sections[SectionIndex].Characteristics & (EFI_IMAGE_SCN_MEM_EXECUTE | EFI_IMAGE_SCN_MEM_WRITE)) == (EFI_IMAGE_SCN_MEM_EXECUTE | EFI_IMAGE_SCN_MEM_WRITE)) { - Sections[SectionIndex].Characteristics &= ~EFI_IMAGE_SCN_MEM_EXECUTE; - DEBUG ((DEBUG_INFO, "OCPE: Fixup W^X for %a\n", SectionName)); + if (InMemoryFixup) { + Sections[SectionIndex].Characteristics &= ~EFI_IMAGE_SCN_MEM_EXECUTE; + } + + DEBUG ((DEBUG_INFO, "OCPE: %u fixup W^X for %a\n", InMemoryFixup, SectionName)); } // @@ -132,6 +142,10 @@ InternalVerifySections ( // Unaligned Image sections have been observed with iPXE Option ROMs and old // Apple Mac OS X bootloaders. // + PreFixupVirtualAddress = Sections[SectionIndex].VirtualAddress; + PostFixupVirtualAddress = PreFixupVirtualAddress; + PreFixupVirtualSize = Sections[SectionIndex].VirtualSize; + PostFixupVirtualSize = PreFixupVirtualSize; if ((PcdGet32 (PcdImageLoaderAlignmentPolicy) & PCD_ALIGNMENT_POLICY_CONTIGUOUS_SECTIONS) == 0) { if (Sections[SectionIndex].VirtualAddress != NextSectRva) { DEBUG_RAISE (); @@ -150,24 +164,29 @@ InternalVerifySections ( // // Fix up section overlap errors in memory. // - FixupOffset = NextSectRva - Sections[SectionIndex].VirtualAddress; - FixupVirtualSize = Sections[SectionIndex].VirtualSize; + FixupOffset = NextSectRva - Sections[SectionIndex].VirtualAddress; if (FixupOffset > Sections[SectionIndex].VirtualSize) { - Sections[SectionIndex].VirtualSize = 0; + PostFixupVirtualSize = 0; } else { - Sections[SectionIndex].VirtualSize -= FixupOffset; + PostFixupVirtualSize -= FixupOffset; + } + + PostFixupVirtualAddress = NextSectRva; + if (InMemoryFixup) { + Sections[SectionIndex].VirtualAddress = PostFixupVirtualAddress; + Sections[SectionIndex].VirtualSize = PostFixupVirtualSize; } DEBUG (( DEBUG_INFO, - "OCPE: Fixup section overlap for %a 0x%X(0x%X)->0x%X(0x%X)\n", + "OCPE: %u fixup section overlap for %a 0x%X(0x%X)->0x%X(0x%X)\n", + InMemoryFixup, SectionName, - Sections[SectionIndex].VirtualAddress, - FixupVirtualSize, - NextSectRva, - Sections[SectionIndex].VirtualSize + PreFixupVirtualAddress, + PreFixupVirtualSize, + PostFixupVirtualAddress, + PostFixupVirtualSize )); - Sections[SectionIndex].VirtualAddress = NextSectRva; } // @@ -176,16 +195,16 @@ InternalVerifySections ( // possible, to ensure the Image can have memory protection applied. // Otherwise, report no alignment for the Image. // - if (!IS_ALIGNED (Sections[SectionIndex].VirtualAddress, Context->SectionAlignment)) { + if (!IS_ALIGNED (PostFixupVirtualAddress, Context->SectionAlignment)) { STATIC_ASSERT ( DEFAULT_PAGE_ALLOCATION_GRANULARITY <= RUNTIME_PAGE_ALLOCATION_GRANULARITY, "This code must be adapted to consider the reversed order." ); - if (IS_ALIGNED (Sections[SectionIndex].VirtualAddress, RUNTIME_PAGE_ALLOCATION_GRANULARITY)) { + if (IS_ALIGNED (PostFixupVirtualAddress, RUNTIME_PAGE_ALLOCATION_GRANULARITY)) { Context->SectionAlignment = RUNTIME_PAGE_ALLOCATION_GRANULARITY; } else if ( (DEFAULT_PAGE_ALLOCATION_GRANULARITY < RUNTIME_PAGE_ALLOCATION_GRANULARITY) - && IS_ALIGNED (Sections[SectionIndex].VirtualAddress, DEFAULT_PAGE_ALLOCATION_GRANULARITY)) + && IS_ALIGNED (PostFixupVirtualAddress, DEFAULT_PAGE_ALLOCATION_GRANULARITY)) { Context->SectionAlignment = DEFAULT_PAGE_ALLOCATION_GRANULARITY; } else { @@ -218,8 +237,8 @@ InternalVerifySections ( // Determine the end of the current Image section. // Overflow = BaseOverflowAddU32 ( - Sections[SectionIndex].VirtualAddress, - Sections[SectionIndex].VirtualSize, + PostFixupVirtualAddress, + PostFixupVirtualSize, &NextSectRva ); if (Overflow) { @@ -375,9 +394,12 @@ InternalValidateRelocInfo ( Used offsets and ranges must be aligned and in the bounds of the raw file. Image section Headers and basic Relocation information must be Well-formed. - @param[in,out] Context The context describing the Image. Must have been - initialised by PeCoffInitializeContext(). - @param[in] FileSize The size, in Bytes, of Context->FileBuffer. + @param[in,out] Context The context describing the Image. Must have been + initialised by PeCoffInitializeContext(). + @param[in] FileSize The size, in Bytes, of Context->FileBuffer. + @param[in] InMemoryFixup If TRUE, fixes are made to image in memory. + If FALSE, Context is initialised as if fixes were + made, but no changes are made to loaded image. @retval RETURN_SUCCESS The PE Image is Well-formed. @retval other The PE Image is malformed. @@ -386,7 +408,8 @@ STATIC RETURN_STATUS InternalInitializePe ( IN OUT PE_COFF_LOADER_IMAGE_CONTEXT *Context, - IN UINT32 FileSize + IN UINT32 FileSize, + IN BOOLEAN InMemoryFixup ) { BOOLEAN Overflow; @@ -685,7 +708,8 @@ InternalInitializePe ( Status = InternalVerifySections ( Context, FileSize, - &StartAddress + &StartAddress, + InMemoryFixup ); if (Status != RETURN_SUCCESS) { DEBUG_RAISE (); @@ -715,12 +739,18 @@ RETURN_STATUS OcPeCoffFixupInitializeContext ( OUT PE_COFF_LOADER_IMAGE_CONTEXT *Context, IN CONST VOID *FileBuffer, - IN UINT32 FileSize + IN UINT32 FileSize, + IN BOOLEAN InMemoryFixup ) { RETURN_STATUS Status; CONST EFI_IMAGE_DOS_HEADER *DosHdr; + #ifndef EFIUSER + // The only expected calling path with InMemoryFixup == FALSE is from AppleEfiSignTool. + ASSERT (InMemoryFixup); + #endif + // // Failure of these asserts can be fixed if needed by not using the Pcd // values above, we do not do this initially to make it simpler to compare @@ -796,7 +826,7 @@ OcPeCoffFixupInitializeContext ( // // Verify the PE Image Header is well-formed. // - Status = InternalInitializePe (Context, FileSize); + Status = InternalInitializePe (Context, FileSize, InMemoryFixup); if (Status != RETURN_SUCCESS) { return Status; } diff --git a/Utilities/AppleEfiSignTool/AppleEfiSignTool.c b/Utilities/AppleEfiSignTool/AppleEfiSignTool.c index d8d2ff0c..a4e85fb1 100644 --- a/Utilities/AppleEfiSignTool/AppleEfiSignTool.c +++ b/Utilities/AppleEfiSignTool/AppleEfiSignTool.c @@ -126,7 +126,8 @@ VerifySignatureAndApfs ( ContextStatus = OcPeCoffFixupInitializeContext ( &Context, Image, - ImageSize + ImageSize, + FALSE ); } @@ -146,17 +147,6 @@ VerifySignatureAndApfs ( ImageSize, Status )); - if (Status == EFI_SECURITY_VIOLATION) { - if (ForceFixup) { - DEBUG ((DEBUG_ERROR, "SIGN: Expected security violation due to -f\n")); - } else { - // - // This would be an image with PE COFF section overlaps but also signed - // or an incorrectly signed image, neither of which are expected. - // - DEBUG ((DEBUG_ERROR, "SIGN: *** Unexpected result! If this is a genuine Apple binary and you wish to verify the signature, please use a version of AppleEfiSignTool from OpenCore 0.8.7 or earlier. ***\n")); - } - } if (!EFI_ERROR (ContextStatus)) { ApfsStatus = InternalPeCoffGetApfsDriverVersionFromContext (&Context, ImageSize, &DriverVersion);