[RFC PATCH 0/4] RFC: increased memory protection

[RFC PATCH 0/4] RFC: increased memory protection

[Ard Biesheuvel]

Hello all,

This is a proof of concept implementation that removes all executable
permissions from writable memory regions, which greatly enhances security.
It is based on Jiewen's recent work, which is a step in the right direction,
but still leaves most of memory exploitable due to the default R+W+X
permissions.

The idea is that the implementation of the CPU arch protocol goes over the
memory map and removes exec permissions from all regions that are not already
marked as 'code. This requires some preparatory work to ensure that the DxeCore
itself is covered by a BootServicesCode region, not a BootServicesData region.
Exec permissions are re-granted selectively, when the PE/COFF loader allocates
the space for it. Combined with Jiewen's code/data split, this removes all
RWX mapped regions.

There is a caveat, though (and there are likely more of that kind): the EBC
driver will need some work to ensure the thunk buffers have the noexec
restriction lifted. This could be done in the EBC driver, but perhaps it is
better to either
a) modify the DXE core so it always removes noexec restrictions when allocating
   code pages, or
b) add AllocateExecPages/AllocateExecPool() functions to the MemoryAllocationLib
   API

Comments please!

Ard Biesheuvel (4):
  MdeModulePkg/DxeCore: allow BootServicesData->BootServicesCode
    conversion
  MdeModulePkg/DxeCore: convert the DxeCore memory region to
    BootServicesCode
  MdeModulePkg/DxeCore: lift non-exec permissions on loaded images
  ArmPkg/CpuDxe: remap all data regions non-executable

 ArmPkg/Drivers/CpuDxe/CpuDxe.c          | 76 ++++++++++++++++++++
 MdeModulePkg/Core/Dxe/DxeMain.h         |  8 +++
 MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c |  2 +
 MdeModulePkg/Core/Dxe/Image/Image.c     |  8 +++
 MdeModulePkg/Core/Dxe/Mem/Page.c        | 18 ++++-
 5 files changed, 111 insertions(+), 1 deletion(-)

-- 
2.7.4

[RFC PATCH 1/4] MdeModulePkg/DxeCore: allow BootServicesData->BootServicesCode conversion

[Ard Biesheuvel]

Unlike all other PE/COFF images loaded after it, the DXE core is loaded
into BootServicesData memory rather than BootServicesCode memory, due to
the fact that the PEI phase memory allocation routines only distinguish
between boot-time and runtime.

So in preparation of adding support for restricted permissions, allow the
direct conversion of BootServicesData to BootServicesCode.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 MdeModulePkg/Core/Dxe/Mem/Page.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
index bda4f6397e91..b0939c596991 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Page.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
@@ -773,7 +773,8 @@ CoreConvertPagesEx (
       //
       // Debug code - verify conversion is allowed
       //
-      if (!(NewType == EfiConventionalMemory ? 1 : 0) ^ (Entry->Type == EfiConventionalMemory ? 1 : 0)) {
+      if (!(NewType == EfiConventionalMemory ? 1 : 0) ^ (Entry->Type == EfiConventionalMemory ? 1 : 0) &&
+          !(NewType == EfiBootServicesCode && Entry->Type == EfiBootServicesData)) {
         DEBUG ((DEBUG_ERROR | DEBUG_PAGE, "ConvertPages: Incompatible memory types\n"));
         return EFI_NOT_FOUND;
       }
-- 
2.7.4

[RFC PATCH 2/4] MdeModulePkg/DxeCore: convert the DxeCore memory region to BootServicesCode

[Ard Biesheuvel]

Before removing exec permissions from all non-code regions, ensure that
the DXE core itself is covered by a BootServicesCode region, by adding
a new function ConvertDxeCoreImage () and calling it at the right time
from DxeMain ().

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 MdeModulePkg/Core/Dxe/DxeMain.h         |  8 ++++++++
 MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c |  2 ++
 MdeModulePkg/Core/Dxe/Mem/Page.c        | 15 +++++++++++++++
 3 files changed, 25 insertions(+)

diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h b/MdeModulePkg/Core/Dxe/DxeMain.h
index b14be9a74d8e..300f19a3aa58 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain.h
+++ b/MdeModulePkg/Core/Dxe/DxeMain.h
@@ -2949,4 +2949,12 @@ MemoryProtectionExitBootServicesCallback (
   VOID
   );
 
+/**
+  Convert DXE core image to BootServicesCode memory
+**/
+VOID
+ConvertDxeCoreImage (
+  VOID
+  );
+
 #endif
diff --git a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
index 91e94a78d205..d3a873e737b1 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
+++ b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
@@ -396,6 +396,8 @@ DxeMain (
 
   MemoryProfileInstallProtocol ();
 
+  ConvertDxeCoreImage ();
+
   CoreInitializePropertiesTable ();
   CoreInitializeMemoryAttributesTable ();
   CoreInitializeMemoryProtection ();
diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
index b0939c596991..73b56fccf965 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Page.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
@@ -1951,8 +1951,23 @@ Done:
 }
 
 
+VOID
+ConvertDxeCoreImage (
+  VOID
+  )
+{
+  CoreAcquireMemoryLock ();
 
+  //
+  // Convert the memory region that backs the DXE core to a 'code' region, so
+  // that the strict permissions handling doesn't take our exec permissions
+  // away.
+  //
+  CoreConvertPages ((UINTN)gDxeCoreLoadedImage->ImageBase,
+    EFI_SIZE_TO_PAGES (gDxeCoreLoadedImage->ImageSize), EfiBootServicesCode);
 
+  CoreReleaseMemoryLock ();
+}
 
 
 
-- 
2.7.4

[RFC PATCH 3/4] MdeModulePkg/DxeCore: lift non-exec permissions on loaded images

[Ard Biesheuvel]

To ensure that loaded PE/COFF images are executable regardless of
the protection policy and the section alignment, clear all permission
restrictions when loading PE/COFF images.

Subsequently, permissions may be restricted again if the protection
policy and section alignment allow it.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 MdeModulePkg/Core/Dxe/Image/Image.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c b/MdeModulePkg/Core/Dxe/Image/Image.c
index 652da8bf1075..cab06e821e39 100644
--- a/MdeModulePkg/Core/Dxe/Image/Image.c
+++ b/MdeModulePkg/Core/Dxe/Image/Image.c
@@ -644,6 +644,14 @@ CoreLoadPeImage (
   InvalidateInstructionCacheRange ((VOID *)(UINTN)Image->ImageContext.ImageAddress, (UINTN)Image->ImageContext.ImageSize);
 
   //
+  // Remove any permission restrictions.
+  //
+  if (gCpu != NULL) {
+    gCpu->SetMemoryAttributes (gCpu, Image->ImageContext.ImageAddress,
+            Image->ImageContext.ImageSize, 0);
+  }
+
+  //
   // Copy the machine type from the context to the image private data. This
   // is needed during image unload to know if we should call an EBC protocol
   // to unload the image.
-- 
2.7.4

[RFC PATCH 4/4] ArmPkg/CpuDxe: remap all data regions non-executable

[Ard Biesheuvel]

When installing the CPU arch protocol, iterate over the UEFI memory map
and remove the executable permissions from each encountered non-code
region. Those will be re-added later selectively, to the extent required
according to the image protection policy and section alignment.

With a strict image protection policy in place, this all but eliminates
any regions that are mapped both writable and executable, which is an
significant improvement in security.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/Drivers/CpuDxe/CpuDxe.c | 76 ++++++++++++++++++++
 1 file changed, 76 insertions(+)

diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.c b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
index 7d328d096b1e..dd3bf44a00b3 100644
--- a/ArmPkg/Drivers/CpuDxe/CpuDxe.c
+++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
@@ -15,6 +15,9 @@
 
 #include "CpuDxe.h"
 
+#include <Library/MemoryAllocationLib.h>
+#include <Library/UefiBootServicesTableLib.h>
+
 #include <Guid/IdleLoopEvent.h>
 
 
@@ -237,6 +240,74 @@ InitializeDma (
   CpuArchProtocol->DmaBufferAlignment = ArmCacheWritebackGranule ();
 }
 
+STATIC
+VOID
+RemapAllDataRegionsNonExec (
+  VOID
+  )
+{
+  UINTN                         MemoryMapSize;
+  UINTN                         MapKey;
+  UINTN                         DescriptorSize;
+  UINT32                        DescriptorVersion;
+  EFI_MEMORY_DESCRIPTOR         *MemoryMap;
+  EFI_MEMORY_DESCRIPTOR         *MemoryMapEntry;
+  EFI_MEMORY_DESCRIPTOR         *MemoryMapEnd;
+  EFI_STATUS                    Status;
+
+  //
+  // Iterate over the memory map, and remove execute permissions from all
+  // memory regions that are not BootServiceCode or RuntimeServicesCode.
+  //
+
+  //
+  // Get the EFI memory map.
+  //
+  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 != EfiBootServicesCode) &&
+        (MemoryMapEntry->Type != EfiRuntimeServicesCode)) {
+
+      CpuSetMemoryAttributes (&mCpu, MemoryMapEntry->PhysicalStart,
+        EFI_PAGES_TO_SIZE(MemoryMapEntry->NumberOfPages), EFI_MEMORY_XP);
+      DEBUG((DEBUG_ERROR, "%a: removing exec permissions from 0x%lx - 0x%lx (Type == 0x%x)\n",
+        __FUNCTION__, MemoryMapEntry->PhysicalStart,
+        MemoryMapEntry->PhysicalStart + EFI_PAGES_TO_SIZE(MemoryMapEntry->NumberOfPages) - 1,
+        MemoryMapEntry->Type));
+    }
+    MemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry, DescriptorSize);
+  }
+  FreePool (MemoryMap);
+}
+
 EFI_STATUS
 CpuDxeInitialize (
   IN EFI_HANDLE         ImageHandle,
@@ -264,6 +335,11 @@ CpuDxeInitialize (
   //
   SyncCacheConfig (&mCpu);
 
+  //
+  // Remap all conventional memory as non-executable.
+  //
+  RemapAllDataRegionsNonExec ();
+
   // If the platform is a MPCore system then install the Configuration Table describing the
   // secondary core states
   if (ArmIsMpCore()) {
-- 
2.7.4

Re: [RFC PATCH 0/4] RFC: increased memory protection

[Yao, Jiewen]

HI Ard
Thanks to protect more. :-)
We did consider the idea to remove EXEC attribute for Data page before. But we got compatibility issue.

We documented some gaps in white paper -
https://github.com/tianocore-docs/Docs/raw/master/White_Papers/A_Tour_Beyond_BIOS_Memory_Map_And_Practices_in_UEFI_BIOS_V2.pdf

I am glad that some limitation is already resolved or we have solution to mitigate it. But there is still some other need consideration.


1)      We observe some 3rd part code allocating data page for code. - That is hardest part. (OS limitation #12)
We might also need consider ReservedMemory/AcpiNvs. There might be code there, too. (Firmware limitation #6 and #7).

If we want to apply the protection, we might need define a new PCD to disable the data protection for compatibility consideration.


2)      About DxeCore in data page. (Firmware limitation #4)
I am thinking if we can fix LoadFile implementation in PeiCore.

MdeModulePkg\Core\Pei\Image\Image.c:
LoadAndRelocatePeCoffImage()
{
ImageContext.ImageAddress = (EFI_PHYSICAL_ADDRESS)(UINTN) AllocatePages (EFI_SIZE_TO_PAGES ((UINT32) AlignImageSize));
}

AllocatePages means to allocate data page.
I think we should use PeiAllocatePages(EfiBootServicesCode, EFI_SIZE_TO_PAGES ((UINT32) AlignImageSize), &ImageContext.ImageAddress);

Does that fix the problem?



3)      I am not worried about EBC. That can be handled separately.


4)      I did not find patch 4/4 in my mail box. Maybe it is lost due to some unknown reason. Would you please send it again?


Thank you
Yao Jiewen


> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Thursday, February 23, 2017 2:25 AM
> To: edk2-devel@lists.01.org; afish@apple.com; leif.lindholm@linaro.org; Kinney,
> Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>;
> Yao, Jiewen <jiewen.yao@intel.com>
> Cc: lersek@redhat.com; Tian, Feng <feng.tian@intel.com>; Zeng, Star
> <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: [RFC PATCH 0/4] RFC: increased memory protection
>
> Hello all,
>
> This is a proof of concept implementation that removes all executable
> permissions from writable memory regions, which greatly enhances security.
> It is based on Jiewen's recent work, which is a step in the right direction,
> but still leaves most of memory exploitable due to the default R+W+X
> permissions.
>
> The idea is that the implementation of the CPU arch protocol goes over the
> memory map and removes exec permissions from all regions that are not already
> marked as 'code. This requires some preparatory work to ensure that the
> DxeCore
> itself is covered by a BootServicesCode region, not a BootServicesData region.
> Exec permissions are re-granted selectively, when the PE/COFF loader allocates
> the space for it. Combined with Jiewen's code/data split, this removes all
> RWX mapped regions.
>
> There is a caveat, though (and there are likely more of that kind): the EBC
> driver will need some work to ensure the thunk buffers have the noexec
> restriction lifted. This could be done in the EBC driver, but perhaps it is
> better to either
> a) modify the DXE core so it always removes noexec restrictions when allocating
>    code pages, or
> b) add AllocateExecPages/AllocateExecPool() functions to the
> MemoryAllocationLib
>    API
>
> Comments please!
>
> Ard Biesheuvel (4):
>   MdeModulePkg/DxeCore: allow BootServicesData->BootServicesCode
>     conversion
>   MdeModulePkg/DxeCore: convert the DxeCore memory region to
>     BootServicesCode
>   MdeModulePkg/DxeCore: lift non-exec permissions on loaded images
>   ArmPkg/CpuDxe: remap all data regions non-executable
>
>  ArmPkg/Drivers/CpuDxe/CpuDxe.c          | 76 ++++++++++++++++++++
>  MdeModulePkg/Core/Dxe/DxeMain.h         |  8 +++
>  MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c |  2 +
>  MdeModulePkg/Core/Dxe/Image/Image.c     |  8 +++
>  MdeModulePkg/Core/Dxe/Mem/Page.c        | 18 ++++-
>  5 files changed, 111 insertions(+), 1 deletion(-)
>
> --
> 2.7.4

Re: [RFC PATCH 0/4] RFC: increased memory protection

[Ard Biesheuvel]

On 23 February 2017 at 08:52, Yao, Jiewen <jiewen.yao@intel.com> wrote:
> HI Ard
>
> Thanks to protect more. :-)
>

Of course! This is a very important topic for me.

> We did consider the idea to remove EXEC attribute for Data page before. But
> we got compatibility issue.
>
>
>
> We documented some gaps in white paper -
>
> https://github.com/tianocore-docs/Docs/raw/master/White_Papers/A_Tour_Beyond_BIOS_Memory_Map_And_Practices_in_UEFI_BIOS_V2.pdf
>

Thanks for the link.

> I am glad that some limitation is already resolved or we have solution to
> mitigate it. But there is still some other need consideration.
>
>
>
> 1)      We observe some 3rd part code allocating data page for code. – That
> is hardest part. (OS limitation #12)
>
> We might also need consider ReservedMemory/AcpiNvs. There might be code
> there, too. (Firmware limitation #6 and #7).
>

OK

> If we want to apply the protection, we might need define a new PCD to
> disable the data protection for compatibility consideration.
>
>
>
> 2)      About DxeCore in data page. (Firmware limitation #4)
>
> I am thinking if we can fix LoadFile implementation in PeiCore.
>
>
>
> MdeModulePkg\Core\Pei\Image\Image.c:
>
> LoadAndRelocatePeCoffImage()
>
> {
>
> ImageContext.ImageAddress = (EFI_PHYSICAL_ADDRESS)(UINTN) AllocatePages
> (EFI_SIZE_TO_PAGES ((UINT32) AlignImageSize));
>
> }
>
>
>
> AllocatePages means to allocate data page.
>
> I think we should use PeiAllocatePages(EfiBootServicesCode,
> EFI_SIZE_TO_PAGES ((UINT32) AlignImageSize), &ImageContext.ImageAddress);
>
>
>
> Does that fix the problem?
>

Using PeiServicesAllocatePage() in the way that you describe does
indeed remove the problem, so I will use that instead.

> 3)      I am not worried about EBC. That can be handled separately.
>

OK


> 4)      I did not find patch 4/4 in my mail box. Maybe it is lost due to
> some unknown reason. Would you please send it again?
>

https://lists.01.org/pipermail/edk2-devel/2017-February/007685.html

I will send out a v2 shortly which, as you suggest, moves the handling
to DXE core. The only problem is that ARM's SyncCacheConfig() removes
the noexec attributes again, so I need to fix that first. Then, the
arch CPU protocol installation event can iterate over the memory map
to set the permissions according to a policy PCD

Thanks,
Ard.

Re: [RFC PATCH 0/4] RFC: increased memory protection

[Yao, Jiewen]

Sounds great.
I look forward to your V2.

Thank you
Yao Jiewen

From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
Sent: Thursday, February 23, 2017 7:39 PM
To: Yao, Jiewen <jiewen.yao@intel.com>
Cc: edk2-devel@lists.01.org; afish@apple.com; leif.lindholm@linaro.org; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>; lersek@redhat.com; Tian, Feng <feng.tian@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: Re: [RFC PATCH 0/4] RFC: increased memory protection

On 23 February 2017 at 08:52, Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> wrote:
> HI Ard
>
> Thanks to protect more. :-)
>

Of course! This is a very important topic for me.

> We did consider the idea to remove EXEC attribute for Data page before. But
> we got compatibility issue.
>
>
>
> We documented some gaps in white paper -
>
> https://github.com/tianocore-docs/Docs/raw/master/White_Papers/A_Tour_Beyond_BIOS_Memory_Map_And_Practices_in_UEFI_BIOS_V2.pdf
>

Thanks for the link.

> I am glad that some limitation is already resolved or we have solution to
> mitigate it. But there is still some other need consideration.
>
>
>
> 1)      We observe some 3rd part code allocating data page for code. – That
> is hardest part. (OS limitation #12)
>
> We might also need consider ReservedMemory/AcpiNvs. There might be code
> there, too. (Firmware limitation #6 and #7).
>

OK

> If we want to apply the protection, we might need define a new PCD to
> disable the data protection for compatibility consideration.
>
>
>
> 2)      About DxeCore in data page. (Firmware limitation #4)
>
> I am thinking if we can fix LoadFile implementation in PeiCore.
>
>
>
> MdeModulePkg\Core\Pei\Image\Image.c:
>
> LoadAndRelocatePeCoffImage()
>
> {
>
> ImageContext.ImageAddress = (EFI_PHYSICAL_ADDRESS)(UINTN) AllocatePages
> (EFI_SIZE_TO_PAGES ((UINT32) AlignImageSize));
>
> }
>
>
>
> AllocatePages means to allocate data page.
>
> I think we should use PeiAllocatePages(EfiBootServicesCode,
> EFI_SIZE_TO_PAGES ((UINT32) AlignImageSize), &ImageContext.ImageAddress);
>
>
>
> Does that fix the problem?
>

Using PeiServicesAllocatePage() in the way that you describe does
indeed remove the problem, so I will use that instead.

> 3)      I am not worried about EBC. That can be handled separately.
>

OK


> 4)      I did not find patch 4/4 in my mail box. Maybe it is lost due to
> some unknown reason. Would you please send it again?
>

https://lists.01.org/pipermail/edk2-devel/2017-February/007685.html

I will send out a v2 shortly which, as you suggest, moves the handling
to DXE core. The only problem is that ARM's SyncCacheConfig() removes
the noexec attributes again, so I need to fix that first. Then, the
arch CPU protocol installation event can iterate over the memory map
to set the permissions according to a policy PCD

Thanks,
Ard.

Re: [RFC PATCH 0/4] RFC: increased memory protection

[Ard Biesheuvel]

On 23 February 2017 at 11:45, Yao, Jiewen <jiewen.yao@intel.com> wrote:
> Sounds great.
>
> I look forward to your V2.
>

Hello Jiewen,

What I am currently struggling with is the fact that we don't use the
GCD RO/XP permissions at all. This means that
RefreshGcdMemoryAttributes () (or SyncCacheConfig() on ARM) will
remove non-exec attributes if we add them in the CPU arch protocol
installation notifier callback.

So there are two approaches imo:
- introduce a way to call into the DXE core to mark all non-code
regions non-exec after RefreshGcdMemoryAttributes () has been called,
or
- add the RO/XP attributes to the GCD memory space map, and enable
them in the attributes.

Option #2 will require a change to CoreAddRange to prevent those RO/XP
attributes to leak into the UEFI memory map, because that results in
all regions have to RO/XP attributes set by default, which is
obviously not what we want.

Any thoughts?

Thanks,
Ard.

Re: [RFC PATCH 0/4] RFC: increased memory protection

[Yao, Jiewen]

HI Ard
In X86 CPU driver - UefiCpuPkg\CpuDxe, we use a global variable – mIsFlushingGCD.

In RefreshGcdMemoryAttributes(), we set mIsFlushingGCD=TRUE.
In CpuSetMemoryAttributes(), we check mIsFlushingGCD. If mIsFlushingGCD is TRUE, CpuSetMemoryAttributes() returns immediately without touching cache attribute or memory attribute.

The reason is that RefreshGcdMemoryAttributes() just sync current CPU hardware setting to GCD software record.
No real need to set cache again.

Previous we purposely skip GCD setting on RO/XP, the reason is still compatibility concern.
We do not want to provide a different memory map to 3rd part code, just in case there is hidden assumption on memory map attributes.


Maybe ARM can use similar way in SyncCacheConfig() and do a simple check in CpuSetMemoryAttributes().

Thank you
Yao Jiewen

From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
Sent: Friday, February 24, 2017 3:33 AM
To: Yao, Jiewen <jiewen.yao@intel.com>
Cc: edk2-devel@lists.01.org; afish@apple.com; leif.lindholm@linaro.org; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>; lersek@redhat.com; Tian, Feng <feng.tian@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: Re: [RFC PATCH 0/4] RFC: increased memory protection

On 23 February 2017 at 11:45, Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> wrote:
> Sounds great.
>
> I look forward to your V2.
>

Hello Jiewen,

What I am currently struggling with is the fact that we don't use the
GCD RO/XP permissions at all. This means that
RefreshGcdMemoryAttributes () (or SyncCacheConfig() on ARM) will
remove non-exec attributes if we add them in the CPU arch protocol
installation notifier callback.

So there are two approaches imo:
- introduce a way to call into the DXE core to mark all non-code
regions non-exec after RefreshGcdMemoryAttributes () has been called,
or
- add the RO/XP attributes to the GCD memory space map, and enable
them in the attributes.

Option #2 will require a change to CoreAddRange to prevent those RO/XP
attributes to leak into the UEFI memory map, because that results in
all regions have to RO/XP attributes set by default, which is
obviously not what we want.

Any thoughts?

Thanks,
Ard.

Re: [RFC PATCH 0/4] RFC: increased memory protection

[Laszlo Ersek]

Hi,

On 02/22/17 19:24, Ard Biesheuvel wrote:
> Hello all,
> 
> This is a proof of concept implementation that removes all executable
> permissions from writable memory regions, which greatly enhances security.
> It is based on Jiewen's recent work, which is a step in the right direction,
> but still leaves most of memory exploitable due to the default R+W+X
> permissions.
> 
> The idea is that the implementation of the CPU arch protocol goes over the
> memory map and removes exec permissions from all regions that are not already
> marked as 'code. This requires some preparatory work to ensure that the DxeCore
> itself is covered by a BootServicesCode region, not a BootServicesData region.
> Exec permissions are re-granted selectively, when the PE/COFF loader allocates
> the space for it. Combined with Jiewen's code/data split, this removes all
> RWX mapped regions.
> 
> There is a caveat, though (and there are likely more of that kind): the EBC
> driver will need some work to ensure the thunk buffers have the noexec
> restriction lifted. This could be done in the EBC driver, but perhaps it is
> better to either
> a) modify the DXE core so it always removes noexec restrictions when allocating
>    code pages, or
> b) add AllocateExecPages/AllocateExecPool() functions to the MemoryAllocationLib
>    API
> 
> Comments please!

My only comment is "good idea, as long as it doesn't regress anything". :)

I haven't done any testing with this; once you think the series is
mature enough for that, I could run my usual tests. Feel free to ping me
for that whenever.

Thanks!
Laszlo

> Ard Biesheuvel (4):
>   MdeModulePkg/DxeCore: allow BootServicesData->BootServicesCode
>     conversion
>   MdeModulePkg/DxeCore: convert the DxeCore memory region to
>     BootServicesCode
>   MdeModulePkg/DxeCore: lift non-exec permissions on loaded images
>   ArmPkg/CpuDxe: remap all data regions non-executable
> 
>  ArmPkg/Drivers/CpuDxe/CpuDxe.c          | 76 ++++++++++++++++++++
>  MdeModulePkg/Core/Dxe/DxeMain.h         |  8 +++
>  MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c |  2 +
>  MdeModulePkg/Core/Dxe/Image/Image.c     |  8 +++
>  MdeModulePkg/Core/Dxe/Mem/Page.c        | 18 ++++-
>  5 files changed, 111 insertions(+), 1 deletion(-)
>