Re: [edk2-devel] [PATCH v5 23/38] EmbeddedPkg/PrePiLib AARCH64: Remap DXE core before execution

["Ard Biesheuvel" <ardb@kernel.org>]

On Thu, 16 Mar 2023 at 14:33, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
>
> On Mon, Mar 13, 2023 at 18:16:59 +0100, Ard Biesheuvel wrote:
> > Deal with DRAM memory potentially being mapped with non-executable
> > permissions, by mapping the DXE core code sections explicitly before
> > launch.
>
> Could you add a note about why LoadPeCoffImage/LoadDxeCoreFromFfsFile
> are made private?
>

Actually, they shouldn't - they are used elsewhere too.

I confused myself into thinking that LoadPeCoffImage() should be made
private as it now has 'special' behavior that only applies to DxeCore,
but in fact, the whole purpose in life of PrePi is to load DxeCore and
run it, so that was kind of implied anyway.

So I intend to simply drop those changes.


>
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  EmbeddedPkg/Include/Library/PrePiLib.h          | 16 ------
> >  EmbeddedPkg/Library/PrePiLib/Arm/RemapDxeCore.c | 51 ++++++++++++++++++++
> >  EmbeddedPkg/Library/PrePiLib/PrePi.h            | 13 +++++
> >  EmbeddedPkg/Library/PrePiLib/PrePiLib.c         |  4 ++
> >  EmbeddedPkg/Library/PrePiLib/PrePiLib.inf       | 12 +++++
> >  EmbeddedPkg/Library/PrePiLib/X86/RemapDxeCore.c | 23 +++++++++
> >  6 files changed, 103 insertions(+), 16 deletions(-)
> >
> > diff --git a/EmbeddedPkg/Include/Library/PrePiLib.h b/EmbeddedPkg/Include/Library/PrePiLib.h
> > index 93a9115eac2d..14f2bbc38dae 100644
> > --- a/EmbeddedPkg/Include/Library/PrePiLib.h
> > +++ b/EmbeddedPkg/Include/Library/PrePiLib.h
> > @@ -758,22 +758,6 @@ AllocateAlignedPages (
> >    IN UINTN  Alignment
> >    );
> >
> > -EFI_STATUS
> > -EFIAPI
> > -LoadPeCoffImage (
> > -  IN  VOID                  *PeCoffImage,
> > -  OUT EFI_PHYSICAL_ADDRESS  *ImageAddress,
> > -  OUT UINT64                *ImageSize,
> > -  OUT EFI_PHYSICAL_ADDRESS  *EntryPoint
> > -  );
> > -
> > -EFI_STATUS
> > -EFIAPI
> > -LoadDxeCoreFromFfsFile (
> > -  IN EFI_PEI_FILE_HANDLE  FileHandle,
> > -  IN UINTN                StackSize
> > -  );
> > -
> >  EFI_STATUS
> >  EFIAPI
> >  LoadDxeCoreFromFv (
> > diff --git a/EmbeddedPkg/Library/PrePiLib/Arm/RemapDxeCore.c b/EmbeddedPkg/Library/PrePiLib/Arm/RemapDxeCore.c
> > new file mode 100644
> > index 000000000000..40d4ed9d77bd
> > --- /dev/null
> > +++ b/EmbeddedPkg/Library/PrePiLib/Arm/RemapDxeCore.c
> > @@ -0,0 +1,51 @@
> > +/** @file
> > +  Copyright (c) 2023, Google LLC. All rights reserved.<BR>
> > +
> > +  SPDX-License-Identifier: BSD-2-Clause-Patent
> > +
> > +**/
> > +
> > +#include "PrePi.h"
> > +
> > +#include <Library/ArmMmuLib.h>
> > +
> > +/**
> > +  Remap the code section of the DXE core with the read-only and executable
> > +  permissions.
> > +
> > +  @param  ImageContext    The image context describing the loaded PE/COFF image
> > +
> > +**/
> > +VOID
> > +EFIAPI
> > +RemapDxeCore (
> > +  IN  CONST PE_COFF_LOADER_IMAGE_CONTEXT  *ImageContext
> > +  )
> > +{
> > +  EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION  Hdr;
> > +  EFI_IMAGE_SECTION_HEADER             *Section;
> > +  UINTN                                Index;
> > +
> > +  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)(ImageContext->ImageAddress + Section[Index].VirtualAddress),
> > +        Section[Index].Misc.VirtualSize
> > +        );
> > +
> > +      ArmClearMemoryRegionNoExec (
> > +        (UINTN)(ImageContext->ImageAddress + Section[Index].VirtualAddress),
> > +        Section[Index].Misc.VirtualSize
> > +        );
> > +    }
> > +  }
> > +}
> > diff --git a/EmbeddedPkg/Library/PrePiLib/PrePi.h b/EmbeddedPkg/Library/PrePiLib/PrePi.h
> > index a00c946512f4..a0f8837d1d37 100644
> > --- a/EmbeddedPkg/Library/PrePiLib/PrePi.h
> > +++ b/EmbeddedPkg/Library/PrePiLib/PrePi.h
> > @@ -37,4 +37,17 @@
> >  #define GET_GUID_HOB_DATA(GuidHob)       ((VOID *) (((UINT8 *) &((GuidHob)->Name)) + sizeof (EFI_GUID)))
> >  #define GET_GUID_HOB_DATA_SIZE(GuidHob)  (((GuidHob)->Header).HobLength - sizeof (EFI_HOB_GUID_TYPE))
> >
> > +/**
> > +  Remap the code section of the DXE core with the read-only and executable
> > +  permissions.
> > +
> > +  @param  ImageContext    The image context describing the loaded PE/COFF image
> > +
> > +**/
> > +VOID
> > +EFIAPI
> > +RemapDxeCore (
> > +  IN  CONST PE_COFF_LOADER_IMAGE_CONTEXT  *ImageContext
> > +  );
> > +
> >  #endif
> > diff --git a/EmbeddedPkg/Library/PrePiLib/PrePiLib.c b/EmbeddedPkg/Library/PrePiLib/PrePiLib.c
> > index 3cf866dab248..188ad5c518a8 100644
> > --- a/EmbeddedPkg/Library/PrePiLib/PrePiLib.c
> > +++ b/EmbeddedPkg/Library/PrePiLib/PrePiLib.c
> > @@ -54,6 +54,7 @@ AllocateCodePages (
> >    return NULL;
> >  }
> >
> > +STATIC
> >  EFI_STATUS
> >  EFIAPI
> >  LoadPeCoffImage (
> > @@ -105,6 +106,8 @@ LoadPeCoffImage (
> >    //
> >    InvalidateInstructionCacheRange ((VOID *)(UINTN)*ImageAddress, (UINTN)*ImageSize);
> >
> > +  RemapDxeCore (&ImageContext);
> > +
> >    return Status;
> >  }
> >
> > @@ -114,6 +117,7 @@ VOID
> >    IN  VOID *HobStart
> >    );
> >
> > +STATIC
> >  EFI_STATUS
> >  EFIAPI
> >  LoadDxeCoreFromFfsFile (
> > diff --git a/EmbeddedPkg/Library/PrePiLib/PrePiLib.inf b/EmbeddedPkg/Library/PrePiLib/PrePiLib.inf
> > index 090bfe888f52..2df5928c51d5 100644
> > --- a/EmbeddedPkg/Library/PrePiLib/PrePiLib.inf
> > +++ b/EmbeddedPkg/Library/PrePiLib/PrePiLib.inf
> > @@ -31,11 +31,20 @@ [Sources.common]
> >    FwVol.c
> >    PrePiLib.c
> >
> > +[Sources.X64, Sources.IA32]
> > +  X86/RemapDxeCore.c
> > +
> > +[Sources.AARCH64, Sources.ARM]
> > +  Arm/RemapDxeCore.c
> > +
> >  [Packages]
> >    MdePkg/MdePkg.dec
> >    EmbeddedPkg/EmbeddedPkg.dec
> >    MdeModulePkg/MdeModulePkg.dec
> >
> > +[Packages.ARM, Packages.AARCH64]
> > +  ArmPkg/ArmPkg.dec
> > +
> >  [LibraryClasses]
> >    BaseLib
> >    DebugLib
> > @@ -50,6 +59,9 @@ [LibraryClasses]
> >    PerformanceLib
> >    HobLib
> >
> > +[LibraryClasses.ARM, LibraryClasses.AARCH64]
> > +  ArmMmuLib
> > +
> >  [Guids]
> >    gEfiMemoryTypeInformationGuid
> >
> > diff --git a/EmbeddedPkg/Library/PrePiLib/X86/RemapDxeCore.c b/EmbeddedPkg/Library/PrePiLib/X86/RemapDxeCore.c
> > new file mode 100644
> > index 000000000000..1899c050fdec
> > --- /dev/null
> > +++ b/EmbeddedPkg/Library/PrePiLib/X86/RemapDxeCore.c
> > @@ -0,0 +1,23 @@
> > +/** @file
> > +  Copyright (c) 2023, Google LLC. All rights reserved.<BR>
> > +
> > +  SPDX-License-Identifier: BSD-2-Clause-Patent
> > +
> > +**/
> > +
> > +#include "PrePi.h"
> > +
> > +/**
> > +  Remap the code section of the DXE core with the read-only and executable
> > +  permissions.
> > +
> > +  @param  ImageContext    The image context describing the loaded PE/COFF image
> > +
> > +**/
> > +VOID
> > +EFIAPI
> > +RemapDxeCore (
> > +  IN  CONST PE_COFF_LOADER_IMAGE_CONTEXT  *ImageContext
> > +  )
> > +{
> > +}
> > --
> > 2.39.2
> >
>
>
> 
>
>