Re: [PATCH v2 7/7] ArmPkg: Extra action to update permissions for S-ELO MM Image

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

On Sat, 2018-07-21 at 20:06 +0900, Ard Biesheuvel wrote:
> On 20 July 2018 at 21:38, Sughosh Ganu <sughosh.ganu@arm.com> wrote:
> > 
> > From: Achin Gupta <achin.gupta@arm.com>
> > 
> > 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: Sughosh Ganu <sughosh.ganu@arm.com>
> Apologies for bringing this up only now, but I don't think I was ever
> cc'ed on these patches.
Apologies if you have missed it. But I am pretty sure it was part of
earlier large patch-set on which you and leif were copied, as it was
part of ArmPkg.
> 
> We are relying on a debug hook in the PE/COFF loader to ensure that
> we
> don't end up with memory that is both writable and executable in the
> secure world. Do we really think that is a good idea?
> 
> (I know this code was derived from a proof of concept that I did
> years
> ago, but that was just a PoC)
I think we need a little bit more details on what is your suggestion?

A little bit background here: This code runs in S-EL0 and Request gets
sent to secure world SPM to ensure that the region permissions are
updated correctly via the "ArmMmuStandaloneMmCoreLib" SVC -
ARM_SVC_ID_SP_SET_MEM_ATTRIBUTES_AARCH64.
 
DebugPeCoffExtraActionLib is just used to extract image region
information, but the region permission
update request is sent to secure world for validation.

With the above explanation, can you provide an insight into what was
your thinking?
Do you want us to create a separate library and call it
as PeCoffExtraActionLib to avoid the "Debug" word though it is a hook
to PeCoffExtraActionLib in MdePkg or do we want to create this library
in a separate package (may be in MdePkg?) or something totally
different.

> 
> > 
> > ---
> >  ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib
> > .inf |   7 +
> >  ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib
> > .c   | 171 +++++++++++++++++++-
> >  2 files changed, 176 insertions(+), 2 deletions(-)
> > 
> > diff --git
> > a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLi
> > b.inf
> > b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLi
> > b.inf
> > index c1f717e5bda1..38bf3993ae99 100644
> > ---
> > a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLi
> > b.inf
> > +++
> > b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLi
> > b.inf
> > @@ -33,7 +33,14 @@ [Sources.common]
> >    DebugPeCoffExtraActionLib.c
> > 
> >  [Packages]
> > +  ArmPkg/ArmPkg.dec
> >    MdePkg/MdePkg.dec
> > +  StandaloneMmPkg/StandaloneMmPkg.dec
> > +
> > +[FeaturePcd]
> > +  gStandaloneMmPkgTokenSpaceGuid.PcdStandaloneMmEnable
> > 
> >  [LibraryClasses]
> > +  ArmMmuLib
> >    DebugLib
> > +  PcdLib
> > diff --git
> > a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLi
> > b.c
> > b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLi
> > b.c
> > index f298e58cdfca..8e621de4a87a 100644
> > ---
> > a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLi
> > b.c
> > +++
> > b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLi
> > b.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,6 +234,14 @@ PeCoffLoaderRelocateImageExtraAction (
> >    CHAR8 Temp[512];
> >  #endif
> > 
> > +  if (PcdGetBool(PcdStandaloneMmEnable) == TRUE) {
> > +    UpdatePeCoffPermissions (
> > +      ImageContext,
> > +      ArmClearMemoryRegionNoExec,
> > +      ArmSetMemoryRegionReadOnly
> > +      );
> > +  }
> > +
> >    if (ImageContext->PdbPointer) {
> >  #ifdef __CC_ARM
> >  #if (__ARMCC_VERSION < 500000)
> > @@ -125,6 +284,14 @@ 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
> > --
> > 2.7.4
> > 
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel