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]
-=-=-=-=-=-=-=-=-=-=-=-