[PATCH v2 0/7] ArmPkg related changes for StandaloneMM package

[PATCH v2 0/7] ArmPkg related changes for StandaloneMM package

[Sughosh Ganu]

The following patch series adds support for Management Mode related
changes for aarch64 based platforms.

Changes since v1: Handled review comments from Leif

Achin Gupta (6):
  ArmPkg: Add PCDs needed for MM communication driver.
  ArmPkg/Drivers: Add EFI_MM_COMMUNICATION_PROTOCOL DXE driver.
  ArmPkg/Include: Add MM interface SVC return codes.
  ArmPkg/ArmMmuLib: Add MMU Library suitable for use in S-EL0.
  ArmPkg/ArmMmuLib: Add MMU library inf file suitable for use in S-EL0.
  ArmPkg: Extra action to update permissions for S-ELO MM Image

Sughosh Ganu (1):
  ArmPkg/Include: Fix the SPM version SVC ID

 ArmPkg/ArmPkg.dec                                                      |   3 +
 ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf                  |  56 +++
 ArmPkg/Library/ArmMmuLib/ArmMmuStandaloneMmCoreLib.inf                 |  37 ++
 ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.inf |   7 +
 ArmPkg/Drivers/MmCommunicationDxe/MmCommunicate.h                      |  28 ++
 ArmPkg/Include/IndustryStandard/ArmMmSvc.h                             |   9 +-
 ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c                    | 395 ++++++++++++++++++++
 ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuStandaloneMmCoreLib.c           | 204 ++++++++++
 ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c   | 171 ++++++++-
 9 files changed, 907 insertions(+), 3 deletions(-)
 create mode 100644 ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf
 create mode 100644 ArmPkg/Library/ArmMmuLib/ArmMmuStandaloneMmCoreLib.inf
 create mode 100644 ArmPkg/Drivers/MmCommunicationDxe/MmCommunicate.h
 create mode 100644 ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
 create mode 100644 ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuStandaloneMmCoreLib.c

-- 
2.7.4

[PATCH v2 1/7] ArmPkg: Add PCDs needed for MM communication driver.

[Sughosh Ganu]

From: Achin Gupta <achin.gupta@arm.com>

This patch defines PCDs to describe the base address and size of
communication buffer between normal world (uefi) and standalone MM
environment in the secure world.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Supreeth Venkatesh <supreeth.venkatesh@arm.com>
---
 ArmPkg/ArmPkg.dec | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
index 3aa229fe2ec9..ce108f2258f4 100644
--- a/ArmPkg/ArmPkg.dec
+++ b/ArmPkg/ArmPkg.dec
@@ -239,6 +239,9 @@ [PcdsFixedAtBuild.common, PcdsDynamic.common, PcdsPatchableInModule.common]
   gArmTokenSpaceGuid.PcdSystemMemoryBase|0|UINT64|0x00000029
   gArmTokenSpaceGuid.PcdSystemMemorySize|0|UINT64|0x0000002A
 
+  gArmTokenSpaceGuid.PcdMmBufferBase|0|UINT64|0x00000045
+  gArmTokenSpaceGuid.PcdMmBufferSize|0|UINT64|0x00000046
+
 [PcdsFixedAtBuild.common, PcdsDynamic.common]
   #
   # ARM Architectural Timer
-- 
2.7.4

[PATCH v2 2/7] ArmPkg/Drivers: Add EFI_MM_COMMUNICATION_PROTOCOL DXE driver.

[Sughosh Ganu]

From: Achin Gupta <achin.gupta@arm.com>

PI v1.5 Specification Volume 4 defines Management Mode Core Interface
and defines EFI_MM_COMMUNICATION_PROTOCOL. This protocol provides a
means of communicating between drivers outside of MM and MMI
handlers inside of MM.

This patch implements the EFI_MM_COMMUNICATION_PROTOCOL DXE runtime
driver for AARCH64 platforms. It uses SMCs allocated from the standard
SMC range defined in DEN0060A_ARM_MM_Interface_Specification.pdf
to communicate with the standalone MM environment in the secure world.

This patch also adds the MM Communication driver (.inf) file to
define entry point for this driver and other compile
related information the driver needs.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Supreeth Venkatesh <supreeth.venkatesh@arm.com>
Signed-off-by: Sughosh Ganu <sughosh.ganu@arm.com>
---
 ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf |  56 +++
 ArmPkg/Drivers/MmCommunicationDxe/MmCommunicate.h     |  28 ++
 ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c   | 395 ++++++++++++++++++++
 3 files changed, 479 insertions(+)

diff --git a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf
new file mode 100644
index 000000000000..88beafa39c05
--- /dev/null
+++ b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf
@@ -0,0 +1,56 @@
+#/** @file
+#
+#  DXE MM Communicate driver
+#
+#  Copyright (c) 2016 - 2018, ARM Limited. All rights reserved.
+#
+#  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.
+#
+#**/
+
+[Defines]
+  INF_VERSION                    = 0x0001001A
+  BASE_NAME                      = ArmMmCommunication
+  FILE_GUID                      = 09EE81D3-F15E-43F4-85B4-CB9873DA5D6B
+  MODULE_TYPE                    = DXE_RUNTIME_DRIVER
+  VERSION_STRING                 = 1.0
+  ENTRY_POINT                    = MmCommunicationInitialize
+
+#
+# The following is for reference only and not required by
+# build tools
+#
+# VALID_ARCHITECTURES            = AARCH64
+#
+
+[Sources.AARCH64]
+  MmCommunication.c
+
+[Packages]
+  ArmPkg/ArmPkg.dec
+  MdePkg/MdePkg.dec
+
+[LibraryClasses]
+  ArmLib
+  ArmSmcLib
+  BaseMemoryLib
+  DebugLib
+  DxeServicesTableLib
+  HobLib
+  UefiDriverEntryPoint
+
+[Protocols]
+  gEfiMmCommunicationProtocolGuid              ## PRODUCES
+
+[Pcd.common]
+  gArmTokenSpaceGuid.PcdMmBufferBase
+  gArmTokenSpaceGuid.PcdMmBufferSize
+
+[Depex]
+  gEfiCpuArchProtocolGuid
diff --git a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunicate.h b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunicate.h
new file mode 100644
index 000000000000..a9758914aa0c
--- /dev/null
+++ b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunicate.h
@@ -0,0 +1,28 @@
+/** @file
+
+  Copyright (c) 2016-2018, ARM Limited. All rights reserved.
+
+  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.
+
+**/
+
+#if !defined _MM_COMMUNICATE_H_
+#define _MM_COMMUNICATE_H_
+
+#define MM_MAJOR_VER_MASK        0xEFFF0000
+#define MM_MINOR_VER_MASK        0x0000FFFF
+#define MM_MAJOR_VER_SHIFT       16
+
+#define MM_MAJOR_VER(x) (((x) & MM_MAJOR_VER_MASK) >> MM_MAJOR_VER_SHIFT)
+#define MM_MINOR_VER(x) ((x) & MM_MINOR_VER_MASK)
+
+#define MM_CALLER_MAJOR_VER	0x1UL
+#define MM_CALLER_MINOR_VER	0x0
+
+#endif /* _MM_COMMUNICATE_H_ */
diff --git a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
new file mode 100644
index 000000000000..487db00c2a87
--- /dev/null
+++ b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
@@ -0,0 +1,395 @@
+/** @file
+
+  Copyright (c) 2016-2018, ARM Limited. All rights reserved.
+
+  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 <Library/ArmLib.h>
+#include <Library/ArmSmcLib.h>
+#include <Library/BaseMemoryLib.h>
+#include <Library/DebugLib.h>
+#include <Library/DxeServicesTableLib.h>
+#include <Library/HobLib.h>
+#include <Library/PcdLib.h>
+#include <Library/UefiBootServicesTableLib.h>
+#include <Library/UefiRuntimeServicesTableLib.h>
+
+#include <Protocol/MmCommunication.h>
+
+#include <IndustryStandard/ArmStdSmc.h>
+
+#include "MmCommunicate.h"
+
+//
+// Address, Length of the pre-allocated buffer for communication with the secure
+// world.
+//
+STATIC ARM_MEMORY_REGION_DESCRIPTOR  mNsCommBuffMemRegion;
+
+// Notification event when virtual address map is set.
+STATIC EFI_EVENT  mSetVirtualAddressMapEvent;
+
+//
+// Handle to install the MM Communication Protocol
+//
+STATIC EFI_HANDLE  mMmCommunicateHandle;
+
+/**
+  Communicates with a registered handler.
+
+  This function provides an interface to send and receive messages to the
+  Standalone MM environment on behalf of UEFI services.  This function is part
+  of the MM Communication Protocol that may be called in physical mode prior to
+  SetVirtualAddressMap() and in virtual mode after SetVirtualAddressMap().
+
+  @param[in]      This                The EFI_MM_COMMUNICATION_PROTOCOL
+                                      instance.
+  @param[in, out] CommBuffer          A pointer to the buffer to convey
+                                      into MMRAM.
+  @param[in, out] CommSize            The size of the data buffer being
+                                      passed in. This is optional.
+
+  @retval EFI_SUCCESS                 The message was successfully posted.
+  @retval EFI_INVALID_PARAMETER       The CommBuffer was NULL.
+  @retval EFI_BAD_BUFFER_SIZE         The buffer size is incorrect for the MM
+                                      implementation. If this error is
+                                      returned, the MessageLength field in
+                                      the CommBuffer header or the integer
+                                      pointed by CommSize are updated to reflect
+                                      the maximum payload size the
+                                      implementation can accommodate.
+  @retval EFI_ACCESS_DENIED           The CommunicateBuffer parameter
+                                      or CommSize parameter, if not omitted,
+                                      are in address range that cannot be
+                                      accessed by the MM environment
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+MmCommunicationCommunicate (
+  IN CONST EFI_MM_COMMUNICATION_PROTOCOL  *This,
+  IN OUT VOID                             *CommBuffer,
+  IN OUT UINTN                            *CommSize OPTIONAL
+  )
+{
+  EFI_MM_COMMUNICATE_HEADER   *CommunicateHeader;
+  ARM_SMC_ARGS                CommunicateSmcArgs;
+  EFI_STATUS                  Status;
+  UINTN                       BufferSize;
+
+  Status = EFI_ACCESS_DENIED;
+  BufferSize = 0;
+
+  ZeroMem (&CommunicateSmcArgs, sizeof (ARM_SMC_ARGS));
+
+  //
+  // Check parameters
+  //
+  if (CommBuffer == NULL) {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  CommunicateHeader = CommBuffer;
+  // CommBuffer is a mandatory parameter. Hence, Rely on
+  // MessageLength + Header to ascertain the
+  // total size of the communication payload rather than
+  // rely on optional CommSize parameter
+  BufferSize = CommunicateHeader->MessageLength +
+               sizeof (CommunicateHeader->HeaderGuid) +
+               sizeof (CommunicateHeader->MessageLength);
+
+  // If the length of the CommBuffer is 0 then return the expected length.
+  if (CommSize) {
+    // This case can be used by the consumer of this driver to find out the
+    // max size that can be used for allocating CommBuffer.
+    if ((*CommSize == 0) ||
+        (*CommSize > mNsCommBuffMemRegion.Length)) {
+      *CommSize = mNsCommBuffMemRegion.Length;
+      return EFI_BAD_BUFFER_SIZE;
+    }
+    //
+    // CommSize must match MessageLength + sizeof (EFI_MM_COMMUNICATE_HEADER);
+    //
+    if (*CommSize != BufferSize) {
+        return EFI_INVALID_PARAMETER;
+    }
+  }
+
+  //
+  // If the buffer size is 0 or greater than what can be tolerated by the MM
+  // environment then return the expected size.
+  //
+  if ((BufferSize == 0) ||
+      (BufferSize > mNsCommBuffMemRegion.Length)) {
+    CommunicateHeader->MessageLength = mNsCommBuffMemRegion.Length -
+                                       sizeof (CommunicateHeader->HeaderGuid) -
+                                       sizeof (CommunicateHeader->MessageLength);
+    return EFI_BAD_BUFFER_SIZE;
+  }
+
+  // SMC Function ID
+  CommunicateSmcArgs.Arg0 = ARM_SMC_ID_MM_COMMUNICATE_AARCH64;
+
+  // Cookie
+  CommunicateSmcArgs.Arg1 = 0;
+
+  // Copy Communication Payload
+  CopyMem ((VOID *)mNsCommBuffMemRegion.VirtualBase, CommBuffer, BufferSize);
+
+  // comm_buffer_address (64-bit physical address)
+  CommunicateSmcArgs.Arg2 = (UINTN)mNsCommBuffMemRegion.PhysicalBase;
+
+  // comm_size_address (not used, indicated by setting to zero)
+  CommunicateSmcArgs.Arg3 = 0;
+
+  // Call the Standalone MM environment.
+  ArmCallSmc (&CommunicateSmcArgs);
+
+  switch (CommunicateSmcArgs.Arg0) {
+  case ARM_SMC_MM_RET_SUCCESS:
+    ZeroMem (CommBuffer, BufferSize);
+    // On successful return, the size of data being returned is inferred from
+    // MessageLength + Header.
+    CommunicateHeader = (EFI_MM_COMMUNICATE_HEADER *)mNsCommBuffMemRegion.VirtualBase;
+    BufferSize = CommunicateHeader->MessageLength +
+                 sizeof (CommunicateHeader->HeaderGuid) +
+                 sizeof (CommunicateHeader->MessageLength);
+
+    CopyMem (
+      CommBuffer,
+      (VOID *)mNsCommBuffMemRegion.VirtualBase,
+      BufferSize
+      );
+    Status = EFI_SUCCESS;
+    break;
+
+  case ARM_SMC_MM_RET_INVALID_PARAMS:
+    Status = EFI_INVALID_PARAMETER;
+    break;
+
+  case ARM_SMC_MM_RET_DENIED:
+    Status = EFI_ACCESS_DENIED;
+    break;
+
+  case ARM_SMC_MM_RET_NO_MEMORY:
+    // Unexpected error since the CommSize was checked for zero length
+    // prior to issuing the SMC
+    Status = EFI_OUT_OF_RESOURCES;
+    ASSERT (0);
+    break;
+
+  default:
+    Status = EFI_ACCESS_DENIED;
+    ASSERT (0);
+  }
+
+  return Status;
+}
+
+//
+// MM Communication Protocol instance
+//
+EFI_MM_COMMUNICATION_PROTOCOL  mMmCommunication = {
+  MmCommunicationCommunicate
+};
+
+/**
+  Notification callback on SetVirtualAddressMap event.
+
+  This function notifies the MM communication protocol interface on
+  SetVirtualAddressMap event and converts pointers used in this driver
+  from physical to virtual address.
+
+  @param  Event          SetVirtualAddressMap event.
+  @param  Context        A context when the SetVirtualAddressMap triggered.
+
+  @retval EFI_SUCCESS    The function executed successfully.
+  @retval Other          Some error occurred when executing this function.
+
+**/
+STATIC
+VOID
+EFIAPI
+NotifySetVirtualAddressMap (
+  IN EFI_EVENT  Event,
+  IN VOID      *Context
+  )
+{
+  EFI_STATUS  Status;
+
+  Status = gRT->ConvertPointer (EFI_OPTIONAL_PTR,
+                                (VOID **)&mNsCommBuffMemRegion.VirtualBase
+                               );
+  if (EFI_ERROR (Status)) {
+    DEBUG ((DEBUG_ERROR, "NotifySetVirtualAddressMap():"
+            " Unable to convert MM runtime pointer. Status:0x%r\n", Status));
+  }
+
+}
+
+STATIC
+EFI_STATUS
+GetMmCompatibility ()
+{
+  EFI_STATUS   Status;
+  UINT32       MmVersion;
+  ARM_SMC_ARGS MmVersionArgs;
+
+  // MM_VERSION uses SMC32 calling conventions
+  MmVersionArgs.Arg0 = ARM_SMC_ID_MM_VERSION_AARCH32;
+
+  ArmCallSmc (&MmVersionArgs);
+
+  MmVersion = MmVersionArgs.Arg0;
+
+  if ((MM_MAJOR_VER(MmVersion) == MM_CALLER_MAJOR_VER) &&
+      (MM_MINOR_VER(MmVersion) >= MM_CALLER_MINOR_VER)) {
+    DEBUG ((DEBUG_INFO, "MM Version: Major=0x%x, Minor=0x%x\n",
+            MM_MAJOR_VER(MmVersion), MM_MINOR_VER(MmVersion)));
+    Status = EFI_SUCCESS;
+  } else {
+    DEBUG ((DEBUG_ERROR, "Incompatible MM Versions.\n Current Version: Major=0x%x, Minor=0x%x.\n Expected: Major=0x%x, Minor>=0x%x.\n",
+            MM_MAJOR_VER(MmVersion), MM_MINOR_VER(MmVersion), MM_CALLER_MAJOR_VER, MM_CALLER_MINOR_VER));
+    Status = EFI_UNSUPPORTED;
+  }
+
+  return Status;
+}
+
+/**
+  The Entry Point for MM Communication
+
+  This function installs the MM communication protocol interface and finds out
+  what type of buffer management will be required prior to invoking the
+  communication SMC.
+
+  @param  ImageHandle    The firmware allocated handle for the EFI image.
+  @param  SystemTable    A pointer to the EFI System Table.
+
+  @retval EFI_SUCCESS    The entry point is executed successfully.
+  @retval Other          Some error occurred when executing this entry point.
+
+**/
+EFI_STATUS
+EFIAPI
+MmCommunicationInitialize (
+  IN EFI_HANDLE         ImageHandle,
+  IN EFI_SYSTEM_TABLE  *SystemTable
+  )
+{
+  EFI_STATUS                 Status;
+
+  // Check if we can make the MM call
+  Status = GetMmCompatibility ();
+  if (EFI_ERROR(Status)) {
+    goto ReturnErrorStatus;
+  }
+
+  mNsCommBuffMemRegion.PhysicalBase = PcdGet64 (PcdMmBufferBase);
+  // During boot , Virtual and Physical are same
+  mNsCommBuffMemRegion.VirtualBase = mNsCommBuffMemRegion.PhysicalBase;
+  mNsCommBuffMemRegion.Length = PcdGet64 (PcdMmBufferSize);
+
+  if (mNsCommBuffMemRegion.PhysicalBase == 0) {
+    DEBUG ((DEBUG_ERROR, "MmCommunicateInitialize: "
+            "Invalid MM Buffer Base Address.\n"));
+    goto ReturnErrorStatus;
+  }
+
+  if (mNsCommBuffMemRegion.Length == 0) {
+    DEBUG ((DEBUG_ERROR, "MmCommunicateInitialize: "
+            "Maximum Buffer Size is zero.\n"));
+    goto ReturnErrorStatus;
+  }
+
+  Status = gDS->AddMemorySpace (EfiGcdMemoryTypeSystemMemory,
+                                mNsCommBuffMemRegion.PhysicalBase,
+                                mNsCommBuffMemRegion.Length,
+                                EFI_MEMORY_WB |
+                                EFI_MEMORY_XP |
+                                EFI_MEMORY_RUNTIME);
+  if (EFI_ERROR (Status)) {
+    DEBUG ((DEBUG_ERROR, "MmCommunicateInitialize: "
+            "Failed to add MM-NS Buffer Memory Space\n"));
+    goto ReturnErrorStatus;
+  }
+
+  Status = gDS->SetMemorySpaceAttributes (
+                  mNsCommBuffMemRegion.PhysicalBase,
+                  mNsCommBuffMemRegion.Length,
+                  EFI_MEMORY_WB | EFI_MEMORY_XP
+                  );
+  if (EFI_ERROR (Status)) {
+    DEBUG ((DEBUG_ERROR, "MmCommunicateInitialize: "
+            "Failed to set MM-NS Buffer Memory attributes\n"));
+    goto CleanAddedMemorySpace;
+  }
+
+  Status = gBS->AllocatePages (
+                  AllocateAddress,
+                  EfiRuntimeServicesData,
+                  EFI_SIZE_TO_PAGES (mNsCommBuffMemRegion.Length),
+                  &mNsCommBuffMemRegion.PhysicalBase
+                  );
+  if (EFI_ERROR (Status)) {
+    DEBUG ((DEBUG_ERROR, "MmCommunicateInitialize: "
+            "Failed to allocate MM-NS Buffer Memory Space\n"));
+    goto CleanAddedMemorySpace;
+  }
+
+  // Install the communication protocol
+  Status = gBS->InstallProtocolInterface (
+                  &mMmCommunicateHandle,
+                  &gEfiMmCommunicationProtocolGuid,
+                  EFI_NATIVE_INTERFACE,
+                  &mMmCommunication
+                  );
+  if (EFI_ERROR(Status)) {
+    DEBUG ((DEBUG_ERROR, "MmCommunicationInitialize: "
+            "Failed to install MM communication protocol\n"));
+    goto CleanAllocatedPages;
+  }
+
+  // Register notification callback when virtual address is associated
+  // with the physical address.
+  // Create a Set Virtual Address Map event.
+  Status = gBS->CreateEvent (
+                  EVT_SIGNAL_VIRTUAL_ADDRESS_CHANGE,
+                  TPL_NOTIFY,
+                  NotifySetVirtualAddressMap,
+                  NULL,
+                  &mSetVirtualAddressMapEvent
+                  );
+  if (Status == EFI_SUCCESS) {
+    return Status;
+  }
+
+  gBS->UninstallProtocolInterface (
+         mMmCommunicateHandle,
+         &gEfiMmCommunicationProtocolGuid,
+         &mMmCommunication
+         );
+
+CleanAllocatedPages:
+  gBS->FreePages (
+         mNsCommBuffMemRegion.PhysicalBase,
+         EFI_SIZE_TO_PAGES (mNsCommBuffMemRegion.Length)
+         );
+
+CleanAddedMemorySpace:
+  gDS->RemoveMemorySpace (
+         mNsCommBuffMemRegion.PhysicalBase,
+         mNsCommBuffMemRegion.Length
+         );
+
+ReturnErrorStatus:
+  return EFI_INVALID_PARAMETER;
+}
-- 
2.7.4

[PATCH v2 3/7] ArmPkg/Include: Fix the SPM version SVC ID

[Sughosh Ganu]

The MM_VERSION SMC call uses SMC32 calling convention. Fix the macro
to reflect the correct value.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Sughosh Ganu <sughosh.ganu@arm.com>
---
 ArmPkg/Include/IndustryStandard/ArmMmSvc.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ArmPkg/Include/IndustryStandard/ArmMmSvc.h b/ArmPkg/Include/IndustryStandard/ArmMmSvc.h
index 4c7b6c338627..81b4654fa5dd 100644
--- a/ArmPkg/Include/IndustryStandard/ArmMmSvc.h
+++ b/ArmPkg/Include/IndustryStandard/ArmMmSvc.h
@@ -20,7 +20,7 @@
  * delegated events and request the Secure partition manager to perform
  * privileged operations on its behalf.
  */
-#define ARM_SVC_ID_SPM_VERSION_AARCH64             0xC4000060
+#define ARM_SVC_ID_SPM_VERSION_AARCH32             0x84000060
 #define ARM_SVC_ID_SP_EVENT_COMPLETE_AARCH64       0xC4000061
 #define ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES_AARCH64   0xC4000064
 #define ARM_SVC_ID_SP_SET_MEM_ATTRIBUTES_AARCH64   0xC4000065
-- 
2.7.4

[PATCH v2 4/7] ArmPkg/Include: Add MM interface SVC return codes.

[Sughosh Ganu]

From: Achin Gupta <achin.gupta@arm.com>

This patch adds the Management Mode(MM) - Secure Partition
Manager(SPM) SVC return codes.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Supreeth Venkatesh <supreeth.venkatesh@arm.com>
---
 ArmPkg/Include/IndustryStandard/ArmMmSvc.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/ArmPkg/Include/IndustryStandard/ArmMmSvc.h b/ArmPkg/Include/IndustryStandard/ArmMmSvc.h
index 81b4654fa5dd..a64b9ec23c4b 100644
--- a/ArmPkg/Include/IndustryStandard/ArmMmSvc.h
+++ b/ArmPkg/Include/IndustryStandard/ArmMmSvc.h
@@ -40,4 +40,11 @@
     ((((c_perm) & SET_MEM_ATTR_CODE_PERM_MASK) << SET_MEM_ATTR_CODE_PERM_SHIFT) | \
     (( (d_perm) & SET_MEM_ATTR_DATA_PERM_MASK) << SET_MEM_ATTR_DATA_PERM_SHIFT))
 
+/* MM SVC Return error codes */
+#define ARM_SVC_SPM_RET_SUCCESS               0
+#define ARM_SVC_SPM_RET_NOT_SUPPORTED        -1
+#define ARM_SVC_SPM_RET_INVALID_PARAMS       -2
+#define ARM_SVC_SPM_RET_DENIED               -3
+#define ARM_SVC_SPM_RET_NO_MEMORY            -5
+
 #endif
-- 
2.7.4

[PATCH v2 5/7] ArmPkg/ArmMmuLib: Add MMU Library suitable for use in S-EL0.

[Sughosh Ganu]

From: Achin Gupta <achin.gupta@arm.com>

The Standalone MM environment runs in S-EL0 in AArch64 on ARM Standard
Platforms. Privileged firmware e.g. ARM Trusted Firmware sets up its
architectural context including the initial translation tables for the
S-EL1/EL0 translation regime. The MM environment will still request ARM
TF to change the memory attributes of memory regions during
initialization.

The Standalone MM image is a FV that encapsulates the MM foundation
and drivers. These are PE-COFF images with data and text segments.
To initialise the MM environment, Arm Trusted Firmware has to create
translation tables with sane default attributes for the memory
occupied by the FV. This library sends SVCs to ARM Trusted Firmware
to request memory permissions change for data and text segments.

This patch adds a simple MMU library suitable for execution in S-EL0 and
requesting memory permissions change operations from Arm Trusted Firmware.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Sughosh Ganu <sughosh.ganu@arm.com>
Signed-off-by: Supreeth Venkatesh <supreeth.venkatesh@arm.com>
---
 ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuStandaloneMmCoreLib.c | 204 ++++++++++++++++++++
 1 file changed, 204 insertions(+)

diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuStandaloneMmCoreLib.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuStandaloneMmCoreLib.c
new file mode 100644
index 000000000000..ee0a80349051
--- /dev/null
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuStandaloneMmCoreLib.c
@@ -0,0 +1,204 @@
+/** @file
+*  File managing the MMU for ARMv8 architecture in S-EL0
+*
+*  Copyright (c) 2017 - 2018, ARM Limited. All rights reserved.
+*
+*  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 <Uefi.h>
+#include <Chipset/AArch64.h>
+#include <IndustryStandard/ArmMmSvc.h>
+
+#include <Library/ArmLib.h>
+#include <Library/ArmMmuLib.h>
+#include <Library/ArmSvcLib.h>
+#include <Library/BaseLib.h>
+#include <Library/DebugLib.h>
+
+EFI_STATUS
+GetMemoryPermissions (
+  IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
+  OUT UINT32                    *MemoryAttributes
+  )
+{
+  ARM_SVC_ARGS  GetMemoryPermissionsSvcArgs = {0};
+
+  GetMemoryPermissionsSvcArgs.Arg0 = ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES_AARCH64;
+  GetMemoryPermissionsSvcArgs.Arg1 = BaseAddress;
+  GetMemoryPermissionsSvcArgs.Arg2 = 0;
+  GetMemoryPermissionsSvcArgs.Arg3 = 0;
+
+  ArmCallSvc (&GetMemoryPermissionsSvcArgs);
+  if (GetMemoryPermissionsSvcArgs.Arg0 == ARM_SVC_SPM_RET_INVALID_PARAMS) {
+    *MemoryAttributes = 0;
+    return EFI_INVALID_PARAMETER;
+  }
+
+  *MemoryAttributes = GetMemoryPermissionsSvcArgs.Arg0;
+  return EFI_SUCCESS;
+}
+
+EFI_STATUS
+RequestMemoryPermissionChange (
+  IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
+  IN  UINT64                    Length,
+  IN  UINTN                     Permissions
+  )
+{
+  EFI_STATUS    Status;
+  ARM_SVC_ARGS  ChangeMemoryPermissionsSvcArgs = {0};
+
+  ChangeMemoryPermissionsSvcArgs.Arg0 = ARM_SVC_ID_SP_SET_MEM_ATTRIBUTES_AARCH64;
+  ChangeMemoryPermissionsSvcArgs.Arg1 = BaseAddress;
+  ChangeMemoryPermissionsSvcArgs.Arg2 = EFI_SIZE_TO_PAGES(Length);
+  ChangeMemoryPermissionsSvcArgs.Arg3 = Permissions;
+
+  ArmCallSvc (&ChangeMemoryPermissionsSvcArgs);
+
+  Status = ChangeMemoryPermissionsSvcArgs.Arg0;
+
+  switch (Status) {
+  case ARM_SVC_SPM_RET_SUCCESS:
+    Status = EFI_SUCCESS;
+    break;
+
+  case ARM_SVC_SPM_RET_NOT_SUPPORTED:
+    Status = EFI_UNSUPPORTED;
+    break;
+
+  case ARM_SVC_SPM_RET_INVALID_PARAMS:
+    Status = EFI_INVALID_PARAMETER;
+    break;
+
+  case ARM_SVC_SPM_RET_DENIED:
+    Status = EFI_ACCESS_DENIED;
+    break;
+
+  case ARM_SVC_SPM_RET_NO_MEMORY:
+    Status = EFI_BAD_BUFFER_SIZE;
+    break;
+
+  default:
+    Status = EFI_ACCESS_DENIED;
+    ASSERT (0);
+  }
+
+  return Status;
+}
+
+EFI_STATUS
+ArmSetMemoryRegionNoExec (
+  IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
+  IN  UINT64                    Length
+  )
+{
+  EFI_STATUS    Status;
+  UINT32 MemoryAttributes;
+  UINT32 CodePermission;
+
+  Status = GetMemoryPermissions (BaseAddress, &MemoryAttributes);
+  if (Status != EFI_INVALID_PARAMETER) {
+    CodePermission = SET_MEM_ATTR_CODE_PERM_XN << SET_MEM_ATTR_CODE_PERM_SHIFT;
+    return RequestMemoryPermissionChange (
+             BaseAddress,
+             Length,
+             MemoryAttributes | CodePermission
+             );
+  }
+  return EFI_INVALID_PARAMETER;
+}
+
+EFI_STATUS
+ArmClearMemoryRegionNoExec (
+  IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
+  IN  UINT64                    Length
+  )
+{
+  EFI_STATUS    Status;
+  UINT32 MemoryAttributes;
+  UINT32 CodePermission;
+
+  Status = GetMemoryPermissions (BaseAddress, &MemoryAttributes);
+  if (Status != EFI_INVALID_PARAMETER) {
+    CodePermission = SET_MEM_ATTR_CODE_PERM_XN << SET_MEM_ATTR_CODE_PERM_SHIFT;
+    return RequestMemoryPermissionChange (
+             BaseAddress,
+             Length,
+             MemoryAttributes & ~CodePermission
+             );
+  }
+  return EFI_INVALID_PARAMETER;
+}
+
+EFI_STATUS
+ArmSetMemoryRegionReadOnly (
+  IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
+  IN  UINT64                    Length
+  )
+{
+  EFI_STATUS    Status;
+  UINT32 MemoryAttributes;
+  UINT32 DataPermission;
+
+  Status = GetMemoryPermissions (BaseAddress, &MemoryAttributes);
+  if (Status != EFI_INVALID_PARAMETER) {
+    DataPermission = SET_MEM_ATTR_DATA_PERM_RO << SET_MEM_ATTR_DATA_PERM_SHIFT;
+    return RequestMemoryPermissionChange (
+             BaseAddress,
+             Length,
+             MemoryAttributes | DataPermission
+             );
+  }
+  return EFI_INVALID_PARAMETER;
+}
+
+EFI_STATUS
+ArmClearMemoryRegionReadOnly (
+  IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
+  IN  UINT64                    Length
+  )
+{
+  EFI_STATUS    Status;
+  UINT32 MemoryAttributes;
+  UINT32 PermissionRequest;
+
+  Status = GetMemoryPermissions (BaseAddress, &MemoryAttributes);
+  if (Status != EFI_INVALID_PARAMETER) {
+    PermissionRequest = SET_MEM_ATTR_MAKE_PERM_REQUEST (SET_MEM_ATTR_DATA_PERM_RW, MemoryAttributes);
+    return RequestMemoryPermissionChange (
+             BaseAddress,
+             Length,
+             PermissionRequest
+             );
+  }
+  return EFI_INVALID_PARAMETER;
+}
+
+EFI_STATUS
+EFIAPI
+ArmConfigureMmu (
+  IN  ARM_MEMORY_REGION_DESCRIPTOR  *MemoryTable,
+  OUT VOID                          **TranslationTableBase OPTIONAL,
+  OUT UINTN                         *TranslationTableSize OPTIONAL
+  )
+{
+  return EFI_UNSUPPORTED;
+}
+
+EFI_STATUS
+EFIAPI
+ArmMmuStandaloneMmCoreLibConstructor (
+  IN EFI_HANDLE            ImageHandle,
+  IN EFI_MM_SYSTEM_TABLE   *MmSystemTable
+  )
+{
+  return EFI_SUCCESS;
+}
-- 
2.7.4

[PATCH v2 6/7] ArmPkg/ArmMmuLib: Add MMU library inf file suitable for use in S-EL0.

[Sughosh Ganu]

From: Achin Gupta <achin.gupta@arm.com>

This patch adds the definitions, sources, packages and library classes
needed to compile and link MMU Library suitable for use in S-EL0.

Currently, this is used only during the Standalone MM Core
initialization and hence defined as MM_CORE_STANDALONE Module.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Supreeth Venkatesh <supreeth.venkatesh@arm.com>
Signed-off-by: Sughosh Ganu <sughosh.ganu@arm.com>
---
 ArmPkg/Library/ArmMmuLib/ArmMmuStandaloneMmCoreLib.inf | 37 ++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/ArmPkg/Library/ArmMmuLib/ArmMmuStandaloneMmCoreLib.inf b/ArmPkg/Library/ArmMmuLib/ArmMmuStandaloneMmCoreLib.inf
new file mode 100644
index 000000000000..9f5593d3f6c8
--- /dev/null
+++ b/ArmPkg/Library/ArmMmuLib/ArmMmuStandaloneMmCoreLib.inf
@@ -0,0 +1,37 @@
+#/** @file
+#
+#  Copyright (c) 2017 - 2018, ARM Limited. All rights reserved.
+#
+#  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.
+#
+#
+#**/
+
+[Defines]
+  INF_VERSION                    = 0x0001001A
+  BASE_NAME                      = ArmMmuStandaloneMmCoreLib
+  FILE_GUID                      = da8f0232-fb14-42f0-922c-63104d2c70bd
+  MODULE_TYPE                    = MM_CORE_STANDALONE
+  VERSION_STRING                 = 1.0
+  LIBRARY_CLASS                  = ArmMmuStandaloneMmCoreLib|MM_CORE_STANDALONE
+  PI_SPECIFICATION_VERSION       = 0x00010032
+  CONSTRUCTOR                    = ArmMmuStandaloneMmCoreLibConstructor
+
+[Sources.AARCH64]
+  AArch64/ArmMmuStandaloneMmCoreLib.c
+
+[Packages]
+  ArmPkg/ArmPkg.dec
+  MdePkg/MdePkg.dec
+
+[LibraryClasses]
+  ArmLib
+  CacheMaintenanceLib
+  MemoryAllocationLib
+
+
-- 
2.7.4

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

[Sughosh Ganu]

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>
---
 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/DebugPeCoffExtraActionLib.inf b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.inf
index c1f717e5bda1..38bf3993ae99 100644
--- a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.inf
+++ b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.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/DebugPeCoffExtraActionLib.c b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
index f298e58cdfca..8e621de4a87a 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 <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

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

[Ard Biesheuvel]

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.

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)

> ---
>  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/DebugPeCoffExtraActionLib.inf b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.inf
> index c1f717e5bda1..38bf3993ae99 100644
> --- a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.inf
> +++ b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.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/DebugPeCoffExtraActionLib.c b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
> index f298e58cdfca..8e621de4a87a 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 <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

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

[Supreeth Venkatesh]

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

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

[Sughosh Ganu]

hi Ard,

On Tue July 23, 2018 at 11:03PM +0530, Supreeth Venkatesh wrote:
> 
> 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.

Supreeth had replied to your comments on the patch. Can you please
check this. If you feel that this needs to be implemented differently,
can you please suggest it to us. Thanks.

-sughosh

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

[Ard Biesheuvel]

On 21 August 2018 at 07:50, Sughosh Ganu <sughosh.ganu@arm.com> wrote:
> hi Ard,
>
> On Tue July 23, 2018 at 11:03PM +0530, Supreeth Venkatesh wrote:
>>
>> 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.
>
> Supreeth had replied to your comments on the patch. Can you please
> check this. If you feel that this needs to be implemented differently,
> can you please suggest it to us. Thanks.
>

My point is that such a fundamental action that needs to occur while
loading the PE/COFF image should not be hooked into the loader this
way.

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

[Achin Gupta]

Hi Ard,

Apologies for the delay! CIL.

On Fri, Aug 24, 2018 at 03:55:29PM +0100, Ard Biesheuvel wrote:
> On 21 August 2018 at 07:50, Sughosh Ganu <sughosh.ganu@arm.com> wrote:
> > hi Ard,
> >
> > On Tue July 23, 2018 at 11:03PM +0530, Supreeth Venkatesh wrote:
> >>
> >> 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.

This is a bit misleading. For Standalone MM drivers, memory is allocated from
the heap which is marked as RW+XN by the SPM in the S-EL1 page tables. This
allows PeCoffLoaderLoadImage() to load the image from the FV into its newly
allocated buffer. Before the driver can be executed, its rodata and code
sections need a permission update from RW+XN to RO+XN and RO+X respectively. 

This patch does these permission changes after the relocation fixups have been
done in PeCoffLoaderRelocateImageExtraAction().

> >> > >
> >> > > 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.
> >
> > Supreeth had replied to your comments on the patch. Can you please
> > check this. If you feel that this needs to be implemented differently,
> > can you please suggest it to us. Thanks.
> >
> 
> My point is that such a fundamental action that needs to occur while
> loading the PE/COFF image should not be hooked into the loader this
> way.

IIUC, your concern is about the way the PeCoffLoaderRelocateImageExtraAction()
has been used? Is it meant to fulfil a different purpose? PE-COFF image loading
and relocation is done by generic code and this seemed like the best mechanism
to introduce an arch. specific hook.

I agree that implementing the hooks in a DebugPeCoffExtraActionLib is not
suitable for upstreaming. We could implement this within the StandaloneMmPkg as
an Aarch64 library since it is unlikely it will ever be used in the ArmPkg
itself.

Please let us know what you think.

cheers,
Achin
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

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

[Achin Gupta]

Hi Ard,

Please see CIL..

On Fri, Aug 24, 2018 at 03:55:29PM +0100, Ard Biesheuvel wrote:
> On 21 August 2018 at 07:50, Sughosh Ganu <sughosh.ganu@arm.com> wrote:
> > hi Ard,
> >
> > On Tue July 23, 2018 at 11:03PM +0530, Supreeth Venkatesh wrote:
> >>
> >> 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.
> >
> > Supreeth had replied to your comments on the patch. Can you please
> > check this. If you feel that this needs to be implemented differently,
> > can you please suggest it to us. Thanks.
> >
> 
> My point is that such a fundamental action that needs to occur while
> loading the PE/COFF image should not be hooked into the loader this
> way.

Based upon our discussion at the Linaro Connect, we investigated leveraging the
DXE Image Protection support [1] in Standalone MM (StMM). Amongst other
challenges, there is a chicken and egg problem. I will try and explain.

DXE Memory protection has dependencies that cannot be fulfilled in StMM. A
non-exhaustive list is:

1. Dependency on CPU_ARCH protocol
2. Dependency on Loaded Image patch protocol
3. Dependency on Boot services 

There is an inherent assumption that this support will never be used in
SMM. Furthermore, in StMM, permissions are changed when the StMM drivers are
first dispatched. A dependency on a driver to change the permissions is the
chicken and egg. So we need a library.

One option is to introduce a memory protection library in StMM i.e. a library
interface like StandaloneMmImageProtect(). This function will be called from
generic code after the PE-COFF loader has loaded and relocated the StMM driver
image. However, this support is not required on x86. They will have to include a
NULL library implementation. This would be in addition to the NULL
PeCoffExtraActionLib they already include through MdePkg.dsc.

I am hesitant to take this approach in the absence of a requirement on x86. At
the same time, the current approach of leveraging the DebugPeCoffExtraActionLib
in ArmPkg does not make sense either.

IMO, the better approach would be to add a AArch64 specific
StandaloneMmPeCoffExtraActionLib in the StandaloneMmPkg. Memory protection will
be implemented in the relocation hook. There will be no impact on x86 or the
ArmPkg. If in future there is a requirement to support this feature on x86 as
well, then a separate library could be implemented.

Please let us know if this sounds reasonable to you. Sughosh will be posting the
patches with this approach in a bit to aid the discussion.

Cheers,
Achin

[1] MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c

> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

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

[Ard Biesheuvel]

On 24 October 2018 at 05:22, Achin Gupta <Achin.Gupta@arm.com> wrote:
> Hi Ard,
>
> Please see CIL..
>

FYI I will be on leave until 5th of November, so I will get to this
once I get back.

-- 
Ard.

> On Fri, Aug 24, 2018 at 03:55:29PM +0100, Ard Biesheuvel wrote:
>> On 21 August 2018 at 07:50, Sughosh Ganu <sughosh.ganu@arm.com> wrote:
>> > hi Ard,
>> >
>> > On Tue July 23, 2018 at 11:03PM +0530, Supreeth Venkatesh wrote:
>> >>
>> >> 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.
>> >
>> > Supreeth had replied to your comments on the patch. Can you please
>> > check this. If you feel that this needs to be implemented differently,
>> > can you please suggest it to us. Thanks.
>> >
>>
>> My point is that such a fundamental action that needs to occur while
>> loading the PE/COFF image should not be hooked into the loader this
>> way.
>
> Based upon our discussion at the Linaro Connect, we investigated leveraging the
> DXE Image Protection support [1] in Standalone MM (StMM). Amongst other
> challenges, there is a chicken and egg problem. I will try and explain.
>
> DXE Memory protection has dependencies that cannot be fulfilled in StMM. A
> non-exhaustive list is:
>
> 1. Dependency on CPU_ARCH protocol
> 2. Dependency on Loaded Image patch protocol
> 3. Dependency on Boot services
>
> There is an inherent assumption that this support will never be used in
> SMM. Furthermore, in StMM, permissions are changed when the StMM drivers are
> first dispatched. A dependency on a driver to change the permissions is the
> chicken and egg. So we need a library.
>
> One option is to introduce a memory protection library in StMM i.e. a library
> interface like StandaloneMmImageProtect(). This function will be called from
> generic code after the PE-COFF loader has loaded and relocated the StMM driver
> image. However, this support is not required on x86. They will have to include a
> NULL library implementation. This would be in addition to the NULL
> PeCoffExtraActionLib they already include through MdePkg.dsc.
>
> I am hesitant to take this approach in the absence of a requirement on x86. At
> the same time, the current approach of leveraging the DebugPeCoffExtraActionLib
> in ArmPkg does not make sense either.
>
> IMO, the better approach would be to add a AArch64 specific
> StandaloneMmPeCoffExtraActionLib in the StandaloneMmPkg. Memory protection will
> be implemented in the relocation hook. There will be no impact on x86 or the
> ArmPkg. If in future there is a requirement to support this feature on x86 as
> well, then a separate library could be implemented.
>
> Please let us know if this sounds reasonable to you. Sughosh will be posting the
> patches with this approach in a bit to aid the discussion.
>
> Cheers,
> Achin
>
> [1] MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel

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

[Achin Gupta]

Hi Ard,

Just a polite poke that Sughosh had posted the patches as I had described below
here [1] & here [2]. Please let us know what you think.

cheers,
Achin

[1] https://lists.01.org/pipermail/edk2-devel/2018-October/031377.html
[2] https://lists.01.org/pipermail/edk2-devel/2018-October/031384.html

On Wed, Oct 24, 2018 at 08:05:22AM -0300, Ard Biesheuvel wrote:
> On 24 October 2018 at 05:22, Achin Gupta <Achin.Gupta@arm.com> wrote:
> > Hi Ard,
> >
> > Please see CIL..
> >
> 
> FYI I will be on leave until 5th of November, so I will get to this
> once I get back.
> 
> -- 
> Ard.
> 
> > On Fri, Aug 24, 2018 at 03:55:29PM +0100, Ard Biesheuvel wrote:
> >> On 21 August 2018 at 07:50, Sughosh Ganu <sughosh.ganu@arm.com> wrote:
> >> > hi Ard,
> >> >
> >> > On Tue July 23, 2018 at 11:03PM +0530, Supreeth Venkatesh wrote:
> >> >>
> >> >> 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.
> >> >
> >> > Supreeth had replied to your comments on the patch. Can you please
> >> > check this. If you feel that this needs to be implemented differently,
> >> > can you please suggest it to us. Thanks.
> >> >
> >>
> >> My point is that such a fundamental action that needs to occur while
> >> loading the PE/COFF image should not be hooked into the loader this
> >> way.
> >
> > Based upon our discussion at the Linaro Connect, we investigated leveraging the
> > DXE Image Protection support [1] in Standalone MM (StMM). Amongst other
> > challenges, there is a chicken and egg problem. I will try and explain.
> >
> > DXE Memory protection has dependencies that cannot be fulfilled in StMM. A
> > non-exhaustive list is:
> >
> > 1. Dependency on CPU_ARCH protocol
> > 2. Dependency on Loaded Image patch protocol
> > 3. Dependency on Boot services
> >
> > There is an inherent assumption that this support will never be used in
> > SMM. Furthermore, in StMM, permissions are changed when the StMM drivers are
> > first dispatched. A dependency on a driver to change the permissions is the
> > chicken and egg. So we need a library.
> >
> > One option is to introduce a memory protection library in StMM i.e. a library
> > interface like StandaloneMmImageProtect(). This function will be called from
> > generic code after the PE-COFF loader has loaded and relocated the StMM driver
> > image. However, this support is not required on x86. They will have to include a
> > NULL library implementation. This would be in addition to the NULL
> > PeCoffExtraActionLib they already include through MdePkg.dsc.
> >
> > I am hesitant to take this approach in the absence of a requirement on x86. At
> > the same time, the current approach of leveraging the DebugPeCoffExtraActionLib
> > in ArmPkg does not make sense either.
> >
> > IMO, the better approach would be to add a AArch64 specific
> > StandaloneMmPeCoffExtraActionLib in the StandaloneMmPkg. Memory protection will
> > be implemented in the relocation hook. There will be no impact on x86 or the
> > ArmPkg. If in future there is a requirement to support this feature on x86 as
> > well, then a separate library could be implemented.
> >
> > Please let us know if this sounds reasonable to you. Sughosh will be posting the
> > patches with this approach in a bit to aid the discussion.
> >
> > Cheers,
> > Achin
> >
> > [1] MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> >
> >> _______________________________________________
> >> edk2-devel mailing list
> >> edk2-devel@lists.01.org
> >> https://lists.01.org/mailman/listinfo/edk2-devel