[edk2-devel] [PATCH v5 18/38] MdeModulePkg/DxeIpl AARCH64: Remap DXE core code section before launch

Ard Biesheuvel posted 38 patches 1 year, 3 months ago
[edk2-devel] [PATCH v5 18/38] MdeModulePkg/DxeIpl AARCH64: Remap DXE core code section before launch
Posted by Ard Biesheuvel 1 year, 3 months ago
To permit the platform to adopt a stricter policy when it comes to
memory protections, and map all memory XP by default, add the necessary
handling to the DXE IPL PEIM to ensure that the DXE core code section is
mapped executable before invoking the DXE core.

It is up to the DXE core itself to manage the executable permissions on
other DXE and UEFI drivers and applications that it dispatches.

Note that this requires that the DXE IPL executes non-shadowed from a FV
that is mapped executable.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c | 73 ++++++++++++++++++++
 MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf        |  1 +
 2 files changed, 74 insertions(+)

diff --git a/MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c b/MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c
index f62b6dcb38a7..c57ffa87e30f 100644
--- a/MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c
+++ b/MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c
@@ -11,6 +11,73 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #include "DxeIpl.h"
 
 #include <Library/ArmMmuLib.h>
+#include <Library/PeCoffLib.h>
+
+/**
+  Discover the code sections of the DXE core, and remap them read-only
+  and executable.
+
+  @param DxeCoreEntryPoint  The entrypoint of the DXE core executable.
+  @param HobList            The list of HOBs passed to the DXE core from PEI.
+**/
+STATIC
+VOID
+RemapDxeCoreCodeReadOnly (
+  IN EFI_PHYSICAL_ADDRESS  DxeCoreEntryPoint,
+  IN EFI_PEI_HOB_POINTERS  HobList
+  )
+{
+  EFI_PEI_HOB_POINTERS                 Hob;
+  PE_COFF_LOADER_IMAGE_CONTEXT         ImageContext;
+  RETURN_STATUS                        Status;
+  EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION  Hdr;
+  EFI_IMAGE_SECTION_HEADER             *Section;
+  UINTN                                Index;
+
+  ImageContext.ImageRead = PeCoffLoaderImageReadFromMemory;
+  ImageContext.Handle    = NULL;
+
+  //
+  // Find the module HOB for the DXE core
+  //
+  for (Hob.Raw = HobList.Raw; !END_OF_HOB_LIST (Hob); Hob.Raw = GET_NEXT_HOB (Hob)) {
+    if ((GET_HOB_TYPE (Hob) == EFI_HOB_TYPE_MEMORY_ALLOCATION) &&
+        (CompareGuid (&Hob.MemoryAllocation->AllocDescriptor.Name, &gEfiHobMemoryAllocModuleGuid)) &&
+        (Hob.MemoryAllocationModule->EntryPoint == DxeCoreEntryPoint))
+    {
+      ImageContext.Handle = (VOID *)(UINTN)Hob.MemoryAllocation->AllocDescriptor.MemoryBaseAddress;
+      break;
+    }
+  }
+
+  ASSERT (ImageContext.Handle != NULL);
+
+  Status = PeCoffLoaderGetImageInfo (&ImageContext);
+  ASSERT_RETURN_ERROR (Status);
+
+  Hdr.Union = (EFI_IMAGE_OPTIONAL_HEADER_UNION *)((UINT8 *)ImageContext.Handle +
+                                                  ImageContext.PeCoffHeaderOffset);
+  ASSERT (Hdr.Pe32->Signature == EFI_IMAGE_NT_SIGNATURE);
+
+  Section = (EFI_IMAGE_SECTION_HEADER *)((UINT8 *)Hdr.Union + sizeof (UINT32) +
+                                         sizeof (EFI_IMAGE_FILE_HEADER) +
+                                         Hdr.Pe32->FileHeader.SizeOfOptionalHeader
+                                         );
+
+  for (Index = 0; Index < Hdr.Pe32->FileHeader.NumberOfSections; Index++) {
+    if ((Section[Index].Characteristics & EFI_IMAGE_SCN_CNT_CODE) != 0) {
+      ArmSetMemoryRegionReadOnly (
+        (UINTN)((UINT8 *)ImageContext.Handle + Section[Index].VirtualAddress),
+        Section[Index].Misc.VirtualSize
+        );
+
+      ArmClearMemoryRegionNoExec (
+        (UINTN)((UINT8 *)ImageContext.Handle + Section[Index].VirtualAddress),
+        Section[Index].Misc.VirtualSize
+        );
+    }
+  }
+}
 
 /**
    Transfers control to DxeCore.
@@ -33,6 +100,12 @@ HandOffToDxeCore (
   VOID        *TopOfStack;
   EFI_STATUS  Status;
 
+  //
+  // DRAM may be mapped with non-executable permissions by default, so
+  // we'll need to map the DXE core code region executable explicitly.
+  //
+  RemapDxeCoreCodeReadOnly (DxeCoreEntryPoint, HobList);
+
   //
   // Allocate 128KB for the Stack
   //
diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
index 62821477d012..d85ca79dc0c3 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
@@ -82,6 +82,7 @@ [LibraryClasses]
 
 [LibraryClasses.ARM, LibraryClasses.AARCH64]
   ArmMmuLib
+  PeCoffLib
 
 [Ppis]
   gEfiDxeIplPpiGuid                      ## PRODUCES
-- 
2.39.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101122): https://edk2.groups.io/g/devel/message/101122
Mute This Topic: https://groups.io/mt/97586019/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v5 18/38] MdeModulePkg/DxeIpl AARCH64: Remap DXE core code section before launch
Posted by Oliver Smith-Denny 1 year, 3 months ago
On 3/13/2023 10:16 AM, Ard Biesheuvel wrote:
One nitpick below.

> To permit the platform to adopt a stricter policy when it comes to
> memory protections, and map all memory XP by default, add the necessary
> handling to the DXE IPL PEIM to ensure that the DXE core code section is
> mapped executable before invoking the DXE core.
> 
> It is up to the DXE core itself to manage the executable permissions on
> other DXE and UEFI drivers and applications that it dispatches.
> 
> Note that this requires that the DXE IPL executes non-shadowed from a FV
> that is mapped executable.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>   MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c | 73 ++++++++++++++++++++
>   MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf        |  1 +
>   2 files changed, 74 insertions(+)
> 
> diff --git a/MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c b/MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c
> index f62b6dcb38a7..c57ffa87e30f 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c
> +++ b/MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c
> @@ -11,6 +11,73 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>   #include "DxeIpl.h"
> 
>   
> 
>   #include <Library/ArmMmuLib.h>
> 
> +#include <Library/PeCoffLib.h>
> 
> +
> 
> +/**
> 
> +  Discover the code sections of the DXE core, and remap them read-only
> 
> +  and executable.
> 
> +
> 
> +  @param DxeCoreEntryPoint  The entrypoint of the DXE core executable.
> 
> +  @param HobList            The list of HOBs passed to the DXE core from PEI.
> 
> +**/
> 
> +STATIC
> 
> +VOID
> 
> +RemapDxeCoreCodeReadOnly (

nit: should this be called RemapDxeCoreCodeReadExecute or something? The 
naming seems confusing here to say map read only when we are mapping it 
read and execute.

> 
> +  IN EFI_PHYSICAL_ADDRESS  DxeCoreEntryPoint,
> 
> +  IN EFI_PEI_HOB_POINTERS  HobList
> 
> +  )
> 
> +{
> 
> +  EFI_PEI_HOB_POINTERS                 Hob;
> 
> +  PE_COFF_LOADER_IMAGE_CONTEXT         ImageContext;
> 
> +  RETURN_STATUS                        Status;
> 
> +  EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION  Hdr;
> 
> +  EFI_IMAGE_SECTION_HEADER             *Section;
> 
> +  UINTN                                Index;
> 
> +
> 
> +  ImageContext.ImageRead = PeCoffLoaderImageReadFromMemory;
> 
> +  ImageContext.Handle    = NULL;
> 
> +
> 
> +  //
> 
> +  // Find the module HOB for the DXE core
> 
> +  //
> 
> +  for (Hob.Raw = HobList.Raw; !END_OF_HOB_LIST (Hob); Hob.Raw = GET_NEXT_HOB (Hob)) {
> 
> +    if ((GET_HOB_TYPE (Hob) == EFI_HOB_TYPE_MEMORY_ALLOCATION) &&
> 
> +        (CompareGuid (&Hob.MemoryAllocation->AllocDescriptor.Name, &gEfiHobMemoryAllocModuleGuid)) &&
> 
> +        (Hob.MemoryAllocationModule->EntryPoint == DxeCoreEntryPoint))
> 
> +    {
> 
> +      ImageContext.Handle = (VOID *)(UINTN)Hob.MemoryAllocation->AllocDescriptor.MemoryBaseAddress;
> 
> +      break;
> 
> +    }
> 
> +  }
> 
> +
> 
> +  ASSERT (ImageContext.Handle != NULL);
> 
> +
> 
> +  Status = PeCoffLoaderGetImageInfo (&ImageContext);
> 
> +  ASSERT_RETURN_ERROR (Status);
> 
> +
> 
> +  Hdr.Union = (EFI_IMAGE_OPTIONAL_HEADER_UNION *)((UINT8 *)ImageContext.Handle +
> 
> +                                                  ImageContext.PeCoffHeaderOffset);
> 
> +  ASSERT (Hdr.Pe32->Signature == EFI_IMAGE_NT_SIGNATURE);
> 
> +
> 
> +  Section = (EFI_IMAGE_SECTION_HEADER *)((UINT8 *)Hdr.Union + sizeof (UINT32) +
> 
> +                                         sizeof (EFI_IMAGE_FILE_HEADER) +
> 
> +                                         Hdr.Pe32->FileHeader.SizeOfOptionalHeader
> 
> +                                         );
> 
> +
> 
> +  for (Index = 0; Index < Hdr.Pe32->FileHeader.NumberOfSections; Index++) {
> 
> +    if ((Section[Index].Characteristics & EFI_IMAGE_SCN_CNT_CODE) != 0) {
> 
> +      ArmSetMemoryRegionReadOnly (
> 
> +        (UINTN)((UINT8 *)ImageContext.Handle + Section[Index].VirtualAddress),
> 
> +        Section[Index].Misc.VirtualSize
> 
> +        );
> 
> +
> 
> +      ArmClearMemoryRegionNoExec (
> 
> +        (UINTN)((UINT8 *)ImageContext.Handle + Section[Index].VirtualAddress),
> 
> +        Section[Index].Misc.VirtualSize
> 
> +        );
> 
> +    }
> 
> +  }
> 
> +}
> 
>   
> 
>   /**
> 
>      Transfers control to DxeCore.
> 
> @@ -33,6 +100,12 @@ HandOffToDxeCore (
>     VOID        *TopOfStack;
> 
>     EFI_STATUS  Status;
> 
>   
> 
> +  //
> 
> +  // DRAM may be mapped with non-executable permissions by default, so
> 
> +  // we'll need to map the DXE core code region executable explicitly.
> 
> +  //
> 
> +  RemapDxeCoreCodeReadOnly (DxeCoreEntryPoint, HobList);
> 
> +
> 
>     //
> 
>     // Allocate 128KB for the Stack
> 
>     //
> 
> diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> index 62821477d012..d85ca79dc0c3 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> +++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> @@ -82,6 +82,7 @@ [LibraryClasses]
>   
> 
>   [LibraryClasses.ARM, LibraryClasses.AARCH64]
> 
>     ArmMmuLib
> 
> +  PeCoffLib
> 
>   
> 
>   [Ppis]
> 
>     gEfiDxeIplPpiGuid                      ## PRODUCES
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101370): https://edk2.groups.io/g/devel/message/101370
Mute This Topic: https://groups.io/mt/97586019/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-