On 3/13/2023 10:17 AM, Ard Biesheuvel wrote:
Currently, we rely on the memory type for loading images being executable by default, and only restrict the permissions if the policy says so, and the image sections are suitably aligned. This requires that the various 'code' memory types are executable by default, which is unfortunate.In order to be able to tighten this, let's update the image protection policy handling so that images that should not be mapped with strict separation of RW- and R-X are remapped RWX explicitly if the memory type used for loading the images is marked as NX by default. Signed-off-by: Ard Biesheuvel <a...@kernel.org> --- MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 98 +++++++++++--------- 1 file changed, 54 insertions(+), 44 deletions(-) diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c index 301ddd6eb053..7c7a946c1b48 100644 --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c @@ -373,11 +373,62 @@ ProtectUefiImage ( }ProtectionPolicy = GetUefiImageProtectionPolicy (LoadedImage, LoadedImageDevicePath); + + ImageAddress = LoadedImage->ImageBase; + + PdbPointer = PeCoffLoaderGetPdbPointer ((VOID *)(UINTN)ImageAddress); + if (PdbPointer != NULL) { + DEBUG ((DEBUG_VERBOSE, " Image - %a\n", PdbPointer)); + } + switch (ProtectionPolicy) { - case DO_NOT_PROTECT: - return EFI_SUCCESS; case PROTECT_IF_ALIGNED_ELSE_ALLOW: - break; + // + // Check PE/COFF image + // + DosHdr = (EFI_IMAGE_DOS_HEADER *)(UINTN)ImageAddress; + PeCoffHeaderOffset = 0; + if (DosHdr->e_magic == EFI_IMAGE_DOS_SIGNATURE) { + PeCoffHeaderOffset = DosHdr->e_lfanew; + } + + Hdr.Pe32 = (EFI_IMAGE_NT_HEADERS32 *)((UINT8 *)(UINTN)ImageAddress + PeCoffHeaderOffset); + if (Hdr.Pe32->Signature != EFI_IMAGE_NT_SIGNATURE) { + DEBUG ((DEBUG_INFO, "Hdr.Pe32->Signature invalid - 0x%x\n", Hdr.Pe32->Signature)); + // It might be image in SMM.
In this case we now fall through to explicitly unprotect the image that does not have the EFI_IMAGE_NT_SIGNATURE. Is it expected that we will always run these images (or that if we shouldn't run it that that will be caught elsewhere) and that giving it full permissions is permissible?
+ } else { + // + // Get SectionAlignment + // + if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) { + SectionAlignment = Hdr.Pe32->OptionalHeader.SectionAlignment; + } else { + SectionAlignment = Hdr.Pe32Plus->OptionalHeader.SectionAlignment; + } + + if (SectionAlignment >= EFI_PAGE_SIZE) { + break; + } + + DEBUG (( + DEBUG_VERBOSE, + "!!!!!!!! ProtectUefiImageCommon - Section Alignment(0x%x) is incorrect !!!!!!!!\n", + SectionAlignment + )); + } + // fall through to unprotect image if needed + case DO_NOT_PROTECT: + if ((PcdGet64 (PcdDxeNxMemoryProtectionPolicy) & + LShiftU64 (1, LoadedImage->ImageCodeType)) != 0) + { + SetUefiImageMemoryAttributes ( + (UINTN)LoadedImage->ImageBase, + (LoadedImage->ImageSize + EFI_PAGE_MASK) & ~(UINT64)EFI_PAGE_MASK, + 0 + ); + } + + return EFI_SUCCESS; default: ASSERT (FALSE); return EFI_SUCCESS; @@ -396,47 +447,6 @@ ProtectUefiImage ( ImageRecord->ImageBase = (EFI_PHYSICAL_ADDRESS)(UINTN)LoadedImage->ImageBase; ImageRecord->ImageSize = LoadedImage->ImageSize;- ImageAddress = LoadedImage->ImageBase; - - PdbPointer = PeCoffLoaderGetPdbPointer ((VOID *)(UINTN)ImageAddress); - if (PdbPointer != NULL) { - DEBUG ((DEBUG_VERBOSE, " Image - %a\n", PdbPointer)); - } - - // - // Check PE/COFF image - // - DosHdr = (EFI_IMAGE_DOS_HEADER *)(UINTN)ImageAddress; - PeCoffHeaderOffset = 0; - if (DosHdr->e_magic == EFI_IMAGE_DOS_SIGNATURE) { - PeCoffHeaderOffset = DosHdr->e_lfanew; - } - - Hdr.Pe32 = (EFI_IMAGE_NT_HEADERS32 *)((UINT8 *)(UINTN)ImageAddress + PeCoffHeaderOffset); - if (Hdr.Pe32->Signature != EFI_IMAGE_NT_SIGNATURE) { - DEBUG ((DEBUG_VERBOSE, "Hdr.Pe32->Signature invalid - 0x%x\n", Hdr.Pe32->Signature)); - // It might be image in SMM. - goto Finish; - } - - // - // Get SectionAlignment - // - if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) { - SectionAlignment = Hdr.Pe32->OptionalHeader.SectionAlignment; - } else { - SectionAlignment = Hdr.Pe32Plus->OptionalHeader.SectionAlignment; - } - - if (SectionAlignment >= EFI_PAGE_SIZE) { - DEBUG (( - DEBUG_VERBOSE, - "!!!!!!!! ProtectUefiImageCommon - Section Alignment(0x%x) is incorrect !!!!!!!!\n", - SectionAlignment - )); - goto Finish; - } - Section = (EFI_IMAGE_SECTION_HEADER *)( (UINT8 *)(UINTN)ImageAddress + PeCoffHeaderOffset +
-=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#101372): https://edk2.groups.io/g/devel/message/101372 Mute This Topic: https://groups.io/mt/97586058/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
