Re: [edk2-devel] [RFC PATCH 07/10] MdeModulePkg/PeiCore: Apply restricted permissions in image loader

["Oliver Smith-Denny" <osde@linux.microsoft.com>]

On 5/25/2023 7:30 AM, Ard Biesheuvel wrote:
> Add a notification callback to the PEI core to grab a reference to the
> memory attributes PPI as soon as it is registered, and use it in the
> image loader to set restricted memory permissions after loading the
> image if the image was loaded into memory.
> 
> There are two use cases for this:
> - when the DXE IPL loads the DXE core using the PEI image services, its
>    mappings will be set according to the PE section permission attributes
>    if the image was built with 4k section alignment; this means DXE core
>    will never run with mappings that are both writable and executable.
> - when PEIMs are shadowed to memory, they will not only be mapped
>    read-only, but any non-exec permissions will also be removed. (Note
>    that this requires the component that installs PEI permanent memory to
>    depex on the memory attributes PPI, to ensure that it is available to
>    manage permissions on permanent memory before it is used to load
>    images)
> 
> With this logic in place *, there is no longer a need for system memory
> to be mapped with both write and execute permissions out of reset.
> Instead, all memory can be mapped with non-executable permissions by
> default, which means that the stack and other allocations used in PEI or
> early in DXE will no longer need to be mapped non-exec explicitly.
> 
> * the DXE core will also need its own method for clearing non-exec
>    permissions on memory ranges, but this is being addressed in a
>    separate series.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>   MdeModulePkg/Core/Pei/Image/Image.c | 160 ++++++++++++++++++++
>   MdeModulePkg/Core/Pei/PeiMain.h     |   6 +
>   MdeModulePkg/Core/Pei/PeiMain.inf   |   1 +
>   3 files changed, 167 insertions(+)
> 
> diff --git a/MdeModulePkg/Core/Pei/Image/Image.c b/MdeModulePkg/Core/Pei/Image/Image.c
> index cee9f09c6ea61e31..3a7de45014b8f772 100644
> --- a/MdeModulePkg/Core/Pei/Image/Image.c
> +++ b/MdeModulePkg/Core/Pei/Image/Image.c
> @@ -18,6 +18,50 @@ EFI_PEI_PPI_DESCRIPTOR  gPpiLoadFilePpiList = {
>     &mPeiLoadImagePpi
> 
>   };
> 
>   
> 
> +/**
> 
> +  Provide a callback for when the memory attributes PPI is installed.
> 
> +
> 
> +  @param PeiServices        An indirect pointer to the EFI_PEI_SERVICES table
> 
> +                            published by the PEI Foundation.
> 
> +  @param NotifyDescriptor   The descriptor for the notification event.
> 
> +  @param Ppi                Pointer to the PPI in question.
> 
> +
> 
> +  @return Always success
> 
> +
> 
> +**/
> 
> +STATIC
> 
> +EFI_STATUS
> 
> +EFIAPI
> 
> +MemoryAttributePpiNotifyCallback (
> 
> +  IN EFI_PEI_SERVICES           **PeiServices,
> 
> +  IN EFI_PEI_NOTIFY_DESCRIPTOR  *NotifyDescriptor,
> 
> +  IN VOID                       *Ppi
> 
> +  )
> 
> +{
> 
> +  PEI_CORE_INSTANCE  *PrivateData;
> 
> +
> 
> +  //
> 
> +  // Get PEI Core private data
> 
> +  //
> 
> +  PrivateData = PEI_CORE_INSTANCE_FROM_PS_THIS (PeiServices);
> 
> +
> 
> +  //
> 
> +  // If there isn't a memory attribute PPI installed, use the one from
> 
> +  // notification
> 
> +  //
> 
> +  if (PrivateData->MemoryAttributePpi == NULL) {
> 
> +    PrivateData->MemoryAttributePpi = (EDKII_MEMORY_ATTRIBUTE_PPI *)Ppi;
> 
> +  }
> 
> +
> 
> +  return EFI_SUCCESS;
> 
> +}
> 
> +
> 
> +STATIC CONST EFI_PEI_NOTIFY_DESCRIPTOR  mNotifyList = {
> 
> +  EFI_PEI_PPI_DESCRIPTOR_NOTIFY_DISPATCH | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST,
> 
> +  &gEdkiiMemoryAttributePpiGuid,
> 
> +  MemoryAttributePpiNotifyCallback
> 
> +};
> 
> +
> 
>   /**
> 
>   
> 
>     Support routine for the PE/COFF Loader that reads a buffer from a PE/COFF file.
> 
> @@ -243,6 +287,106 @@ GetPeCoffImageFixLoadingAssignedAddress (
>     return Status;
> 
>   }
> 
>   
> 
> +/**
> 
> +  Remap the memory region covering a loaded image so it can be executed.
> 
> +
> 
> +  @param ImageContext    Pointer to the image context structure that describes the
> 
> +                         PE/COFF image that needs to be examined by this function.
> 
> +  @param FileType        The FFS file type of the image
> 
> +  @param ImageAddress    The start of the memory region covering the image
> 
> +  @param ImageSize       The size of the memory region covering the image
> 
> +
> 
> +  @retval EFI_SUCCESS           The image is ready to be executed
> 
> +  @retval EFI_OUT_OF_RESOURCES  Not enough memory available to update the memory
> 
> +                                mapping
> 
> +
> 
> +**/
> 
> +STATIC
> 
> +EFI_STATUS
> 
> +RemapLoadedImageForExecution (
> 
> +  IN  CONST PE_COFF_LOADER_IMAGE_CONTEXT  *ImageContext,
> 
> +  IN  EFI_FV_FILETYPE                     FileType,
> 
> +  IN  PHYSICAL_ADDRESS                    ImageAddress,
> 
> +  IN  UINT64                              ImageSize
> 
> +  )
> 
> +{
> 
> +  PEI_CORE_INSTANCE                    *Private;
> 
> +  EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION  Hdr;
> 
> +  EFI_IMAGE_SECTION_HEADER             *Section;
> 
> +  PHYSICAL_ADDRESS                     SectionAddress;
> 
> +  EFI_STATUS                           Status;
> 
> +  UINT64                               Permissions;
> 
> +  UINTN                                Index;
> 
> +
> 
> +  Private = PEI_CORE_INSTANCE_FROM_PS_THIS (GetPeiServicesTablePointer ());
> 
> +
> 
> +  if (Private->MemoryAttributePpi == NULL) {
> 
> +    return EFI_SUCCESS;

We will have a gap here before the MemoryAttributePpi is installed,
obviously, when CpuPei is installed. Is the expectation that only
dependencies for CpuPei will be launched before and everything else
will have a dependency on CpuPei?

Or, is it that shadowed PEIMs won't happen before CpuPei? I am curious
how big or small of a gap this really is.

> 
> +  }
> 
> +
> 
> +  //
> 
> +  // PEI phase executables must be able to execute in place from read-only NOR
> 
> +  // flash, and so they can be mapped read-only in their entirety.
> 
> +  //
> 
> +  if ((FileType == EFI_FV_FILETYPE_PEI_CORE) ||
> 
> +      (FileType == EFI_FV_FILETYPE_PEIM) ||
> 
> +      (FileType == EFI_FV_FILETYPE_COMBINED_PEIM_DRIVER))
> 
> +  {

We are calling this from PEI Core, will we have more images of type
PEI Core? I understand if this is for completeness, but just making
sure I understand the flow.

Thanks,
Oliver

> 
> +    return Private->MemoryAttributePpi->SetPermissions (
> 
> +                                          Private->MemoryAttributePpi,
> 
> +                                          ImageAddress,
> 
> +                                          ImageSize,
> 
> +                                          EFI_MEMORY_RO,
> 
> +                                          EFI_MEMORY_XP
> 
> +                                          );
> 
> +  }
> 
> +
> 
> +  //
> 
> +  // Only PE images with minimum 4k section alignment can be remapped with
> 
> +  // restricted permissions.
> 
> +  //
> 
> +  if (ImageContext->IsTeImage ||
> 
> +      (ImageContext->SectionAlignment < EFI_PAGE_SIZE))
> 
> +  {
> 
> +    return EFI_UNSUPPORTED;
> 
> +  }
> 
> +
> 
> +  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++) {
> 
> +    SectionAddress = ImageContext->ImageAddress + Section[Index].VirtualAddress;
> 
> +    Permissions    = 0;
> 
> +
> 
> +    if ((Section[Index].Characteristics & EFI_IMAGE_SCN_MEM_WRITE) == 0) {
> 
> +      Permissions |= EFI_MEMORY_RO;
> 
> +    }
> 
> +
> 
> +    if ((Section[Index].Characteristics & EFI_IMAGE_SCN_MEM_EXECUTE) == 0) {
> 
> +      Permissions |= EFI_MEMORY_XP;
> 
> +    }
> 
> +
> 
> +    Status = Private->MemoryAttributePpi->SetPermissions (
> 
> +                                            Private->MemoryAttributePpi,
> 
> +                                            SectionAddress,
> 
> +                                            Section[Index].Misc.VirtualSize,
> 
> +                                            Permissions,
> 
> +                                            Permissions ^ EFI_MEMORY_RO ^ EFI_MEMORY_XP
> 
> +                                            );
> 
> +    if (EFI_ERROR (Status)) {
> 
> +      return Status;
> 
> +    }
> 
> +  }
> 
> +
> 
> +  return EFI_SUCCESS;
> 
> +}
> 
> +
> 
>   /**
> 
>   
> 
>     Loads and relocates a PE/COFF image into memory.
> 
> @@ -456,9 +600,24 @@ LoadAndRelocatePeCoffImage (
>   
> 
>     //
> 
>     // Flush the instruction cache so the image data is written before we execute it
> 
> +  // Also ensure that the pages are mapped for execution
> 
>     //
> 
>     if (ImageContext.ImageAddress != (EFI_PHYSICAL_ADDRESS)(UINTN)Pe32Data) {
> 
>       InvalidateInstructionCacheRange ((VOID *)(UINTN)ImageContext.ImageAddress, (UINTN)ImageContext.ImageSize);
> 
> +
> 
> +    Status = RemapLoadedImageForExecution (
> 
> +               &ImageContext,
> 
> +               FileInfo.FileType,
> 
> +               ImageContext.ImageAddress & ~(UINT64)EFI_PAGE_MASK,
> 
> +               ALIGN_VALUE (
> 
> +                 AlignImageSize + (ImageContext.ImageAddress & EFI_PAGE_MASK),
> 
> +                 EFI_PAGE_SIZE
> 
> +                 )
> 
> +               );
> 
> +    if (EFI_ERROR (Status)) {
> 
> +      ASSERT_EFI_ERROR (Status);
> 
> +      return Status;
> 
> +    }
> 
>     }
> 
>   
> 
>     *ImageAddress = ImageContext.ImageAddress;
> 
> @@ -972,6 +1131,7 @@ InitializeImageServices (
>       //
> 
>       PrivateData->XipLoadFile = &gPpiLoadFilePpiList;
> 
>       PeiServicesInstallPpi (PrivateData->XipLoadFile);
> 
> +    PeiServicesNotifyPpi (&mNotifyList);
> 
>     } else {
> 
>       //
> 
>       // 2nd time we are running from memory so replace the XIP version with the
> 
> diff --git a/MdeModulePkg/Core/Pei/PeiMain.h b/MdeModulePkg/Core/Pei/PeiMain.h
> index 556beddad533989f..5499d53b0bbaf641 100644
> --- a/MdeModulePkg/Core/Pei/PeiMain.h
> +++ b/MdeModulePkg/Core/Pei/PeiMain.h
> @@ -26,6 +26,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>   #include <Ppi/TemporaryRamDone.h>
> 
>   #include <Ppi/SecHobData.h>
> 
>   #include <Ppi/PeiCoreFvLocation.h>
> 
> +#include <Ppi/MemoryAttribute.h>
> 
>   #include <Library/DebugLib.h>
> 
>   #include <Library/PeiCoreEntryPoint.h>
> 
>   #include <Library/BaseLib.h>
> 
> @@ -302,6 +303,11 @@ struct _PEI_CORE_INSTANCE {
>     //
> 
>     EFI_GUID                          *TempFileGuid;
> 
>   
> 
> +  //
> 
> +  // Pointer to the memory attribute PPI
> 
> +  //
> 
> +  EDKII_MEMORY_ATTRIBUTE_PPI        *MemoryAttributePpi;
> 
> +
> 
>     //
> 
>     // Temp Memory Range is not covered by PeiTempMem and Stack.
> 
>     // Those Memory Range will be migrated into physical memory.
> 
> diff --git a/MdeModulePkg/Core/Pei/PeiMain.inf b/MdeModulePkg/Core/Pei/PeiMain.inf
> index 0cf357371a16d872..55d8eb3e862d6418 100644
> --- a/MdeModulePkg/Core/Pei/PeiMain.inf
> +++ b/MdeModulePkg/Core/Pei/PeiMain.inf
> @@ -100,6 +100,7 @@ [Ppis]
>     gEfiPeiReset2PpiGuid                          ## SOMETIMES_CONSUMES
> 
>     gEfiSecHobDataPpiGuid                         ## SOMETIMES_CONSUMES
> 
>     gEfiPeiCoreFvLocationPpiGuid                  ## SOMETIMES_CONSUMES
> 
> +  gEdkiiMemoryAttributePpiGuid                  ## SOMETIMES_CONSUMES
> 
>   
> 
>   [Pcd]
> 
>     gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxPeiStackSize                  ## CONSUMES
>