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 <ardb@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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-