Re: [PATCH v1 15/18] ArmPkg: Extra action to update permissions for S-ELO MM Image.

[Supreeth Venkatesh <Supreeth.Venkatesh@arm.com>]

My response inline.

-----Original Message-----
From: Achin Gupta
Sent: Monday, April 30, 2018 2:49 PM
To: Supreeth Venkatesh <Supreeth.Venkatesh@arm.com>
Cc: edk2-devel@lists.01.org; michael.d.kinney@intel.com; liming.gao@intel.com; jiewen.yao@intel.com; leif.lindholm@linaro.org; ard.biesheuvel@linaro.org; nd <nd@arm.com>
Subject: Re: [PATCH v1 15/18] ArmPkg: Extra action to update permissions for S-ELO MM Image.

Hi Supreeth,

This file was originally contributed by Ard a while back so worth poking him and Leif for review. If MM is expected to be the only use case of this library then it might make sense to pull in under the StandaloneMmPkg instead of relying on PcdStandaloneMmEnable.
[Supreeth] I will let Ard and Leif comment on it.
It's basically a debate between code duplication of the entire library in StandaloneMmPkg Vs addition of one PCD check and one function in existing ArmPkg library.

Cheers,
Achin

On Fri, Apr 06, 2018 at 03:42:20PM +0100, Supreeth Venkatesh wrote:
> The Standalone MM drivers runs in S-EL0 in AArch64 on ARM Standard
> Platforms and is deployed during SEC phase. The memory allocated to
> the Standalone MM drivers should be marked as RO+X.
>
> During PE/COFF Image section parsing, this patch implements extra
> action "UpdatePeCoffPermissions" to request the privileged firmware in
> EL3 to update the permissions.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Achin Gupta <achin.gupta@arm.com>
> Signed-off-by: Supreeth Venkatesh <supreeth.venkatesh@arm.com>
> ---
>  .../DebugPeCoffExtraActionLib.c                    | 185 +++++++++++++++++++--
>  .../DebugPeCoffExtraActionLib.inf                  |   7 +
>  2 files changed, 181 insertions(+), 11 deletions(-)
>
> diff --git
> a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
> b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
> index f298e58cdf..c87aaf05c7 100644
> ---
> a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
> +++ b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionL
> +++ ib.c
> @@ -15,14 +15,165 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>  **/
>
>  #include <PiDxe.h>
> -#include <Library/PeCoffLib.h>
>
> +#include <Library/ArmMmuLib.h>
>  #include <Library/BaseLib.h>
> -#include <Library/DebugLib.h>
>  #include <Library/BaseMemoryLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/PcdLib.h>
> +#include <Library/PeCoffLib.h>
>  #include <Library/PeCoffExtraActionLib.h>  #include
> <Library/PrintLib.h>
>
> +typedef RETURN_STATUS (*REGION_PERMISSION_UPDATE_FUNC) (
> +  IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
> +  IN  UINT64                    Length
> +  );
> +
> +STATIC
> +RETURN_STATUS
> +UpdatePeCoffPermissions (
> +  IN  CONST PE_COFF_LOADER_IMAGE_CONTEXT      *ImageContext,
> +  IN  REGION_PERMISSION_UPDATE_FUNC           NoExecUpdater,
> +  IN  REGION_PERMISSION_UPDATE_FUNC           ReadOnlyUpdater
> +  )
> +{
> +  RETURN_STATUS                         Status;
> +  EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION   Hdr;
> +  EFI_IMAGE_OPTIONAL_HEADER_UNION       HdrData;
> +  UINTN                                 Size;
> +  UINTN                                 ReadSize;
> +  UINT32                                SectionHeaderOffset;
> +  UINTN                                 NumberOfSections;
> +  UINTN                                 Index;
> +  EFI_IMAGE_SECTION_HEADER              SectionHeader;
> +  PE_COFF_LOADER_IMAGE_CONTEXT          TmpContext;
> +  EFI_PHYSICAL_ADDRESS                  Base;
> +
> +  //
> +  // We need to copy ImageContext since PeCoffLoaderGetImageInfo ()
> + // will mangle the ImageAddress field  //  CopyMem (&TmpContext,
> + ImageContext, sizeof (TmpContext));
> +
> +  if (TmpContext.PeCoffHeaderOffset == 0) {
> +    Status = PeCoffLoaderGetImageInfo (&TmpContext);
> +    if (RETURN_ERROR (Status)) {
> +      DEBUG ((DEBUG_ERROR,
> +        "%a: PeCoffLoaderGetImageInfo () failed (Status = %r)\n",
> +        __FUNCTION__, Status));
> +      return Status;
> +    }
> +  }
> +
> +  if (TmpContext.IsTeImage &&
> +      TmpContext.ImageAddress == ImageContext->ImageAddress) {
> +    DEBUG ((DEBUG_INFO, "%a: ignoring XIP TE image at 0x%lx\n", __FUNCTION__,
> +      ImageContext->ImageAddress));
> +    return RETURN_SUCCESS;
> +  }
> +
> +  if (TmpContext.SectionAlignment < EFI_PAGE_SIZE) {
> +    //
> +    // The sections need to be at least 4 KB aligned, since that is the
> +    // granularity at which we can tighten permissions. So just clear the
> +    // noexec permissions on the entire region.
> +    //
> +    if (!TmpContext.IsTeImage) {
> +      DEBUG ((DEBUG_WARN,
> +        "%a: non-TE Image at 0x%lx has SectionAlignment < 4 KB (%lu)\n",
> +        __FUNCTION__, ImageContext->ImageAddress, TmpContext.SectionAlignment));
> +    }
> +    Base = ImageContext->ImageAddress & ~(EFI_PAGE_SIZE - 1);
> +    Size = ImageContext->ImageAddress - Base + ImageContext->ImageSize;
> +    return NoExecUpdater (Base, ALIGN_VALUE (Size, EFI_PAGE_SIZE));
> + }
> +
> +  //
> +  // Read the PE/COFF Header. For PE32 (32-bit) this will read in too
> + much  // data, but that should not hurt anything.
> + Hdr.Pe32->OptionalHeader.Magic  // determines if this is a PE32 or
> + PE32+ image. The magic is in the same  // location in both images.
> +  //
> +  Hdr.Union = &HdrData;
> +  Size = sizeof (EFI_IMAGE_OPTIONAL_HEADER_UNION);
> +  ReadSize = Size;
> +  Status = TmpContext.ImageRead (TmpContext.Handle,
> +                         TmpContext.PeCoffHeaderOffset, &Size,
> + Hdr.Pe32);  if (RETURN_ERROR (Status) || (Size != ReadSize)) {
> +    DEBUG ((DEBUG_ERROR,
> +      "%a: TmpContext.ImageRead () failed (Status = %r)\n",
> +      __FUNCTION__, Status));
> +    return Status;
> +  }
> +
> +  ASSERT (Hdr.Pe32->Signature == EFI_IMAGE_NT_SIGNATURE);
> +
> +  SectionHeaderOffset = TmpContext.PeCoffHeaderOffset + sizeof (UINT32) +
> +                        sizeof (EFI_IMAGE_FILE_HEADER);
> +  NumberOfSections    = (UINTN)(Hdr.Pe32->FileHeader.NumberOfSections);
> +
> +  switch (Hdr.Pe32->OptionalHeader.Magic) {
> +    case EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC:
> +      SectionHeaderOffset += Hdr.Pe32->FileHeader.SizeOfOptionalHeader;
> +      break;
> +    case EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC:
> +      SectionHeaderOffset += Hdr.Pe32Plus->FileHeader.SizeOfOptionalHeader;
> +      break;
> +    default:
> +      ASSERT (FALSE);
> +  }
> +
> +  //
> +  // Iterate over the sections
> +  //
> +  for (Index = 0; Index < NumberOfSections; Index++) {
> +    //
> +    // Read section header from file
> +    //
> +    Size = sizeof (EFI_IMAGE_SECTION_HEADER);
> +    ReadSize = Size;
> +    Status = TmpContext.ImageRead (TmpContext.Handle, SectionHeaderOffset,
> +                                   &Size, &SectionHeader);
> +    if (RETURN_ERROR (Status) || (Size != ReadSize)) {
> +      DEBUG ((DEBUG_ERROR,
> +        "%a: TmpContext.ImageRead () failed (Status = %r)\n",
> +        __FUNCTION__, Status));
> +      return Status;
> +    }
> +
> +    Base = TmpContext.ImageAddress + SectionHeader.VirtualAddress;
> +
> +    if ((SectionHeader.Characteristics & EFI_IMAGE_SCN_MEM_EXECUTE)
> + == 0) {
> +
> +      if ((SectionHeader.Characteristics & EFI_IMAGE_SCN_MEM_WRITE) == 0 &&
> +          TmpContext.ImageType !=
> + EFI_IMAGE_SUBSYSTEM_EFI_RUNTIME_DRIVER) {
> +
> +        DEBUG ((DEBUG_INFO,
> +          "%a: Mapping section %d of image at 0x%lx with RO-XN permissions and size 0x%x\n",
> +          __FUNCTION__, Index, Base, SectionHeader.Misc.VirtualSize));
> +        ReadOnlyUpdater (Base, SectionHeader.Misc.VirtualSize);
> +      } else {
> +        DEBUG ((DEBUG_WARN,
> +          "%a: Mapping section %d of image at 0x%lx with RW-XN permissions and size 0x%x\n",
> +          __FUNCTION__, Index, Base, SectionHeader.Misc.VirtualSize));
> +      }
> +    } else {
> +        DEBUG ((DEBUG_INFO,
> +          "%a: Mapping section %d of image at 0x%lx with RO-XN permissions and size 0x%x\n",
> +           __FUNCTION__, Index, Base, SectionHeader.Misc.VirtualSize));
> +        ReadOnlyUpdater (Base, SectionHeader.Misc.VirtualSize);
> +
> +        DEBUG ((DEBUG_INFO,
> +          "%a: Mapping section %d of image at 0x%lx with RO-X permissions and size 0x%x\n",
> +          __FUNCTION__, Index, Base, SectionHeader.Misc.VirtualSize));
> +        NoExecUpdater (Base, SectionHeader.Misc.VirtualSize);
> +    }
> +
> +    SectionHeaderOffset += sizeof (EFI_IMAGE_SECTION_HEADER);
> +  }
> +  return RETURN_SUCCESS;
> +}
>
>  /**
>    If the build is done on cygwin the paths are cygpaths.
> @@ -83,23 +234,29 @@ PeCoffLoaderRelocateImageExtraAction (
>    CHAR8 Temp[512];
>  #endif
>
> +  if (PcdGetBool(PcdStandaloneMmEnable) == TRUE)  {
> +     UpdatePeCoffPermissions (ImageContext, ArmClearMemoryRegionNoExec,
> +                              ArmSetMemoryRegionReadOnly);  }
> +
>    if (ImageContext->PdbPointer) {
>  #ifdef __CC_ARM
>  #if (__ARMCC_VERSION < 500000)
>      // Print out the command for the RVD debugger to load symbols for this image
> -    DEBUG ((EFI_D_LOAD | EFI_D_INFO, "load /a /ni /np %a &0x%p\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)), (UINTN)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders)));
> +    DEBUG ((DEBUG_LOAD | DEBUG_INFO, "load /a /ni /np %a &0x%p\n",
> + DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof
> + (Temp)), (UINTN)(ImageContext->ImageAddress +
> + ImageContext->SizeOfHeaders)));
>  #else
>      // Print out the command for the DS-5 to load symbols for this image
> -    DEBUG ((EFI_D_LOAD | EFI_D_INFO, "add-symbol-file %a 0x%p\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)), (UINTN)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders)));
> +    DEBUG ((DEBUG_LOAD | DEBUG_INFO, "add-symbol-file %a 0x%p\n",
> + DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof
> + (Temp)), (UINTN)(ImageContext->ImageAddress +
> + ImageContext->SizeOfHeaders)));
>  #endif
>  #elif __GNUC__
>      // This may not work correctly if you generate PE/COFF directlyas then the Offset would not be required
> -    DEBUG ((EFI_D_LOAD | EFI_D_INFO, "add-symbol-file %a 0x%p\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)), (UINTN)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders)));
> +    DEBUG ((DEBUG_LOAD | DEBUG_INFO, "add-symbol-file %a 0x%p\n",
> + DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof
> + (Temp)), (UINTN)(ImageContext->ImageAddress +
> + ImageContext->SizeOfHeaders)));
>  #else
> -    DEBUG ((EFI_D_LOAD | EFI_D_INFO, "Loading driver at 0x%11p EntryPoint=0x%11p\n", (VOID *)(UINTN) ImageContext->ImageAddress, FUNCTION_ENTRY_POINT (ImageContext->EntryPoint)));
> +    DEBUG ((DEBUG_LOAD | DEBUG_INFO, "Loading driver at 0x%11p
> + EntryPoint=0x%11p\n", (VOID *)(UINTN) ImageContext->ImageAddress,
> + FUNCTION_ENTRY_POINT (ImageContext->EntryPoint)));
>  #endif
>    } else {
> -    DEBUG ((EFI_D_LOAD | EFI_D_INFO, "Loading driver at 0x%11p EntryPoint=0x%11p\n", (VOID *)(UINTN) ImageContext->ImageAddress, FUNCTION_ENTRY_POINT (ImageContext->EntryPoint)));
> +    DEBUG ((DEBUG_LOAD | DEBUG_INFO, "Loading driver at 0x%11p
> + EntryPoint=0x%11p\n", (VOID *)(UINTN) ImageContext->ImageAddress,
> + FUNCTION_ENTRY_POINT (ImageContext->EntryPoint)));
>    }
>  }
>
> @@ -125,17 +282,23 @@ PeCoffLoaderUnloadImageExtraAction (
>    CHAR8 Temp[512];
>  #endif
>
> +  if (PcdGetBool(PcdStandaloneMmEnable) == TRUE)  {
> +     UpdatePeCoffPermissions (ImageContext, ArmSetMemoryRegionNoExec,
> +                              ArmClearMemoryRegionReadOnly);  }
> +
>    if (ImageContext->PdbPointer) {
>  #ifdef __CC_ARM
>      // Print out the command for the RVD debugger to load symbols for this image
> -    DEBUG ((EFI_D_ERROR, "unload symbols_only %a\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp))));
> +    DEBUG ((DEBUG_ERROR, "unload symbols_only %a\n",
> + DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof
> + (Temp))));
>  #elif __GNUC__
>      // This may not work correctly if you generate PE/COFF directlyas then the Offset would not be required
> -    DEBUG ((EFI_D_ERROR, "remove-symbol-file %a 0x%08x\n", DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)), (UINTN)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders)));
> +    DEBUG ((DEBUG_ERROR, "remove-symbol-file %a 0x%08x\n",
> + DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof
> + (Temp)), (UINTN)(ImageContext->ImageAddress +
> + ImageContext->SizeOfHeaders)));
>  #else
> -    DEBUG ((EFI_D_ERROR, "Unloading %a\n", ImageContext->PdbPointer));
> +    DEBUG ((DEBUG_ERROR, "Unloading %a\n",
> + ImageContext->PdbPointer));
>  #endif
>    } else {
> -    DEBUG ((EFI_D_ERROR, "Unloading driver at 0x%11p\n", (VOID *)(UINTN) ImageContext->ImageAddress));
> +    DEBUG ((DEBUG_ERROR, "Unloading driver at 0x%11p\n", (VOID
> + *)(UINTN) ImageContext->ImageAddress));
>    }
>  }
> diff --git
> a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.i
> nf
> b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.i
> nf
> index c1f717e5bd..38bf3993ae 100644
> ---
> a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.i
> nf
> +++ b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionL
> +++ ib.inf
> @@ -33,7 +33,14 @@
>    DebugPeCoffExtraActionLib.c
>
>  [Packages]
> +  ArmPkg/ArmPkg.dec
>    MdePkg/MdePkg.dec
> +  StandaloneMmPkg/StandaloneMmPkg.dec
> +
> +[FeaturePcd]
> +  gStandaloneMmPkgTokenSpaceGuid.PcdStandaloneMmEnable
>
>  [LibraryClasses]
> +  ArmMmuLib
>    DebugLib
> +  PcdLib
> --
> 2.16.2
>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.