ArmVirtPkg/HighMemDxe: preserve non-exec permissions on newly added regions

Using DxeServices::SetMemorySpaceAttributes to set cacheability
attributes has the side effect of stripping permission attributes,
given that those are bits in the same bitfield, and so setting the
Attributes argument to EFI_MEMORY_WB implies not setting EFI_MEMORY_XP
or EFI_MEMORY_RO attributes.

In fact, the situation is even worse, given that the descriptor returned
by DxeServices::GetMemorySpaceDescriptor does not reflect the permission
attributes that may have been set by the preceding call to
DxeServices::AddMemorySpace if PcdDxeNxMemoryProtectionPolicy has been
configured to map EfiConventionalMemory with non-executable permissions.

Note that this applies equally to the non-executable stack and to PE/COFF
sections that may have been mapped with R-X or RW- permissions. This is
due to the ambiguity in the meaning of the EFI_MEMORY_RO/EFI_MEMORY_XP
attributes when used in the GCD memory map, i.e., between signifying
that an underlying RAM region has the controls to be configured as
read-only or non-executable, and signifying that the contents of a
certain UEFI memory region allow them to be mapped with certain
restricted permissions.

So let's check the policy in PcdDxeNxMemoryProtectionPolicy directly,
and set the EFI_MEMORY_XP attribute if appropriate for
EfiConventionalMemory regions.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>

2 files changed, 17 insertions, 2 deletions

diff --git a/ArmVirtPkg/HighMemDxe/HighMemDxe.c b/ArmVirtPkg/HighMemDxe/HighMemDxe.c
index 22f738279b..f70978f641 100644
--- a/ArmVirtPkg/HighMemDxe/HighMemDxe.c
+++ b/ArmVirtPkg/HighMemDxe/HighMemDxe.c
@@ -37,6 +37,7 @@ InitializeHighMemDxe (
   UINTN                 AddressCells, SizeCells;

   UINT64                CurBase;

   UINT64                CurSize;

+  UINT64                Attributes;

 

   Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL,

                   (VOID **)&FdtClient);

@@ -77,8 +78,21 @@ InitializeHighMemDxe (
           continue;

         }

 

-        Status = gDS->SetMemorySpaceAttributes (CurBase, CurSize,

-                        EFI_MEMORY_WB);

+        //

+        // Take care not to strip any permission attributes that will have been

+        // set by DxeCore on the region we just added if a strict permission

+        // policy is in effect for EfiConventionalMemory regions.

+        // Unfortunately, we cannot interrogate the GCD memory space map for

+        // those permissions, since they are not recorded there (for historical

+        // reasons), so check the policy directly.

+        //

+        Attributes = EFI_MEMORY_WB;

+        if ((PcdGet64 (PcdDxeNxMemoryProtectionPolicy) &

+             (1U << (UINT32)EfiConventionalMemory)) != 0) {

+          Attributes |= EFI_MEMORY_XP;

+        }

+

+        Status = gDS->SetMemorySpaceAttributes (CurBase, CurSize, Attributes);

 

         if (EFI_ERROR (Status)) {

           DEBUG ((EFI_D_ERROR,

diff --git a/ArmVirtPkg/HighMemDxe/HighMemDxe.inf b/ArmVirtPkg/HighMemDxe/HighMemDxe.inf
index 3661cfd8c8..89c743ebe0 100644
--- a/ArmVirtPkg/HighMemDxe/HighMemDxe.inf
+++ b/ArmVirtPkg/HighMemDxe/HighMemDxe.inf
@@ -45,6 +45,7 @@
 

 [Pcd]

   gArmTokenSpaceGuid.PcdSystemMemoryBase

+  gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy

 

 [Depex]

   gEfiCpuArchProtocolGuid AND gFdtClientProtocolGuid