[edk2-devel] [PATCH v5 35/38] MdeModulePkg/DxeCore: Clear NX permissions on non-protected images

Ard Biesheuvel posted 38 patches 1 year, 3 months ago
[edk2-devel] [PATCH v5 35/38] MdeModulePkg/DxeCore: Clear NX permissions on non-protected images
Posted by Ard Biesheuvel 1 year, 3 months ago
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.
+      } 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 +
-- 
2.39.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101139): https://edk2.groups.io/g/devel/message/101139
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]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v5 35/38] MdeModulePkg/DxeCore: Clear NX permissions on non-protected images
Posted by Oliver Smith-Denny 1 year, 3 months ago
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]
-=-=-=-=-=-=-=-=-=-=-=-