[PATCH v3 0/2] StandaloneMM: Update permissions for Standalone MM drivers memory area

[PATCH v3 0/2] StandaloneMM: Update permissions for Standalone MM drivers memory area

[Sughosh Ganu]

Changes since v2:
Incorporate review comments from Achin to move
StandaloneMmPeCoffExtraActionLib.c under AArch64 directory.

Changes since v1:
A new patch has been added to reflect the library class added for
changing the MMU attributes in StandaloneMM image, based on review
comments from Ard Biesheuvel.

Sughosh Ganu (2):
  StandaloneMM: Include the newly added library class for MMU functions
  StandaloneMM: Update permissions for Standalone MM drivers memory area

 StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf                                                                                   |   2 +-
 ArmPkg/Library/RvdPeCoffExtraActionLib/RvdPeCoffExtraActionLib.inf => StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/StandaloneMmPeCoffExtraActionLib.inf |  18 +-
 StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/AArch64/StandaloneMmPeCoffExtraActionLib.c                                                                 | 222 ++++++++++++++++++++
 3 files changed, 234 insertions(+), 8 deletions(-)
 copy ArmPkg/Library/RvdPeCoffExtraActionLib/RvdPeCoffExtraActionLib.inf => StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/StandaloneMmPeCoffExtraActionLib.inf (72%)
 create mode 100644 StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/AArch64/StandaloneMmPeCoffExtraActionLib.c

-- 
2.7.4

[PATCH v3 1/2] StandaloneMM: Include the newly added library class for MMU functions

[Sughosh Ganu]

The MMU functions needed for StandaloneMM image are now exported
through a separate library class. Make the corresponding change in the
core's entry point inf file so that it references the correct library
class for modifying the MMU attributes.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Sughosh Ganu <sughosh.ganu@arm.com>
---
 StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
index 66184c4a00f3..3222cd359f3e 100644
--- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
@@ -45,7 +45,7 @@ [LibraryClasses]
   DebugLib
 
 [LibraryClasses.AARCH64]
-  ArmMmuLib
+  StandaloneMmMmuLib
   ArmSvcLib
 
 [Guids]
-- 
2.7.4

[PATCH v3 2/2] StandaloneMM: Update permissions for Standalone MM drivers memory area

[Sughosh Ganu]

The StandaloneMM image executes in S-EL0 on reference Arm platforms
and is deployed by the trusted firmware as BL32 image. Memory for the
Standalone MM drivers is marked as RW+XN initially, allowing the
drivers to be loaded into the memory. Once loaded, the memory
attributes need to be changed to RO+XN for rodata sections and RO+X
for code sections.

Achieve this through the 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>
---
 ArmPkg/Library/RvdPeCoffExtraActionLib/RvdPeCoffExtraActionLib.inf => StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/StandaloneMmPeCoffExtraActionLib.inf |  18 +-
 StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/AArch64/StandaloneMmPeCoffExtraActionLib.c                                                                 | 222 ++++++++++++++++++++
 2 files changed, 233 insertions(+), 7 deletions(-)

diff --git a/ArmPkg/Library/RvdPeCoffExtraActionLib/RvdPeCoffExtraActionLib.inf b/StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/StandaloneMmPeCoffExtraActionLib.inf
similarity index 72%
copy from ArmPkg/Library/RvdPeCoffExtraActionLib/RvdPeCoffExtraActionLib.inf
copy to StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/StandaloneMmPeCoffExtraActionLib.inf
index 3be0237a3689..eef3d7c6e253 100644
--- a/ArmPkg/Library/RvdPeCoffExtraActionLib/RvdPeCoffExtraActionLib.inf
+++ b/StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/StandaloneMmPeCoffExtraActionLib.inf
@@ -16,9 +16,9 @@
 #**/
 
 [Defines]
-  INF_VERSION                    = 0x00010005
-  BASE_NAME                      = RvdUnixPeCoffExtraActionLib
-  FILE_GUID                      = 5EDEB7E7-EA55-4E92-8216-335AC98A3B11
+  INF_VERSION                    = 0x0001000A
+  BASE_NAME                      = StandaloneMmPeCoffExtraActionLib
+  FILE_GUID                      = 8B40543B-9588-48F8-840C-5A60E6DB1B03
   MODULE_TYPE                    = BASE
   VERSION_STRING                 = 1.0
   LIBRARY_CLASS                  = PeCoffExtraActionLib
@@ -30,12 +30,16 @@ [Defines]
 #
 
 [Sources.common]
-  RvdPeCoffExtraActionLib.c
+  AArch64/StandaloneMmPeCoffExtraActionLib.c
 
 [Packages]
-  MdePkg/MdePkg.dec
   ArmPkg/ArmPkg.dec
+  MdePkg/MdePkg.dec
+  StandaloneMmPkg/StandaloneMmPkg.dec
+
+[FeaturePcd]
+  gStandaloneMmPkgTokenSpaceGuid.PcdStandaloneMmEnable
 
 [LibraryClasses]
-  DebugLib
-  SemihostLib
+  StandaloneMmMmuLib
+  PcdLib
diff --git a/StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/AArch64/StandaloneMmPeCoffExtraActionLib.c b/StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/AArch64/StandaloneMmPeCoffExtraActionLib.c
new file mode 100644
index 000000000000..1c9fec201916
--- /dev/null
+++ b/StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/AArch64/StandaloneMmPeCoffExtraActionLib.c
@@ -0,0 +1,222 @@
+/**@file
+
+Copyright (c) 2006 - 2009, Intel Corporation. All rights reserved.<BR>
+Portions copyright (c) 2008 - 2010, Apple Inc. All rights reserved.<BR>
+Portions copyright (c) 2011 - 2018, ARM Ltd. All rights reserved.<BR>
+
+This program and the accompanying materials
+are licensed and made available under the terms and conditions of the BSD License
+which accompanies this distribution.  The full text of the license may be found at
+http://opensource.org/licenses/bsd-license.php
+
+THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#include <PiDxe.h>
+
+#include <Library/ArmMmuLib.h>
+#include <Library/BaseLib.h>
+#include <Library/BaseMemoryLib.h>
+#include <Library/DebugLib.h>
+#include <Library/PcdLib.h>
+#include <Library/PeCoffLib.h>
+#include <Library/PeCoffExtraActionLib.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;
+}
+
+/**
+  Performs additional actions after a PE/COFF image has been loaded and relocated.
+
+  If ImageContext is NULL, then ASSERT().
+
+  @param  ImageContext  Pointer to the image context structure that describes the
+                        PE/COFF image that has already been loaded and relocated.
+
+**/
+VOID
+EFIAPI
+PeCoffLoaderRelocateImageExtraAction (
+  IN OUT PE_COFF_LOADER_IMAGE_CONTEXT  *ImageContext
+  )
+{
+  UpdatePeCoffPermissions (
+    ImageContext,
+    ArmClearMemoryRegionNoExec,
+    ArmSetMemoryRegionReadOnly
+    );
+}
+
+
+
+/**
+  Performs additional actions just before a PE/COFF image is unloaded.  Any resources
+  that were allocated by PeCoffLoaderRelocateImageExtraAction() must be freed.
+
+  If ImageContext is NULL, then ASSERT().
+
+  @param  ImageContext  Pointer to the image context structure that describes the
+                        PE/COFF image that is being unloaded.
+
+**/
+VOID
+EFIAPI
+PeCoffLoaderUnloadImageExtraAction (
+  IN OUT PE_COFF_LOADER_IMAGE_CONTEXT  *ImageContext
+  )
+{
+  UpdatePeCoffPermissions (
+    ImageContext,
+    ArmSetMemoryRegionNoExec,
+    ArmClearMemoryRegionReadOnly
+    );
+}
-- 
2.7.4

Re: [PATCH v3 0/2] StandaloneMM: Update permissions for Standalone MM drivers memory area

[Achin Gupta]

Hi Sughosh,

On Mon, Dec 03, 2018 at 12:49:00PM +0530, Sughosh Ganu wrote:
> Changes since v2:
> Incorporate review comments from Achin to move
> StandaloneMmPeCoffExtraActionLib.c under AArch64 directory.
> 
> Changes since v1:
> A new patch has been added to reflect the library class added for
> changing the MMU attributes in StandaloneMM image, based on review
> comments from Ard Biesheuvel.
> 
> Sughosh Ganu (2):
>   StandaloneMM: Include the newly added library class for MMU functions
>   StandaloneMM: Update permissions for Standalone MM drivers memory area
> 
>  StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf                                                                                   |   2 +-
>  ArmPkg/Library/RvdPeCoffExtraActionLib/RvdPeCoffExtraActionLib.inf => StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/StandaloneMmPeCoffExtraActionLib.inf |  18 +-
>  StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/AArch64/StandaloneMmPeCoffExtraActionLib.c                                                                 | 222 ++++++++++++++++++++
>  3 files changed, 234 insertions(+), 8 deletions(-)
>  copy ArmPkg/Library/RvdPeCoffExtraActionLib/RvdPeCoffExtraActionLib.inf => StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/StandaloneMmPeCoffExtraActionLib.inf (72%)
>  create mode 100644 StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/AArch64/StandaloneMmPeCoffExtraActionLib.c
> 
> -- 
> 2.7.4
> 
> 

Reviewed-by: Achin Gupta <achin.gupta@arm.com>

Pushed as 34b1d7eafee0..f7f94ffe8828