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

2018-05-04 Thread Supreeth Venkatesh 
My response inline.

-Original Message-
From: Achin Gupta
Sent: Monday, April 30, 2018 2:49 PM
To: Supreeth Venkatesh 
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 

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 
> Signed-off-by: Supreeth Venkatesh 
> ---
>  .../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 
> -#include 
>
> +#include 
>  #include 
> -#include 
>  #include 
> +#include 
> +#include 
> +#include 
>  #include   #include
> 
>
> +typedef RETURN_STATUS (*REGION_PERMISSION_UPDATE_FUNC) (
> +  IN  EFI_PHYSICAL_ADDRESS  BaseAddress,
> +  IN  UINT64Length
> +  );
> +
> +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;
> +  UINT32SectionHeaderOffset;
> +  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 (,
> + ImageContext, sizeof (TmpContext));
> +
> +  if (TmpContext.PeCoffHeaderOffset == 0) {
> +Status = PeCoffLoaderGetImageInfo ();
> +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

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

2018-04-30 Thread Achin Gupta 
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.

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 
> Signed-off-by: Supreeth Venkatesh 
> ---
>  .../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/DebugPeCoffExtraActionLib.c
> @@ -15,14 +15,165 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, 
> EITHER EXPRESS OR IMPLIED.
>  **/
>  
>  #include 
> -#include 
>  
> +#include 
>  #include 
> -#include 
>  #include 
> +#include 
> +#include 
> +#include 
>  #include 
>  #include 
>  
> +typedef RETURN_STATUS (*REGION_PERMISSION_UPDATE_FUNC) (
> +  IN  EFI_PHYSICAL_ADDRESS  BaseAddress,
> +  IN  UINT64Length
> +  );
> +
> +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;
> +  UINT32SectionHeaderOffset;
> +  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 (, ImageContext, sizeof (TmpContext));
> +
> +  if (TmpContext.PeCoffHeaderOffset == 0) {
> +Status = PeCoffLoaderGetImageInfo ();
> +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 = 
> +  Size = sizeof (EFI_IMAGE_OPTIONAL_HEADER_UNION);
> +  ReadSize = Size;
> +  Status = TmpContext.ImageRead (TmpContext.Handle,
> + TmpContext.PeCoffHeaderOffset, , 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

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

2018-04-06 Thread Supreeth Venkatesh 
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 
Signed-off-by: Supreeth Venkatesh 
---
 .../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/DebugPeCoffExtraActionLib.c
@@ -15,14 +15,165 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 **/
 
 #include 
-#include 
 
+#include 
 #include 
-#include 
 #include 
+#include 
+#include 
+#include 
 #include 
 #include 
 
+typedef RETURN_STATUS (*REGION_PERMISSION_UPDATE_FUNC) (
+  IN  EFI_PHYSICAL_ADDRESS  BaseAddress,
+  IN  UINT64Length
+  );
+
+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;
+  UINT32SectionHeaderOffset;
+  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 (, ImageContext, sizeof (TmpContext));
+
+  if (TmpContext.PeCoffHeaderOffset == 0) {
+Status = PeCoffLoaderGetImageInfo ();
+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 = 
+  Size = sizeof (EFI_IMAGE_OPTIONAL_HEADER_UNION);
+  ReadSize = Size;
+  Status = TmpContext.ImageRead (TmpContext.Handle,
+ TmpContext.PeCoffHeaderOffset, , 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);
+  }
+
+  //
+  //