* [PATCH v2 1/5] ArmPkg/CpuDxe: ignore attribute changes during SyncCacheConfig()
2017-02-24 15:04 [PATCH v2 0/5] RFC: increased memory protection Ard Biesheuvel
@ 2017-02-24 15:04 ` Ard Biesheuvel
2017-02-24 15:04 ` [PATCH v2 2/5] MdeModulePkg/PeiCore: allocate BootServicesCode memory for PE/COFF images Ard Biesheuvel
` (4 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Ard Biesheuvel @ 2017-02-24 15:04 UTC (permalink / raw)
To: edk2-devel, afish, leif.lindholm, michael.d.kinney, liming.gao,
jiewen.yao
Cc: lersek, feng.tian, star.zeng, Ard Biesheuvel
To prevent the initial MMU->GCD memory space map synchronization from
stripping permissions attributes [which we cannot use in the GCD memory
space map, unfortunately], implement the same approach as x86, and ignore
SetMemoryAttributes() calls during the time SyncCacheConfig() is in
progress. This is a horrible hack, but is currently the only way we can
implement strict permissions on arbitrary memory regions [as opposed to
PE/COFF text/data sections only]
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
ArmPkg/Drivers/CpuDxe/CpuDxe.c | 3 +++
ArmPkg/Drivers/CpuDxe/CpuDxe.h | 1 +
ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c | 4 ++++
3 files changed, 8 insertions(+)
diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.c b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
index 5aa5b874144a..1955d1dece03 100644
--- a/ArmPkg/Drivers/CpuDxe/CpuDxe.c
+++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
@@ -17,6 +17,7 @@
#include <Guid/IdleLoopEvent.h>
+BOOLEAN gIsFlushingGCD;
/**
This function flushes the range of addresses from Start to Start+Length
@@ -261,7 +262,9 @@ CpuDxeInitialize (
// and that calls EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes, so this code needs to go
// after the protocol is installed
//
+ gIsFlushingGCD = TRUE;
SyncCacheConfig (&mCpu);
+ gIsFlushingGCD = FALSE;
// If the platform is a MPCore system then install the Configuration Table describing the
// secondary core states
diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.h b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
index a00fc3064362..085e4cab2921 100644
--- a/ArmPkg/Drivers/CpuDxe/CpuDxe.h
+++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
@@ -37,6 +37,7 @@
#include <Protocol/DebugSupportPeriodicCallback.h>
#include <Protocol/LoadedImage.h>
+extern BOOLEAN gIsFlushingGCD;
/**
This function registers and enables the handler specified by InterruptHandler for a processor
diff --git a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
index ebe593d1c325..6dfec7e55888 100644
--- a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
+++ b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
@@ -188,6 +188,10 @@ CpuSetMemoryAttributes (
UINTN RegionLength;
UINTN RegionArmAttributes;
+ if (gIsFlushingGCD) {
+ return EFI_SUCCESS;
+ }
+
if ((BaseAddress & (SIZE_4KB - 1)) != 0) {
// Minimum granularity is SIZE_4KB (4KB on ARM)
DEBUG ((EFI_D_PAGE, "CpuSetMemoryAttributes(%lx, %lx, %lx): Minimum ganularity is SIZE_4KB\n", BaseAddress, Length, EfiAttributes));
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH v2 2/5] MdeModulePkg/PeiCore: allocate BootServicesCode memory for PE/COFF images
2017-02-24 15:04 [PATCH v2 0/5] RFC: increased memory protection Ard Biesheuvel
2017-02-24 15:04 ` [PATCH v2 1/5] ArmPkg/CpuDxe: ignore attribute changes during SyncCacheConfig() Ard Biesheuvel
@ 2017-02-24 15:04 ` Ard Biesheuvel
2017-02-24 15:04 ` [PATCH v2 3/5] MdeModulePkg/DxeCore: pass pool type to CoreFreePoolPages () Ard Biesheuvel
` (3 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Ard Biesheuvel @ 2017-02-24 15:04 UTC (permalink / raw)
To: edk2-devel, afish, leif.lindholm, michael.d.kinney, liming.gao,
jiewen.yao
Cc: lersek, feng.tian, star.zeng, Ard Biesheuvel
Ensure that any memory allocated for PE/COFF images is identifiable as
a boot services code region, so that we know it requires its executable
permissions to be preserved when we tighten mapping permissions later on.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
MdeModulePkg/Core/Pei/Image/Image.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/MdeModulePkg/Core/Pei/Image/Image.c b/MdeModulePkg/Core/Pei/Image/Image.c
index d659de8b3e64..8cc9ed93e9b6 100644
--- a/MdeModulePkg/Core/Pei/Image/Image.c
+++ b/MdeModulePkg/Core/Pei/Image/Image.c
@@ -453,12 +453,16 @@ LoadAndRelocatePeCoffImage (
//
// The PEIM is not assiged valid address, try to allocate page to load it.
//
- ImageContext.ImageAddress = (EFI_PHYSICAL_ADDRESS)(UINTN) AllocatePages (EFI_SIZE_TO_PAGES ((UINT32) AlignImageSize));
+ Status = PeiServicesAllocatePages (EfiBootServicesCode,
+ EFI_SIZE_TO_PAGES ((UINT32) AlignImageSize),
+ &ImageContext.ImageAddress);
}
} else {
- ImageContext.ImageAddress = (EFI_PHYSICAL_ADDRESS)(UINTN) AllocatePages (EFI_SIZE_TO_PAGES ((UINT32) AlignImageSize));
+ Status = PeiServicesAllocatePages (EfiBootServicesCode,
+ EFI_SIZE_TO_PAGES ((UINT32) AlignImageSize),
+ &ImageContext.ImageAddress);
}
- if (ImageContext.ImageAddress != 0) {
+ if (!EFI_ERROR (Status)) {
//
// Adjust the Image Address to make sure it is section alignment.
//
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH v2 3/5] MdeModulePkg/DxeCore: pass pool type to CoreFreePoolPages ()
2017-02-24 15:04 [PATCH v2 0/5] RFC: increased memory protection Ard Biesheuvel
2017-02-24 15:04 ` [PATCH v2 1/5] ArmPkg/CpuDxe: ignore attribute changes during SyncCacheConfig() Ard Biesheuvel
2017-02-24 15:04 ` [PATCH v2 2/5] MdeModulePkg/PeiCore: allocate BootServicesCode memory for PE/COFF images Ard Biesheuvel
@ 2017-02-24 15:04 ` Ard Biesheuvel
2017-02-24 15:04 ` [PATCH v2 4/5] MdeModulePkg: define PCD for DXE memory protection policy Ard Biesheuvel
` (2 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Ard Biesheuvel @ 2017-02-24 15:04 UTC (permalink / raw)
To: edk2-devel, afish, leif.lindholm, michael.d.kinney, liming.gao,
jiewen.yao
Cc: lersek, feng.tian, star.zeng, Ard Biesheuvel
In order to make it easier to manage memory permission attributes of
page allocations performed on behalf of the pool allocator, pass the
pool type when freeing pages. This way, we can easily check whether
the freed pages need to have their permission attributes changed.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
MdeModulePkg/Core/Dxe/Mem/Imem.h | 2 ++
MdeModulePkg/Core/Dxe/Mem/Page.c | 1 +
MdeModulePkg/Core/Dxe/Mem/Pool.c | 5 +++--
3 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/MdeModulePkg/Core/Dxe/Mem/Imem.h b/MdeModulePkg/Core/Dxe/Mem/Imem.h
index fb53f95575f0..fde533100d37 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Imem.h
+++ b/MdeModulePkg/Core/Dxe/Mem/Imem.h
@@ -77,12 +77,14 @@ CoreAllocatePoolPages (
/**
Internal function. Frees pool pages allocated via AllocatePoolPages ()
+ @param PoolType The memory type of the pool pages
@param Memory The base address to free
@param NumberOfPages The number of pages to free
**/
VOID
CoreFreePoolPages (
+ IN EFI_MEMORY_TYPE PoolType,
IN EFI_PHYSICAL_ADDRESS Memory,
IN UINTN NumberOfPages
);
diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
index bda4f6397e91..6330d41e7b3b 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Page.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
@@ -1871,6 +1871,7 @@ CoreAllocatePoolPages (
**/
VOID
CoreFreePoolPages (
+ IN EFI_MEMORY_TYPE PoolType,
IN EFI_PHYSICAL_ADDRESS Memory,
IN UINTN NumberOfPages
)
diff --git a/MdeModulePkg/Core/Dxe/Mem/Pool.c b/MdeModulePkg/Core/Dxe/Mem/Pool.c
index 7afd2d312c1d..9e15e0cfb5b2 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Pool.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Pool.c
@@ -624,7 +624,7 @@ CoreFreePoolI (
//
NoPages = EFI_SIZE_TO_PAGES(Size) + EFI_SIZE_TO_PAGES (Granularity) - 1;
NoPages &= ~(UINTN)(EFI_SIZE_TO_PAGES (Granularity) - 1);
- CoreFreePoolPages ((EFI_PHYSICAL_ADDRESS) (UINTN) Head, NoPages);
+ CoreFreePoolPages (Pool->MemoryType, (EFI_PHYSICAL_ADDRESS) (UINTN) Head, NoPages);
} else {
@@ -680,7 +680,8 @@ CoreFreePoolI (
//
// Free the page
//
- CoreFreePoolPages ((EFI_PHYSICAL_ADDRESS) (UINTN)NewPage, EFI_SIZE_TO_PAGES (Granularity));
+ CoreFreePoolPages (Pool->MemoryType, (EFI_PHYSICAL_ADDRESS) (UINTN)NewPage,
+ EFI_SIZE_TO_PAGES (Granularity));
}
}
}
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH v2 4/5] MdeModulePkg: define PCD for DXE memory protection policy
2017-02-24 15:04 [PATCH v2 0/5] RFC: increased memory protection Ard Biesheuvel
` (2 preceding siblings ...)
2017-02-24 15:04 ` [PATCH v2 3/5] MdeModulePkg/DxeCore: pass pool type to CoreFreePoolPages () Ard Biesheuvel
@ 2017-02-24 15:04 ` Ard Biesheuvel
2017-02-24 15:04 ` [PATCH v2 5/5] MdeModulePkg/DxeCore: implement " Ard Biesheuvel
2017-02-25 4:04 ` [PATCH v2 0/5] RFC: increased memory protection Yao, Jiewen
5 siblings, 0 replies; 9+ messages in thread
From: Ard Biesheuvel @ 2017-02-24 15:04 UTC (permalink / raw)
To: edk2-devel, afish, leif.lindholm, michael.d.kinney, liming.gao,
jiewen.yao
Cc: lersek, feng.tian, star.zeng, Ard Biesheuvel
Define a new fixed/patchable PCD that sets the DXE memory protection
policy: its primary use is to define which memory types should have
their executable permissions removed. Combined with the image protection
policy, this can be used to implement a strict W^X policy, i.e.. a policy
where no regions exist that are both executable and writable at the same
time.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
MdeModulePkg/MdeModulePkg.dec | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index 426634fbbd4d..ea64cdf3772d 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -1107,6 +1107,22 @@ [PcdsFixedAtBuild, PcdsPatchableInModule]
# @ValidRange 0x80000002 | 0x00000000 - 0x0000001F
gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy|0x00000002|UINT32|0x00001047
+ ## Set DXE memory protection policy. The policy is bitwise.
+ # If a bit is set, memory regions of the associated type will be mapped
+ # non-executable.<BR><BR>
+ # BIT0 - EfiLoaderData. <BR>
+ # - EfiBootServicesData. <BR>
+ # - EfiRuntimeServicesData. <BR>
+ # - EfiConventionalMemory. <BR>
+ # - EfiUnusableMemory. <BR>
+ # - EfiPersistentMemory. <BR>
+ # - EfiACPIReclaimMemory. <BR>
+ # BIT1 - EfiReservedMemoryType. <BR>
+ # BIT2 - EfiACPIMemoryNVS. <BR>
+ # @Prompt Set DXE memory protection policy.
+ # @ValidRange 0x80000002 | 0x00000000 - 0x00000007
+ gEfiMdeModulePkgTokenSpaceGuid.PcdDxeMemoryProtectionPolicy|0x0000007|UINT32|0x00001048
+
## PCI Serial Device Info. It is an array of Device, Function, and Power Management
# information that describes the path that contains zero or more PCI to PCI briges
# followed by a PCI serial device. Each array entry is 4-bytes in length. The
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH v2 5/5] MdeModulePkg/DxeCore: implement memory protection policy
2017-02-24 15:04 [PATCH v2 0/5] RFC: increased memory protection Ard Biesheuvel
` (3 preceding siblings ...)
2017-02-24 15:04 ` [PATCH v2 4/5] MdeModulePkg: define PCD for DXE memory protection policy Ard Biesheuvel
@ 2017-02-24 15:04 ` Ard Biesheuvel
2017-02-25 4:04 ` [PATCH v2 0/5] RFC: increased memory protection Yao, Jiewen
5 siblings, 0 replies; 9+ messages in thread
From: Ard Biesheuvel @ 2017-02-24 15:04 UTC (permalink / raw)
To: edk2-devel, afish, leif.lindholm, michael.d.kinney, liming.gao,
jiewen.yao
Cc: lersek, feng.tian, star.zeng, Ard Biesheuvel
This implements a DXE memory protection policy that ensure that regions
that don't require executable permissions are mapped with the non-exec
attribute set.
First of all, it iterates over all entries in the UEFI memory map, and
removes executable permissions according to the configured DXE memory
protection policy, as recorded in PcdDxeMemoryProtectionPolicy.
Secondly, it sets or clears the non-executable attribute when allocating
or freeing pages, both for page based or pool based allocations.
Note that this complements the image protection facility, which applies
strict permissions to BootServicesCode/RuntimeServicesCode regions when
the section alignment allows it. The memory protection configured by this
patch operates on non-code regions only.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
MdeModulePkg/Core/Dxe/DxeMain.inf | 1 +
MdeModulePkg/Core/Dxe/Mem/Page.c | 105 ++++++++++++++++++++
MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 104 ++++++++++++++++++-
3 files changed, 209 insertions(+), 1 deletion(-)
diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf b/MdeModulePkg/Core/Dxe/DxeMain.inf
index 371e91cb0d7e..871868dbf305 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain.inf
+++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
@@ -191,6 +191,7 @@ [Pcd]
gEfiMdeModulePkgTokenSpaceGuid.PcdMemoryProfileDriverPath ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy ## CONSUMES
+ gEfiMdeModulePkgTokenSpaceGuid.PcdDxeMemoryProtectionPolicy ## CONSUMES
# [Hob]
# RESOURCE_DESCRIPTOR ## CONSUMES
diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
index 6330d41e7b3b..569238c77f98 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Page.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
@@ -1305,6 +1305,101 @@ Done:
}
/**
+ Return the EFI memory permission attribute associated with memory type 'Type'
+ under the configured DXE memory protection policy.
+**/
+STATIC
+UINT64
+EFIAPI
+GetPermissionAttributeForMemoryType (
+ IN EFI_MEMORY_TYPE Type
+ )
+{
+ switch (Type) {
+ case EfiBootServicesCode:
+ case EfiRuntimeServicesCode:
+ case EfiLoaderCode:
+ break;
+
+ default:
+ if ((PcdGet32 (PcdDxeMemoryProtectionPolicy) & BIT0) != 0) {
+ return EFI_MEMORY_XP;
+ }
+ break;
+ case EfiReservedMemoryType:
+ if ((PcdGet32 (PcdDxeMemoryProtectionPolicy) & BIT1) != 0) {
+ return EFI_MEMORY_XP;
+ }
+ break;
+ case EfiACPIMemoryNVS:
+ if ((PcdGet32 (PcdDxeMemoryProtectionPolicy) & BIT2) != 0) {
+ return EFI_MEMORY_XP;
+ }
+ break;
+ }
+ return 0;
+}
+
+/**
+ Manage memory permission attributes on a memory range, according to the
+ configured DXE memory protection policy.
+
+ @param OldType The old memory type of the range
+ @param NewType The new memory type of the range
+ @param Memory The base address of the range
+ @param Length The size of the range (in bytes)
+
+ @return EFI_SUCCESS If the the CPU arch protocol is not installed yet
+ @return EFI_SUCCESS If no DXE memory protection policy has been configured
+ @return EFI_SUCCESS If OldType and NewType use the same permission attributes
+ @return other Return value of gCpu->SetMemoryAttributes()
+
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+ApplyMemoryProtection (
+ IN EFI_MEMORY_TYPE OldType,
+ IN EFI_MEMORY_TYPE NewType,
+ IN EFI_PHYSICAL_ADDRESS Memory,
+ IN UINT64 Length
+ )
+{
+ UINT64 OldAttributes;
+ UINT64 NewAttributes;
+
+ //
+ // If the CPU arch protocol is not installed yet, we cannot manage memory
+ // permission attributes, and it is the job of the driver that installs this
+ // protocol to set the permissions on existing allocations.
+ //
+ if (gCpu == NULL) {
+ return EFI_SUCCESS;
+ }
+
+ //
+ // Check if a DXE memory protection policy has been configured
+ //
+ if (PcdGet32 (PcdDxeMemoryProtectionPolicy) == 0) {
+ return EFI_SUCCESS;
+ }
+
+ //
+ // Update the executable permissions according to the DXE memory
+ // protection policy, but only if the policy is different between
+ // the old and the new type.
+ //
+ OldAttributes = GetPermissionAttributeForMemoryType (OldType);
+ NewAttributes = GetPermissionAttributeForMemoryType (NewType);
+
+ if (OldAttributes == NewAttributes) {
+ return EFI_SUCCESS;
+ }
+
+ return gCpu->SetMemoryAttributes (gCpu, Memory, Length, NewAttributes);
+}
+
+/**
Allocates pages from the memory map.
@param Type The type of allocation to perform
@@ -1344,6 +1439,8 @@ CoreAllocatePages (
NULL
);
InstallMemoryAttributesTableOnMemoryAllocation (MemoryType);
+ ApplyMemoryProtection (EfiConventionalMemory, MemoryType, *Memory,
+ EFI_PAGES_TO_SIZE (NumberOfPages));
}
return Status;
}
@@ -1460,6 +1557,8 @@ CoreFreePages (
NULL
);
InstallMemoryAttributesTableOnMemoryAllocation (MemoryType);
+ ApplyMemoryProtection (MemoryType, EfiConventionalMemory, Memory,
+ EFI_PAGES_TO_SIZE (NumberOfPages));
}
return Status;
}
@@ -1856,6 +1955,9 @@ CoreAllocatePoolPages (
DEBUG ((DEBUG_ERROR | DEBUG_PAGE, "AllocatePoolPages: failed to allocate %d pages\n", (UINT32)NumberOfPages));
} else {
CoreConvertPages (Start, NumberOfPages, PoolType);
+
+ ApplyMemoryProtection (EfiConventionalMemory, PoolType, Start,
+ EFI_PAGES_TO_SIZE (NumberOfPages));
}
return (VOID *)(UINTN) Start;
@@ -1877,6 +1979,9 @@ CoreFreePoolPages (
)
{
CoreConvertPages (Memory, NumberOfPages, EfiConventionalMemory);
+
+ ApplyMemoryProtection (PoolType, EfiConventionalMemory, Memory,
+ EFI_PAGES_TO_SIZE (NumberOfPages));
}
diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
index c36612a1b1f2..1142dcc5a83d 100644
--- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
+++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
@@ -639,6 +639,97 @@ UnprotectUefiImage (
}
/**
+ Remove exec permissions from all regions whose type is identified by
+ PcdDxeMemoryProtectionPolicy
+**/
+STATIC
+VOID
+ApplyDxeMemoryProtectionPolicy (
+ VOID
+ )
+{
+ UINTN MemoryMapSize;
+ UINTN MapKey;
+ UINTN DescriptorSize;
+ UINT32 DescriptorVersion;
+ EFI_MEMORY_DESCRIPTOR *MemoryMap;
+ EFI_MEMORY_DESCRIPTOR *MemoryMapEntry;
+ EFI_MEMORY_DESCRIPTOR *MemoryMapEnd;
+ EFI_STATUS Status;
+
+ //
+ // 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);
+
+ DEBUG((DEBUG_ERROR, "%a: removing exec permissions from memory regions\n",
+ __FUNCTION__));
+
+ for (MemoryMapEntry = MemoryMap,
+ MemoryMapEnd = (EFI_MEMORY_DESCRIPTOR *) ((UINT8 *) MemoryMap + MemoryMapSize);
+ (UINTN) MemoryMapEntry < (UINTN) MemoryMapEnd;
+ MemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry, DescriptorSize)) {
+
+ switch (MemoryMapEntry->Type) {
+ case EfiBootServicesCode:
+ case EfiRuntimeServicesCode:
+ case EfiLoaderCode:
+ continue;
+
+ default:
+ if ((PcdGet32 (PcdDxeMemoryProtectionPolicy) & BIT0) == 0) {
+ continue;
+ }
+ break;
+
+ case EfiReservedMemoryType:
+ if ((PcdGet32 (PcdDxeMemoryProtectionPolicy) & BIT1) == 0) {
+ continue;
+ }
+ break;
+
+ case EfiACPIMemoryNVS:
+ if ((PcdGet32 (PcdDxeMemoryProtectionPolicy) & BIT2) == 0) {
+ continue;
+ }
+ break;
+ }
+
+ SetUefiImageMemoryAttributes (
+ MemoryMapEntry->PhysicalStart,
+ EFI_PAGES_TO_SIZE (MemoryMapEntry->NumberOfPages),
+ EFI_MEMORY_XP);
+ }
+ FreePool (MemoryMap);
+}
+
+
+/**
A notification for CPU_ARCH protocol.
@param[in] Event Event whose notification function is being invoked.
@@ -666,6 +757,17 @@ MemoryProtectionCpuArchProtocolNotify (
return;
}
+ //
+ // Apply the memory protection policy on non-BScode/RTcode regions.
+ //
+ if (PcdGet32 (PcdDxeMemoryProtectionPolicy) != 0) {
+ ApplyDxeMemoryProtectionPolicy ();
+ }
+
+ if (mImageProtectionPolicy == 0) {
+ return;
+ }
+
Status = gBS->LocateHandleBuffer (
ByProtocol,
&gEfiLoadedImageProtocolGuid,
@@ -745,7 +847,7 @@ CoreInitializeMemoryProtection (
mImageProtectionPolicy = PcdGet32(PcdImageProtectionPolicy);
- if (mImageProtectionPolicy != 0) {
+ if (mImageProtectionPolicy != 0 || PcdGet32 (PcdDxeMemoryProtectionPolicy) != 0) {
Status = CoreCreateEvent (
EVT_NOTIFY_SIGNAL,
TPL_CALLBACK,
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v2 0/5] RFC: increased memory protection
2017-02-24 15:04 [PATCH v2 0/5] RFC: increased memory protection Ard Biesheuvel
` (4 preceding siblings ...)
2017-02-24 15:04 ` [PATCH v2 5/5] MdeModulePkg/DxeCore: implement " Ard Biesheuvel
@ 2017-02-25 4:04 ` Yao, Jiewen
2017-02-26 15:09 ` Ard Biesheuvel
5 siblings, 1 reply; 9+ messages in thread
From: Yao, Jiewen @ 2017-02-25 4:04 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel@lists.01.org, afish@apple.com,
leif.lindholm@linaro.org, Kinney, Michael D, Gao, Liming
Cc: Tian, Feng, lersek@redhat.com, Zeng, Star, Yao, Jiewen
Thank you Ard. I like this patch - simple and obvious.
I put all my comment together for your consideration.
1) Patch V2 1/5 -- reviewed-by: Jiewen.yao@Intel.com
2) Patch V2 2/5 - reviewed-by: Jiewen.yao@intel.com
3) Patch V2 3/5 - reviewed-by: Jiewen.yao@intel.com
4) Patch V2 4/5 -
4.1) Can we follow the style of other memory type definition? (Such as PcdMemoryProfileMemoryType)
The reason is that people may want to have fine granularity control for loader data or persistent memory.
My proposal is below:
//////////////////////////
## Set DXE memory protection policy. The policy is bitwise.
# If a bit is set, memory regions of the associated type will be mapped
# non-executable.<BR><BR>
#
# Below is bit mask for this PCD: (Order is same as UEFI spec)<BR>
# EfiReservedMemoryType 0x0001<BR>
# EfiLoaderCode 0x0002<BR>
# EfiLoaderData 0x0004<BR>
# EfiBootServicesCode 0x0008<BR>
# EfiBootServicesData 0x0010<BR>
# EfiRuntimeServicesCode 0x0020<BR>
# EfiRuntimeServicesData 0x0040<BR>
# EfiConventionalMemory 0x0080<BR>
# EfiUnusableMemory 0x0100<BR>
# EfiACPIReclaimMemory 0x0200<BR>
# EfiACPIMemoryNVS 0x0400<BR>
# EfiMemoryMappedIO 0x0800<BR>
# EfiMemoryMappedIOPortSpace 0x1000<BR>
# EfiPalCode 0x2000<BR>
# EfiPersistentMemory 0x4000<BR>
# OEM Reserved 0x4000000000000000<BR>
# OS Reserved 0x8000000000000000<BR>
#
# NOTE: User must NOT set NX protection for EfiLoaderCode / EfiBootServicesCode / EfiRuntimeServicesCode. <BR>
#
# e.g. 0x7FD5 can be used for all memory except Code. <BR>
# e.g. 0x7BD4 can be used for all memory except Code and ACPINVS/Reserved. <BR>
#
# @Prompt Set DXE memory protection policy.
gEfiMdeModulePkgTokenSpaceGuid.PcdDxeMemoryProtectionPolicy|0x0000000|UINT64|0x00001048
//////////////////////////
Then the C-code can be like below:
//////////////////////////
UINT64
GetPermissionAttributeForMemoryType (
IN EFI_MEMORY_TYPE MemoryType
)
{
UINT64 TestBit;
if ((UINT32) MemoryType >= MEMORY_TYPE_OS_RESERVED_MIN) {
TestBit = BIT63;
} else if ((UINT32) MemoryType >= MEMORY_TYPE_OEM_RESERVED_MIN) {
TestBit = BIT62;
} else {
TestBit = LShiftU64 (1, MemoryType);
}
if ((PcdGet64 (PcdMemoryProfileMemoryType) & TestBit) != 0) {
return EFI_MEMORY_XP;
} else {
return 0;
}
}
//////////////////////////
4.2) I prefer to setting default value to be 0x0 - to keep the compatibility, at least for X86 platform. (I have no strong opinion for ARM.)
4.3) I feel we might use a better name - PcdDxeNxMemoryProtectionPolicy (add "NX" keyword), so that people can know this PCD is to control NX attribute.
Maybe we can apply other protection such as RO or RP later.
What about your idea?
5) Patch V2 5/5 -
5.1) I think we should check the allocation happens IsInSmm, and skip ApplyMemoryProtection() if it is in Smm.
The reason is that SMM maintains its own page table.
Below code is for your reference.
//////////////////////////
BOOLEAN
IsInSmm (
VOID
)
{
BOOLEAN InSmm;
InSmm = FALSE;
if (gSmmBase2 != NULL) {
gSmmBase2->InSmm (gSmmBase2, &InSmm);
}
return InSmm;
}
//////////////////////////
5.2) I think we are not able to call ApplyMemoryProtection() inside of CoreAllocatePoolPages() and CoreFreePoolPages().
The reason is that: X86 CPU page table update algo might call AllocatePages(), to support page table split from big page to small page.
CoreAcquireMemoryLock() may fail in such case, because the memory map is locked in AllocatePool().
I think a safety way is to call ApplyMemoryProtection() at CoreAllocatePool(), after InstallMemoryAttributesTableOnMemoryAllocation(). We do same thing as CoreAllocatePage().
We can update CoreInternalAllocatePool() to return the necessary parameters back to indicate if CoreAllocatePoolPages() happens, and where is the new pages.
Same thing for CoreFreePool().
5.3) In order to reduce the fragmentation of X86 page table, I recommend we do a little enhancement in ApplyDxeMemoryProtectionPolicy().
Can we can combine the memory need NX together and call SetUefiImageMemoryAttributes() once?
You may refer to MergeMemoryMapForNotPresentEntry() in UefiCpuPkg\PiSmmCpuDxeSmm\SmmCpuMemoryManagement.c,
which combines the memory map entry together, if the adjacent entry requires same not-present attribute.
In this case, we could define MergeMemoryMapForNonExecutable() in MemoryProtection.c, and used by ApplyDxeMemoryProtectionPolicy().
I believe it helps X86 platforms.
Thank you
Yao Jiewen
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard
> Biesheuvel
> Sent: Friday, February 24, 2017 11:05 PM
> 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: Tian, Feng <feng.tian@intel.com>; lersek@redhat.com; Zeng, Star
> <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: [edk2] [PATCH v2 0/5] 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.
>
> Changes since v1:
> - allocate code pages for PE/COFF images in PeiCore, so that DxeCore pages have
> the expected memory type (as suggested by Jiewen)
> - add patch to inhibit page table updates while syncing the GCD memory space
> map with the page tables
> - add PCD to set memory protection policy, which allows the policy for reserved
> and ACPI/NVS memory to be configured separately
> - move attribute manipulation into DxeCore page allocation code: this way, we
> should be able to solve the EBC case by allocating BootServicesCode pool
> memory explicitly.
>
> Ard Biesheuvel (5):
> ArmPkg/CpuDxe: ignore attribute changes during SyncCacheConfig()
> MdeModulePkg/PeiCore: allocate BootServicesCode memory for PE/COFF
> images
> MdeModulePkg/DxeCore: pass pool type to CoreFreePoolPages ()
> MdeModulePkg: define PCD for DXE memory protection policy
> MdeModulePkg/DxeCore: implement memory protection policy
>
> ArmPkg/Drivers/CpuDxe/CpuDxe.c | 3 +
> ArmPkg/Drivers/CpuDxe/CpuDxe.h | 1 +
> ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c | 4 +
> MdeModulePkg/Core/Dxe/DxeMain.inf | 1 +
> MdeModulePkg/Core/Dxe/Mem/Imem.h | 2 +
> MdeModulePkg/Core/Dxe/Mem/Page.c | 106
> ++++++++++++++++++++
> MdeModulePkg/Core/Dxe/Mem/Pool.c | 5 +-
> MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 104
> ++++++++++++++++++-
> MdeModulePkg/Core/Pei/Image/Image.c | 10 +-
> MdeModulePkg/MdeModulePkg.dec | 16 +++
> 10 files changed, 246 insertions(+), 6 deletions(-)
>
> --
> 2.7.4
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 0/5] RFC: increased memory protection
2017-02-25 4:04 ` [PATCH v2 0/5] RFC: increased memory protection Yao, Jiewen
@ 2017-02-26 15:09 ` Ard Biesheuvel
2017-02-27 8:56 ` Laszlo Ersek
0 siblings, 1 reply; 9+ messages in thread
From: Ard Biesheuvel @ 2017-02-26 15:09 UTC (permalink / raw)
To: Yao, Jiewen
Cc: edk2-devel@lists.01.org, afish@apple.com,
leif.lindholm@linaro.org, Kinney, Michael D, Gao, Liming,
Tian, Feng, lersek@redhat.com, Zeng, Star
On 25 February 2017 at 04:04, Yao, Jiewen <jiewen.yao@intel.com> wrote:
> Thank you Ard. I like this patch - simple and obvious.
>
Thank you
> I put all my comment together for your consideration.
>
> 1) Patch V2 1/5 -- reviewed-by: Jiewen.yao@Intel.com
> 2) Patch V2 2/5 - reviewed-by: Jiewen.yao@intel.com
OK
> 3) Patch V2 3/5 - reviewed-by: Jiewen.yao@intel.com
>
I may be able to drop this if the ApplyMemoryProtection() calls need
to be moved elsewhere for pool allocations.
> 4) Patch V2 4/5 -
> 4.1) Can we follow the style of other memory type definition? (Such as
> PcdMemoryProfileMemoryType)
>
> The reason is that people may want to have fine granularity control for
> loader data or persistent memory.
>
> My proposal is below:
> //////////////////////////
> ## Set DXE memory protection policy. The policy is bitwise.
> # If a bit is set, memory regions of the associated type will be mapped
> # non-executable.<BR><BR>
> #
> # Below is bit mask for this PCD: (Order is same as UEFI spec)<BR>
> # EfiReservedMemoryType 0x0001<BR>
> # EfiLoaderCode 0x0002<BR>
> # EfiLoaderData 0x0004<BR>
> # EfiBootServicesCode 0x0008<BR>
> # EfiBootServicesData 0x0010<BR>
> # EfiRuntimeServicesCode 0x0020<BR>
> # EfiRuntimeServicesData 0x0040<BR>
> # EfiConventionalMemory 0x0080<BR>
> # EfiUnusableMemory 0x0100<BR>
> # EfiACPIReclaimMemory 0x0200<BR>
> # EfiACPIMemoryNVS 0x0400<BR>
> # EfiMemoryMappedIO 0x0800<BR>
> # EfiMemoryMappedIOPortSpace 0x1000<BR>
> # EfiPalCode 0x2000<BR>
> # EfiPersistentMemory 0x4000<BR>
> # OEM Reserved 0x4000000000000000<BR>
> # OS Reserved 0x8000000000000000<BR>
> #
> # NOTE: User must NOT set NX protection for EfiLoaderCode /
> EfiBootServicesCode / EfiRuntimeServicesCode. <BR>
> #
> # e.g. 0x7FD5 can be used for all memory except Code. <BR>
> # e.g. 0x7BD4 can be used for all memory except Code and ACPINVS/Reserved.
> <BR>
> #
> # @Prompt Set DXE memory protection policy.
>
> gEfiMdeModulePkgTokenSpaceGuid.PcdDxeMemoryProtectionPolicy|0x0000000|UINT64|0x00001048
> //////////////////////////
>
> Then the C-code can be like below:
>
> //////////////////////////
> UINT64
> GetPermissionAttributeForMemoryType (
> IN EFI_MEMORY_TYPE MemoryType
> )
> {
> UINT64 TestBit;
>
> if ((UINT32) MemoryType >= MEMORY_TYPE_OS_RESERVED_MIN) {
> TestBit = BIT63;
> } else if ((UINT32) MemoryType >= MEMORY_TYPE_OEM_RESERVED_MIN) {
> TestBit = BIT62;
> } else {
> TestBit = LShiftU64 (1, MemoryType);
> }
>
> if ((PcdGet64 (PcdMemoryProfileMemoryType) & TestBit) != 0) {
> return EFI_MEMORY_XP;
> } else {
> return 0;
> }
> }
> //////////////////////////
>
Thanks, I will use your definition instead.
> 4.2) I prefer to setting default value to be 0x0 - to keep the
> compatibility, at least for X86 platform. (I have no strong opinion for
> ARM.)
>
Yes, naturally. For this RFC series, I used a default that enables the
feature, but I agree that this should be opt-in
> 4.3) I feel we might use a better name - PcdDxeNxMemoryProtectionPolicy (add
> "NX" keyword), so that people can know this PCD is to control NX attribute.
> Maybe we can apply other protection such as RO or RP later.
> What about your idea?
>
OK
> 5) Patch V2 5/5 -
> 5.1) I think we should check the allocation happens IsInSmm, and skip
> ApplyMemoryProtection() if it is in Smm.
>
> The reason is that SMM maintains its own page table.
>
> Below code is for your reference.
>
> //////////////////////////
> BOOLEAN
> IsInSmm (
> VOID
> )
> {
> BOOLEAN InSmm;
>
> InSmm = FALSE;
> if (gSmmBase2 != NULL) {
> gSmmBase2->InSmm (gSmmBase2, &InSmm);
> }
> return InSmm;
> }
> //////////////////////////
>
OK
> 5.2) I think we are not able to call ApplyMemoryProtection() inside of
> CoreAllocatePoolPages() and CoreFreePoolPages().
> The reason is that: X86 CPU page table update algo might call
> AllocatePages(), to support page table split from big page to small page.
> CoreAcquireMemoryLock() may fail in such case, because the memory map is
> locked in AllocatePool().
>
> I think a safety way is to call ApplyMemoryProtection() at
> CoreAllocatePool(), after InstallMemoryAttributesTableOnMemoryAllocation().
> We do same thing as CoreAllocatePage().
>
> We can update CoreInternalAllocatePool() to return the necessary parameters
> back to indicate if CoreAllocatePoolPages() happens, and where is the new
> pages.
> Same thing for CoreFreePool().
>
I did realise this. But in my implementation, EfiConventionalMemory
and EfiBootServicesData always have the same policy, so the recursion
can never happen. Of course, with your version of the PCD, this could
occur, and we need to address it.
> 5.3) In order to reduce the fragmentation of X86 page table, I recommend we
> do a little enhancement in ApplyDxeMemoryProtectionPolicy().
> Can we can combine the memory need NX together and call
> SetUefiImageMemoryAttributes() once?
>
> You may refer to MergeMemoryMapForNotPresentEntry() in
> UefiCpuPkg\PiSmmCpuDxeSmm\SmmCpuMemoryManagement.c,
> which combines the memory map entry together, if the adjacent entry requires
> same not-present attribute.
>
> In this case, we could define MergeMemoryMapForNonExecutable() in
> MemoryProtection.c, and used by ApplyDxeMemoryProtectionPolicy().
> I believe it helps X86 platforms.
>
Sure. I also need to copy SortMemoryMap() then, which performs a
bubble sort :-( And BaseSortLib cannot be used in DXE_CORE modules.
In any case, I will proceed with respinning these patches,
Thanks for the feedback,
Ard.
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard
>> Biesheuvel
>> Sent: Friday, February 24, 2017 11:05 PM
>> 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: Tian, Feng <feng.tian@intel.com>; lersek@redhat.com; Zeng, Star
>> <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Subject: [edk2] [PATCH v2 0/5] 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.
>>
>> Changes since v1:
>> - allocate code pages for PE/COFF images in PeiCore, so that DxeCore pages
>> have
>> the expected memory type (as suggested by Jiewen)
>> - add patch to inhibit page table updates while syncing the GCD memory
>> space
>> map with the page tables
>> - add PCD to set memory protection policy, which allows the policy for
>> reserved
>> and ACPI/NVS memory to be configured separately
>> - move attribute manipulation into DxeCore page allocation code: this way,
>> we
>> should be able to solve the EBC case by allocating BootServicesCode pool
>> memory explicitly.
>>
>> Ard Biesheuvel (5):
>> ArmPkg/CpuDxe: ignore attribute changes during SyncCacheConfig()
>> MdeModulePkg/PeiCore: allocate BootServicesCode memory for PE/COFF
>> images
>> MdeModulePkg/DxeCore: pass pool type to CoreFreePoolPages ()
>> MdeModulePkg: define PCD for DXE memory protection policy
>> MdeModulePkg/DxeCore: implement memory protection policy
>>
>> ArmPkg/Drivers/CpuDxe/CpuDxe.c | 3 +
>> ArmPkg/Drivers/CpuDxe/CpuDxe.h | 1 +
>> ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c | 4 +
>> MdeModulePkg/Core/Dxe/DxeMain.inf | 1 +
>> MdeModulePkg/Core/Dxe/Mem/Imem.h | 2 +
>> MdeModulePkg/Core/Dxe/Mem/Page.c | 106
>> ++++++++++++++++++++
>> MdeModulePkg/Core/Dxe/Mem/Pool.c | 5 +-
>> MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 104
>> ++++++++++++++++++-
>> MdeModulePkg/Core/Pei/Image/Image.c | 10 +-
>> MdeModulePkg/MdeModulePkg.dec | 16 +++
>> 10 files changed, 246 insertions(+), 6 deletions(-)
>>
>> --
>> 2.7.4
>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 0/5] RFC: increased memory protection
2017-02-26 15:09 ` Ard Biesheuvel
@ 2017-02-27 8:56 ` Laszlo Ersek
0 siblings, 0 replies; 9+ messages in thread
From: Laszlo Ersek @ 2017-02-27 8:56 UTC (permalink / raw)
To: Ard Biesheuvel, Yao, Jiewen
Cc: edk2-devel@lists.01.org, afish@apple.com,
leif.lindholm@linaro.org, Kinney, Michael D, Gao, Liming,
Tian, Feng, Zeng, Star
On 02/26/17 16:09, Ard Biesheuvel wrote:
> On 25 February 2017 at 04:04, Yao, Jiewen <jiewen.yao@intel.com> wrote:
>> Thank you Ard. I like this patch - simple and obvious.
>>
>
> Thank you
>
>> I put all my comment together for your consideration.
>>
>> 1) Patch V2 1/5 -- reviewed-by: Jiewen.yao@Intel.com
>> 2) Patch V2 2/5 - reviewed-by: Jiewen.yao@intel.com
>
> OK
>
>> 3) Patch V2 3/5 - reviewed-by: Jiewen.yao@intel.com
>>
>
> I may be able to drop this if the ApplyMemoryProtection() calls need
> to be moved elsewhere for pool allocations.
>
>> 4) Patch V2 4/5 -
>> 4.1) Can we follow the style of other memory type definition? (Such as
>> PcdMemoryProfileMemoryType)
>>
>> The reason is that people may want to have fine granularity control for
>> loader data or persistent memory.
>>
>> My proposal is below:
>> //////////////////////////
>> ## Set DXE memory protection policy. The policy is bitwise.
>> # If a bit is set, memory regions of the associated type will be mapped
>> # non-executable.<BR><BR>
>> #
>> # Below is bit mask for this PCD: (Order is same as UEFI spec)<BR>
>> # EfiReservedMemoryType 0x0001<BR>
>> # EfiLoaderCode 0x0002<BR>
>> # EfiLoaderData 0x0004<BR>
>> # EfiBootServicesCode 0x0008<BR>
>> # EfiBootServicesData 0x0010<BR>
>> # EfiRuntimeServicesCode 0x0020<BR>
>> # EfiRuntimeServicesData 0x0040<BR>
>> # EfiConventionalMemory 0x0080<BR>
>> # EfiUnusableMemory 0x0100<BR>
>> # EfiACPIReclaimMemory 0x0200<BR>
>> # EfiACPIMemoryNVS 0x0400<BR>
>> # EfiMemoryMappedIO 0x0800<BR>
>> # EfiMemoryMappedIOPortSpace 0x1000<BR>
>> # EfiPalCode 0x2000<BR>
>> # EfiPersistentMemory 0x4000<BR>
>> # OEM Reserved 0x4000000000000000<BR>
>> # OS Reserved 0x8000000000000000<BR>
>> #
>> # NOTE: User must NOT set NX protection for EfiLoaderCode /
>> EfiBootServicesCode / EfiRuntimeServicesCode. <BR>
>> #
>> # e.g. 0x7FD5 can be used for all memory except Code. <BR>
>> # e.g. 0x7BD4 can be used for all memory except Code and ACPINVS/Reserved.
>> <BR>
>> #
>> # @Prompt Set DXE memory protection policy.
>>
>> gEfiMdeModulePkgTokenSpaceGuid.PcdDxeMemoryProtectionPolicy|0x0000000|UINT64|0x00001048
>> //////////////////////////
>>
>> Then the C-code can be like below:
>>
>> //////////////////////////
>> UINT64
>> GetPermissionAttributeForMemoryType (
>> IN EFI_MEMORY_TYPE MemoryType
>> )
>> {
>> UINT64 TestBit;
>>
>> if ((UINT32) MemoryType >= MEMORY_TYPE_OS_RESERVED_MIN) {
>> TestBit = BIT63;
>> } else if ((UINT32) MemoryType >= MEMORY_TYPE_OEM_RESERVED_MIN) {
>> TestBit = BIT62;
>> } else {
>> TestBit = LShiftU64 (1, MemoryType);
>> }
>>
>> if ((PcdGet64 (PcdMemoryProfileMemoryType) & TestBit) != 0) {
>> return EFI_MEMORY_XP;
>> } else {
>> return 0;
>> }
>> }
>> //////////////////////////
>>
>
> Thanks, I will use your definition instead.
>
>> 4.2) I prefer to setting default value to be 0x0 - to keep the
>> compatibility, at least for X86 platform. (I have no strong opinion for
>> ARM.)
>>
>
> Yes, naturally. For this RFC series, I used a default that enables the
> feature, but I agree that this should be opt-in
>
>> 4.3) I feel we might use a better name - PcdDxeNxMemoryProtectionPolicy (add
>> "NX" keyword), so that people can know this PCD is to control NX attribute.
>> Maybe we can apply other protection such as RO or RP later.
>> What about your idea?
>>
>
> OK
>
>> 5) Patch V2 5/5 -
>> 5.1) I think we should check the allocation happens IsInSmm, and skip
>> ApplyMemoryProtection() if it is in Smm.
>>
>> The reason is that SMM maintains its own page table.
>>
>> Below code is for your reference.
>>
>> //////////////////////////
>> BOOLEAN
>> IsInSmm (
>> VOID
>> )
>> {
>> BOOLEAN InSmm;
>>
>> InSmm = FALSE;
>> if (gSmmBase2 != NULL) {
>> gSmmBase2->InSmm (gSmmBase2, &InSmm);
>> }
>> return InSmm;
>> }
>> //////////////////////////
>>
>
> OK
>
>> 5.2) I think we are not able to call ApplyMemoryProtection() inside of
>> CoreAllocatePoolPages() and CoreFreePoolPages().
>> The reason is that: X86 CPU page table update algo might call
>> AllocatePages(), to support page table split from big page to small page.
>> CoreAcquireMemoryLock() may fail in such case, because the memory map is
>> locked in AllocatePool().
>>
>> I think a safety way is to call ApplyMemoryProtection() at
>> CoreAllocatePool(), after InstallMemoryAttributesTableOnMemoryAllocation().
>> We do same thing as CoreAllocatePage().
>>
>> We can update CoreInternalAllocatePool() to return the necessary parameters
>> back to indicate if CoreAllocatePoolPages() happens, and where is the new
>> pages.
>> Same thing for CoreFreePool().
>>
>
> I did realise this. But in my implementation, EfiConventionalMemory
> and EfiBootServicesData always have the same policy, so the recursion
> can never happen. Of course, with your version of the PCD, this could
> occur, and we need to address it.
>
>> 5.3) In order to reduce the fragmentation of X86 page table, I recommend we
>> do a little enhancement in ApplyDxeMemoryProtectionPolicy().
>> Can we can combine the memory need NX together and call
>> SetUefiImageMemoryAttributes() once?
>>
>> You may refer to MergeMemoryMapForNotPresentEntry() in
>> UefiCpuPkg\PiSmmCpuDxeSmm\SmmCpuMemoryManagement.c,
>> which combines the memory map entry together, if the adjacent entry requires
>> same not-present attribute.
>>
>> In this case, we could define MergeMemoryMapForNonExecutable() in
>> MemoryProtection.c, and used by ApplyDxeMemoryProtectionPolicy().
>> I believe it helps X86 platforms.
>>
>
> Sure. I also need to copy SortMemoryMap() then, which performs a
> bubble sort :-( And BaseSortLib cannot be used in DXE_CORE modules.
You might want to check out
MdePkg/Include/Library/OrderedCollectionLib.h
MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf
The library instance is consumable for all modules, as long as they have
DebugLib and MemoryAllocationLib resolutions. When used in DXE_CORE (for
which FreePool() has an actual implementation in
"MdeModulePkg/Library/DxeCoreMemoryAllocationLib/MemoryAllocationLib.c"),
it won't even leak memory (as opposed to usage in PEIMs, where
FreePool() does nothing).
An example that uses this library for sorting can be found in
"OvmfPkg/Library/QemuBootOrderLib/ExtraRootBusMap.c".
Feel free to decide against it, I just thought I should mention it.
Thanks
Laszlo
>
> In any case, I will proceed with respinning these patches,
>
> Thanks for the feedback,
> Ard.
>
>
>>> -----Original Message-----
>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard
>>> Biesheuvel
>>> Sent: Friday, February 24, 2017 11:05 PM
>>> 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: Tian, Feng <feng.tian@intel.com>; lersek@redhat.com; Zeng, Star
>>> <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> Subject: [edk2] [PATCH v2 0/5] 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.
>>>
>>> Changes since v1:
>>> - allocate code pages for PE/COFF images in PeiCore, so that DxeCore pages
>>> have
>>> the expected memory type (as suggested by Jiewen)
>>> - add patch to inhibit page table updates while syncing the GCD memory
>>> space
>>> map with the page tables
>>> - add PCD to set memory protection policy, which allows the policy for
>>> reserved
>>> and ACPI/NVS memory to be configured separately
>>> - move attribute manipulation into DxeCore page allocation code: this way,
>>> we
>>> should be able to solve the EBC case by allocating BootServicesCode pool
>>> memory explicitly.
>>>
>>> Ard Biesheuvel (5):
>>> ArmPkg/CpuDxe: ignore attribute changes during SyncCacheConfig()
>>> MdeModulePkg/PeiCore: allocate BootServicesCode memory for PE/COFF
>>> images
>>> MdeModulePkg/DxeCore: pass pool type to CoreFreePoolPages ()
>>> MdeModulePkg: define PCD for DXE memory protection policy
>>> MdeModulePkg/DxeCore: implement memory protection policy
>>>
>>> ArmPkg/Drivers/CpuDxe/CpuDxe.c | 3 +
>>> ArmPkg/Drivers/CpuDxe/CpuDxe.h | 1 +
>>> ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c | 4 +
>>> MdeModulePkg/Core/Dxe/DxeMain.inf | 1 +
>>> MdeModulePkg/Core/Dxe/Mem/Imem.h | 2 +
>>> MdeModulePkg/Core/Dxe/Mem/Page.c | 106
>>> ++++++++++++++++++++
>>> MdeModulePkg/Core/Dxe/Mem/Pool.c | 5 +-
>>> MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 104
>>> ++++++++++++++++++-
>>> MdeModulePkg/Core/Pei/Image/Image.c | 10 +-
>>> MdeModulePkg/MdeModulePkg.dec | 16 +++
>>> 10 files changed, 246 insertions(+), 6 deletions(-)
>>>
>>> --
>>> 2.7.4
>>>
>>> _______________________________________________
>>> edk2-devel mailing list
>>> edk2-devel@lists.01.org
>>> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 9+ messages in thread