* [PATCH v3 0/6] RFC: increased memory protection
@ 2017-02-26 18:29 Ard Biesheuvel
2017-02-26 18:30 ` [PATCH v3 1/6] ArmPkg/CpuDxe: ignore attribute changes during SyncCacheConfig() Ard Biesheuvel
` (7 more replies)
0 siblings, 8 replies; 21+ messages in thread
From: Ard Biesheuvel @ 2017-02-26 18:29 UTC (permalink / raw)
To: edk2-devel, jiewen.yao, leif.lindholm
Cc: afish, michael.d.kinney, liming.gao, lersek, feng.tian, star.zeng,
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.
Changes since v2:
- added patch to make EBC use EfiBootServicesCode pool allocations for thunks
- redefine PCD according to Jiewen's feedback, including default value
- use sorted memory map and merge adjacent entries with the same policy, to
prevent unnecessary page table splitting
- ignore policy when executing in SMM
- refactor the logic for managing permission attributes of pool allocations
- added some R-b's
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.
Series can be found here:
https://git.linaro.org/people/ard.biesheuvel/uefi-next.git/log/?h=memprot-take2
Note that to test this properly, the default value of 0 should be changed
to 0x7FD5, which applies non-exec permissions to everything except Efi*Code
regions.
Ard Biesheuvel (6):
ArmPkg/CpuDxe: ignore attribute changes during SyncCacheConfig()
MdeModulePkg/PeiCore: allocate BootServicesCode memory for PE/COFF
images
MdeModulePkg/EbcDxe: use EfiBootServicesCode memory for thunks
MdeModulePkg/DxeCore: use separate lock for pool allocations
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.h | 24 ++
MdeModulePkg/Core/Dxe/DxeMain.inf | 1 +
MdeModulePkg/Core/Dxe/Mem/Page.c | 4 +
MdeModulePkg/Core/Dxe/Mem/Pool.c | 60 +++-
MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 306 +++++++++++++++++++-
MdeModulePkg/Core/Pei/Image/Image.c | 10 +-
MdeModulePkg/MdeModulePkg.dec | 31 ++
MdeModulePkg/Universal/EbcDxe/AArch64/EbcSupport.c | 2 +-
MdeModulePkg/Universal/EbcDxe/EbcInt.c | 23 ++
MdeModulePkg/Universal/EbcDxe/EbcInt.h | 14 +
MdeModulePkg/Universal/EbcDxe/Ia32/EbcSupport.c | 2 +-
MdeModulePkg/Universal/EbcDxe/Ipf/EbcSupport.c | 2 +-
MdeModulePkg/Universal/EbcDxe/X64/EbcSupport.c | 2 +-
16 files changed, 471 insertions(+), 18 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3 1/6] ArmPkg/CpuDxe: ignore attribute changes during SyncCacheConfig()
2017-02-26 18:29 [PATCH v3 0/6] RFC: increased memory protection Ard Biesheuvel
@ 2017-02-26 18:30 ` Ard Biesheuvel
2017-02-26 18:30 ` [PATCH v3 2/6] MdeModulePkg/PeiCore: allocate BootServicesCode memory for PE/COFF images Ard Biesheuvel
` (6 subsequent siblings)
7 siblings, 0 replies; 21+ messages in thread
From: Ard Biesheuvel @ 2017-02-26 18:30 UTC (permalink / raw)
To: edk2-devel, jiewen.yao, leif.lindholm
Cc: afish, michael.d.kinney, liming.gao, 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>
Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
---
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] 21+ messages in thread
* [PATCH v3 2/6] MdeModulePkg/PeiCore: allocate BootServicesCode memory for PE/COFF images
2017-02-26 18:29 [PATCH v3 0/6] RFC: increased memory protection Ard Biesheuvel
2017-02-26 18:30 ` [PATCH v3 1/6] ArmPkg/CpuDxe: ignore attribute changes during SyncCacheConfig() Ard Biesheuvel
@ 2017-02-26 18:30 ` Ard Biesheuvel
2017-02-27 6:43 ` Gao, Liming
2017-02-26 18:30 ` [PATCH v3 3/6] MdeModulePkg/EbcDxe: use EfiBootServicesCode memory for thunks Ard Biesheuvel
` (5 subsequent siblings)
7 siblings, 1 reply; 21+ messages in thread
From: Ard Biesheuvel @ 2017-02-26 18:30 UTC (permalink / raw)
To: edk2-devel, jiewen.yao, leif.lindholm
Cc: afish, michael.d.kinney, liming.gao, 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>
Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
---
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] 21+ messages in thread
* [PATCH v3 3/6] MdeModulePkg/EbcDxe: use EfiBootServicesCode memory for thunks
2017-02-26 18:29 [PATCH v3 0/6] RFC: increased memory protection Ard Biesheuvel
2017-02-26 18:30 ` [PATCH v3 1/6] ArmPkg/CpuDxe: ignore attribute changes during SyncCacheConfig() Ard Biesheuvel
2017-02-26 18:30 ` [PATCH v3 2/6] MdeModulePkg/PeiCore: allocate BootServicesCode memory for PE/COFF images Ard Biesheuvel
@ 2017-02-26 18:30 ` Ard Biesheuvel
2017-02-26 18:30 ` [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock for pool allocations Ard Biesheuvel
` (4 subsequent siblings)
7 siblings, 0 replies; 21+ messages in thread
From: Ard Biesheuvel @ 2017-02-26 18:30 UTC (permalink / raw)
To: edk2-devel, jiewen.yao, leif.lindholm
Cc: afish, michael.d.kinney, liming.gao, lersek, feng.tian, star.zeng,
Ard Biesheuvel
The EBC driver emits thunks for native to EBC calls, which are short
instructions sequences that bridge the gap between the native execution
environment and the EBC virtual machine.
Since these thunks are allocated using MemoryAllocationLib::AllocatePool(),
they are emitted into EfiBootServicesData regions, which does not reflect
the nature of these thunks accurately, and interferes with strict memory
protection policies that map data regions non-executable.
So instead, create a new helper EbcAllocatePoolForThunk() that invokes the
AllocatePool() boot services directly to allocate EfiBootServicesCode pool
memory explicitly, and wire up this helper for the various architecture
specific thunk generation routines.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
MdeModulePkg/Universal/EbcDxe/AArch64/EbcSupport.c | 2 +-
MdeModulePkg/Universal/EbcDxe/EbcInt.c | 23 ++++++++++++++++++++
MdeModulePkg/Universal/EbcDxe/EbcInt.h | 14 ++++++++++++
MdeModulePkg/Universal/EbcDxe/Ia32/EbcSupport.c | 2 +-
MdeModulePkg/Universal/EbcDxe/Ipf/EbcSupport.c | 2 +-
MdeModulePkg/Universal/EbcDxe/X64/EbcSupport.c | 2 +-
6 files changed, 41 insertions(+), 4 deletions(-)
diff --git a/MdeModulePkg/Universal/EbcDxe/AArch64/EbcSupport.c b/MdeModulePkg/Universal/EbcDxe/AArch64/EbcSupport.c
index ade47c4d0622..7c13ce12a38b 100644
--- a/MdeModulePkg/Universal/EbcDxe/AArch64/EbcSupport.c
+++ b/MdeModulePkg/Universal/EbcDxe/AArch64/EbcSupport.c
@@ -383,7 +383,7 @@ EbcCreateThunks (
return EFI_INVALID_PARAMETER;
}
- InstructionBuffer = AllocatePool (sizeof (EBC_INSTRUCTION_BUFFER));
+ InstructionBuffer = EbcAllocatePoolForThunk (sizeof (EBC_INSTRUCTION_BUFFER));
if (InstructionBuffer == NULL) {
return EFI_OUT_OF_RESOURCES;
}
diff --git a/MdeModulePkg/Universal/EbcDxe/EbcInt.c b/MdeModulePkg/Universal/EbcDxe/EbcInt.c
index 6fd2aaf5af27..727ba8bcae44 100644
--- a/MdeModulePkg/Universal/EbcDxe/EbcInt.c
+++ b/MdeModulePkg/Universal/EbcDxe/EbcInt.c
@@ -1410,3 +1410,26 @@ EbcVmTestUnsupported (
return EFI_UNSUPPORTED;
}
+/**
+ Allocates a buffer of type EfiBootServicesCode.
+
+ @param AllocationSize The number of bytes to allocate.
+
+ @return A pointer to the allocated buffer or NULL if allocation fails.
+
+**/
+VOID *
+EFIAPI
+EbcAllocatePoolForThunk (
+ IN UINTN AllocationSize
+ )
+{
+ VOID *Buffer;
+ EFI_STATUS Status;
+
+ Status = gBS->AllocatePool (EfiBootServicesCode, AllocationSize, &Buffer);
+ if (EFI_ERROR (Status)) {
+ return NULL;
+ }
+ return Buffer;
+}
diff --git a/MdeModulePkg/Universal/EbcDxe/EbcInt.h b/MdeModulePkg/Universal/EbcDxe/EbcInt.h
index 75017a23e75e..8aa7a4abbd63 100644
--- a/MdeModulePkg/Universal/EbcDxe/EbcInt.h
+++ b/MdeModulePkg/Universal/EbcDxe/EbcInt.h
@@ -246,4 +246,18 @@ typedef struct {
CR(a, EBC_PROTOCOL_PRIVATE_DATA, EbcProtocol, EBC_PROTOCOL_PRIVATE_DATA_SIGNATURE)
+/**
+ Allocates a buffer of type EfiBootServicesCode.
+
+ @param AllocationSize The number of bytes to allocate.
+
+ @return A pointer to the allocated buffer or NULL if allocation fails.
+
+**/
+VOID *
+EFIAPI
+EbcAllocatePoolForThunk (
+ IN UINTN AllocationSize
+ );
+
#endif // #ifndef _EBC_INT_H_
diff --git a/MdeModulePkg/Universal/EbcDxe/Ia32/EbcSupport.c b/MdeModulePkg/Universal/EbcDxe/Ia32/EbcSupport.c
index 8e660b93ad64..a825846f89c3 100644
--- a/MdeModulePkg/Universal/EbcDxe/Ia32/EbcSupport.c
+++ b/MdeModulePkg/Universal/EbcDxe/Ia32/EbcSupport.c
@@ -484,7 +484,7 @@ EbcCreateThunks (
ThunkSize = sizeof(mInstructionBufferTemplate);
- Ptr = AllocatePool (sizeof(mInstructionBufferTemplate));
+ Ptr = EbcAllocatePoolForThunk (sizeof(mInstructionBufferTemplate));
if (Ptr == NULL) {
return EFI_OUT_OF_RESOURCES;
diff --git a/MdeModulePkg/Universal/EbcDxe/Ipf/EbcSupport.c b/MdeModulePkg/Universal/EbcDxe/Ipf/EbcSupport.c
index 95837cb67865..f99348f181a9 100644
--- a/MdeModulePkg/Universal/EbcDxe/Ipf/EbcSupport.c
+++ b/MdeModulePkg/Universal/EbcDxe/Ipf/EbcSupport.c
@@ -403,7 +403,7 @@ EbcCreateThunks (
//
Size = EBC_THUNK_SIZE + EBC_THUNK_ALIGNMENT - 1;
ThunkSize = Size;
- Ptr = AllocatePool (Size);
+ Ptr = EbcAllocatePoolForThunk (Size);
if (Ptr == NULL) {
return EFI_OUT_OF_RESOURCES;
diff --git a/MdeModulePkg/Universal/EbcDxe/X64/EbcSupport.c b/MdeModulePkg/Universal/EbcDxe/X64/EbcSupport.c
index 4325e2e52710..33a174917b69 100644
--- a/MdeModulePkg/Universal/EbcDxe/X64/EbcSupport.c
+++ b/MdeModulePkg/Universal/EbcDxe/X64/EbcSupport.c
@@ -441,7 +441,7 @@ EbcCreateThunks (
ThunkSize = sizeof(mInstructionBufferTemplate);
- Ptr = AllocatePool (sizeof(mInstructionBufferTemplate));
+ Ptr = EbcAllocatePoolForThunk (sizeof(mInstructionBufferTemplate));
if (Ptr == NULL) {
return EFI_OUT_OF_RESOURCES;
--
2.7.4
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock for pool allocations
2017-02-26 18:29 [PATCH v3 0/6] RFC: increased memory protection Ard Biesheuvel
` (2 preceding siblings ...)
2017-02-26 18:30 ` [PATCH v3 3/6] MdeModulePkg/EbcDxe: use EfiBootServicesCode memory for thunks Ard Biesheuvel
@ 2017-02-26 18:30 ` Ard Biesheuvel
2017-02-26 19:52 ` Ard Biesheuvel
` (2 more replies)
2017-02-26 18:30 ` [PATCH v3 5/6] MdeModulePkg: define PCD for DXE memory protection policy Ard Biesheuvel
` (3 subsequent siblings)
7 siblings, 3 replies; 21+ messages in thread
From: Ard Biesheuvel @ 2017-02-26 18:30 UTC (permalink / raw)
To: edk2-devel, jiewen.yao, leif.lindholm
Cc: afish, michael.d.kinney, liming.gao, lersek, feng.tian, star.zeng,
Ard Biesheuvel
In preparation of adding memory permission attribute management to
the pool allocator, split off the locking of the pool metadata into
a separate lock. This is an improvement in itself, given that pool
allocation can only interfere with the page allocation bookkeeping
if pool pages are allocated or released. But it is also required to
ensure that the permission attribute management does not deadlock,
given that it may trigger page table splits leading to additional
page tables being allocated.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
MdeModulePkg/Core/Dxe/Mem/Pool.c | 53 ++++++++++++++++----
1 file changed, 43 insertions(+), 10 deletions(-)
diff --git a/MdeModulePkg/Core/Dxe/Mem/Pool.c b/MdeModulePkg/Core/Dxe/Mem/Pool.c
index 7afd2d312c1d..410615e0dee9 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Pool.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Pool.c
@@ -15,6 +15,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
#include "DxeMain.h"
#include "Imem.h"
+STATIC EFI_LOCK mPoolMemoryLock = EFI_INITIALIZE_LOCK_VARIABLE (TPL_NOTIFY);
+
#define POOL_FREE_SIGNATURE SIGNATURE_32('p','f','r','0')
typedef struct {
UINT32 Signature;
@@ -239,13 +241,13 @@ CoreInternalAllocatePool (
//
// Acquire the memory lock and make the allocation
//
- Status = CoreAcquireLockOrFail (&gMemoryLock);
+ Status = CoreAcquireLockOrFail (&mPoolMemoryLock);
if (EFI_ERROR (Status)) {
return EFI_OUT_OF_RESOURCES;
}
*Buffer = CoreAllocatePoolI (PoolType, Size);
- CoreReleaseMemoryLock ();
+ CoreReleaseLock (&mPoolMemoryLock);
return (*Buffer != NULL) ? EFI_SUCCESS : EFI_OUT_OF_RESOURCES;
}
@@ -289,6 +291,23 @@ CoreAllocatePool (
return Status;
}
+STATIC
+VOID *
+CoreAllocatePoolPagesI (
+ IN EFI_MEMORY_TYPE PoolType,
+ IN UINTN NoPages,
+ IN UINTN Granularity
+ )
+{
+ VOID *Buffer;
+
+ CoreAcquireMemoryLock ();
+ Buffer = CoreAllocatePoolPages (PoolType, NoPages, Granularity);
+ CoreReleaseMemoryLock ();
+
+ return Buffer;
+}
+
/**
Internal function to allocate pool of a particular type.
Caller must have the memory lock held
@@ -317,7 +336,7 @@ CoreAllocatePoolI (
UINTN NoPages;
UINTN Granularity;
- ASSERT_LOCKED (&gMemoryLock);
+ ASSERT_LOCKED (&mPoolMemoryLock);
if (PoolType == EfiACPIReclaimMemory ||
PoolType == EfiACPIMemoryNVS ||
@@ -355,7 +374,7 @@ CoreAllocatePoolI (
if (Index >= SIZE_TO_LIST (Granularity)) {
NoPages = EFI_SIZE_TO_PAGES(Size) + EFI_SIZE_TO_PAGES (Granularity) - 1;
NoPages &= ~(UINTN)(EFI_SIZE_TO_PAGES (Granularity) - 1);
- Head = CoreAllocatePoolPages (PoolType, NoPages, Granularity);
+ Head = CoreAllocatePoolPagesI (PoolType, NoPages, Granularity);
goto Done;
}
@@ -383,7 +402,7 @@ CoreAllocatePoolI (
//
// Get another page
//
- NewPage = CoreAllocatePoolPages(PoolType, EFI_SIZE_TO_PAGES (Granularity), Granularity);
+ NewPage = CoreAllocatePoolPagesI (PoolType, EFI_SIZE_TO_PAGES (Granularity), Granularity);
if (NewPage == NULL) {
goto Done;
}
@@ -486,9 +505,9 @@ CoreInternalFreePool (
return EFI_INVALID_PARAMETER;
}
- CoreAcquireMemoryLock ();
+ CoreAcquireLock (&mPoolMemoryLock);
Status = CoreFreePoolI (Buffer, PoolType);
- CoreReleaseMemoryLock ();
+ CoreReleaseLock (&mPoolMemoryLock);
return Status;
}
@@ -525,6 +544,19 @@ CoreFreePool (
return Status;
}
+STATIC
+VOID
+CoreFreePoolPagesI (
+ IN EFI_MEMORY_TYPE PoolType,
+ IN EFI_PHYSICAL_ADDRESS Memory,
+ IN UINTN NoPages
+ )
+{
+ CoreAcquireMemoryLock ();
+ CoreFreePoolPages (Memory, NoPages);
+ CoreReleaseMemoryLock ();
+}
+
/**
Internal function to free a pool entry.
Caller must have the memory lock held
@@ -573,7 +605,7 @@ CoreFreePoolI (
//
ASSERT (Tail->Signature == POOL_TAIL_SIGNATURE);
ASSERT (Head->Size == Tail->Size);
- ASSERT_LOCKED (&gMemoryLock);
+ ASSERT_LOCKED (&mPoolMemoryLock);
if (Tail->Signature != POOL_TAIL_SIGNATURE) {
return EFI_INVALID_PARAMETER;
@@ -624,7 +656,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);
+ CoreFreePoolPagesI (Pool->MemoryType, (EFI_PHYSICAL_ADDRESS) (UINTN) Head, NoPages);
} else {
@@ -680,7 +712,8 @@ CoreFreePoolI (
//
// Free the page
//
- CoreFreePoolPages ((EFI_PHYSICAL_ADDRESS) (UINTN)NewPage, EFI_SIZE_TO_PAGES (Granularity));
+ CoreFreePoolPagesI (Pool->MemoryType, (EFI_PHYSICAL_ADDRESS) (UINTN)NewPage,
+ EFI_SIZE_TO_PAGES (Granularity));
}
}
}
--
2.7.4
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v3 5/6] MdeModulePkg: define PCD for DXE memory protection policy
2017-02-26 18:29 [PATCH v3 0/6] RFC: increased memory protection Ard Biesheuvel
` (3 preceding siblings ...)
2017-02-26 18:30 ` [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock for pool allocations Ard Biesheuvel
@ 2017-02-26 18:30 ` Ard Biesheuvel
2017-02-26 18:30 ` [PATCH v3 6/6] MdeModulePkg/DxeCore: implement " Ard Biesheuvel
` (2 subsequent siblings)
7 siblings, 0 replies; 21+ messages in thread
From: Ard Biesheuvel @ 2017-02-26 18:30 UTC (permalink / raw)
To: edk2-devel, jiewen.yao, leif.lindholm
Cc: afish, michael.d.kinney, liming.gao, 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 | 31 ++++++++++++++++++++
1 file changed, 31 insertions(+)
diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index 426634fbbd4d..fb6a39c9e354 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -1107,6 +1107,37 @@ [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>
+ #
+ # 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
+
## 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] 21+ messages in thread
* [PATCH v3 6/6] MdeModulePkg/DxeCore: implement memory protection policy
2017-02-26 18:29 [PATCH v3 0/6] RFC: increased memory protection Ard Biesheuvel
` (4 preceding siblings ...)
2017-02-26 18:30 ` [PATCH v3 5/6] MdeModulePkg: define PCD for DXE memory protection policy Ard Biesheuvel
@ 2017-02-26 18:30 ` Ard Biesheuvel
2017-02-27 6:46 ` Gao, Liming
2017-02-27 9:56 ` Laszlo Ersek
2017-02-27 5:19 ` [PATCH v3 0/6] RFC: increased memory protection Yao, Jiewen
2017-02-27 13:14 ` Laszlo Ersek
7 siblings, 2 replies; 21+ messages in thread
From: Ard Biesheuvel @ 2017-02-26 18:30 UTC (permalink / raw)
To: edk2-devel, jiewen.yao, leif.lindholm
Cc: afish, michael.d.kinney, liming.gao, 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.h | 24 ++
MdeModulePkg/Core/Dxe/DxeMain.inf | 1 +
MdeModulePkg/Core/Dxe/Mem/Page.c | 4 +
MdeModulePkg/Core/Dxe/Mem/Pool.c | 7 +
MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 306 +++++++++++++++++++-
5 files changed, 341 insertions(+), 1 deletion(-)
diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h b/MdeModulePkg/Core/Dxe/DxeMain.h
index b14be9a74d8e..5668c1f2d648 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain.h
+++ b/MdeModulePkg/Core/Dxe/DxeMain.h
@@ -2949,4 +2949,28 @@ MemoryProtectionExitBootServicesCallback (
VOID
);
+/**
+ 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()
+
+**/
+EFI_STATUS
+EFIAPI
+ApplyMemoryProtectionPolicy (
+ IN EFI_MEMORY_TYPE OldType,
+ IN EFI_MEMORY_TYPE NewType,
+ IN EFI_PHYSICAL_ADDRESS Memory,
+ IN UINT64 Length
+ );
+
#endif
diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf b/MdeModulePkg/Core/Dxe/DxeMain.inf
index 371e91cb0d7e..30d5984f7c1f 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.PcdDxeNxMemoryProtectionPolicy ## CONSUMES
# [Hob]
# RESOURCE_DESCRIPTOR ## CONSUMES
diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
index bda4f6397e91..86874906de58 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Page.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
@@ -1344,6 +1344,8 @@ CoreAllocatePages (
NULL
);
InstallMemoryAttributesTableOnMemoryAllocation (MemoryType);
+ ApplyMemoryProtectionPolicy (EfiConventionalMemory, MemoryType, *Memory,
+ EFI_PAGES_TO_SIZE (NumberOfPages));
}
return Status;
}
@@ -1460,6 +1462,8 @@ CoreFreePages (
NULL
);
InstallMemoryAttributesTableOnMemoryAllocation (MemoryType);
+ ApplyMemoryProtectionPolicy (MemoryType, EfiConventionalMemory, Memory,
+ EFI_PAGES_TO_SIZE (NumberOfPages));
}
return Status;
}
diff --git a/MdeModulePkg/Core/Dxe/Mem/Pool.c b/MdeModulePkg/Core/Dxe/Mem/Pool.c
index 410615e0dee9..63b9983b88b5 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Pool.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Pool.c
@@ -305,6 +305,10 @@ CoreAllocatePoolPagesI (
Buffer = CoreAllocatePoolPages (PoolType, NoPages, Granularity);
CoreReleaseMemoryLock ();
+ if (Buffer != NULL) {
+ ApplyMemoryProtectionPolicy (EfiConventionalMemory, PoolType,
+ (EFI_PHYSICAL_ADDRESS)(UINTN)Buffer, EFI_PAGES_TO_SIZE (NoPages));
+ }
return Buffer;
}
@@ -555,6 +559,9 @@ CoreFreePoolPagesI (
CoreAcquireMemoryLock ();
CoreFreePoolPages (Memory, NoPages);
CoreReleaseMemoryLock ();
+
+ ApplyMemoryProtectionPolicy (PoolType, EfiConventionalMemory,
+ (EFI_PHYSICAL_ADDRESS)(UINTN)Memory, EFI_PAGES_TO_SIZE (NoPages));
}
/**
diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
index 46d88463d417..f2a69a3d0df9 100644
--- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
+++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
@@ -64,6 +64,12 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
#define DO_NOT_PROTECT 0x00000000
#define PROTECT_IF_ALIGNED_ELSE_ALLOW 0x00000001
+#define MEMORY_TYPE_OS_RESERVED_MIN 0x80000000
+#define MEMORY_TYPE_OEM_RESERVED_MIN 0x70000000
+
+#define PREVIOUS_MEMORY_DESCRIPTOR(MemoryDescriptor, Size) \
+ ((EFI_MEMORY_DESCRIPTOR *)((UINT8 *)(MemoryDescriptor) - (Size)))
+
UINT32 mImageProtectionPolicy;
/**
@@ -647,6 +653,210 @@ UnprotectUefiImage (
}
/**
+ Return the EFI memory permission attribute associated with memory
+ type 'Type' under the configured DXE memory protection policy.
+**/
+STATIC
+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 (PcdDxeNxMemoryProtectionPolicy) & TestBit) != 0) {
+ return EFI_MEMORY_XP;
+ } else {
+ return 0;
+ }
+}
+
+/**
+ Sort memory map entries based upon PhysicalStart, from low to high.
+
+ @param MemoryMap A pointer to the buffer in which firmware places
+ the current memory map.
+ @param MemoryMapSize Size, in bytes, of the MemoryMap buffer.
+ @param DescriptorSize Size, in bytes, of an individual EFI_MEMORY_DESCRIPTOR.
+**/
+STATIC
+VOID
+SortMemoryMap (
+ IN OUT EFI_MEMORY_DESCRIPTOR *MemoryMap,
+ IN UINTN MemoryMapSize,
+ IN UINTN DescriptorSize
+ )
+{
+ EFI_MEMORY_DESCRIPTOR *MemoryMapEntry;
+ EFI_MEMORY_DESCRIPTOR *NextMemoryMapEntry;
+ EFI_MEMORY_DESCRIPTOR *MemoryMapEnd;
+ EFI_MEMORY_DESCRIPTOR TempMemoryMap;
+
+ MemoryMapEntry = MemoryMap;
+ NextMemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry, DescriptorSize);
+ MemoryMapEnd = (EFI_MEMORY_DESCRIPTOR *) ((UINT8 *) MemoryMap + MemoryMapSize);
+ while (MemoryMapEntry < MemoryMapEnd) {
+ while (NextMemoryMapEntry < MemoryMapEnd) {
+ if (MemoryMapEntry->PhysicalStart > NextMemoryMapEntry->PhysicalStart) {
+ CopyMem (&TempMemoryMap, MemoryMapEntry, sizeof(EFI_MEMORY_DESCRIPTOR));
+ CopyMem (MemoryMapEntry, NextMemoryMapEntry, sizeof(EFI_MEMORY_DESCRIPTOR));
+ CopyMem (NextMemoryMapEntry, &TempMemoryMap, sizeof(EFI_MEMORY_DESCRIPTOR));
+ }
+
+ NextMemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (NextMemoryMapEntry, DescriptorSize);
+ }
+
+ MemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry, DescriptorSize);
+ NextMemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry, DescriptorSize);
+ }
+}
+
+/**
+ Merge adjacent memory map entries if they use the same memory protection policy
+
+ @param[in, out] MemoryMap A pointer to the buffer in which firmware places
+ the current memory map.
+ @param[in, out] MemoryMapSize A pointer to the size, in bytes, of the
+ MemoryMap buffer. On input, this is the size of
+ the current memory map. On output,
+ it is the size of new memory map after merge.
+ @param[in] DescriptorSize Size, in bytes, of an individual EFI_MEMORY_DESCRIPTOR.
+**/
+STATIC
+VOID
+MergeMemoryMapForProtectionPolicy (
+ IN OUT EFI_MEMORY_DESCRIPTOR *MemoryMap,
+ IN OUT UINTN *MemoryMapSize,
+ IN UINTN DescriptorSize
+ )
+{
+ EFI_MEMORY_DESCRIPTOR *MemoryMapEntry;
+ EFI_MEMORY_DESCRIPTOR *MemoryMapEnd;
+ UINT64 MemoryBlockLength;
+ EFI_MEMORY_DESCRIPTOR *NewMemoryMapEntry;
+ EFI_MEMORY_DESCRIPTOR *NextMemoryMapEntry;
+ UINT64 Attributes;
+
+ SortMemoryMap (MemoryMap, *MemoryMapSize, DescriptorSize);
+
+ MemoryMapEntry = MemoryMap;
+ NewMemoryMapEntry = MemoryMap;
+ MemoryMapEnd = (EFI_MEMORY_DESCRIPTOR *) ((UINT8 *) MemoryMap + *MemoryMapSize);
+ while ((UINTN)MemoryMapEntry < (UINTN)MemoryMapEnd) {
+ CopyMem (NewMemoryMapEntry, MemoryMapEntry, sizeof(EFI_MEMORY_DESCRIPTOR));
+ NextMemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry, DescriptorSize);
+
+ do {
+ MemoryBlockLength = (UINT64) (EFI_PAGES_TO_SIZE((UINTN)MemoryMapEntry->NumberOfPages));
+ Attributes = GetPermissionAttributeForMemoryType (MemoryMapEntry->Type);
+
+ if (((UINTN)NextMemoryMapEntry < (UINTN)MemoryMapEnd) &&
+ Attributes == GetPermissionAttributeForMemoryType (NextMemoryMapEntry->Type) &&
+ ((MemoryMapEntry->PhysicalStart + MemoryBlockLength) == NextMemoryMapEntry->PhysicalStart)) {
+ MemoryMapEntry->NumberOfPages += NextMemoryMapEntry->NumberOfPages;
+ if (NewMemoryMapEntry != MemoryMapEntry) {
+ NewMemoryMapEntry->NumberOfPages += NextMemoryMapEntry->NumberOfPages;
+ }
+
+ NextMemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (NextMemoryMapEntry, DescriptorSize);
+ continue;
+ } else {
+ MemoryMapEntry = PREVIOUS_MEMORY_DESCRIPTOR (NextMemoryMapEntry, DescriptorSize);
+ break;
+ }
+ } while (TRUE);
+
+ MemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry, DescriptorSize);
+ NewMemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (NewMemoryMapEntry, DescriptorSize);
+ }
+
+ *MemoryMapSize = (UINTN)NewMemoryMapEntry - (UINTN)MemoryMap;
+
+ return ;
+}
+
+
+/**
+ Remove exec permissions from all regions whose type is identified by
+ PcdDxeNxMemoryProtectionPolicy
+**/
+STATIC
+VOID
+InitializeDxeNxMemoryProtectionPolicy (
+ VOID
+ )
+{
+ UINTN MemoryMapSize;
+ UINTN MapKey;
+ UINTN DescriptorSize;
+ UINT32 DescriptorVersion;
+ EFI_MEMORY_DESCRIPTOR *MemoryMap;
+ EFI_MEMORY_DESCRIPTOR *MemoryMapEntry;
+ EFI_MEMORY_DESCRIPTOR *MemoryMapEnd;
+ EFI_STATUS Status;
+ UINT64 Attributes;
+
+ //
+ // 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__));
+
+ MergeMemoryMapForProtectionPolicy (MemoryMap, &MemoryMapSize, DescriptorSize);
+
+ MemoryMapEntry = MemoryMap;
+ MemoryMapEnd = (EFI_MEMORY_DESCRIPTOR *) ((UINT8 *) MemoryMap + MemoryMapSize);
+ while ((UINTN) MemoryMapEntry < (UINTN) MemoryMapEnd) {
+
+ Attributes = GetPermissionAttributeForMemoryType (MemoryMapEntry->Type);
+ if (Attributes != 0) {
+ SetUefiImageMemoryAttributes (
+ MemoryMapEntry->PhysicalStart,
+ EFI_PAGES_TO_SIZE (MemoryMapEntry->NumberOfPages),
+ Attributes);
+ }
+ MemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry, DescriptorSize);
+ }
+ FreePool (MemoryMap);
+}
+
+
+/**
A notification for CPU_ARCH protocol.
@param[in] Event Event whose notification function is being invoked.
@@ -674,6 +884,17 @@ MemoryProtectionCpuArchProtocolNotify (
return;
}
+ //
+ // Apply the memory protection policy on non-BScode/RTcode regions.
+ //
+ if (PcdGet64 (PcdDxeNxMemoryProtectionPolicy) != 0) {
+ InitializeDxeNxMemoryProtectionPolicy ();
+ }
+
+ if (mImageProtectionPolicy == 0) {
+ return;
+ }
+
Status = gBS->LocateHandleBuffer (
ByProtocol,
&gEfiLoadedImageProtocolGuid,
@@ -753,7 +974,7 @@ CoreInitializeMemoryProtection (
mImageProtectionPolicy = PcdGet32(PcdImageProtectionPolicy);
- if (mImageProtectionPolicy != 0) {
+ if (mImageProtectionPolicy != 0 || PcdGet64 (PcdDxeNxMemoryProtectionPolicy) != 0) {
Status = CoreCreateEvent (
EVT_NOTIFY_SIGNAL,
TPL_CALLBACK,
@@ -775,3 +996,86 @@ CoreInitializeMemoryProtection (
}
return ;
}
+
+STATIC
+BOOLEAN
+IsInSmm (
+ VOID
+ )
+{
+ BOOLEAN InSmm;
+
+ InSmm = FALSE;
+ if (gSmmBase2 != NULL) {
+ gSmmBase2->InSmm (gSmmBase2, &InSmm);
+ }
+ return InSmm;
+}
+
+/**
+ 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 we are executing in SMM mode. No permission attributes
+ are updated in this case
+ @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()
+
+**/
+EFI_STATUS
+EFIAPI
+ApplyMemoryProtectionPolicy (
+ IN EFI_MEMORY_TYPE OldType,
+ IN EFI_MEMORY_TYPE NewType,
+ IN EFI_PHYSICAL_ADDRESS Memory,
+ IN UINT64 Length
+ )
+{
+ UINT64 OldAttributes;
+ UINT64 NewAttributes;
+
+ //
+ // The policy configured in PcdDxeNxMemoryProtectionPolicy
+ // does not apply to allocations performed in SMM mode.
+ //
+ if (IsInSmm ()) {
+ return EFI_SUCCESS;
+ }
+
+ //
+ // 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 (PcdGet64 (PcdDxeNxMemoryProtectionPolicy) == 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);
+}
--
2.7.4
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock for pool allocations
2017-02-26 18:30 ` [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock for pool allocations Ard Biesheuvel
@ 2017-02-26 19:52 ` Ard Biesheuvel
2017-02-27 1:56 ` Zeng, Star
2017-02-27 6:43 ` Gao, Liming
2 siblings, 0 replies; 21+ messages in thread
From: Ard Biesheuvel @ 2017-02-26 19:52 UTC (permalink / raw)
To: edk2-devel@lists.01.org, Yao, Jiewen, Leif Lindholm
Cc: afish@apple.com, Kinney, Michael D, Gao, Liming, Laszlo Ersek,
Tian, Feng, Zeng, Star, Ard Biesheuvel
On 26 February 2017 at 18:30, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> In preparation of adding memory permission attribute management to
> the pool allocator, split off the locking of the pool metadata into
> a separate lock. This is an improvement in itself, given that pool
> allocation can only interfere with the page allocation bookkeeping
> if pool pages are allocated or released. But it is also required to
> ensure that the permission attribute management does not deadlock,
> given that it may trigger page table splits leading to additional
> page tables being allocated.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> MdeModulePkg/Core/Dxe/Mem/Pool.c | 53 ++++++++++++++++----
> 1 file changed, 43 insertions(+), 10 deletions(-)
>
> diff --git a/MdeModulePkg/Core/Dxe/Mem/Pool.c b/MdeModulePkg/Core/Dxe/Mem/Pool.c
> index 7afd2d312c1d..410615e0dee9 100644
> --- a/MdeModulePkg/Core/Dxe/Mem/Pool.c
> +++ b/MdeModulePkg/Core/Dxe/Mem/Pool.c
> @@ -15,6 +15,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> #include "DxeMain.h"
> #include "Imem.h"
>
> +STATIC EFI_LOCK mPoolMemoryLock = EFI_INITIALIZE_LOCK_VARIABLE (TPL_NOTIFY);
> +
> #define POOL_FREE_SIGNATURE SIGNATURE_32('p','f','r','0')
> typedef struct {
> UINT32 Signature;
> @@ -239,13 +241,13 @@ CoreInternalAllocatePool (
> //
> // Acquire the memory lock and make the allocation
> //
> - Status = CoreAcquireLockOrFail (&gMemoryLock);
> + Status = CoreAcquireLockOrFail (&mPoolMemoryLock);
> if (EFI_ERROR (Status)) {
> return EFI_OUT_OF_RESOURCES;
> }
>
> *Buffer = CoreAllocatePoolI (PoolType, Size);
> - CoreReleaseMemoryLock ();
> + CoreReleaseLock (&mPoolMemoryLock);
> return (*Buffer != NULL) ? EFI_SUCCESS : EFI_OUT_OF_RESOURCES;
> }
>
> @@ -289,6 +291,23 @@ CoreAllocatePool (
> return Status;
> }
>
> +STATIC
> +VOID *
> +CoreAllocatePoolPagesI (
> + IN EFI_MEMORY_TYPE PoolType,
> + IN UINTN NoPages,
> + IN UINTN Granularity
> + )
> +{
> + VOID *Buffer;
> +
> + CoreAcquireMemoryLock ();
This should probably be
EFI_STATUS Status;
Status = CoreAcquireLockOrFail (&gMemoryLock);
if (EFI_ERROR (Status)) {
return NULL;
}
to preserve the old behavior.
> + Buffer = CoreAllocatePoolPages (PoolType, NoPages, Granularity);
> + CoreReleaseMemoryLock ();
> +
> + return Buffer;
> +}
> +
> /**
> Internal function to allocate pool of a particular type.
> Caller must have the memory lock held
> @@ -317,7 +336,7 @@ CoreAllocatePoolI (
> UINTN NoPages;
> UINTN Granularity;
>
> - ASSERT_LOCKED (&gMemoryLock);
> + ASSERT_LOCKED (&mPoolMemoryLock);
>
> if (PoolType == EfiACPIReclaimMemory ||
> PoolType == EfiACPIMemoryNVS ||
> @@ -355,7 +374,7 @@ CoreAllocatePoolI (
> if (Index >= SIZE_TO_LIST (Granularity)) {
> NoPages = EFI_SIZE_TO_PAGES(Size) + EFI_SIZE_TO_PAGES (Granularity) - 1;
> NoPages &= ~(UINTN)(EFI_SIZE_TO_PAGES (Granularity) - 1);
> - Head = CoreAllocatePoolPages (PoolType, NoPages, Granularity);
> + Head = CoreAllocatePoolPagesI (PoolType, NoPages, Granularity);
> goto Done;
> }
>
> @@ -383,7 +402,7 @@ CoreAllocatePoolI (
> //
> // Get another page
> //
> - NewPage = CoreAllocatePoolPages(PoolType, EFI_SIZE_TO_PAGES (Granularity), Granularity);
> + NewPage = CoreAllocatePoolPagesI (PoolType, EFI_SIZE_TO_PAGES (Granularity), Granularity);
> if (NewPage == NULL) {
> goto Done;
> }
> @@ -486,9 +505,9 @@ CoreInternalFreePool (
> return EFI_INVALID_PARAMETER;
> }
>
> - CoreAcquireMemoryLock ();
> + CoreAcquireLock (&mPoolMemoryLock);
> Status = CoreFreePoolI (Buffer, PoolType);
> - CoreReleaseMemoryLock ();
> + CoreReleaseLock (&mPoolMemoryLock);
> return Status;
> }
>
> @@ -525,6 +544,19 @@ CoreFreePool (
> return Status;
> }
>
> +STATIC
> +VOID
> +CoreFreePoolPagesI (
> + IN EFI_MEMORY_TYPE PoolType,
> + IN EFI_PHYSICAL_ADDRESS Memory,
> + IN UINTN NoPages
> + )
> +{
> + CoreAcquireMemoryLock ();
> + CoreFreePoolPages (Memory, NoPages);
> + CoreReleaseMemoryLock ();
> +}
> +
> /**
> Internal function to free a pool entry.
> Caller must have the memory lock held
> @@ -573,7 +605,7 @@ CoreFreePoolI (
> //
> ASSERT (Tail->Signature == POOL_TAIL_SIGNATURE);
> ASSERT (Head->Size == Tail->Size);
> - ASSERT_LOCKED (&gMemoryLock);
> + ASSERT_LOCKED (&mPoolMemoryLock);
>
> if (Tail->Signature != POOL_TAIL_SIGNATURE) {
> return EFI_INVALID_PARAMETER;
> @@ -624,7 +656,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);
> + CoreFreePoolPagesI (Pool->MemoryType, (EFI_PHYSICAL_ADDRESS) (UINTN) Head, NoPages);
>
> } else {
>
> @@ -680,7 +712,8 @@ CoreFreePoolI (
> //
> // Free the page
> //
> - CoreFreePoolPages ((EFI_PHYSICAL_ADDRESS) (UINTN)NewPage, EFI_SIZE_TO_PAGES (Granularity));
> + CoreFreePoolPagesI (Pool->MemoryType, (EFI_PHYSICAL_ADDRESS) (UINTN)NewPage,
> + EFI_SIZE_TO_PAGES (Granularity));
> }
> }
> }
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock for pool allocations
2017-02-26 18:30 ` [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock for pool allocations Ard Biesheuvel
2017-02-26 19:52 ` Ard Biesheuvel
@ 2017-02-27 1:56 ` Zeng, Star
2017-02-27 8:15 ` Ard Biesheuvel
2017-02-27 6:43 ` Gao, Liming
2 siblings, 1 reply; 21+ messages in thread
From: Zeng, Star @ 2017-02-27 1:56 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel@lists.01.org, Yao, Jiewen,
leif.lindholm@linaro.org
Cc: Tian, Feng, afish@apple.com, Gao, Liming, Kinney, Michael D,
lersek@redhat.com, Zeng, Star
Minor comment: CoreFreePoolPagesI() has no need to have PoolType parameter, how about to remove it?
Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard Biesheuvel
Sent: Monday, February 27, 2017 2:30 AM
To: edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>; leif.lindholm@linaro.org
Cc: Tian, Feng <feng.tian@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>; afish@apple.com; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; lersek@redhat.com; Zeng, Star <star.zeng@intel.com>
Subject: [edk2] [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock for pool allocations
In preparation of adding memory permission attribute management to the pool allocator, split off the locking of the pool metadata into a separate lock. This is an improvement in itself, given that pool allocation can only interfere with the page allocation bookkeeping if pool pages are allocated or released. But it is also required to ensure that the permission attribute management does not deadlock, given that it may trigger page table splits leading to additional page tables being allocated.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
MdeModulePkg/Core/Dxe/Mem/Pool.c | 53 ++++++++++++++++----
1 file changed, 43 insertions(+), 10 deletions(-)
diff --git a/MdeModulePkg/Core/Dxe/Mem/Pool.c b/MdeModulePkg/Core/Dxe/Mem/Pool.c
index 7afd2d312c1d..410615e0dee9 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Pool.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Pool.c
@@ -15,6 +15,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
#include "DxeMain.h"
#include "Imem.h"
+STATIC EFI_LOCK mPoolMemoryLock = EFI_INITIALIZE_LOCK_VARIABLE
+(TPL_NOTIFY);
+
#define POOL_FREE_SIGNATURE SIGNATURE_32('p','f','r','0')
typedef struct {
UINT32 Signature;
@@ -239,13 +241,13 @@ CoreInternalAllocatePool (
//
// Acquire the memory lock and make the allocation
//
- Status = CoreAcquireLockOrFail (&gMemoryLock);
+ Status = CoreAcquireLockOrFail (&mPoolMemoryLock);
if (EFI_ERROR (Status)) {
return EFI_OUT_OF_RESOURCES;
}
*Buffer = CoreAllocatePoolI (PoolType, Size);
- CoreReleaseMemoryLock ();
+ CoreReleaseLock (&mPoolMemoryLock);
return (*Buffer != NULL) ? EFI_SUCCESS : EFI_OUT_OF_RESOURCES; }
@@ -289,6 +291,23 @@ CoreAllocatePool (
return Status;
}
+STATIC
+VOID *
+CoreAllocatePoolPagesI (
+ IN EFI_MEMORY_TYPE PoolType,
+ IN UINTN NoPages,
+ IN UINTN Granularity
+ )
+{
+ VOID *Buffer;
+
+ CoreAcquireMemoryLock ();
+ Buffer = CoreAllocatePoolPages (PoolType, NoPages, Granularity);
+ CoreReleaseMemoryLock ();
+
+ return Buffer;
+}
+
/**
Internal function to allocate pool of a particular type.
Caller must have the memory lock held @@ -317,7 +336,7 @@ CoreAllocatePoolI (
UINTN NoPages;
UINTN Granularity;
- ASSERT_LOCKED (&gMemoryLock);
+ ASSERT_LOCKED (&mPoolMemoryLock);
if (PoolType == EfiACPIReclaimMemory ||
PoolType == EfiACPIMemoryNVS ||
@@ -355,7 +374,7 @@ CoreAllocatePoolI (
if (Index >= SIZE_TO_LIST (Granularity)) {
NoPages = EFI_SIZE_TO_PAGES(Size) + EFI_SIZE_TO_PAGES (Granularity) - 1;
NoPages &= ~(UINTN)(EFI_SIZE_TO_PAGES (Granularity) - 1);
- Head = CoreAllocatePoolPages (PoolType, NoPages, Granularity);
+ Head = CoreAllocatePoolPagesI (PoolType, NoPages, Granularity);
goto Done;
}
@@ -383,7 +402,7 @@ CoreAllocatePoolI (
//
// Get another page
//
- NewPage = CoreAllocatePoolPages(PoolType, EFI_SIZE_TO_PAGES (Granularity), Granularity);
+ NewPage = CoreAllocatePoolPagesI (PoolType, EFI_SIZE_TO_PAGES
+ (Granularity), Granularity);
if (NewPage == NULL) {
goto Done;
}
@@ -486,9 +505,9 @@ CoreInternalFreePool (
return EFI_INVALID_PARAMETER;
}
- CoreAcquireMemoryLock ();
+ CoreAcquireLock (&mPoolMemoryLock);
Status = CoreFreePoolI (Buffer, PoolType);
- CoreReleaseMemoryLock ();
+ CoreReleaseLock (&mPoolMemoryLock);
return Status;
}
@@ -525,6 +544,19 @@ CoreFreePool (
return Status;
}
+STATIC
+VOID
+CoreFreePoolPagesI (
+ IN EFI_MEMORY_TYPE PoolType,
+ IN EFI_PHYSICAL_ADDRESS Memory,
+ IN UINTN NoPages
+ )
+{
+ CoreAcquireMemoryLock ();
+ CoreFreePoolPages (Memory, NoPages);
+ CoreReleaseMemoryLock ();
+}
+
/**
Internal function to free a pool entry.
Caller must have the memory lock held @@ -573,7 +605,7 @@ CoreFreePoolI (
//
ASSERT (Tail->Signature == POOL_TAIL_SIGNATURE);
ASSERT (Head->Size == Tail->Size);
- ASSERT_LOCKED (&gMemoryLock);
+ ASSERT_LOCKED (&mPoolMemoryLock);
if (Tail->Signature != POOL_TAIL_SIGNATURE) {
return EFI_INVALID_PARAMETER;
@@ -624,7 +656,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);
+ CoreFreePoolPagesI (Pool->MemoryType, (EFI_PHYSICAL_ADDRESS)
+ (UINTN) Head, NoPages);
} else {
@@ -680,7 +712,8 @@ CoreFreePoolI (
//
// Free the page
//
- CoreFreePoolPages ((EFI_PHYSICAL_ADDRESS) (UINTN)NewPage, EFI_SIZE_TO_PAGES (Granularity));
+ CoreFreePoolPagesI (Pool->MemoryType, (EFI_PHYSICAL_ADDRESS) (UINTN)NewPage,
+ EFI_SIZE_TO_PAGES (Granularity));
}
}
}
--
2.7.4
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v3 0/6] RFC: increased memory protection
2017-02-26 18:29 [PATCH v3 0/6] RFC: increased memory protection Ard Biesheuvel
` (5 preceding siblings ...)
2017-02-26 18:30 ` [PATCH v3 6/6] MdeModulePkg/DxeCore: implement " Ard Biesheuvel
@ 2017-02-27 5:19 ` Yao, Jiewen
2017-02-27 13:14 ` Laszlo Ersek
7 siblings, 0 replies; 21+ messages in thread
From: Yao, Jiewen @ 2017-02-27 5:19 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel@lists.01.org, leif.lindholm@linaro.org
Cc: afish@apple.com, Kinney, Michael D, Gao, Liming,
lersek@redhat.com, Tian, Feng, Zeng, Star
Thanks Ard.
I found V3 5/6 has typo below:
+ gEfiMdeModulePkgTokenSpaceGuid.PcdDxeMemoryProtectionPolicy|0x0000000|UINT64|0x00001048
It should be PcdDxeNxMemoryProtectionPolicy. Or I got build failure.
With above typo update, all series reviewed-by: Jiewen.yao@intel.com<mailto:Jiewen.yao@intel.com>
Regression Tested-by: Jiewen.yao@intel.com<mailto:Jiewen.yao@intel.com>
1) Default build (NX protection disable), boot Intel X86 system (X64 build) to UEFI Windows 10.
2) Default build (NX protection disable), boot Intel X86 system (IA32 build) to UEFI Shell.
Thank you
Yao Jiewen
> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Monday, February 27, 2017 2:30 AM
> To: edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>;
> leif.lindholm@linaro.org
> Cc: afish@apple.com; 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>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>
> Subject: [PATCH v3 0/6] 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 v2:
> - added patch to make EBC use EfiBootServicesCode pool allocations for thunks
> - redefine PCD according to Jiewen's feedback, including default value
> - use sorted memory map and merge adjacent entries with the same policy, to
> prevent unnecessary page table splitting
> - ignore policy when executing in SMM
> - refactor the logic for managing permission attributes of pool allocations
> - added some R-b's
>
> 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.
>
> Series can be found here:
> https://git.linaro.org/people/ard.biesheuvel/uefi-next.git/log/?h=memprot-tak
> e2
>
> Note that to test this properly, the default value of 0 should be changed
> to 0x7FD5, which applies non-exec permissions to everything except Efi*Code
> regions.
>
> Ard Biesheuvel (6):
> ArmPkg/CpuDxe: ignore attribute changes during SyncCacheConfig()
> MdeModulePkg/PeiCore: allocate BootServicesCode memory for PE/COFF
> images
> MdeModulePkg/EbcDxe: use EfiBootServicesCode memory for thunks
> MdeModulePkg/DxeCore: use separate lock for pool allocations
> 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.h | 24 ++
> MdeModulePkg/Core/Dxe/DxeMain.inf | 1 +
> MdeModulePkg/Core/Dxe/Mem/Page.c | 4 +
> MdeModulePkg/Core/Dxe/Mem/Pool.c | 60 +++-
> MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 306
> +++++++++++++++++++-
> MdeModulePkg/Core/Pei/Image/Image.c | 10 +-
> MdeModulePkg/MdeModulePkg.dec | 31 ++
> MdeModulePkg/Universal/EbcDxe/AArch64/EbcSupport.c | 2 +-
> MdeModulePkg/Universal/EbcDxe/EbcInt.c | 23 ++
> MdeModulePkg/Universal/EbcDxe/EbcInt.h | 14 +
> MdeModulePkg/Universal/EbcDxe/Ia32/EbcSupport.c | 2 +-
> MdeModulePkg/Universal/EbcDxe/Ipf/EbcSupport.c | 2 +-
> MdeModulePkg/Universal/EbcDxe/X64/EbcSupport.c | 2 +-
> 16 files changed, 471 insertions(+), 18 deletions(-)
>
> --
> 2.7.4
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock for pool allocations
2017-02-26 18:30 ` [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock for pool allocations Ard Biesheuvel
2017-02-26 19:52 ` Ard Biesheuvel
2017-02-27 1:56 ` Zeng, Star
@ 2017-02-27 6:43 ` Gao, Liming
2017-02-27 6:50 ` Zeng, Star
2 siblings, 1 reply; 21+ messages in thread
From: Gao, Liming @ 2017-02-27 6:43 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel@lists.01.org, Yao, Jiewen,
leif.lindholm@linaro.org
Cc: afish@apple.com, Kinney, Michael D, lersek@redhat.com, Tian, Feng,
Zeng, Star
Ard:
I agree to separate lock for pool allocations. I suggest you update CoreAllocatePoolPages() and CoreFreePoolPages() implementation by adding CoreAcquireMemoryLock() and CoreReleaseMemoryLock(). If so, you don't need to introduce new CoreAllocatePoolPagesI () and CoreFreePoolPagesI ().
Thanks
Liming
>-----Original Message-----
>From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>Sent: Monday, February 27, 2017 2:30 AM
>To: edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>;
>leif.lindholm@linaro.org
>Cc: afish@apple.com; 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>; Ard Biesheuvel
><ard.biesheuvel@linaro.org>
>Subject: [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock for pool
>allocations
>
>In preparation of adding memory permission attribute management to
>the pool allocator, split off the locking of the pool metadata into
>a separate lock. This is an improvement in itself, given that pool
>allocation can only interfere with the page allocation bookkeeping
>if pool pages are allocated or released. But it is also required to
>ensure that the permission attribute management does not deadlock,
>given that it may trigger page table splits leading to additional
>page tables being allocated.
>
>Contributed-under: TianoCore Contribution Agreement 1.0
>Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>---
> MdeModulePkg/Core/Dxe/Mem/Pool.c | 53 ++++++++++++++++----
> 1 file changed, 43 insertions(+), 10 deletions(-)
>
>diff --git a/MdeModulePkg/Core/Dxe/Mem/Pool.c
>b/MdeModulePkg/Core/Dxe/Mem/Pool.c
>index 7afd2d312c1d..410615e0dee9 100644
>--- a/MdeModulePkg/Core/Dxe/Mem/Pool.c
>+++ b/MdeModulePkg/Core/Dxe/Mem/Pool.c
>@@ -15,6 +15,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY
>KIND, EITHER EXPRESS OR IMPLIED.
> #include "DxeMain.h"
> #include "Imem.h"
>
>+STATIC EFI_LOCK mPoolMemoryLock = EFI_INITIALIZE_LOCK_VARIABLE
>(TPL_NOTIFY);
>+
> #define POOL_FREE_SIGNATURE SIGNATURE_32('p','f','r','0')
> typedef struct {
> UINT32 Signature;
>@@ -239,13 +241,13 @@ CoreInternalAllocatePool (
> //
> // Acquire the memory lock and make the allocation
> //
>- Status = CoreAcquireLockOrFail (&gMemoryLock);
>+ Status = CoreAcquireLockOrFail (&mPoolMemoryLock);
> if (EFI_ERROR (Status)) {
> return EFI_OUT_OF_RESOURCES;
> }
>
> *Buffer = CoreAllocatePoolI (PoolType, Size);
>- CoreReleaseMemoryLock ();
>+ CoreReleaseLock (&mPoolMemoryLock);
> return (*Buffer != NULL) ? EFI_SUCCESS : EFI_OUT_OF_RESOURCES;
> }
>
>@@ -289,6 +291,23 @@ CoreAllocatePool (
> return Status;
> }
>
>+STATIC
>+VOID *
>+CoreAllocatePoolPagesI (
>+ IN EFI_MEMORY_TYPE PoolType,
>+ IN UINTN NoPages,
>+ IN UINTN Granularity
>+ )
>+{
>+ VOID *Buffer;
>+
>+ CoreAcquireMemoryLock ();
>+ Buffer = CoreAllocatePoolPages (PoolType, NoPages, Granularity);
>+ CoreReleaseMemoryLock ();
>+
>+ return Buffer;
>+}
>+
> /**
> Internal function to allocate pool of a particular type.
> Caller must have the memory lock held
>@@ -317,7 +336,7 @@ CoreAllocatePoolI (
> UINTN NoPages;
> UINTN Granularity;
>
>- ASSERT_LOCKED (&gMemoryLock);
>+ ASSERT_LOCKED (&mPoolMemoryLock);
>
> if (PoolType == EfiACPIReclaimMemory ||
> PoolType == EfiACPIMemoryNVS ||
>@@ -355,7 +374,7 @@ CoreAllocatePoolI (
> if (Index >= SIZE_TO_LIST (Granularity)) {
> NoPages = EFI_SIZE_TO_PAGES(Size) + EFI_SIZE_TO_PAGES (Granularity) -
>1;
> NoPages &= ~(UINTN)(EFI_SIZE_TO_PAGES (Granularity) - 1);
>- Head = CoreAllocatePoolPages (PoolType, NoPages, Granularity);
>+ Head = CoreAllocatePoolPagesI (PoolType, NoPages, Granularity);
> goto Done;
> }
>
>@@ -383,7 +402,7 @@ CoreAllocatePoolI (
> //
> // Get another page
> //
>- NewPage = CoreAllocatePoolPages(PoolType, EFI_SIZE_TO_PAGES
>(Granularity), Granularity);
>+ NewPage = CoreAllocatePoolPagesI (PoolType, EFI_SIZE_TO_PAGES
>(Granularity), Granularity);
> if (NewPage == NULL) {
> goto Done;
> }
>@@ -486,9 +505,9 @@ CoreInternalFreePool (
> return EFI_INVALID_PARAMETER;
> }
>
>- CoreAcquireMemoryLock ();
>+ CoreAcquireLock (&mPoolMemoryLock);
> Status = CoreFreePoolI (Buffer, PoolType);
>- CoreReleaseMemoryLock ();
>+ CoreReleaseLock (&mPoolMemoryLock);
> return Status;
> }
>
>@@ -525,6 +544,19 @@ CoreFreePool (
> return Status;
> }
>
>+STATIC
>+VOID
>+CoreFreePoolPagesI (
>+ IN EFI_MEMORY_TYPE PoolType,
>+ IN EFI_PHYSICAL_ADDRESS Memory,
>+ IN UINTN NoPages
>+ )
>+{
>+ CoreAcquireMemoryLock ();
>+ CoreFreePoolPages (Memory, NoPages);
>+ CoreReleaseMemoryLock ();
>+}
>+
> /**
> Internal function to free a pool entry.
> Caller must have the memory lock held
>@@ -573,7 +605,7 @@ CoreFreePoolI (
> //
> ASSERT (Tail->Signature == POOL_TAIL_SIGNATURE);
> ASSERT (Head->Size == Tail->Size);
>- ASSERT_LOCKED (&gMemoryLock);
>+ ASSERT_LOCKED (&mPoolMemoryLock);
>
> if (Tail->Signature != POOL_TAIL_SIGNATURE) {
> return EFI_INVALID_PARAMETER;
>@@ -624,7 +656,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);
>+ CoreFreePoolPagesI (Pool->MemoryType, (EFI_PHYSICAL_ADDRESS)
>(UINTN) Head, NoPages);
>
> } else {
>
>@@ -680,7 +712,8 @@ CoreFreePoolI (
> //
> // Free the page
> //
>- CoreFreePoolPages ((EFI_PHYSICAL_ADDRESS) (UINTN)NewPage,
>EFI_SIZE_TO_PAGES (Granularity));
>+ CoreFreePoolPagesI (Pool->MemoryType, (EFI_PHYSICAL_ADDRESS)
>(UINTN)NewPage,
>+ EFI_SIZE_TO_PAGES (Granularity));
> }
> }
> }
>--
>2.7.4
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 2/6] MdeModulePkg/PeiCore: allocate BootServicesCode memory for PE/COFF images
2017-02-26 18:30 ` [PATCH v3 2/6] MdeModulePkg/PeiCore: allocate BootServicesCode memory for PE/COFF images Ard Biesheuvel
@ 2017-02-27 6:43 ` Gao, Liming
2017-02-27 8:13 ` Ard Biesheuvel
0 siblings, 1 reply; 21+ messages in thread
From: Gao, Liming @ 2017-02-27 6:43 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel@lists.01.org, Yao, Jiewen,
leif.lindholm@linaro.org
Cc: Tian, Feng, afish@apple.com, Kinney, Michael D, lersek@redhat.com,
Zeng, Star
Ard:
In line 128, there is another AllocatePages() to allocate memory to store the code. To be consistent, could you help also update it?
Thanks
Liming
>-----Original Message-----
>From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard
>Biesheuvel
>Sent: Monday, February 27, 2017 2:30 AM
>To: edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>;
>leif.lindholm@linaro.org
>Cc: Tian, Feng <feng.tian@intel.com>; Ard Biesheuvel
><ard.biesheuvel@linaro.org>; afish@apple.com; Gao, Liming
><liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
>lersek@redhat.com; Zeng, Star <star.zeng@intel.com>
>Subject: [edk2] [PATCH v3 2/6] MdeModulePkg/PeiCore: allocate
>BootServicesCode memory for PE/COFF images
>
>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>
>Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
>---
> 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
>
>_______________________________________________
>edk2-devel mailing list
>edk2-devel@lists.01.org
>https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 6/6] MdeModulePkg/DxeCore: implement memory protection policy
2017-02-26 18:30 ` [PATCH v3 6/6] MdeModulePkg/DxeCore: implement " Ard Biesheuvel
@ 2017-02-27 6:46 ` Gao, Liming
2017-02-27 9:56 ` Laszlo Ersek
1 sibling, 0 replies; 21+ messages in thread
From: Gao, Liming @ 2017-02-27 6:46 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel@lists.01.org, Yao, Jiewen,
leif.lindholm@linaro.org
Cc: afish@apple.com, Kinney, Michael D, lersek@redhat.com, Tian, Feng,
Zeng, Star
Ard:
I have minor comment. GetPermissionAttributeForMemoryType() function header comment doesn't match its definition, and IsInSmm() has no function header.
Thanks
Liming
>-----Original Message-----
>From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>Sent: Monday, February 27, 2017 2:30 AM
>To: edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>;
>leif.lindholm@linaro.org
>Cc: afish@apple.com; 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>; Ard Biesheuvel
><ard.biesheuvel@linaro.org>
>Subject: [PATCH v3 6/6] MdeModulePkg/DxeCore: implement memory
>protection policy
>
>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.h | 24 ++
> MdeModulePkg/Core/Dxe/DxeMain.inf | 1 +
> MdeModulePkg/Core/Dxe/Mem/Page.c | 4 +
> MdeModulePkg/Core/Dxe/Mem/Pool.c | 7 +
> MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 306
>+++++++++++++++++++-
> 5 files changed, 341 insertions(+), 1 deletion(-)
>
>diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h
>b/MdeModulePkg/Core/Dxe/DxeMain.h
>index b14be9a74d8e..5668c1f2d648 100644
>--- a/MdeModulePkg/Core/Dxe/DxeMain.h
>+++ b/MdeModulePkg/Core/Dxe/DxeMain.h
>@@ -2949,4 +2949,28 @@ MemoryProtectionExitBootServicesCallback (
> VOID
> );
>
>+/**
>+ 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()
>+
>+**/
>+EFI_STATUS
>+EFIAPI
>+ApplyMemoryProtectionPolicy (
>+ IN EFI_MEMORY_TYPE OldType,
>+ IN EFI_MEMORY_TYPE NewType,
>+ IN EFI_PHYSICAL_ADDRESS Memory,
>+ IN UINT64 Length
>+ );
>+
> #endif
>diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf
>b/MdeModulePkg/Core/Dxe/DxeMain.inf
>index 371e91cb0d7e..30d5984f7c1f 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.PcdDxeNxMemoryProtectionPolicy
>## CONSUMES
>
> # [Hob]
> # RESOURCE_DESCRIPTOR ## CONSUMES
>diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c
>b/MdeModulePkg/Core/Dxe/Mem/Page.c
>index bda4f6397e91..86874906de58 100644
>--- a/MdeModulePkg/Core/Dxe/Mem/Page.c
>+++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
>@@ -1344,6 +1344,8 @@ CoreAllocatePages (
> NULL
> );
> InstallMemoryAttributesTableOnMemoryAllocation (MemoryType);
>+ ApplyMemoryProtectionPolicy (EfiConventionalMemory, MemoryType,
>*Memory,
>+ EFI_PAGES_TO_SIZE (NumberOfPages));
> }
> return Status;
> }
>@@ -1460,6 +1462,8 @@ CoreFreePages (
> NULL
> );
> InstallMemoryAttributesTableOnMemoryAllocation (MemoryType);
>+ ApplyMemoryProtectionPolicy (MemoryType, EfiConventionalMemory,
>Memory,
>+ EFI_PAGES_TO_SIZE (NumberOfPages));
> }
> return Status;
> }
>diff --git a/MdeModulePkg/Core/Dxe/Mem/Pool.c
>b/MdeModulePkg/Core/Dxe/Mem/Pool.c
>index 410615e0dee9..63b9983b88b5 100644
>--- a/MdeModulePkg/Core/Dxe/Mem/Pool.c
>+++ b/MdeModulePkg/Core/Dxe/Mem/Pool.c
>@@ -305,6 +305,10 @@ CoreAllocatePoolPagesI (
> Buffer = CoreAllocatePoolPages (PoolType, NoPages, Granularity);
> CoreReleaseMemoryLock ();
>
>+ if (Buffer != NULL) {
>+ ApplyMemoryProtectionPolicy (EfiConventionalMemory, PoolType,
>+ (EFI_PHYSICAL_ADDRESS)(UINTN)Buffer, EFI_PAGES_TO_SIZE
>(NoPages));
>+ }
> return Buffer;
> }
>
>@@ -555,6 +559,9 @@ CoreFreePoolPagesI (
> CoreAcquireMemoryLock ();
> CoreFreePoolPages (Memory, NoPages);
> CoreReleaseMemoryLock ();
>+
>+ ApplyMemoryProtectionPolicy (PoolType, EfiConventionalMemory,
>+ (EFI_PHYSICAL_ADDRESS)(UINTN)Memory, EFI_PAGES_TO_SIZE
>(NoPages));
> }
>
> /**
>diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
>b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
>index 46d88463d417..f2a69a3d0df9 100644
>--- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
>+++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
>@@ -64,6 +64,12 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY
>KIND, EITHER EXPRESS OR IMPLIED.
> #define DO_NOT_PROTECT 0x00000000
> #define PROTECT_IF_ALIGNED_ELSE_ALLOW 0x00000001
>
>+#define MEMORY_TYPE_OS_RESERVED_MIN 0x80000000
>+#define MEMORY_TYPE_OEM_RESERVED_MIN 0x70000000
>+
>+#define PREVIOUS_MEMORY_DESCRIPTOR(MemoryDescriptor, Size) \
>+ ((EFI_MEMORY_DESCRIPTOR *)((UINT8 *)(MemoryDescriptor) - (Size)))
>+
> UINT32 mImageProtectionPolicy;
>
> /**
>@@ -647,6 +653,210 @@ UnprotectUefiImage (
> }
>
> /**
>+ Return the EFI memory permission attribute associated with memory
>+ type 'Type' under the configured DXE memory protection policy.
>+**/
>+STATIC
>+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 (PcdDxeNxMemoryProtectionPolicy) & TestBit) != 0) {
>+ return EFI_MEMORY_XP;
>+ } else {
>+ return 0;
>+ }
>+}
>+
>+/**
>+ Sort memory map entries based upon PhysicalStart, from low to high.
>+
>+ @param MemoryMap A pointer to the buffer in which firmware
>places
>+ the current memory map.
>+ @param MemoryMapSize Size, in bytes, of the MemoryMap buffer.
>+ @param DescriptorSize Size, in bytes, of an individual
>EFI_MEMORY_DESCRIPTOR.
>+**/
>+STATIC
>+VOID
>+SortMemoryMap (
>+ IN OUT EFI_MEMORY_DESCRIPTOR *MemoryMap,
>+ IN UINTN MemoryMapSize,
>+ IN UINTN DescriptorSize
>+ )
>+{
>+ EFI_MEMORY_DESCRIPTOR *MemoryMapEntry;
>+ EFI_MEMORY_DESCRIPTOR *NextMemoryMapEntry;
>+ EFI_MEMORY_DESCRIPTOR *MemoryMapEnd;
>+ EFI_MEMORY_DESCRIPTOR TempMemoryMap;
>+
>+ MemoryMapEntry = MemoryMap;
>+ NextMemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry,
>DescriptorSize);
>+ MemoryMapEnd = (EFI_MEMORY_DESCRIPTOR *) ((UINT8 *) MemoryMap
>+ MemoryMapSize);
>+ while (MemoryMapEntry < MemoryMapEnd) {
>+ while (NextMemoryMapEntry < MemoryMapEnd) {
>+ if (MemoryMapEntry->PhysicalStart > NextMemoryMapEntry-
>>PhysicalStart) {
>+ CopyMem (&TempMemoryMap, MemoryMapEntry,
>sizeof(EFI_MEMORY_DESCRIPTOR));
>+ CopyMem (MemoryMapEntry, NextMemoryMapEntry,
>sizeof(EFI_MEMORY_DESCRIPTOR));
>+ CopyMem (NextMemoryMapEntry, &TempMemoryMap,
>sizeof(EFI_MEMORY_DESCRIPTOR));
>+ }
>+
>+ NextMemoryMapEntry = NEXT_MEMORY_DESCRIPTOR
>(NextMemoryMapEntry, DescriptorSize);
>+ }
>+
>+ MemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry,
>DescriptorSize);
>+ NextMemoryMapEntry = NEXT_MEMORY_DESCRIPTOR
>(MemoryMapEntry, DescriptorSize);
>+ }
>+}
>+
>+/**
>+ Merge adjacent memory map entries if they use the same memory
>protection policy
>+
>+ @param[in, out] MemoryMap A pointer to the buffer in which
>firmware places
>+ the current memory map.
>+ @param[in, out] MemoryMapSize A pointer to the size, in bytes, of
>the
>+ MemoryMap buffer. On input, this is the size of
>+ the current memory map. On output,
>+ it is the size of new memory map after merge.
>+ @param[in] DescriptorSize Size, in bytes, of an individual
>EFI_MEMORY_DESCRIPTOR.
>+**/
>+STATIC
>+VOID
>+MergeMemoryMapForProtectionPolicy (
>+ IN OUT EFI_MEMORY_DESCRIPTOR *MemoryMap,
>+ IN OUT UINTN *MemoryMapSize,
>+ IN UINTN DescriptorSize
>+ )
>+{
>+ EFI_MEMORY_DESCRIPTOR *MemoryMapEntry;
>+ EFI_MEMORY_DESCRIPTOR *MemoryMapEnd;
>+ UINT64 MemoryBlockLength;
>+ EFI_MEMORY_DESCRIPTOR *NewMemoryMapEntry;
>+ EFI_MEMORY_DESCRIPTOR *NextMemoryMapEntry;
>+ UINT64 Attributes;
>+
>+ SortMemoryMap (MemoryMap, *MemoryMapSize, DescriptorSize);
>+
>+ MemoryMapEntry = MemoryMap;
>+ NewMemoryMapEntry = MemoryMap;
>+ MemoryMapEnd = (EFI_MEMORY_DESCRIPTOR *) ((UINT8 *) MemoryMap
>+ *MemoryMapSize);
>+ while ((UINTN)MemoryMapEntry < (UINTN)MemoryMapEnd) {
>+ CopyMem (NewMemoryMapEntry, MemoryMapEntry,
>sizeof(EFI_MEMORY_DESCRIPTOR));
>+ NextMemoryMapEntry = NEXT_MEMORY_DESCRIPTOR
>(MemoryMapEntry, DescriptorSize);
>+
>+ do {
>+ MemoryBlockLength = (UINT64)
>(EFI_PAGES_TO_SIZE((UINTN)MemoryMapEntry->NumberOfPages));
>+ Attributes = GetPermissionAttributeForMemoryType
>(MemoryMapEntry->Type);
>+
>+ if (((UINTN)NextMemoryMapEntry < (UINTN)MemoryMapEnd) &&
>+ Attributes == GetPermissionAttributeForMemoryType
>(NextMemoryMapEntry->Type) &&
>+ ((MemoryMapEntry->PhysicalStart + MemoryBlockLength) ==
>NextMemoryMapEntry->PhysicalStart)) {
>+ MemoryMapEntry->NumberOfPages += NextMemoryMapEntry-
>>NumberOfPages;
>+ if (NewMemoryMapEntry != MemoryMapEntry) {
>+ NewMemoryMapEntry->NumberOfPages += NextMemoryMapEntry-
>>NumberOfPages;
>+ }
>+
>+ NextMemoryMapEntry = NEXT_MEMORY_DESCRIPTOR
>(NextMemoryMapEntry, DescriptorSize);
>+ continue;
>+ } else {
>+ MemoryMapEntry = PREVIOUS_MEMORY_DESCRIPTOR
>(NextMemoryMapEntry, DescriptorSize);
>+ break;
>+ }
>+ } while (TRUE);
>+
>+ MemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry,
>DescriptorSize);
>+ NewMemoryMapEntry = NEXT_MEMORY_DESCRIPTOR
>(NewMemoryMapEntry, DescriptorSize);
>+ }
>+
>+ *MemoryMapSize = (UINTN)NewMemoryMapEntry -
>(UINTN)MemoryMap;
>+
>+ return ;
>+}
>+
>+
>+/**
>+ Remove exec permissions from all regions whose type is identified by
>+ PcdDxeNxMemoryProtectionPolicy
>+**/
>+STATIC
>+VOID
>+InitializeDxeNxMemoryProtectionPolicy (
>+ VOID
>+ )
>+{
>+ UINTN MemoryMapSize;
>+ UINTN MapKey;
>+ UINTN DescriptorSize;
>+ UINT32 DescriptorVersion;
>+ EFI_MEMORY_DESCRIPTOR *MemoryMap;
>+ EFI_MEMORY_DESCRIPTOR *MemoryMapEntry;
>+ EFI_MEMORY_DESCRIPTOR *MemoryMapEnd;
>+ EFI_STATUS Status;
>+ UINT64 Attributes;
>+
>+ //
>+ // 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__));
>+
>+ MergeMemoryMapForProtectionPolicy (MemoryMap, &MemoryMapSize,
>DescriptorSize);
>+
>+ MemoryMapEntry = MemoryMap;
>+ MemoryMapEnd = (EFI_MEMORY_DESCRIPTOR *) ((UINT8 *) MemoryMap
>+ MemoryMapSize);
>+ while ((UINTN) MemoryMapEntry < (UINTN) MemoryMapEnd) {
>+
>+ Attributes = GetPermissionAttributeForMemoryType (MemoryMapEntry-
>>Type);
>+ if (Attributes != 0) {
>+ SetUefiImageMemoryAttributes (
>+ MemoryMapEntry->PhysicalStart,
>+ EFI_PAGES_TO_SIZE (MemoryMapEntry->NumberOfPages),
>+ Attributes);
>+ }
>+ MemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry,
>DescriptorSize);
>+ }
>+ FreePool (MemoryMap);
>+}
>+
>+
>+/**
> A notification for CPU_ARCH protocol.
>
> @param[in] Event Event whose notification function is being
>invoked.
>@@ -674,6 +884,17 @@ MemoryProtectionCpuArchProtocolNotify (
> return;
> }
>
>+ //
>+ // Apply the memory protection policy on non-BScode/RTcode regions.
>+ //
>+ if (PcdGet64 (PcdDxeNxMemoryProtectionPolicy) != 0) {
>+ InitializeDxeNxMemoryProtectionPolicy ();
>+ }
>+
>+ if (mImageProtectionPolicy == 0) {
>+ return;
>+ }
>+
> Status = gBS->LocateHandleBuffer (
> ByProtocol,
> &gEfiLoadedImageProtocolGuid,
>@@ -753,7 +974,7 @@ CoreInitializeMemoryProtection (
>
> mImageProtectionPolicy = PcdGet32(PcdImageProtectionPolicy);
>
>- if (mImageProtectionPolicy != 0) {
>+ if (mImageProtectionPolicy != 0 || PcdGet64
>(PcdDxeNxMemoryProtectionPolicy) != 0) {
> Status = CoreCreateEvent (
> EVT_NOTIFY_SIGNAL,
> TPL_CALLBACK,
>@@ -775,3 +996,86 @@ CoreInitializeMemoryProtection (
> }
> return ;
> }
>+
>+STATIC
>+BOOLEAN
>+IsInSmm (
>+ VOID
>+ )
>+{
>+ BOOLEAN InSmm;
>+
>+ InSmm = FALSE;
>+ if (gSmmBase2 != NULL) {
>+ gSmmBase2->InSmm (gSmmBase2, &InSmm);
>+ }
>+ return InSmm;
>+}
>+
>+/**
>+ 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 we are executing in SMM mode. No permission
>attributes
>+ are updated in this case
>+ @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()
>+
>+**/
>+EFI_STATUS
>+EFIAPI
>+ApplyMemoryProtectionPolicy (
>+ IN EFI_MEMORY_TYPE OldType,
>+ IN EFI_MEMORY_TYPE NewType,
>+ IN EFI_PHYSICAL_ADDRESS Memory,
>+ IN UINT64 Length
>+ )
>+{
>+ UINT64 OldAttributes;
>+ UINT64 NewAttributes;
>+
>+ //
>+ // The policy configured in PcdDxeNxMemoryProtectionPolicy
>+ // does not apply to allocations performed in SMM mode.
>+ //
>+ if (IsInSmm ()) {
>+ return EFI_SUCCESS;
>+ }
>+
>+ //
>+ // 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 (PcdGet64 (PcdDxeNxMemoryProtectionPolicy) == 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);
>+}
>--
>2.7.4
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock for pool allocations
2017-02-27 6:43 ` Gao, Liming
@ 2017-02-27 6:50 ` Zeng, Star
2017-02-27 8:15 ` Ard Biesheuvel
0 siblings, 1 reply; 21+ messages in thread
From: Zeng, Star @ 2017-02-27 6:50 UTC (permalink / raw)
To: Gao, Liming, Ard Biesheuvel, edk2-devel@lists.01.org, Yao, Jiewen,
leif.lindholm@linaro.org
Cc: afish@apple.com, Kinney, Michael D, lersek@redhat.com, Tian, Feng,
Zeng, Star
CoreAllocatePoolPages() could not be updated simply by adding CoreAcquireMemoryLock() and CoreReleaseMemoryLock(), it is also used by AllocateMemoryMapEntry() with the lock locked.
Thanks,
Star
-----Original Message-----
From: Gao, Liming
Sent: Monday, February 27, 2017 2:43 PM
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>; leif.lindholm@linaro.org
Cc: afish@apple.com; Kinney, Michael D <michael.d.kinney@intel.com>; lersek@redhat.com; Tian, Feng <feng.tian@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: RE: [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock for pool allocations
Ard:
I agree to separate lock for pool allocations. I suggest you update CoreAllocatePoolPages() and CoreFreePoolPages() implementation by adding CoreAcquireMemoryLock() and CoreReleaseMemoryLock(). If so, you don't need to introduce new CoreAllocatePoolPagesI () and CoreFreePoolPagesI ().
Thanks
Liming
>-----Original Message-----
>From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>Sent: Monday, February 27, 2017 2:30 AM
>To: edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>;
>leif.lindholm@linaro.org
>Cc: afish@apple.com; 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>; Ard Biesheuvel
><ard.biesheuvel@linaro.org>
>Subject: [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock for
>pool allocations
>
>In preparation of adding memory permission attribute management to the
>pool allocator, split off the locking of the pool metadata into a
>separate lock. This is an improvement in itself, given that pool
>allocation can only interfere with the page allocation bookkeeping if
>pool pages are allocated or released. But it is also required to ensure
>that the permission attribute management does not deadlock, given that
>it may trigger page table splits leading to additional page tables
>being allocated.
>
>Contributed-under: TianoCore Contribution Agreement 1.0
>Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>---
> MdeModulePkg/Core/Dxe/Mem/Pool.c | 53 ++++++++++++++++----
> 1 file changed, 43 insertions(+), 10 deletions(-)
>
>diff --git a/MdeModulePkg/Core/Dxe/Mem/Pool.c
>b/MdeModulePkg/Core/Dxe/Mem/Pool.c
>index 7afd2d312c1d..410615e0dee9 100644
>--- a/MdeModulePkg/Core/Dxe/Mem/Pool.c
>+++ b/MdeModulePkg/Core/Dxe/Mem/Pool.c
>@@ -15,6 +15,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,
>EITHER EXPRESS OR IMPLIED.
> #include "DxeMain.h"
> #include "Imem.h"
>
>+STATIC EFI_LOCK mPoolMemoryLock = EFI_INITIALIZE_LOCK_VARIABLE
>(TPL_NOTIFY);
>+
> #define POOL_FREE_SIGNATURE SIGNATURE_32('p','f','r','0')
> typedef struct {
> UINT32 Signature;
>@@ -239,13 +241,13 @@ CoreInternalAllocatePool (
> //
> // Acquire the memory lock and make the allocation
> //
>- Status = CoreAcquireLockOrFail (&gMemoryLock);
>+ Status = CoreAcquireLockOrFail (&mPoolMemoryLock);
> if (EFI_ERROR (Status)) {
> return EFI_OUT_OF_RESOURCES;
> }
>
> *Buffer = CoreAllocatePoolI (PoolType, Size);
>- CoreReleaseMemoryLock ();
>+ CoreReleaseLock (&mPoolMemoryLock);
> return (*Buffer != NULL) ? EFI_SUCCESS : EFI_OUT_OF_RESOURCES; }
>
>@@ -289,6 +291,23 @@ CoreAllocatePool (
> return Status;
> }
>
>+STATIC
>+VOID *
>+CoreAllocatePoolPagesI (
>+ IN EFI_MEMORY_TYPE PoolType,
>+ IN UINTN NoPages,
>+ IN UINTN Granularity
>+ )
>+{
>+ VOID *Buffer;
>+
>+ CoreAcquireMemoryLock ();
>+ Buffer = CoreAllocatePoolPages (PoolType, NoPages, Granularity);
>+ CoreReleaseMemoryLock ();
>+
>+ return Buffer;
>+}
>+
> /**
> Internal function to allocate pool of a particular type.
> Caller must have the memory lock held @@ -317,7 +336,7 @@
>CoreAllocatePoolI (
> UINTN NoPages;
> UINTN Granularity;
>
>- ASSERT_LOCKED (&gMemoryLock);
>+ ASSERT_LOCKED (&mPoolMemoryLock);
>
> if (PoolType == EfiACPIReclaimMemory ||
> PoolType == EfiACPIMemoryNVS ||
>@@ -355,7 +374,7 @@ CoreAllocatePoolI (
> if (Index >= SIZE_TO_LIST (Granularity)) {
> NoPages = EFI_SIZE_TO_PAGES(Size) + EFI_SIZE_TO_PAGES
>(Granularity) - 1;
> NoPages &= ~(UINTN)(EFI_SIZE_TO_PAGES (Granularity) - 1);
>- Head = CoreAllocatePoolPages (PoolType, NoPages, Granularity);
>+ Head = CoreAllocatePoolPagesI (PoolType, NoPages, Granularity);
> goto Done;
> }
>
>@@ -383,7 +402,7 @@ CoreAllocatePoolI (
> //
> // Get another page
> //
>- NewPage = CoreAllocatePoolPages(PoolType, EFI_SIZE_TO_PAGES
>(Granularity), Granularity);
>+ NewPage = CoreAllocatePoolPagesI (PoolType, EFI_SIZE_TO_PAGES
>(Granularity), Granularity);
> if (NewPage == NULL) {
> goto Done;
> }
>@@ -486,9 +505,9 @@ CoreInternalFreePool (
> return EFI_INVALID_PARAMETER;
> }
>
>- CoreAcquireMemoryLock ();
>+ CoreAcquireLock (&mPoolMemoryLock);
> Status = CoreFreePoolI (Buffer, PoolType);
>- CoreReleaseMemoryLock ();
>+ CoreReleaseLock (&mPoolMemoryLock);
> return Status;
> }
>
>@@ -525,6 +544,19 @@ CoreFreePool (
> return Status;
> }
>
>+STATIC
>+VOID
>+CoreFreePoolPagesI (
>+ IN EFI_MEMORY_TYPE PoolType,
>+ IN EFI_PHYSICAL_ADDRESS Memory,
>+ IN UINTN NoPages
>+ )
>+{
>+ CoreAcquireMemoryLock ();
>+ CoreFreePoolPages (Memory, NoPages);
>+ CoreReleaseMemoryLock ();
>+}
>+
> /**
> Internal function to free a pool entry.
> Caller must have the memory lock held @@ -573,7 +605,7 @@
>CoreFreePoolI (
> //
> ASSERT (Tail->Signature == POOL_TAIL_SIGNATURE);
> ASSERT (Head->Size == Tail->Size);
>- ASSERT_LOCKED (&gMemoryLock);
>+ ASSERT_LOCKED (&mPoolMemoryLock);
>
> if (Tail->Signature != POOL_TAIL_SIGNATURE) {
> return EFI_INVALID_PARAMETER;
>@@ -624,7 +656,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);
>+ CoreFreePoolPagesI (Pool->MemoryType, (EFI_PHYSICAL_ADDRESS)
>(UINTN) Head, NoPages);
>
> } else {
>
>@@ -680,7 +712,8 @@ CoreFreePoolI (
> //
> // Free the page
> //
>- CoreFreePoolPages ((EFI_PHYSICAL_ADDRESS) (UINTN)NewPage,
>EFI_SIZE_TO_PAGES (Granularity));
>+ CoreFreePoolPagesI (Pool->MemoryType, (EFI_PHYSICAL_ADDRESS)
>(UINTN)NewPage,
>+ EFI_SIZE_TO_PAGES (Granularity));
> }
> }
> }
>--
>2.7.4
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 2/6] MdeModulePkg/PeiCore: allocate BootServicesCode memory for PE/COFF images
2017-02-27 6:43 ` Gao, Liming
@ 2017-02-27 8:13 ` Ard Biesheuvel
0 siblings, 0 replies; 21+ messages in thread
From: Ard Biesheuvel @ 2017-02-27 8:13 UTC (permalink / raw)
To: Gao, Liming
Cc: edk2-devel@lists.01.org, Yao, Jiewen, leif.lindholm@linaro.org,
Tian, Feng, afish@apple.com, Kinney, Michael D, lersek@redhat.com,
Zeng, Star
On 27 February 2017 at 06:43, Gao, Liming <liming.gao@intel.com> wrote:
> Ard:
> In line 128, there is another AllocatePages() to allocate memory to store the code. To be consistent, could you help also update it?
>
OK
>>-----Original Message-----
>>From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard
>>Biesheuvel
>>Sent: Monday, February 27, 2017 2:30 AM
>>To: edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>;
>>leif.lindholm@linaro.org
>>Cc: Tian, Feng <feng.tian@intel.com>; Ard Biesheuvel
>><ard.biesheuvel@linaro.org>; afish@apple.com; Gao, Liming
>><liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
>>lersek@redhat.com; Zeng, Star <star.zeng@intel.com>
>>Subject: [edk2] [PATCH v3 2/6] MdeModulePkg/PeiCore: allocate
>>BootServicesCode memory for PE/COFF images
>>
>>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>
>>Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
>>---
>> 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
>>
>>_______________________________________________
>>edk2-devel mailing list
>>edk2-devel@lists.01.org
>>https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock for pool allocations
2017-02-27 6:50 ` Zeng, Star
@ 2017-02-27 8:15 ` Ard Biesheuvel
0 siblings, 0 replies; 21+ messages in thread
From: Ard Biesheuvel @ 2017-02-27 8:15 UTC (permalink / raw)
To: Zeng, Star
Cc: Gao, Liming, edk2-devel@lists.01.org, Yao, Jiewen,
leif.lindholm@linaro.org, afish@apple.com, Kinney, Michael D,
lersek@redhat.com, Tian, Feng
On 27 February 2017 at 06:50, Zeng, Star <star.zeng@intel.com> wrote:
> CoreAllocatePoolPages() could not be updated simply by adding CoreAcquireMemoryLock() and CoreReleaseMemoryLock(), it is also used by AllocateMemoryMapEntry() with the lock locked.
>
Indeed.
But I am wondering now if that means some code paths don't set the
protection correctly. If EfiBootServicesData and EfiConventionalMemory
use the same policy (which should be the case 99.9% of the time) it
doesn't matter, but if the configured policy is different, the
permissions will go out of sync.
> -----Original Message-----
> From: Gao, Liming
> Sent: Monday, February 27, 2017 2:43 PM
> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>; leif.lindholm@linaro.org
> Cc: afish@apple.com; Kinney, Michael D <michael.d.kinney@intel.com>; lersek@redhat.com; Tian, Feng <feng.tian@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: RE: [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock for pool allocations
>
> Ard:
> I agree to separate lock for pool allocations. I suggest you update CoreAllocatePoolPages() and CoreFreePoolPages() implementation by adding CoreAcquireMemoryLock() and CoreReleaseMemoryLock(). If so, you don't need to introduce new CoreAllocatePoolPagesI () and CoreFreePoolPagesI ().
>
> Thanks
> Liming
>>-----Original Message-----
>>From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>>Sent: Monday, February 27, 2017 2:30 AM
>>To: edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>;
>>leif.lindholm@linaro.org
>>Cc: afish@apple.com; 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>; Ard Biesheuvel
>><ard.biesheuvel@linaro.org>
>>Subject: [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock for
>>pool allocations
>>
>>In preparation of adding memory permission attribute management to the
>>pool allocator, split off the locking of the pool metadata into a
>>separate lock. This is an improvement in itself, given that pool
>>allocation can only interfere with the page allocation bookkeeping if
>>pool pages are allocated or released. But it is also required to ensure
>>that the permission attribute management does not deadlock, given that
>>it may trigger page table splits leading to additional page tables
>>being allocated.
>>
>>Contributed-under: TianoCore Contribution Agreement 1.0
>>Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>---
>> MdeModulePkg/Core/Dxe/Mem/Pool.c | 53 ++++++++++++++++----
>> 1 file changed, 43 insertions(+), 10 deletions(-)
>>
>>diff --git a/MdeModulePkg/Core/Dxe/Mem/Pool.c
>>b/MdeModulePkg/Core/Dxe/Mem/Pool.c
>>index 7afd2d312c1d..410615e0dee9 100644
>>--- a/MdeModulePkg/Core/Dxe/Mem/Pool.c
>>+++ b/MdeModulePkg/Core/Dxe/Mem/Pool.c
>>@@ -15,6 +15,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,
>>EITHER EXPRESS OR IMPLIED.
>> #include "DxeMain.h"
>> #include "Imem.h"
>>
>>+STATIC EFI_LOCK mPoolMemoryLock = EFI_INITIALIZE_LOCK_VARIABLE
>>(TPL_NOTIFY);
>>+
>> #define POOL_FREE_SIGNATURE SIGNATURE_32('p','f','r','0')
>> typedef struct {
>> UINT32 Signature;
>>@@ -239,13 +241,13 @@ CoreInternalAllocatePool (
>> //
>> // Acquire the memory lock and make the allocation
>> //
>>- Status = CoreAcquireLockOrFail (&gMemoryLock);
>>+ Status = CoreAcquireLockOrFail (&mPoolMemoryLock);
>> if (EFI_ERROR (Status)) {
>> return EFI_OUT_OF_RESOURCES;
>> }
>>
>> *Buffer = CoreAllocatePoolI (PoolType, Size);
>>- CoreReleaseMemoryLock ();
>>+ CoreReleaseLock (&mPoolMemoryLock);
>> return (*Buffer != NULL) ? EFI_SUCCESS : EFI_OUT_OF_RESOURCES; }
>>
>>@@ -289,6 +291,23 @@ CoreAllocatePool (
>> return Status;
>> }
>>
>>+STATIC
>>+VOID *
>>+CoreAllocatePoolPagesI (
>>+ IN EFI_MEMORY_TYPE PoolType,
>>+ IN UINTN NoPages,
>>+ IN UINTN Granularity
>>+ )
>>+{
>>+ VOID *Buffer;
>>+
>>+ CoreAcquireMemoryLock ();
>>+ Buffer = CoreAllocatePoolPages (PoolType, NoPages, Granularity);
>>+ CoreReleaseMemoryLock ();
>>+
>>+ return Buffer;
>>+}
>>+
>> /**
>> Internal function to allocate pool of a particular type.
>> Caller must have the memory lock held @@ -317,7 +336,7 @@
>>CoreAllocatePoolI (
>> UINTN NoPages;
>> UINTN Granularity;
>>
>>- ASSERT_LOCKED (&gMemoryLock);
>>+ ASSERT_LOCKED (&mPoolMemoryLock);
>>
>> if (PoolType == EfiACPIReclaimMemory ||
>> PoolType == EfiACPIMemoryNVS ||
>>@@ -355,7 +374,7 @@ CoreAllocatePoolI (
>> if (Index >= SIZE_TO_LIST (Granularity)) {
>> NoPages = EFI_SIZE_TO_PAGES(Size) + EFI_SIZE_TO_PAGES
>>(Granularity) - 1;
>> NoPages &= ~(UINTN)(EFI_SIZE_TO_PAGES (Granularity) - 1);
>>- Head = CoreAllocatePoolPages (PoolType, NoPages, Granularity);
>>+ Head = CoreAllocatePoolPagesI (PoolType, NoPages, Granularity);
>> goto Done;
>> }
>>
>>@@ -383,7 +402,7 @@ CoreAllocatePoolI (
>> //
>> // Get another page
>> //
>>- NewPage = CoreAllocatePoolPages(PoolType, EFI_SIZE_TO_PAGES
>>(Granularity), Granularity);
>>+ NewPage = CoreAllocatePoolPagesI (PoolType, EFI_SIZE_TO_PAGES
>>(Granularity), Granularity);
>> if (NewPage == NULL) {
>> goto Done;
>> }
>>@@ -486,9 +505,9 @@ CoreInternalFreePool (
>> return EFI_INVALID_PARAMETER;
>> }
>>
>>- CoreAcquireMemoryLock ();
>>+ CoreAcquireLock (&mPoolMemoryLock);
>> Status = CoreFreePoolI (Buffer, PoolType);
>>- CoreReleaseMemoryLock ();
>>+ CoreReleaseLock (&mPoolMemoryLock);
>> return Status;
>> }
>>
>>@@ -525,6 +544,19 @@ CoreFreePool (
>> return Status;
>> }
>>
>>+STATIC
>>+VOID
>>+CoreFreePoolPagesI (
>>+ IN EFI_MEMORY_TYPE PoolType,
>>+ IN EFI_PHYSICAL_ADDRESS Memory,
>>+ IN UINTN NoPages
>>+ )
>>+{
>>+ CoreAcquireMemoryLock ();
>>+ CoreFreePoolPages (Memory, NoPages);
>>+ CoreReleaseMemoryLock ();
>>+}
>>+
>> /**
>> Internal function to free a pool entry.
>> Caller must have the memory lock held @@ -573,7 +605,7 @@
>>CoreFreePoolI (
>> //
>> ASSERT (Tail->Signature == POOL_TAIL_SIGNATURE);
>> ASSERT (Head->Size == Tail->Size);
>>- ASSERT_LOCKED (&gMemoryLock);
>>+ ASSERT_LOCKED (&mPoolMemoryLock);
>>
>> if (Tail->Signature != POOL_TAIL_SIGNATURE) {
>> return EFI_INVALID_PARAMETER;
>>@@ -624,7 +656,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);
>>+ CoreFreePoolPagesI (Pool->MemoryType, (EFI_PHYSICAL_ADDRESS)
>>(UINTN) Head, NoPages);
>>
>> } else {
>>
>>@@ -680,7 +712,8 @@ CoreFreePoolI (
>> //
>> // Free the page
>> //
>>- CoreFreePoolPages ((EFI_PHYSICAL_ADDRESS) (UINTN)NewPage,
>>EFI_SIZE_TO_PAGES (Granularity));
>>+ CoreFreePoolPagesI (Pool->MemoryType, (EFI_PHYSICAL_ADDRESS)
>>(UINTN)NewPage,
>>+ EFI_SIZE_TO_PAGES (Granularity));
>> }
>> }
>> }
>>--
>>2.7.4
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock for pool allocations
2017-02-27 1:56 ` Zeng, Star
@ 2017-02-27 8:15 ` Ard Biesheuvel
2017-02-27 8:20 ` Zeng, Star
0 siblings, 1 reply; 21+ messages in thread
From: Ard Biesheuvel @ 2017-02-27 8:15 UTC (permalink / raw)
To: Zeng, Star
Cc: edk2-devel@lists.01.org, Yao, Jiewen, leif.lindholm@linaro.org,
Tian, Feng, afish@apple.com, Gao, Liming, Kinney, Michael D,
lersek@redhat.com
On 27 February 2017 at 01:56, Zeng, Star <star.zeng@intel.com> wrote:
> Minor comment: CoreFreePoolPagesI() has no need to have PoolType parameter, how about to remove it?
>
I need it after patch 6. But perhaps it is better to only add it
there, not here.
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard Biesheuvel
> Sent: Monday, February 27, 2017 2:30 AM
> To: edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>; leif.lindholm@linaro.org
> Cc: Tian, Feng <feng.tian@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>; afish@apple.com; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; lersek@redhat.com; Zeng, Star <star.zeng@intel.com>
> Subject: [edk2] [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock for pool allocations
>
> In preparation of adding memory permission attribute management to the pool allocator, split off the locking of the pool metadata into a separate lock. This is an improvement in itself, given that pool allocation can only interfere with the page allocation bookkeeping if pool pages are allocated or released. But it is also required to ensure that the permission attribute management does not deadlock, given that it may trigger page table splits leading to additional page tables being allocated.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> MdeModulePkg/Core/Dxe/Mem/Pool.c | 53 ++++++++++++++++----
> 1 file changed, 43 insertions(+), 10 deletions(-)
>
> diff --git a/MdeModulePkg/Core/Dxe/Mem/Pool.c b/MdeModulePkg/Core/Dxe/Mem/Pool.c
> index 7afd2d312c1d..410615e0dee9 100644
> --- a/MdeModulePkg/Core/Dxe/Mem/Pool.c
> +++ b/MdeModulePkg/Core/Dxe/Mem/Pool.c
> @@ -15,6 +15,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> #include "DxeMain.h"
> #include "Imem.h"
>
> +STATIC EFI_LOCK mPoolMemoryLock = EFI_INITIALIZE_LOCK_VARIABLE
> +(TPL_NOTIFY);
> +
> #define POOL_FREE_SIGNATURE SIGNATURE_32('p','f','r','0')
> typedef struct {
> UINT32 Signature;
> @@ -239,13 +241,13 @@ CoreInternalAllocatePool (
> //
> // Acquire the memory lock and make the allocation
> //
> - Status = CoreAcquireLockOrFail (&gMemoryLock);
> + Status = CoreAcquireLockOrFail (&mPoolMemoryLock);
> if (EFI_ERROR (Status)) {
> return EFI_OUT_OF_RESOURCES;
> }
>
> *Buffer = CoreAllocatePoolI (PoolType, Size);
> - CoreReleaseMemoryLock ();
> + CoreReleaseLock (&mPoolMemoryLock);
> return (*Buffer != NULL) ? EFI_SUCCESS : EFI_OUT_OF_RESOURCES; }
>
> @@ -289,6 +291,23 @@ CoreAllocatePool (
> return Status;
> }
>
> +STATIC
> +VOID *
> +CoreAllocatePoolPagesI (
> + IN EFI_MEMORY_TYPE PoolType,
> + IN UINTN NoPages,
> + IN UINTN Granularity
> + )
> +{
> + VOID *Buffer;
> +
> + CoreAcquireMemoryLock ();
> + Buffer = CoreAllocatePoolPages (PoolType, NoPages, Granularity);
> + CoreReleaseMemoryLock ();
> +
> + return Buffer;
> +}
> +
> /**
> Internal function to allocate pool of a particular type.
> Caller must have the memory lock held @@ -317,7 +336,7 @@ CoreAllocatePoolI (
> UINTN NoPages;
> UINTN Granularity;
>
> - ASSERT_LOCKED (&gMemoryLock);
> + ASSERT_LOCKED (&mPoolMemoryLock);
>
> if (PoolType == EfiACPIReclaimMemory ||
> PoolType == EfiACPIMemoryNVS ||
> @@ -355,7 +374,7 @@ CoreAllocatePoolI (
> if (Index >= SIZE_TO_LIST (Granularity)) {
> NoPages = EFI_SIZE_TO_PAGES(Size) + EFI_SIZE_TO_PAGES (Granularity) - 1;
> NoPages &= ~(UINTN)(EFI_SIZE_TO_PAGES (Granularity) - 1);
> - Head = CoreAllocatePoolPages (PoolType, NoPages, Granularity);
> + Head = CoreAllocatePoolPagesI (PoolType, NoPages, Granularity);
> goto Done;
> }
>
> @@ -383,7 +402,7 @@ CoreAllocatePoolI (
> //
> // Get another page
> //
> - NewPage = CoreAllocatePoolPages(PoolType, EFI_SIZE_TO_PAGES (Granularity), Granularity);
> + NewPage = CoreAllocatePoolPagesI (PoolType, EFI_SIZE_TO_PAGES
> + (Granularity), Granularity);
> if (NewPage == NULL) {
> goto Done;
> }
> @@ -486,9 +505,9 @@ CoreInternalFreePool (
> return EFI_INVALID_PARAMETER;
> }
>
> - CoreAcquireMemoryLock ();
> + CoreAcquireLock (&mPoolMemoryLock);
> Status = CoreFreePoolI (Buffer, PoolType);
> - CoreReleaseMemoryLock ();
> + CoreReleaseLock (&mPoolMemoryLock);
> return Status;
> }
>
> @@ -525,6 +544,19 @@ CoreFreePool (
> return Status;
> }
>
> +STATIC
> +VOID
> +CoreFreePoolPagesI (
> + IN EFI_MEMORY_TYPE PoolType,
> + IN EFI_PHYSICAL_ADDRESS Memory,
> + IN UINTN NoPages
> + )
> +{
> + CoreAcquireMemoryLock ();
> + CoreFreePoolPages (Memory, NoPages);
> + CoreReleaseMemoryLock ();
> +}
> +
> /**
> Internal function to free a pool entry.
> Caller must have the memory lock held @@ -573,7 +605,7 @@ CoreFreePoolI (
> //
> ASSERT (Tail->Signature == POOL_TAIL_SIGNATURE);
> ASSERT (Head->Size == Tail->Size);
> - ASSERT_LOCKED (&gMemoryLock);
> + ASSERT_LOCKED (&mPoolMemoryLock);
>
> if (Tail->Signature != POOL_TAIL_SIGNATURE) {
> return EFI_INVALID_PARAMETER;
> @@ -624,7 +656,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);
> + CoreFreePoolPagesI (Pool->MemoryType, (EFI_PHYSICAL_ADDRESS)
> + (UINTN) Head, NoPages);
>
> } else {
>
> @@ -680,7 +712,8 @@ CoreFreePoolI (
> //
> // Free the page
> //
> - CoreFreePoolPages ((EFI_PHYSICAL_ADDRESS) (UINTN)NewPage, EFI_SIZE_TO_PAGES (Granularity));
> + CoreFreePoolPagesI (Pool->MemoryType, (EFI_PHYSICAL_ADDRESS) (UINTN)NewPage,
> + EFI_SIZE_TO_PAGES (Granularity));
> }
> }
> }
> --
> 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] 21+ messages in thread
* Re: [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock for pool allocations
2017-02-27 8:15 ` Ard Biesheuvel
@ 2017-02-27 8:20 ` Zeng, Star
0 siblings, 0 replies; 21+ messages in thread
From: Zeng, Star @ 2017-02-27 8:20 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Tian, Feng, edk2-devel@lists.01.org, afish@apple.com, Gao, Liming,
Yao, Jiewen, leif.lindholm@linaro.org, Kinney, Michael D,
lersek@redhat.com, Zeng, Star
If it is really needed, I am fine to either this patch or another patch to add it. :)
Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard Biesheuvel
Sent: Monday, February 27, 2017 4:16 PM
To: Zeng, Star <star.zeng@intel.com>
Cc: Tian, Feng <feng.tian@intel.com>; edk2-devel@lists.01.org; afish@apple.com; Gao, Liming <liming.gao@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; leif.lindholm@linaro.org; Kinney, Michael D <michael.d.kinney@intel.com>; lersek@redhat.com
Subject: Re: [edk2] [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock for pool allocations
On 27 February 2017 at 01:56, Zeng, Star <star.zeng@intel.com> wrote:
> Minor comment: CoreFreePoolPagesI() has no need to have PoolType parameter, how about to remove it?
>
I need it after patch 6. But perhaps it is better to only add it there, not here.
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Ard Biesheuvel
> Sent: Monday, February 27, 2017 2:30 AM
> To: edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>;
> leif.lindholm@linaro.org
> Cc: Tian, Feng <feng.tian@intel.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>; afish@apple.com; Gao, Liming
> <liming.gao@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; lersek@redhat.com; Zeng, Star
> <star.zeng@intel.com>
> Subject: [edk2] [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock
> for pool allocations
>
> In preparation of adding memory permission attribute management to the pool allocator, split off the locking of the pool metadata into a separate lock. This is an improvement in itself, given that pool allocation can only interfere with the page allocation bookkeeping if pool pages are allocated or released. But it is also required to ensure that the permission attribute management does not deadlock, given that it may trigger page table splits leading to additional page tables being allocated.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> MdeModulePkg/Core/Dxe/Mem/Pool.c | 53 ++++++++++++++++----
> 1 file changed, 43 insertions(+), 10 deletions(-)
>
> diff --git a/MdeModulePkg/Core/Dxe/Mem/Pool.c
> b/MdeModulePkg/Core/Dxe/Mem/Pool.c
> index 7afd2d312c1d..410615e0dee9 100644
> --- a/MdeModulePkg/Core/Dxe/Mem/Pool.c
> +++ b/MdeModulePkg/Core/Dxe/Mem/Pool.c
> @@ -15,6 +15,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> #include "DxeMain.h"
> #include "Imem.h"
>
> +STATIC EFI_LOCK mPoolMemoryLock = EFI_INITIALIZE_LOCK_VARIABLE
> +(TPL_NOTIFY);
> +
> #define POOL_FREE_SIGNATURE SIGNATURE_32('p','f','r','0')
> typedef struct {
> UINT32 Signature;
> @@ -239,13 +241,13 @@ CoreInternalAllocatePool (
> //
> // Acquire the memory lock and make the allocation
> //
> - Status = CoreAcquireLockOrFail (&gMemoryLock);
> + Status = CoreAcquireLockOrFail (&mPoolMemoryLock);
> if (EFI_ERROR (Status)) {
> return EFI_OUT_OF_RESOURCES;
> }
>
> *Buffer = CoreAllocatePoolI (PoolType, Size);
> - CoreReleaseMemoryLock ();
> + CoreReleaseLock (&mPoolMemoryLock);
> return (*Buffer != NULL) ? EFI_SUCCESS : EFI_OUT_OF_RESOURCES; }
>
> @@ -289,6 +291,23 @@ CoreAllocatePool (
> return Status;
> }
>
> +STATIC
> +VOID *
> +CoreAllocatePoolPagesI (
> + IN EFI_MEMORY_TYPE PoolType,
> + IN UINTN NoPages,
> + IN UINTN Granularity
> + )
> +{
> + VOID *Buffer;
> +
> + CoreAcquireMemoryLock ();
> + Buffer = CoreAllocatePoolPages (PoolType, NoPages, Granularity);
> + CoreReleaseMemoryLock ();
> +
> + return Buffer;
> +}
> +
> /**
> Internal function to allocate pool of a particular type.
> Caller must have the memory lock held @@ -317,7 +336,7 @@ CoreAllocatePoolI (
> UINTN NoPages;
> UINTN Granularity;
>
> - ASSERT_LOCKED (&gMemoryLock);
> + ASSERT_LOCKED (&mPoolMemoryLock);
>
> if (PoolType == EfiACPIReclaimMemory ||
> PoolType == EfiACPIMemoryNVS ||
> @@ -355,7 +374,7 @@ CoreAllocatePoolI (
> if (Index >= SIZE_TO_LIST (Granularity)) {
> NoPages = EFI_SIZE_TO_PAGES(Size) + EFI_SIZE_TO_PAGES (Granularity) - 1;
> NoPages &= ~(UINTN)(EFI_SIZE_TO_PAGES (Granularity) - 1);
> - Head = CoreAllocatePoolPages (PoolType, NoPages, Granularity);
> + Head = CoreAllocatePoolPagesI (PoolType, NoPages, Granularity);
> goto Done;
> }
>
> @@ -383,7 +402,7 @@ CoreAllocatePoolI (
> //
> // Get another page
> //
> - NewPage = CoreAllocatePoolPages(PoolType, EFI_SIZE_TO_PAGES (Granularity), Granularity);
> + NewPage = CoreAllocatePoolPagesI (PoolType, EFI_SIZE_TO_PAGES
> + (Granularity), Granularity);
> if (NewPage == NULL) {
> goto Done;
> }
> @@ -486,9 +505,9 @@ CoreInternalFreePool (
> return EFI_INVALID_PARAMETER;
> }
>
> - CoreAcquireMemoryLock ();
> + CoreAcquireLock (&mPoolMemoryLock);
> Status = CoreFreePoolI (Buffer, PoolType);
> - CoreReleaseMemoryLock ();
> + CoreReleaseLock (&mPoolMemoryLock);
> return Status;
> }
>
> @@ -525,6 +544,19 @@ CoreFreePool (
> return Status;
> }
>
> +STATIC
> +VOID
> +CoreFreePoolPagesI (
> + IN EFI_MEMORY_TYPE PoolType,
> + IN EFI_PHYSICAL_ADDRESS Memory,
> + IN UINTN NoPages
> + )
> +{
> + CoreAcquireMemoryLock ();
> + CoreFreePoolPages (Memory, NoPages);
> + CoreReleaseMemoryLock ();
> +}
> +
> /**
> Internal function to free a pool entry.
> Caller must have the memory lock held @@ -573,7 +605,7 @@ CoreFreePoolI (
> //
> ASSERT (Tail->Signature == POOL_TAIL_SIGNATURE);
> ASSERT (Head->Size == Tail->Size);
> - ASSERT_LOCKED (&gMemoryLock);
> + ASSERT_LOCKED (&mPoolMemoryLock);
>
> if (Tail->Signature != POOL_TAIL_SIGNATURE) {
> return EFI_INVALID_PARAMETER;
> @@ -624,7 +656,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);
> + CoreFreePoolPagesI (Pool->MemoryType, (EFI_PHYSICAL_ADDRESS)
> + (UINTN) Head, NoPages);
>
> } else {
>
> @@ -680,7 +712,8 @@ CoreFreePoolI (
> //
> // Free the page
> //
> - CoreFreePoolPages ((EFI_PHYSICAL_ADDRESS) (UINTN)NewPage, EFI_SIZE_TO_PAGES (Granularity));
> + CoreFreePoolPagesI (Pool->MemoryType, (EFI_PHYSICAL_ADDRESS) (UINTN)NewPage,
> + EFI_SIZE_TO_PAGES (Granularity));
> }
> }
> }
> --
> 2.7.4
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 6/6] MdeModulePkg/DxeCore: implement memory protection policy
2017-02-26 18:30 ` [PATCH v3 6/6] MdeModulePkg/DxeCore: implement " Ard Biesheuvel
2017-02-27 6:46 ` Gao, Liming
@ 2017-02-27 9:56 ` Laszlo Ersek
2017-02-27 9:57 ` Ard Biesheuvel
1 sibling, 1 reply; 21+ messages in thread
From: Laszlo Ersek @ 2017-02-27 9:56 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel, jiewen.yao, leif.lindholm
Cc: feng.tian, afish, liming.gao, michael.d.kinney, star.zeng
On 02/26/17 19:30, Ard Biesheuvel wrote:
> 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.h | 24 ++
> MdeModulePkg/Core/Dxe/DxeMain.inf | 1 +
> MdeModulePkg/Core/Dxe/Mem/Page.c | 4 +
> MdeModulePkg/Core/Dxe/Mem/Pool.c | 7 +
> MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 306 +++++++++++++++++++-
> 5 files changed, 341 insertions(+), 1 deletion(-)
[snip]
> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf b/MdeModulePkg/Core/Dxe/DxeMain.inf
> index 371e91cb0d7e..30d5984f7c1f 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.PcdDxeNxMemoryProtectionPolicy ## CONSUMES
>
> # [Hob]
> # RESOURCE_DESCRIPTOR ## CONSUMES
The series doesn't build for me:
.../MdeModulePkg/Core/Dxe/DxeMain.inf(194): error 3000: PCD
[gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy] in
[.../MdeModulePkg/Core/Dxe/DxeMain.inf] is not found in dependent packages:
.../MdePkg/MdePkg.dec
.../MdeModulePkg/MdeModulePkg.dec
I think you forgot to add the .dec hunk to this patch.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 6/6] MdeModulePkg/DxeCore: implement memory protection policy
2017-02-27 9:56 ` Laszlo Ersek
@ 2017-02-27 9:57 ` Ard Biesheuvel
0 siblings, 0 replies; 21+ messages in thread
From: Ard Biesheuvel @ 2017-02-27 9:57 UTC (permalink / raw)
To: Laszlo Ersek
Cc: edk2-devel@lists.01.org, Yao, Jiewen, Leif Lindholm, Tian, Feng,
afish@apple.com, Gao, Liming, Kinney, Michael D, Zeng, Star
On 27 February 2017 at 09:56, Laszlo Ersek <lersek@redhat.com> wrote:
> On 02/26/17 19:30, Ard Biesheuvel wrote:
>> 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.h | 24 ++
>> MdeModulePkg/Core/Dxe/DxeMain.inf | 1 +
>> MdeModulePkg/Core/Dxe/Mem/Page.c | 4 +
>> MdeModulePkg/Core/Dxe/Mem/Pool.c | 7 +
>> MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 306 +++++++++++++++++++-
>> 5 files changed, 341 insertions(+), 1 deletion(-)
>
> [snip]
>
>> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf b/MdeModulePkg/Core/Dxe/DxeMain.inf
>> index 371e91cb0d7e..30d5984f7c1f 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.PcdDxeNxMemoryProtectionPolicy ## CONSUMES
>>
>> # [Hob]
>> # RESOURCE_DESCRIPTOR ## CONSUMES
>
> The series doesn't build for me:
>
> .../MdeModulePkg/Core/Dxe/DxeMain.inf(194): error 3000: PCD
> [gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy] in
> [.../MdeModulePkg/Core/Dxe/DxeMain.inf] is not found in dependent packages:
> .../MdePkg/MdePkg.dec
> .../MdeModulePkg/MdeModulePkg.dec
>
> I think you forgot to add the .dec hunk to this patch.
>
Apologies. I got the name wrong in the .dec, but I did update the
branch I pushed to the link above with the only change being a fix for
this.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 0/6] RFC: increased memory protection
2017-02-26 18:29 [PATCH v3 0/6] RFC: increased memory protection Ard Biesheuvel
` (6 preceding siblings ...)
2017-02-27 5:19 ` [PATCH v3 0/6] RFC: increased memory protection Yao, Jiewen
@ 2017-02-27 13:14 ` Laszlo Ersek
7 siblings, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2017-02-27 13:14 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel, jiewen.yao, leif.lindholm
Cc: feng.tian, afish, liming.gao, michael.d.kinney, star.zeng
On 02/26/17 19:29, 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.
>
> Changes since v2:
> - added patch to make EBC use EfiBootServicesCode pool allocations for thunks
> - redefine PCD according to Jiewen's feedback, including default value
> - use sorted memory map and merge adjacent entries with the same policy, to
> prevent unnecessary page table splitting
> - ignore policy when executing in SMM
> - refactor the logic for managing permission attributes of pool allocations
> - added some R-b's
>
> 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.
>
> Series can be found here:
> https://git.linaro.org/people/ard.biesheuvel/uefi-next.git/log/?h=memprot-take2
>
> Note that to test this properly, the default value of 0 should be changed
> to 0x7FD5, which applies non-exec permissions to everything except Efi*Code
> regions.
>
> Ard Biesheuvel (6):
> ArmPkg/CpuDxe: ignore attribute changes during SyncCacheConfig()
> MdeModulePkg/PeiCore: allocate BootServicesCode memory for PE/COFF
> images
> MdeModulePkg/EbcDxe: use EfiBootServicesCode memory for thunks
> MdeModulePkg/DxeCore: use separate lock for pool allocations
> 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.h | 24 ++
> MdeModulePkg/Core/Dxe/DxeMain.inf | 1 +
> MdeModulePkg/Core/Dxe/Mem/Page.c | 4 +
> MdeModulePkg/Core/Dxe/Mem/Pool.c | 60 +++-
> MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 306 +++++++++++++++++++-
> MdeModulePkg/Core/Pei/Image/Image.c | 10 +-
> MdeModulePkg/MdeModulePkg.dec | 31 ++
> MdeModulePkg/Universal/EbcDxe/AArch64/EbcSupport.c | 2 +-
> MdeModulePkg/Universal/EbcDxe/EbcInt.c | 23 ++
> MdeModulePkg/Universal/EbcDxe/EbcInt.h | 14 +
> MdeModulePkg/Universal/EbcDxe/Ia32/EbcSupport.c | 2 +-
> MdeModulePkg/Universal/EbcDxe/Ipf/EbcSupport.c | 2 +-
> MdeModulePkg/Universal/EbcDxe/X64/EbcSupport.c | 2 +-
> 16 files changed, 471 insertions(+), 18 deletions(-)
>
with the default 0 value for the PCD:
Regression-tested-by: Laszlo Ersek <lersek@redhat.com>
For testing I more or less used
<https://github.com/tianocore/tianocore.github.io/wiki/Testing-SMM-with-QEMU,-KVM-and-libvirt>,
plus booted a few guests on aarch64/KVM with this (Fedora 24, RHEL-7.3,
openSUSE Tumbleweed).
Thanks
Laszlo
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2017-02-27 13:14 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-02-26 18:29 [PATCH v3 0/6] RFC: increased memory protection Ard Biesheuvel
2017-02-26 18:30 ` [PATCH v3 1/6] ArmPkg/CpuDxe: ignore attribute changes during SyncCacheConfig() Ard Biesheuvel
2017-02-26 18:30 ` [PATCH v3 2/6] MdeModulePkg/PeiCore: allocate BootServicesCode memory for PE/COFF images Ard Biesheuvel
2017-02-27 6:43 ` Gao, Liming
2017-02-27 8:13 ` Ard Biesheuvel
2017-02-26 18:30 ` [PATCH v3 3/6] MdeModulePkg/EbcDxe: use EfiBootServicesCode memory for thunks Ard Biesheuvel
2017-02-26 18:30 ` [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock for pool allocations Ard Biesheuvel
2017-02-26 19:52 ` Ard Biesheuvel
2017-02-27 1:56 ` Zeng, Star
2017-02-27 8:15 ` Ard Biesheuvel
2017-02-27 8:20 ` Zeng, Star
2017-02-27 6:43 ` Gao, Liming
2017-02-27 6:50 ` Zeng, Star
2017-02-27 8:15 ` Ard Biesheuvel
2017-02-26 18:30 ` [PATCH v3 5/6] MdeModulePkg: define PCD for DXE memory protection policy Ard Biesheuvel
2017-02-26 18:30 ` [PATCH v3 6/6] MdeModulePkg/DxeCore: implement " Ard Biesheuvel
2017-02-27 6:46 ` Gao, Liming
2017-02-27 9:56 ` Laszlo Ersek
2017-02-27 9:57 ` Ard Biesheuvel
2017-02-27 5:19 ` [PATCH v3 0/6] RFC: increased memory protection Yao, Jiewen
2017-02-27 13:14 ` Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox