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

Ard Biesheuvel posted 38 patches 1 year, 3 months ago
[edk2-devel] [PATCH v5 23/38] EmbeddedPkg/PrePiLib AARCH64: Remap DXE core before execution
Posted by Ard Biesheuvel 1 year, 3 months ago
Deal with DRAM memory potentially being mapped with non-executable
permissions, by mapping the DXE core code sections explicitly before
launch.

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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101127): https://edk2.groups.io/g/devel/message/101127
Mute This Topic: https://groups.io/mt/97586028/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v5 23/38] EmbeddedPkg/PrePiLib AARCH64: Remap DXE core before execution
Posted by Leif Lindholm 1 year, 3 months ago
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?

/
    Leif

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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101270): https://edk2.groups.io/g/devel/message/101270
Mute This Topic: https://groups.io/mt/97586028/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v5 23/38] EmbeddedPkg/PrePiLib AARCH64: Remap DXE core before execution
Posted by Ard Biesheuvel 1 year, 3 months ago
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
> >
>
>
> 
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101272): https://edk2.groups.io/g/devel/message/101272
Mute This Topic: https://groups.io/mt/97586028/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v5 23/38] EmbeddedPkg/PrePiLib AARCH64: Remap DXE core before execution
Posted by Leif Lindholm 1 year, 3 months ago
On 2023-03-16 13:50, Ard Biesheuvel wrote:
> 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.

Works for me :)
With that:
Reviewed-by: Leif Lindholm <quic_llindhol@quicinc.com>

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



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