[PATCH 0/3] Apply NX protections more strictly

[PATCH 0/3] Apply NX protections more strictly

[Ard Biesheuvel]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1975 bytes --]

This fixes an issue reported by Marvin, where NX memory protections are
applied in a rather unreliable manner, resulting in the possibility that
memory mappings may exist that are using different attributes than
intended.

The reason for this approach was that applying memory protections
eagerly (i.e., after every alloc/free even if the memory attributes are
not expected to change as a result) may result in unbounded recursion in
the page table code, due to the fact that the page tables it allocates
need to be remapped with the correct attributes as well.

This has not been reported as being an issue on x86, but on ARM, this
needs a couple of fixes so that converting between EfiConventionalMemory
and EfiBootServicesData will never trigger a block entry split. With
that fixed, we can just remove the shortcut from DXE core and always
call SetMemoryAttributes.

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3316

Cc: Michael Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Michael Kubacki <michael.kubacki@microsoft.com>
Cc: Sean Brogan <sean.brogan@microsoft.com>
Cc: Rebecca Cran <quic_rcran@quicinc.com>
Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Cc: Taylor Beebe <t@taylorbeebe.com>
Cc: Marvin Häuser <mhaeuser@posteo.de>

Ard Biesheuvel (3):
  ArmPkg/ArmMmuLib: Avoid splitting block entries if possible
  ArmPkg/CpuDxe: Perform preliminary NX remap of free memory
  MdeModulePkg/DxeCore: Unconditionally set memory protections

 ArmPkg/Drivers/CpuDxe/CpuDxe.c                   | 77 ++++++++++++++++++++
 ArmPkg/Drivers/CpuDxe/CpuDxe.inf                 |  2 +
 ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c |  9 +++
 MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c    | 29 --------
 4 files changed, 88 insertions(+), 29 deletions(-)

-- 
2.39.1

[PATCH 1/3] ArmPkg/ArmMmuLib: Avoid splitting block entries if possible

[Ard Biesheuvel]

Currently, the AArch64 MMU page table logic will break down any block
entry that overlaps with the region being mapped, even if the block
entry in question is using the same attributes as the new region.

This means that creating a non-executable mapping inside a region that
is already mapped non-executable at a coarser granularity may trigger a
call to AllocatePages (), which may recurse back into the page table
code to update the attributes on the newly allocated page tables.

Let's avoid this, by preserving the block entry if it already covers the
region being mapped with the correct attributes.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index 1cf8dc090012..28191938aeb1 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -251,6 +251,15 @@ UpdateRegionMappingRecursive (
       ASSERT (Level < 3);
 
       if (!IsTableEntry (*Entry, Level)) {
+        //
+        // If the region we are trying to map is already covered by a block
+        // entry with the right attributes, don't bother splitting it up.
+        //
+        if (IsBlockEntry (*Entry, Level) &&
+            ((*Entry & TT_ATTRIBUTES_MASK & ~AttributeClearMask) == AttributeSetMask)) {
+          continue;
+        }
+
         //
         // No table entry exists yet, so we need to allocate a page table
         // for the next level.
-- 
2.39.1

[PATCH 2/3] ArmPkg/CpuDxe: Perform preliminary NX remap of free memory

[Ard Biesheuvel]

The DXE core implementation of PcdDxeNxMemoryProtectionPolicy already
contains an assertion that EfiConventionalMemory and EfiBootServicesData
are subjected to the same policy when it comes to the use of NX
permissions. The reason for this is that we may otherwise end up with
unbounded recursion in the page table code, given that allocating a page
table would then involve a permission attribute change, and this could
result in the need for a block entry to be split, which would trigger
the allocation of a page table recursively.

For the same reason, a shortcut exists in ApplyMemoryProtectionPolicy()
where, instead of setting the memory attributes unconditionally, we
compare the NX policies and avoid touching the page tables if they are
the same for the old and the new memory types. Without this shortcut, we
may end up in a situation where, as the CPU arch protocol DXE driver is
ramping up, the same unbounded recursion is triggered, due to the fact
that the NX policy for EfiConventionalMemory has not been applied yet.

To break this cycle, let's remap all EfiConventionalMemory regions
according to the NX policy for EfiBootServicesData before exposing the
CPU arch protocol to the DXE core and other drivers. This ensures that
creating EfiBootServicesData allocations does not result in memory
attribute changes, and therefore no recursion.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmPkg/Drivers/CpuDxe/CpuDxe.c   | 77 ++++++++++++++++++++
 ArmPkg/Drivers/CpuDxe/CpuDxe.inf |  2 +
 2 files changed, 79 insertions(+)

diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.c b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
index d04958e79e52..83fd6fd4e476 100644
--- a/ArmPkg/Drivers/CpuDxe/CpuDxe.c
+++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
@@ -11,6 +11,8 @@
 
 #include <Guid/IdleLoopEvent.h>
 
+#include <Library/MemoryAllocationLib.h>
+
 BOOLEAN  mIsFlushingGCD;
 
 /**
@@ -227,6 +229,69 @@ InitializeDma (
   CpuArchProtocol->DmaBufferAlignment = ArmCacheWritebackGranule ();
 }
 
+STATIC
+VOID
+RemapUnusedMemoryNx (
+  VOID
+  )
+{
+  UINT64                     TestBit;
+  UINTN                      MemoryMapSize;
+  UINTN                      MapKey;
+  UINTN                      DescriptorSize;
+  UINT32                     DescriptorVersion;
+  EFI_MEMORY_DESCRIPTOR      *MemoryMap;
+  EFI_MEMORY_DESCRIPTOR      *MemoryMapEntry;
+  EFI_MEMORY_DESCRIPTOR      *MemoryMapEnd;
+  EFI_STATUS                 Status;
+
+  TestBit = LShiftU64 (1, EfiBootServicesData);
+  if ((PcdGet64 (PcdDxeNxMemoryProtectionPolicy) & TestBit) == 0) {
+    return;
+  }
+
+  MemoryMapSize = 0;
+  MemoryMap     = NULL;
+
+  Status = gBS->GetMemoryMap (
+                  &MemoryMapSize,
+                  MemoryMap,
+                  &MapKey,
+                  &DescriptorSize,
+                  &DescriptorVersion
+                  );
+  ASSERT (Status == EFI_BUFFER_TOO_SMALL);
+  do {
+    MemoryMap = (EFI_MEMORY_DESCRIPTOR *)AllocatePool (MemoryMapSize);
+    ASSERT (MemoryMap != NULL);
+    Status = gBS->GetMemoryMap (
+                    &MemoryMapSize,
+                    MemoryMap,
+                    &MapKey,
+                    &DescriptorSize,
+                    &DescriptorVersion
+                    );
+    if (EFI_ERROR (Status)) {
+      FreePool (MemoryMap);
+    }
+  } while (Status == EFI_BUFFER_TOO_SMALL);
+
+  ASSERT_EFI_ERROR (Status);
+
+  MemoryMapEntry = MemoryMap;
+  MemoryMapEnd   = (EFI_MEMORY_DESCRIPTOR *)((UINT8 *)MemoryMap + MemoryMapSize);
+  while ((UINTN)MemoryMapEntry < (UINTN)MemoryMapEnd) {
+    if (MemoryMapEntry->Type == EfiConventionalMemory) {
+      ArmSetMemoryRegionNoExec (
+        MemoryMapEntry->PhysicalStart,
+        EFI_PAGES_TO_SIZE (MemoryMapEntry->NumberOfPages)
+        );
+    }
+
+    MemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry, DescriptorSize);
+  }
+}
+
 EFI_STATUS
 CpuDxeInitialize (
   IN EFI_HANDLE        ImageHandle,
@@ -240,6 +305,18 @@ CpuDxeInitialize (
 
   InitializeDma (&mCpu);
 
+  //
+  // Once we install the CPU arch protocol, the DXE core's memory
+  // protection routines will invoke them to manage the permissions of page
+  // allocations as they are created. Given that this includes pages
+  // allocated for page tables by this driver, we must ensure that unused
+  // memory is mapped with the same permissions as boot services data
+  // regions. Otherwise, we may end up with unbounded recursion, due to the
+  // fact that updating permissions on a newly allocated page table may trigger
+  // a block entry split, which triggers a page table allocation, etc etc
+  //
+  RemapUnusedMemoryNx ();
+
   Status = gBS->InstallMultipleProtocolInterfaces (
                   &mCpuHandle,
                   &gEfiCpuArchProtocolGuid,
diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.inf b/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
index e732e21cb94a..8fd0f4133088 100644
--- a/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
+++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
@@ -48,6 +48,7 @@ [LibraryClasses]
   DefaultExceptionHandlerLib
   DxeServicesTableLib
   HobLib
+  MemoryAllocationLib
   PeCoffGetEntryPointLib
   UefiDriverEntryPoint
   UefiLib
@@ -64,6 +65,7 @@ [Guids]
 
 [Pcd.common]
   gArmTokenSpaceGuid.PcdVFPEnabled
+  gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy
 
 [FeaturePcd.common]
   gArmTokenSpaceGuid.PcdDebuggerExceptionSupport
-- 
2.39.1

Re: [PATCH 2/3] ArmPkg/CpuDxe: Perform preliminary NX remap of free memory

[Marvin Häuser]

Thanks!! :) Comments inline.

> On 8. Feb 2023, at 18:58, Ard Biesheuvel <ardb@kernel.org> wrote:
> 
> The DXE core implementation of PcdDxeNxMemoryProtectionPolicy already
> contains an assertion that EfiConventionalMemory and EfiBootServicesData
> are subjected to the same policy when it comes to the use of NX
> permissions. The reason for this is that we may otherwise end up with
> unbounded recursion in the page table code, given that allocating a page
> table would then involve a permission attribute change, and this could
> result in the need for a block entry to be split, which would trigger
> the allocation of a page table recursively.
> 
> For the same reason, a shortcut exists in ApplyMemoryProtectionPolicy()
> where, instead of setting the memory attributes unconditionally, we
> compare the NX policies and avoid touching the page tables if they are
> the same for the old and the new memory types. Without this shortcut, we
> may end up in a situation where, as the CPU arch protocol DXE driver is
> ramping up, the same unbounded recursion is triggered, due to the fact
> that the NX policy for EfiConventionalMemory has not been applied yet.
> 
> To break this cycle, let's remap all EfiConventionalMemory regions
> according to the NX policy for EfiBootServicesData before exposing the
> CPU arch protocol to the DXE core and other drivers. This ensures that
> creating EfiBootServicesData allocations does not result in memory
> attribute changes, and therefore no recursion.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
> ArmPkg/Drivers/CpuDxe/CpuDxe.c   | 77 ++++++++++++++++++++
> ArmPkg/Drivers/CpuDxe/CpuDxe.inf |  2 +
> 2 files changed, 79 insertions(+)
> 
> diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.c b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
> index d04958e79e52..83fd6fd4e476 100644
> --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.c
> +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
> @@ -11,6 +11,8 @@
> 
> #include <Guid/IdleLoopEvent.h>
> 
> +#include <Library/MemoryAllocationLib.h>
> +
> BOOLEAN  mIsFlushingGCD;
> 
> /**
> @@ -227,6 +229,69 @@ InitializeDma (
>   CpuArchProtocol->DmaBufferAlignment = ArmCacheWritebackGranule ();
> }
> 
> +STATIC
> +VOID
> +RemapUnusedMemoryNx (
> +  VOID
> +  )
> +{
> +  UINT64                     TestBit;
> +  UINTN                      MemoryMapSize;
> +  UINTN                      MapKey;
> +  UINTN                      DescriptorSize;
> +  UINT32                     DescriptorVersion;
> +  EFI_MEMORY_DESCRIPTOR      *MemoryMap;
> +  EFI_MEMORY_DESCRIPTOR      *MemoryMapEntry;
> +  EFI_MEMORY_DESCRIPTOR      *MemoryMapEnd;
> +  EFI_STATUS                 Status;
> +
> +  TestBit = LShiftU64 (1, EfiBootServicesData);
> +  if ((PcdGet64 (PcdDxeNxMemoryProtectionPolicy) & TestBit) == 0) {
> +    return;
> +  }
> +
> +  MemoryMapSize = 0;
> +  MemoryMap     = NULL;
> +
> +  Status = gBS->GetMemoryMap (
> +                  &MemoryMapSize,
> +                  MemoryMap,
> +                  &MapKey,
> +                  &DescriptorSize,
> +                  &DescriptorVersion
> +                  );
> +  ASSERT (Status == EFI_BUFFER_TOO_SMALL);
> +  do {
> +    MemoryMap = (EFI_MEMORY_DESCRIPTOR *)AllocatePool (MemoryMapSize);

nitpick: *If* there is a V2, maybe drop the cast?

> +    ASSERT (MemoryMap != NULL);

I know ASSERTing for NULL is a common pattern for some reason, but I'd rather not have more code with this added.

> +    Status = gBS->GetMemoryMap (
> +                    &MemoryMapSize,
> +                    MemoryMap,
> +                    &MapKey,
> +                    &DescriptorSize,
> +                    &DescriptorVersion
> +                    );

Another nitpick, isn't it common practice to call the Core* functions directly within *Core? I know there is code that doesn't, but the former should be more efficient.

> +    if (EFI_ERROR (Status)) {
> +      FreePool (MemoryMap);
> +    }
> +  } while (Status == EFI_BUFFER_TOO_SMALL);

Is this guaranteed to terminate? I mean, I get the idea - try again with the larger size and when allocating the bigger buffer, its potential new entry will already be accounted for. However, I saw downstream code that tried something like this (they actually added a constant overhead ahead of time) bounded by something like 5 iterations. Might just have been defensive programming - probably was -, but it's not trivial to verify, I think, is it?

Regarding the added constant, the spec actually says the following, which obviously is just to shortcut a second round of GetMemoryMap(), but still:
"The actual size of the buffer allocated for the consequent call to GetMemoryMap() should be bigger then the value returned in MemoryMapSize"

It appears the spec did not define a canonical way to call GetMemoryMap(), did it? :(

> +
> +  ASSERT_EFI_ERROR (Status);
> +
> +  MemoryMapEntry = MemoryMap;
> +  MemoryMapEnd   = (EFI_MEMORY_DESCRIPTOR *)((UINT8 *)MemoryMap + MemoryMapSize);
> +  while ((UINTN)MemoryMapEntry < (UINTN)MemoryMapEnd) {
> +    if (MemoryMapEntry->Type == EfiConventionalMemory) {
> +      ArmSetMemoryRegionNoExec (
> +        MemoryMapEntry->PhysicalStart,
> +        EFI_PAGES_TO_SIZE (MemoryMapEntry->NumberOfPages)
> +        );
> +    }
> +
> +    MemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry, DescriptorSize);
> +  }
> +}
> +
> EFI_STATUS
> CpuDxeInitialize (
>   IN EFI_HANDLE        ImageHandle,
> @@ -240,6 +305,18 @@ CpuDxeInitialize (
> 
>   InitializeDma (&mCpu);
> 
> +  //
> +  // Once we install the CPU arch protocol, the DXE core's memory
> +  // protection routines will invoke them to manage the permissions of page
> +  // allocations as they are created. Given that this includes pages
> +  // allocated for page tables by this driver, we must ensure that unused
> +  // memory is mapped with the same permissions as boot services data
> +  // regions. Otherwise, we may end up with unbounded recursion, due to the
> +  // fact that updating permissions on a newly allocated page table may trigger
> +  // a block entry split, which triggers a page table allocation, etc etc
> +  //
> +  RemapUnusedMemoryNx ();

Hmm. I might be too tired, but why is this not already done by InitializeDxeNxMemoryProtectionPolicy(), assuming BS_Data and ConvMem have the same XP configuration?

This reassures me that the CPU Arch protocol producers should be linked into DxeCore rather than loaded at some arbitrary point in time... Unrelated to the patch, of course.

Best regards,
Marvin

> +
>   Status = gBS->InstallMultipleProtocolInterfaces (
>                   &mCpuHandle,
>                   &gEfiCpuArchProtocolGuid,
> diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.inf b/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
> index e732e21cb94a..8fd0f4133088 100644
> --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
> +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
> @@ -48,6 +48,7 @@ [LibraryClasses]
>   DefaultExceptionHandlerLib
>   DxeServicesTableLib
>   HobLib
> +  MemoryAllocationLib
>   PeCoffGetEntryPointLib
>   UefiDriverEntryPoint
>   UefiLib
> @@ -64,6 +65,7 @@ [Guids]
> 
> [Pcd.common]
>   gArmTokenSpaceGuid.PcdVFPEnabled
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy
> 
> [FeaturePcd.common]
>   gArmTokenSpaceGuid.PcdDebuggerExceptionSupport
> -- 
> 2.39.1
> 

Re: [edk2-devel] [PATCH 2/3] ArmPkg/CpuDxe: Perform preliminary NX remap of free memory

[Ard Biesheuvel]

On Wed, 8 Feb 2023 at 19:32, Marvin Häuser <mhaeuser@posteo.de> wrote:
>
> Thanks!! :) Comments inline.
>
> > On 8. Feb 2023, at 18:58, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > The DXE core implementation of PcdDxeNxMemoryProtectionPolicy already
> > contains an assertion that EfiConventionalMemory and EfiBootServicesData
> > are subjected to the same policy when it comes to the use of NX
> > permissions. The reason for this is that we may otherwise end up with
> > unbounded recursion in the page table code, given that allocating a page
> > table would then involve a permission attribute change, and this could
> > result in the need for a block entry to be split, which would trigger
> > the allocation of a page table recursively.
> >
> > For the same reason, a shortcut exists in ApplyMemoryProtectionPolicy()
> > where, instead of setting the memory attributes unconditionally, we
> > compare the NX policies and avoid touching the page tables if they are
> > the same for the old and the new memory types. Without this shortcut, we
> > may end up in a situation where, as the CPU arch protocol DXE driver is
> > ramping up, the same unbounded recursion is triggered, due to the fact
> > that the NX policy for EfiConventionalMemory has not been applied yet.
> >
> > To break this cycle, let's remap all EfiConventionalMemory regions
> > according to the NX policy for EfiBootServicesData before exposing the
> > CPU arch protocol to the DXE core and other drivers. This ensures that
> > creating EfiBootServicesData allocations does not result in memory
> > attribute changes, and therefore no recursion.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> > ArmPkg/Drivers/CpuDxe/CpuDxe.c   | 77 ++++++++++++++++++++
> > ArmPkg/Drivers/CpuDxe/CpuDxe.inf |  2 +
> > 2 files changed, 79 insertions(+)
> >
> > diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.c b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
> > index d04958e79e52..83fd6fd4e476 100644
> > --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.c
> > +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
> > @@ -11,6 +11,8 @@
> >
> > #include <Guid/IdleLoopEvent.h>
> >
> > +#include <Library/MemoryAllocationLib.h>
> > +
> > BOOLEAN  mIsFlushingGCD;
> >
> > /**
> > @@ -227,6 +229,69 @@ InitializeDma (
> >   CpuArchProtocol->DmaBufferAlignment = ArmCacheWritebackGranule ();
> > }
> >
> > +STATIC
> > +VOID
> > +RemapUnusedMemoryNx (
> > +  VOID
> > +  )
> > +{
> > +  UINT64                     TestBit;
> > +  UINTN                      MemoryMapSize;
> > +  UINTN                      MapKey;
> > +  UINTN                      DescriptorSize;
> > +  UINT32                     DescriptorVersion;
> > +  EFI_MEMORY_DESCRIPTOR      *MemoryMap;
> > +  EFI_MEMORY_DESCRIPTOR      *MemoryMapEntry;
> > +  EFI_MEMORY_DESCRIPTOR      *MemoryMapEnd;
> > +  EFI_STATUS                 Status;
> > +
> > +  TestBit = LShiftU64 (1, EfiBootServicesData);
> > +  if ((PcdGet64 (PcdDxeNxMemoryProtectionPolicy) & TestBit) == 0) {
> > +    return;
> > +  }
> > +
> > +  MemoryMapSize = 0;
> > +  MemoryMap     = NULL;
> > +
> > +  Status = gBS->GetMemoryMap (
> > +                  &MemoryMapSize,
> > +                  MemoryMap,
> > +                  &MapKey,
> > +                  &DescriptorSize,
> > +                  &DescriptorVersion
> > +                  );
> > +  ASSERT (Status == EFI_BUFFER_TOO_SMALL);
> > +  do {
> > +    MemoryMap = (EFI_MEMORY_DESCRIPTOR *)AllocatePool (MemoryMapSize);
>
> nitpick: *If* there is a V2, maybe drop the cast?
>
> > +    ASSERT (MemoryMap != NULL);
>
> I know ASSERTing for NULL is a common pattern for some reason, but I'd rather not have more code with this added.
>
> > +    Status = gBS->GetMemoryMap (
> > +                    &MemoryMapSize,
> > +                    MemoryMap,
> > +                    &MapKey,
> > +                    &DescriptorSize,
> > +                    &DescriptorVersion
> > +                    );
>
> Another nitpick, isn't it common practice to call the Core* functions directly within *Core? I know there is code that doesn't, but the former should be more efficient.
>
> > +    if (EFI_ERROR (Status)) {
> > +      FreePool (MemoryMap);
> > +    }
> > +  } while (Status == EFI_BUFFER_TOO_SMALL);
>
> Is this guaranteed to terminate? I mean, I get the idea - try again with the larger size and when allocating the bigger buffer, its potential new entry will already be accounted for. However, I saw downstream code that tried something like this (they actually added a constant overhead ahead of time) bounded by something like 5 iterations. Might just have been defensive programming - probably was -, but it's not trivial to verify, I think, is it?
>
> Regarding the added constant, the spec actually says the following, which obviously is just to shortcut a second round of GetMemoryMap(), but still:
> "The actual size of the buffer allocated for the consequent call to GetMemoryMap() should be bigger then the value returned in MemoryMapSize"
>
> It appears the spec did not define a canonical way to call GetMemoryMap(), did it? :(
>

This is all copy-pasted from MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c

> > +
> > +  ASSERT_EFI_ERROR (Status);
> > +
> > +  MemoryMapEntry = MemoryMap;
> > +  MemoryMapEnd   = (EFI_MEMORY_DESCRIPTOR *)((UINT8 *)MemoryMap + MemoryMapSize);
> > +  while ((UINTN)MemoryMapEntry < (UINTN)MemoryMapEnd) {
> > +    if (MemoryMapEntry->Type == EfiConventionalMemory) {
> > +      ArmSetMemoryRegionNoExec (
> > +        MemoryMapEntry->PhysicalStart,
> > +        EFI_PAGES_TO_SIZE (MemoryMapEntry->NumberOfPages)
> > +        );
> > +    }
> > +
> > +    MemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry, DescriptorSize);
> > +  }
> > +}
> > +
> > EFI_STATUS
> > CpuDxeInitialize (
> >   IN EFI_HANDLE        ImageHandle,
> > @@ -240,6 +305,18 @@ CpuDxeInitialize (
> >
> >   InitializeDma (&mCpu);
> >
> > +  //
> > +  // Once we install the CPU arch protocol, the DXE core's memory
> > +  // protection routines will invoke them to manage the permissions of page
> > +  // allocations as they are created. Given that this includes pages
> > +  // allocated for page tables by this driver, we must ensure that unused
> > +  // memory is mapped with the same permissions as boot services data
> > +  // regions. Otherwise, we may end up with unbounded recursion, due to the
> > +  // fact that updating permissions on a newly allocated page table may trigger
> > +  // a block entry split, which triggers a page table allocation, etc etc
> > +  //
> > +  RemapUnusedMemoryNx ();
>
> Hmm. I might be too tired, but why is this not already done by InitializeDxeNxMemoryProtectionPolicy(), assuming BS_Data and ConvMem have the same XP configuration?
>
> This reassures me that the CPU Arch protocol producers should be linked into DxeCore rather than loaded at some arbitrary point in time... Unrelated to the patch, of course.
>

The ordering here is a bit tricky. As soon as the CPU arch protocol is
exposed, every allocation will be remapped individually, resulting in
page table splits and therefore recursion.


>
> > +
> >   Status = gBS->InstallMultipleProtocolInterfaces (
> >                   &mCpuHandle,
> >                   &gEfiCpuArchProtocolGuid,
> > diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.inf b/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
> > index e732e21cb94a..8fd0f4133088 100644
> > --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
> > +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
> > @@ -48,6 +48,7 @@ [LibraryClasses]
> >   DefaultExceptionHandlerLib
> >   DxeServicesTableLib
> >   HobLib
> > +  MemoryAllocationLib
> >   PeCoffGetEntryPointLib
> >   UefiDriverEntryPoint
> >   UefiLib
> > @@ -64,6 +65,7 @@ [Guids]
> >
> > [Pcd.common]
> >   gArmTokenSpaceGuid.PcdVFPEnabled
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy
> >
> > [FeaturePcd.common]
> >   gArmTokenSpaceGuid.PcdDebuggerExceptionSupport
> > --
> > 2.39.1
> >
>
>
>
> 
>
>

Re: [edk2-devel] [PATCH 2/3] ArmPkg/CpuDxe: Perform preliminary NX remap of free memory

[Taylor Beebe]

On 2/8/2023 10:49 AM, Ard Biesheuvel wrote:
> On Wed, 8 Feb 2023 at 19:32, Marvin Häuser <mhaeuser@posteo.de> wrote:
>>
>> Thanks!! :) Comments inline.
>>
>>> On 8. Feb 2023, at 18:58, Ard Biesheuvel <ardb@kernel.org> wrote:
>>>
>>> The DXE core implementation of PcdDxeNxMemoryProtectionPolicy already
>>> contains an assertion that EfiConventionalMemory and EfiBootServicesData
>>> are subjected to the same policy when it comes to the use of NX
>>> permissions. The reason for this is that we may otherwise end up with
>>> unbounded recursion in the page table code, given that allocating a page
>>> table would then involve a permission attribute change, and this could
>>> result in the need for a block entry to be split, which would trigger
>>> the allocation of a page table recursively.
>>>
>>> For the same reason, a shortcut exists in ApplyMemoryProtectionPolicy()
>>> where, instead of setting the memory attributes unconditionally, we
>>> compare the NX policies and avoid touching the page tables if they are
>>> the same for the old and the new memory types. Without this shortcut, we
>>> may end up in a situation where, as the CPU arch protocol DXE driver is
>>> ramping up, the same unbounded recursion is triggered, due to the fact
>>> that the NX policy for EfiConventionalMemory has not been applied yet.
>>>
>>> To break this cycle, let's remap all EfiConventionalMemory regions
>>> according to the NX policy for EfiBootServicesData before exposing the
>>> CPU arch protocol to the DXE core and other drivers. This ensures that
>>> creating EfiBootServicesData allocations does not result in memory
>>> attribute changes, and therefore no recursion.
>>>
>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>>> ---
>>> ArmPkg/Drivers/CpuDxe/CpuDxe.c   | 77 ++++++++++++++++++++
>>> ArmPkg/Drivers/CpuDxe/CpuDxe.inf |  2 +
>>> 2 files changed, 79 insertions(+)
>>>
>>> diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.c b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
>>> index d04958e79e52..83fd6fd4e476 100644
>>> --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.c
>>> +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
>>> @@ -11,6 +11,8 @@
>>>
>>> #include <Guid/IdleLoopEvent.h>
>>>
>>> +#include <Library/MemoryAllocationLib.h>
>>> +
>>> BOOLEAN  mIsFlushingGCD;
>>>
>>> /**
>>> @@ -227,6 +229,69 @@ InitializeDma (
>>>    CpuArchProtocol->DmaBufferAlignment = ArmCacheWritebackGranule ();
>>> }
>>>
>>> +STATIC
>>> +VOID
>>> +RemapUnusedMemoryNx (
>>> +  VOID
>>> +  )
>>> +{

This feels somewhat hacky but it's strictly better than what we have 
right now and a value add so I'm all for this change.

>>> +  UINT64                     TestBit;
>>> +  UINTN                      MemoryMapSize;
>>> +  UINTN                      MapKey;
>>> +  UINTN                      DescriptorSize;
>>> +  UINT32                     DescriptorVersion;
>>> +  EFI_MEMORY_DESCRIPTOR      *MemoryMap;
>>> +  EFI_MEMORY_DESCRIPTOR      *MemoryMapEntry;
>>> +  EFI_MEMORY_DESCRIPTOR      *MemoryMapEnd;
>>> +  EFI_STATUS                 Status;
>>> +
>>> +  TestBit = LShiftU64 (1, EfiBootServicesData);
>>> +  if ((PcdGet64 (PcdDxeNxMemoryProtectionPolicy) & TestBit) == 0) {
>>> +    return;
>>> +  }
>>> +
>>> +  MemoryMapSize = 0;
>>> +  MemoryMap     = NULL;
>>> +
>>> +  Status = gBS->GetMemoryMap (
>>> +                  &MemoryMapSize,
>>> +                  MemoryMap,
>>> +                  &MapKey,
>>> +                  &DescriptorSize,
>>> +                  &DescriptorVersion
>>> +                  );
>>> +  ASSERT (Status == EFI_BUFFER_TOO_SMALL);
>>> +  do {
>>> +    MemoryMap = (EFI_MEMORY_DESCRIPTOR *)AllocatePool (MemoryMapSize);
>>
>> nitpick: *If* there is a V2, maybe drop the cast?
>>
>>> +    ASSERT (MemoryMap != NULL);
>>
>> I know ASSERTing for NULL is a common pattern for some reason, but I'd rather not have more code with this added.
>>
>>> +    Status = gBS->GetMemoryMap (
>>> +                    &MemoryMapSize,
>>> +                    MemoryMap,
>>> +                    &MapKey,
>>> +                    &DescriptorSize,
>>> +                    &DescriptorVersion
>>> +                    );
>>
>> Another nitpick, isn't it common practice to call the Core* functions directly within *Core? I know there is code that doesn't, but the former should be more efficient.
>>
>>> +    if (EFI_ERROR (Status)) {
>>> +      FreePool (MemoryMap);
>>> +    }
>>> +  } while (Status == EFI_BUFFER_TOO_SMALL);
>>
>> Is this guaranteed to terminate? I mean, I get the idea - try again with the larger size and when allocating the bigger buffer, its potential new entry will already be accounted for. However, I saw downstream code that tried something like this (they actually added a constant overhead ahead of time) bounded by something like 5 iterations. Might just have been defensive programming - probably was -, but it's not trivial to verify, I think, is it?
>>
>> Regarding the added constant, the spec actually says the following, which obviously is just to shortcut a second round of GetMemoryMap(), but still:
>> "The actual size of the buffer allocated for the consequent call to GetMemoryMap() should be bigger then the value returned in MemoryMapSize"
>>
>> It appears the spec did not define a canonical way to call GetMemoryMap(), did it? :(
>>
> 
> This is all copy-pasted from MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> 
>>> +
>>> +  ASSERT_EFI_ERROR (Status);
>>> +
>>> +  MemoryMapEntry = MemoryMap;
>>> +  MemoryMapEnd   = (EFI_MEMORY_DESCRIPTOR *)((UINT8 *)MemoryMap + MemoryMapSize);
>>> +  while ((UINTN)MemoryMapEntry < (UINTN)MemoryMapEnd) {
>>> +    if (MemoryMapEntry->Type == EfiConventionalMemory) {
>>> +      ArmSetMemoryRegionNoExec (
>>> +        MemoryMapEntry->PhysicalStart,
>>> +        EFI_PAGES_TO_SIZE (MemoryMapEntry->NumberOfPages)
>>> +        );
>>> +    }
>>> +
>>> +    MemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry, DescriptorSize);
>>> +  }
>>> +}
>>> +
>>> EFI_STATUS
>>> CpuDxeInitialize (
>>>    IN EFI_HANDLE        ImageHandle,
>>> @@ -240,6 +305,18 @@ CpuDxeInitialize (
>>>
>>>    InitializeDma (&mCpu);
>>>
>>> +  //
>>> +  // Once we install the CPU arch protocol, the DXE core's memory
>>> +  // protection routines will invoke them to manage the permissions of page
>>> +  // allocations as they are created. Given that this includes pages
>>> +  // allocated for page tables by this driver, we must ensure that unused
>>> +  // memory is mapped with the same permissions as boot services data
>>> +  // regions. Otherwise, we may end up with unbounded recursion, due to the
>>> +  // fact that updating permissions on a newly allocated page table may trigger
>>> +  // a block entry split, which triggers a page table allocation, etc etc
>>> +  //
>>> +  RemapUnusedMemoryNx ();
>>
>> Hmm. I might be too tired, but why is this not already done by InitializeDxeNxMemoryProtectionPolicy(), assuming BS_Data and ConvMem have the same XP configuration?
>>
>> This reassures me that the CPU Arch protocol producers should be linked into DxeCore rather than loaded at some arbitrary point in time... Unrelated to the patch, of course.
>>
> 
> The ordering here is a bit tricky. As soon as the CPU arch protocol is
> exposed, every allocation will be remapped individually, resulting in
> page table splits and therefore recursion.
> 
> 

As Ard says, this is done in InitializeDxeNxMemoryProtectionPolicy(), 
but at that point the calls to ApplyMemoryProtectionPolicy() will cause 
the infinite recursion issue when you no longer skip applying attributes 
if the to-from types have the same NX policy.

Yes, it is arbitrary that protection is linked to the CPU Arch Protocol. 
But, it would also be bad practice to apply the memory protection policy 
before the interface for manipulating attributes has been published for 
use by modules outside of the core. Page table and translation table 
manipulation is architecturally defined, so I think a good long term 
goal should be to allow the manipulation of attributes via a library 
rather than a protocol in DXE as ARM platforms currently do.

>>
>>> +
>>>    Status = gBS->InstallMultipleProtocolInterfaces (
>>>                    &mCpuHandle,
>>>                    &gEfiCpuArchProtocolGuid,
>>> diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.inf b/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
>>> index e732e21cb94a..8fd0f4133088 100644
>>> --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
>>> +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
>>> @@ -48,6 +48,7 @@ [LibraryClasses]
>>>    DefaultExceptionHandlerLib
>>>    DxeServicesTableLib
>>>    HobLib
>>> +  MemoryAllocationLib
>>>    PeCoffGetEntryPointLib
>>>    UefiDriverEntryPoint
>>>    UefiLib
>>> @@ -64,6 +65,7 @@ [Guids]
>>>
>>> [Pcd.common]
>>>    gArmTokenSpaceGuid.PcdVFPEnabled
>>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy
>>>
>>> [FeaturePcd.common]
>>>    gArmTokenSpaceGuid.PcdDebuggerExceptionSupport
>>> --
>>> 2.39.1
>>>
>>
>>
>>
>> 
>>
>>

Re: [edk2-devel] [PATCH 2/3] ArmPkg/CpuDxe: Perform preliminary NX remap of free memory

[Ard Biesheuvel]

On Wed, 8 Feb 2023 at 19:57, Taylor Beebe <t@taylorbeebe.com> wrote:
>
>
>
> On 2/8/2023 10:49 AM, Ard Biesheuvel wrote:
> > On Wed, 8 Feb 2023 at 19:32, Marvin Häuser <mhaeuser@posteo.de> wrote:
> >>
> >> Thanks!! :) Comments inline.
> >>
> >>> On 8. Feb 2023, at 18:58, Ard Biesheuvel <ardb@kernel.org> wrote:
> >>>
> >>> The DXE core implementation of PcdDxeNxMemoryProtectionPolicy already
> >>> contains an assertion that EfiConventionalMemory and EfiBootServicesData
> >>> are subjected to the same policy when it comes to the use of NX
> >>> permissions. The reason for this is that we may otherwise end up with
> >>> unbounded recursion in the page table code, given that allocating a page
> >>> table would then involve a permission attribute change, and this could
> >>> result in the need for a block entry to be split, which would trigger
> >>> the allocation of a page table recursively.
> >>>
> >>> For the same reason, a shortcut exists in ApplyMemoryProtectionPolicy()
> >>> where, instead of setting the memory attributes unconditionally, we
> >>> compare the NX policies and avoid touching the page tables if they are
> >>> the same for the old and the new memory types. Without this shortcut, we
> >>> may end up in a situation where, as the CPU arch protocol DXE driver is
> >>> ramping up, the same unbounded recursion is triggered, due to the fact
> >>> that the NX policy for EfiConventionalMemory has not been applied yet.
> >>>
> >>> To break this cycle, let's remap all EfiConventionalMemory regions
> >>> according to the NX policy for EfiBootServicesData before exposing the
> >>> CPU arch protocol to the DXE core and other drivers. This ensures that
> >>> creating EfiBootServicesData allocations does not result in memory
> >>> attribute changes, and therefore no recursion.
> >>>
> >>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> >>> ---
> >>> ArmPkg/Drivers/CpuDxe/CpuDxe.c   | 77 ++++++++++++++++++++
> >>> ArmPkg/Drivers/CpuDxe/CpuDxe.inf |  2 +
> >>> 2 files changed, 79 insertions(+)
> >>>
> >>> diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.c b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
> >>> index d04958e79e52..83fd6fd4e476 100644
> >>> --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.c
> >>> +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
> >>> @@ -11,6 +11,8 @@
> >>>
> >>> #include <Guid/IdleLoopEvent.h>
> >>>
> >>> +#include <Library/MemoryAllocationLib.h>
> >>> +
> >>> BOOLEAN  mIsFlushingGCD;
> >>>
> >>> /**
> >>> @@ -227,6 +229,69 @@ InitializeDma (
> >>>    CpuArchProtocol->DmaBufferAlignment = ArmCacheWritebackGranule ();
> >>> }
> >>>
> >>> +STATIC
> >>> +VOID
> >>> +RemapUnusedMemoryNx (
> >>> +  VOID
> >>> +  )
> >>> +{
>
> This feels somewhat hacky but it's strictly better than what we have
> right now and a value add so I'm all for this change.
>
> >>> +  UINT64                     TestBit;
> >>> +  UINTN                      MemoryMapSize;
> >>> +  UINTN                      MapKey;
> >>> +  UINTN                      DescriptorSize;
> >>> +  UINT32                     DescriptorVersion;
> >>> +  EFI_MEMORY_DESCRIPTOR      *MemoryMap;
> >>> +  EFI_MEMORY_DESCRIPTOR      *MemoryMapEntry;
> >>> +  EFI_MEMORY_DESCRIPTOR      *MemoryMapEnd;
> >>> +  EFI_STATUS                 Status;
> >>> +
> >>> +  TestBit = LShiftU64 (1, EfiBootServicesData);
> >>> +  if ((PcdGet64 (PcdDxeNxMemoryProtectionPolicy) & TestBit) == 0) {
> >>> +    return;
> >>> +  }
> >>> +
> >>> +  MemoryMapSize = 0;
> >>> +  MemoryMap     = NULL;
> >>> +
> >>> +  Status = gBS->GetMemoryMap (
> >>> +                  &MemoryMapSize,
> >>> +                  MemoryMap,
> >>> +                  &MapKey,
> >>> +                  &DescriptorSize,
> >>> +                  &DescriptorVersion
> >>> +                  );
> >>> +  ASSERT (Status == EFI_BUFFER_TOO_SMALL);
> >>> +  do {
> >>> +    MemoryMap = (EFI_MEMORY_DESCRIPTOR *)AllocatePool (MemoryMapSize);
> >>
> >> nitpick: *If* there is a V2, maybe drop the cast?
> >>
> >>> +    ASSERT (MemoryMap != NULL);
> >>
> >> I know ASSERTing for NULL is a common pattern for some reason, but I'd rather not have more code with this added.
> >>
> >>> +    Status = gBS->GetMemoryMap (
> >>> +                    &MemoryMapSize,
> >>> +                    MemoryMap,
> >>> +                    &MapKey,
> >>> +                    &DescriptorSize,
> >>> +                    &DescriptorVersion
> >>> +                    );
> >>
> >> Another nitpick, isn't it common practice to call the Core* functions directly within *Core? I know there is code that doesn't, but the former should be more efficient.
> >>
> >>> +    if (EFI_ERROR (Status)) {
> >>> +      FreePool (MemoryMap);
> >>> +    }
> >>> +  } while (Status == EFI_BUFFER_TOO_SMALL);
> >>
> >> Is this guaranteed to terminate? I mean, I get the idea - try again with the larger size and when allocating the bigger buffer, its potential new entry will already be accounted for. However, I saw downstream code that tried something like this (they actually added a constant overhead ahead of time) bounded by something like 5 iterations. Might just have been defensive programming - probably was -, but it's not trivial to verify, I think, is it?
> >>
> >> Regarding the added constant, the spec actually says the following, which obviously is just to shortcut a second round of GetMemoryMap(), but still:
> >> "The actual size of the buffer allocated for the consequent call to GetMemoryMap() should be bigger then the value returned in MemoryMapSize"
> >>
> >> It appears the spec did not define a canonical way to call GetMemoryMap(), did it? :(
> >>
> >
> > This is all copy-pasted from MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> >
> >>> +
> >>> +  ASSERT_EFI_ERROR (Status);
> >>> +
> >>> +  MemoryMapEntry = MemoryMap;
> >>> +  MemoryMapEnd   = (EFI_MEMORY_DESCRIPTOR *)((UINT8 *)MemoryMap + MemoryMapSize);
> >>> +  while ((UINTN)MemoryMapEntry < (UINTN)MemoryMapEnd) {
> >>> +    if (MemoryMapEntry->Type == EfiConventionalMemory) {
> >>> +      ArmSetMemoryRegionNoExec (
> >>> +        MemoryMapEntry->PhysicalStart,
> >>> +        EFI_PAGES_TO_SIZE (MemoryMapEntry->NumberOfPages)
> >>> +        );
> >>> +    }
> >>> +
> >>> +    MemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry, DescriptorSize);
> >>> +  }
> >>> +}
> >>> +
> >>> EFI_STATUS
> >>> CpuDxeInitialize (
> >>>    IN EFI_HANDLE        ImageHandle,
> >>> @@ -240,6 +305,18 @@ CpuDxeInitialize (
> >>>
> >>>    InitializeDma (&mCpu);
> >>>
> >>> +  //
> >>> +  // Once we install the CPU arch protocol, the DXE core's memory
> >>> +  // protection routines will invoke them to manage the permissions of page
> >>> +  // allocations as they are created. Given that this includes pages
> >>> +  // allocated for page tables by this driver, we must ensure that unused
> >>> +  // memory is mapped with the same permissions as boot services data
> >>> +  // regions. Otherwise, we may end up with unbounded recursion, due to the
> >>> +  // fact that updating permissions on a newly allocated page table may trigger
> >>> +  // a block entry split, which triggers a page table allocation, etc etc
> >>> +  //
> >>> +  RemapUnusedMemoryNx ();
> >>
> >> Hmm. I might be too tired, but why is this not already done by InitializeDxeNxMemoryProtectionPolicy(), assuming BS_Data and ConvMem have the same XP configuration?
> >>
> >> This reassures me that the CPU Arch protocol producers should be linked into DxeCore rather than loaded at some arbitrary point in time... Unrelated to the patch, of course.
> >>
> >
> > The ordering here is a bit tricky. As soon as the CPU arch protocol is
> > exposed, every allocation will be remapped individually, resulting in
> > page table splits and therefore recursion.
> >
> >
>
> As Ard says, this is done in InitializeDxeNxMemoryProtectionPolicy(),
> but at that point the calls to ApplyMemoryProtectionPolicy() will cause
> the infinite recursion issue when you no longer skip applying attributes
> if the to-from types have the same NX policy.
>
> Yes, it is arbitrary that protection is linked to the CPU Arch Protocol.
> But, it would also be bad practice to apply the memory protection policy
> before the interface for manipulating attributes has been published for
> use by modules outside of the core. Page table and translation table
> manipulation is architecturally defined, so I think a good long term
> goal should be to allow the manipulation of attributes via a library
> rather than a protocol in DXE as ARM platforms currently do.
>

Note that the routine above is slightly dodgy in the sense that the
memory map might change while it is being traversed, due to the page
allocations that result from splitting block entries. This is why it
is important that EfiConventionalMemory and EfiBootServicesData use
the same NX policy, so that such changes to the memory map do not
affect the outcome of the traversal.

If we want to move this into generic code, I think it should be
feasible for the DXE core to
- grab the CPU arch protocol but don't make it available globally
right away, so that AllocatePages() will not recurse
- call the CPU arch protocol to map all EfiConventionalMemory as NX
(with the caveat above)
- proceed with remapping the active regions, which will now set the
attributes on all EfiBootServicesData regions again.

That would make the thing a bit more robust, but I am reluctant to add
this to generic code and deal with the fallout on x86 or RISC-V if it
breaks something there.

[PATCH 3/3] MdeModulePkg/DxeCore: Unconditionally set memory protections

[Ard Biesheuvel]

Instead of relying on a questionable heuristic that avoids calling into
the SetMemoryAttributes () DXE service when the old memory type and the
new one are subjected to the same NX memory protection policy, make this
call unconditionally. This avoids corner cases where memory region
attributes are out of sync with the policy, either due to the fact that
we are in the middle of ramping up the protections, or due to explicit
invocations of SetMemoryAttributes() by drivers.

This requires the architecture page table code to be able to deal with
this, in particular, it needs to be robust against potential recursion
due to NX policies being applied to newly allocated page tables.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 29 --------------------
 1 file changed, 29 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
index 36987843f142..503feb72b5d0 100644
--- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
+++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
@@ -1263,9 +1263,7 @@ ApplyMemoryProtectionPolicy (
   IN  UINT64                Length
   )
 {
-  UINT64      OldAttributes;
   UINT64      NewAttributes;
-  EFI_STATUS  Status;
 
   //
   // The policy configured in PcdDxeNxMemoryProtectionPolicy
@@ -1320,32 +1318,5 @@ ApplyMemoryProtectionPolicy (
   //
   NewAttributes = GetPermissionAttributeForMemoryType (NewType);
 
-  if (OldType != EfiMaxMemoryType) {
-    OldAttributes = GetPermissionAttributeForMemoryType (OldType);
-    if (!mAfterDxeNxMemoryProtectionInit &&
-        (OldAttributes == NewAttributes)) {
-      return EFI_SUCCESS;
-    }
-
-    //
-    // If available, use the EFI memory attribute protocol to obtain
-    // the current attributes of the region. If the entire region is
-    // covered and the attributes match, we don't have to do anything.
-    //
-    if (mMemoryAttribute != NULL) {
-      Status = mMemoryAttribute->GetMemoryAttributes (mMemoryAttribute,
-                                                      Memory,
-                                                      Length,
-                                                      &OldAttributes
-                                                      );
-      if (!EFI_ERROR (Status) && (OldAttributes == NewAttributes)) {
-        return EFI_SUCCESS;
-      }
-    }
-  } else if (NewAttributes == 0) {
-    // newly added region of a type that does not require protection
-    return EFI_SUCCESS;
-  }
-
   return gCpu->SetMemoryAttributes (gCpu, Memory, Length, NewAttributes);
 }
-- 
2.39.1

Re: [PATCH 3/3] MdeModulePkg/DxeCore: Unconditionally set memory protections

[Ard Biesheuvel]

On Wed, 8 Feb 2023 at 18:58, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> Instead of relying on a questionable heuristic that avoids calling into
> the SetMemoryAttributes () DXE service when the old memory type and the
> new one are subjected to the same NX memory protection policy, make this
> call unconditionally. This avoids corner cases where memory region
> attributes are out of sync with the policy, either due to the fact that
> we are in the middle of ramping up the protections, or due to explicit
> invocations of SetMemoryAttributes() by drivers.
>
> This requires the architecture page table code to be able to deal with
> this, in particular, it needs to be robust against potential recursion
> due to NX policies being applied to newly allocated page tables.
>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 29 --------------------
>  1 file changed, 29 deletions(-)
>
> diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> index 36987843f142..503feb72b5d0 100644
> --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> @@ -1263,9 +1263,7 @@ ApplyMemoryProtectionPolicy (
>    IN  UINT64                Length
>    )
>  {
> -  UINT64      OldAttributes;
>    UINT64      NewAttributes;
> -  EFI_STATUS  Status;
>
>    //
>    // The policy configured in PcdDxeNxMemoryProtectionPolicy
> @@ -1320,32 +1318,5 @@ ApplyMemoryProtectionPolicy (
>    //
>    NewAttributes = GetPermissionAttributeForMemoryType (NewType);
>
> -  if (OldType != EfiMaxMemoryType) {
> -    OldAttributes = GetPermissionAttributeForMemoryType (OldType);
> -    if (!mAfterDxeNxMemoryProtectionInit &&
> -        (OldAttributes == NewAttributes)) {
> -      return EFI_SUCCESS;
> -    }
> -

This removes some code that does not actually exist - apologies.

It comes down to just removing the conditional checks here, though,
and perform the tail call below unconditionally.

> -    //
> -    // If available, use the EFI memory attribute protocol to obtain
> -    // the current attributes of the region. If the entire region is
> -    // covered and the attributes match, we don't have to do anything.
> -    //
> -    if (mMemoryAttribute != NULL) {
> -      Status = mMemoryAttribute->GetMemoryAttributes (mMemoryAttribute,
> -                                                      Memory,
> -                                                      Length,
> -                                                      &OldAttributes
> -                                                      );
> -      if (!EFI_ERROR (Status) && (OldAttributes == NewAttributes)) {
> -        return EFI_SUCCESS;
> -      }
> -    }
> -  } else if (NewAttributes == 0) {
> -    // newly added region of a type that does not require protection
> -    return EFI_SUCCESS;
> -  }
> -
>    return gCpu->SetMemoryAttributes (gCpu, Memory, Length, NewAttributes);
>  }
> --
> 2.39.1
>

Re: [PATCH 3/3] MdeModulePkg/DxeCore: Unconditionally set memory protections

[Marvin Häuser]

> On 8. Feb 2023, at 19:49, Ard Biesheuvel <ardb@kernel.org> wrote:
> 
> This is all copy-pasted from MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c

:(

> 
> The ordering here is a bit tricky. As soon as the CPU arch protocol is
> exposed, every allocation will be remapped individually, resulting in
> page table splits and therefore recursion.

So the issue is the order of event handlers or allocations within the event dispatcher? :( Oh lord...

Can we maybe clarify the comment with something like "While DxeCore/InitializeDxeNxMemoryProtectionPolicy() would in theory perform this task, allocations between the protocol installation and the invocation of its event handler may trigger the issue."?

Re: [PATCH 3/3] MdeModulePkg/DxeCore: Unconditionally set memory protections

[Taylor Beebe]

I ran some tests and did some quick napkin math. Based on the time it 
takes to perform the SetMemoryAttributes() routine on QEMU, as long as 
<79% of the calls to ApplyMemoryProtectionPolicy() actually require a 
subsequent call to SetMemoryAttributes(), it is more efficient to walk 
the page/translation table to check if the attributes actually need to 
be updated. Even on a platform with varied NX policy settings, the 
number of times the attributes need to be updated is less than 0.0005% 
of all calls to ApplyMemoryProtectionPolicy() (likely due to most 
allocations being BootServicesData).

Once the ARM and X64 implementations of the Memory Attribute Protocol 
are in, would you be open to updating this block to utilize the protocol 
to check the attributes of the region being updated?

On 2/8/2023 10:25 AM, Ard Biesheuvel wrote:
> On Wed, 8 Feb 2023 at 18:58, Ard Biesheuvel <ardb@kernel.org> wrote:
>>
>> Instead of relying on a questionable heuristic that avoids calling into
>> the SetMemoryAttributes () DXE service when the old memory type and the
>> new one are subjected to the same NX memory protection policy, make this
>> call unconditionally. This avoids corner cases where memory region
>> attributes are out of sync with the policy, either due to the fact that
>> we are in the middle of ramping up the protections, or due to explicit
>> invocations of SetMemoryAttributes() by drivers.
>>
>> This requires the architecture page table code to be able to deal with
>> this, in particular, it needs to be robust against potential recursion
>> due to NX policies being applied to newly allocated page tables.
>>
>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>> ---
>>   MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 29 --------------------
>>   1 file changed, 29 deletions(-)
>>
>> diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
>> index 36987843f142..503feb72b5d0 100644
>> --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
>> +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
>> @@ -1263,9 +1263,7 @@ ApplyMemoryProtectionPolicy (
>>     IN  UINT64                Length
>>     )
>>   {
>> -  UINT64      OldAttributes;
>>     UINT64      NewAttributes;
>> -  EFI_STATUS  Status;
>>
>>     //
>>     // The policy configured in PcdDxeNxMemoryProtectionPolicy
>> @@ -1320,32 +1318,5 @@ ApplyMemoryProtectionPolicy (
>>     //
>>     NewAttributes = GetPermissionAttributeForMemoryType (NewType);
>>
>> -  if (OldType != EfiMaxMemoryType) {
>> -    OldAttributes = GetPermissionAttributeForMemoryType (OldType);
>> -    if (!mAfterDxeNxMemoryProtectionInit &&
>> -        (OldAttributes == NewAttributes)) {
>> -      return EFI_SUCCESS;
>> -    }
>> -
> 
> This removes some code that does not actually exist - apologies.
> 
> It comes down to just removing the conditional checks here, though,
> and perform the tail call below unconditionally.
> 
>> -    //
>> -    // If available, use the EFI memory attribute protocol to obtain
>> -    // the current attributes of the region. If the entire region is
>> -    // covered and the attributes match, we don't have to do anything.
>> -    //
>> -    if (mMemoryAttribute != NULL) {
>> -      Status = mMemoryAttribute->GetMemoryAttributes (mMemoryAttribute,
>> -                                                      Memory,
>> -                                                      Length,
>> -                                                      &OldAttributes
>> -                                                      );
>> -      if (!EFI_ERROR (Status) && (OldAttributes == NewAttributes)) {
>> -        return EFI_SUCCESS;
>> -      }
>> -    }
>> -  } else if (NewAttributes == 0) {
>> -    // newly added region of a type that does not require protection
>> -    return EFI_SUCCESS;
>> -  }
>> -
>>     return gCpu->SetMemoryAttributes (gCpu, Memory, Length, NewAttributes);
>>   }
>> --
>> 2.39.1
>>

Re: [PATCH 3/3] MdeModulePkg/DxeCore: Unconditionally set memory protections

[Ard Biesheuvel]

On Wed, 8 Feb 2023 at 20:12, Taylor Beebe <t@taylorbeebe.com> wrote:
>
> I ran some tests and did some quick napkin math. Based on the time it
> takes to perform the SetMemoryAttributes() routine on QEMU, as long as
> <79% of the calls to ApplyMemoryProtectionPolicy() actually require a
> subsequent call to SetMemoryAttributes(), it is more efficient to walk
> the page/translation table to check if the attributes actually need to
> be updated. Even on a platform with varied NX policy settings, the
> number of times the attributes need to be updated is less than 0.0005%
> of all calls to ApplyMemoryProtectionPolicy() (likely due to most
> allocations being BootServicesData).
>
> Once the ARM and X64 implementations of the Memory Attribute Protocol
> are in, would you be open to updating this block to utilize the protocol
> to check the attributes of the region being updated?
>

Yes, that was what I had in my initial prototype.

However, I'm not sure how walking the page tables to retrieve all
existing attributes is fundamentally different from walking the page
tables to set them, given that everything is cached and we are running
uniprocessor at this point.


> On 2/8/2023 10:25 AM, Ard Biesheuvel wrote:
> > On Wed, 8 Feb 2023 at 18:58, Ard Biesheuvel <ardb@kernel.org> wrote:
> >>
> >> Instead of relying on a questionable heuristic that avoids calling into
> >> the SetMemoryAttributes () DXE service when the old memory type and the
> >> new one are subjected to the same NX memory protection policy, make this
> >> call unconditionally. This avoids corner cases where memory region
> >> attributes are out of sync with the policy, either due to the fact that
> >> we are in the middle of ramping up the protections, or due to explicit
> >> invocations of SetMemoryAttributes() by drivers.
> >>
> >> This requires the architecture page table code to be able to deal with
> >> this, in particular, it needs to be robust against potential recursion
> >> due to NX policies being applied to newly allocated page tables.
> >>
> >> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> >> ---
> >>   MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 29 --------------------
> >>   1 file changed, 29 deletions(-)
> >>
> >> diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> >> index 36987843f142..503feb72b5d0 100644
> >> --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> >> +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> >> @@ -1263,9 +1263,7 @@ ApplyMemoryProtectionPolicy (
> >>     IN  UINT64                Length
> >>     )
> >>   {
> >> -  UINT64      OldAttributes;
> >>     UINT64      NewAttributes;
> >> -  EFI_STATUS  Status;
> >>
> >>     //
> >>     // The policy configured in PcdDxeNxMemoryProtectionPolicy
> >> @@ -1320,32 +1318,5 @@ ApplyMemoryProtectionPolicy (
> >>     //
> >>     NewAttributes = GetPermissionAttributeForMemoryType (NewType);
> >>
> >> -  if (OldType != EfiMaxMemoryType) {
> >> -    OldAttributes = GetPermissionAttributeForMemoryType (OldType);
> >> -    if (!mAfterDxeNxMemoryProtectionInit &&
> >> -        (OldAttributes == NewAttributes)) {
> >> -      return EFI_SUCCESS;
> >> -    }
> >> -
> >
> > This removes some code that does not actually exist - apologies.
> >
> > It comes down to just removing the conditional checks here, though,
> > and perform the tail call below unconditionally.
> >
> >> -    //
> >> -    // If available, use the EFI memory attribute protocol to obtain
> >> -    // the current attributes of the region. If the entire region is
> >> -    // covered and the attributes match, we don't have to do anything.
> >> -    //
> >> -    if (mMemoryAttribute != NULL) {
> >> -      Status = mMemoryAttribute->GetMemoryAttributes (mMemoryAttribute,
> >> -                                                      Memory,
> >> -                                                      Length,
> >> -                                                      &OldAttributes
> >> -                                                      );
> >> -      if (!EFI_ERROR (Status) && (OldAttributes == NewAttributes)) {
> >> -        return EFI_SUCCESS;
> >> -      }
> >> -    }
> >> -  } else if (NewAttributes == 0) {
> >> -    // newly added region of a type that does not require protection
> >> -    return EFI_SUCCESS;
> >> -  }
> >> -
> >>     return gCpu->SetMemoryAttributes (gCpu, Memory, Length, NewAttributes);
> >>   }
> >> --
> >> 2.39.1
> >>

Re: [PATCH 3/3] MdeModulePkg/DxeCore: Unconditionally set memory protections

[Taylor Beebe]

On 2/8/2023 2:08 PM, Ard Biesheuvel wrote:
> On Wed, 8 Feb 2023 at 20:12, Taylor Beebe <t@taylorbeebe.com> wrote:
>>
>> I ran some tests and did some quick napkin math. Based on the time it
>> takes to perform the SetMemoryAttributes() routine on QEMU, as long as
>> <79% of the calls to ApplyMemoryProtectionPolicy() actually require a
>> subsequent call to SetMemoryAttributes(), it is more efficient to walk
>> the page/translation table to check if the attributes actually need to
>> be updated. Even on a platform with varied NX policy settings, the
>> number of times the attributes need to be updated is less than 0.0005%
>> of all calls to ApplyMemoryProtectionPolicy() (likely due to most
>> allocations being BootServicesData).
>>
>> Once the ARM and X64 implementations of the Memory Attribute Protocol
>> are in, would you be open to updating this block to utilize the protocol
>> to check the attributes of the region being updated?
>>
> 
> Yes, that was what I had in my initial prototype.
> 
> However, I'm not sure how walking the page tables to retrieve all
> existing attributes is fundamentally different from walking the page
> tables to set them, given that everything is cached and we are running
> uniprocessor at this point.
> 

I was also skeptical, but running the experiment revealed that checking 
the page table is the clear winner. My guess is that branch prediction 
expects the table walk to reveal that the SetAttributes() call is 
unnecessary, and the cost of doing an instruction pipeline flush due to 
improper branch prediction is similar to the cost of doing a TLB 
flush/invalidation after the call to SetAttributes() making the upside 
outweigh the downside in almost all cases.

> 
>> On 2/8/2023 10:25 AM, Ard Biesheuvel wrote:
>>> On Wed, 8 Feb 2023 at 18:58, Ard Biesheuvel <ardb@kernel.org> wrote:
>>>>
>>>> Instead of relying on a questionable heuristic that avoids calling into
>>>> the SetMemoryAttributes () DXE service when the old memory type and the
>>>> new one are subjected to the same NX memory protection policy, make this
>>>> call unconditionally. This avoids corner cases where memory region
>>>> attributes are out of sync with the policy, either due to the fact that
>>>> we are in the middle of ramping up the protections, or due to explicit
>>>> invocations of SetMemoryAttributes() by drivers.
>>>>
>>>> This requires the architecture page table code to be able to deal with
>>>> this, in particular, it needs to be robust against potential recursion
>>>> due to NX policies being applied to newly allocated page tables.
>>>>
>>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>>>> ---
>>>>    MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 29 --------------------
>>>>    1 file changed, 29 deletions(-)
>>>>
>>>> diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
>>>> index 36987843f142..503feb72b5d0 100644
>>>> --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
>>>> +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
>>>> @@ -1263,9 +1263,7 @@ ApplyMemoryProtectionPolicy (
>>>>      IN  UINT64                Length
>>>>      )
>>>>    {
>>>> -  UINT64      OldAttributes;
>>>>      UINT64      NewAttributes;
>>>> -  EFI_STATUS  Status;
>>>>
>>>>      //
>>>>      // The policy configured in PcdDxeNxMemoryProtectionPolicy
>>>> @@ -1320,32 +1318,5 @@ ApplyMemoryProtectionPolicy (
>>>>      //
>>>>      NewAttributes = GetPermissionAttributeForMemoryType (NewType);
>>>>
>>>> -  if (OldType != EfiMaxMemoryType) {
>>>> -    OldAttributes = GetPermissionAttributeForMemoryType (OldType);
>>>> -    if (!mAfterDxeNxMemoryProtectionInit &&
>>>> -        (OldAttributes == NewAttributes)) {
>>>> -      return EFI_SUCCESS;
>>>> -    }
>>>> -
>>>
>>> This removes some code that does not actually exist - apologies.
>>>
>>> It comes down to just removing the conditional checks here, though,
>>> and perform the tail call below unconditionally.
>>>
>>>> -    //
>>>> -    // If available, use the EFI memory attribute protocol to obtain
>>>> -    // the current attributes of the region. If the entire region is
>>>> -    // covered and the attributes match, we don't have to do anything.
>>>> -    //
>>>> -    if (mMemoryAttribute != NULL) {
>>>> -      Status = mMemoryAttribute->GetMemoryAttributes (mMemoryAttribute,
>>>> -                                                      Memory,
>>>> -                                                      Length,
>>>> -                                                      &OldAttributes
>>>> -                                                      );
>>>> -      if (!EFI_ERROR (Status) && (OldAttributes == NewAttributes)) {
>>>> -        return EFI_SUCCESS;
>>>> -      }
>>>> -    }
>>>> -  } else if (NewAttributes == 0) {
>>>> -    // newly added region of a type that does not require protection
>>>> -    return EFI_SUCCESS;
>>>> -  }
>>>> -
>>>>      return gCpu->SetMemoryAttributes (gCpu, Memory, Length, NewAttributes);
>>>>    }
>>>> --
>>>> 2.39.1
>>>>

-- 
Taylor Beebe
Software Engineer @ Microsoft