My response inline.
-----Original Message-----
From: Achin Gupta
Sent: Monday, April 30, 2018 2:49 PM
To: Supreeth Venkatesh <supreeth.venkat...@arm.com>
Cc: edk2-devel@lists.01.org; michael.d.kin...@intel.com; liming....@intel.com;
jiewen....@intel.com; leif.lindh...@linaro.org; ard.biesheu...@linaro.org; nd
<n...@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.gu...@arm.com>
> Signed-off-by: Supreeth Venkatesh <supreeth.venkat...@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.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
