* [PATCH v4 0/7] MdeModulePkg/DxeCore: increased memory protection
@ 2017-02-27 14:38 Ard Biesheuvel
2017-02-27 14:38 ` [PATCH v4 1/7] ArmPkg/CpuDxe: ignore attribute changes during SyncCacheConfig() Ard Biesheuvel
` (8 more replies)
0 siblings, 9 replies; 26+ messages in thread
From: Ard Biesheuvel @ 2017-02-27 14:38 UTC (permalink / raw)
To: edk2-devel, afish, leif.lindholm, michael.d.kinney, liming.gao,
jiewen.yao
Cc: lersek, feng.tian, star.zeng, Ard Biesheuvel
Hello all,
First of all, thanks for the reviews and regression testing. However, I did
not add the tested-by tags nor some of the R-b's, given the changes in this v4.
This series implements a memory protection policy 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 v3:
- mandate that the same policy applies to EfiConventionalMemory regions and
EfiBootServicesData regions: they are unlikely to differ in practice, and
dealing with that corner case greatly complicates the implementation, given
the way DxeCore allocates memory for itself in the implementation of the page
and pool allocation routines.
- apply the EfiConventionalMemory policy to untested RAM regions in the GCD
memory space map: without this, we may still have a large region of RAM that
is exploitable, and it also removes the need to apply memory protections in
PromoteMemoryResource (), which is very difficult to achieve without a major
restructuring of the code due to the way locking is implemented here.
- add missing ApplyMemoryProtectionPolicy() call to CoreAddMemoryDescriptor()
- use CoreAcquireLockOrFail() on gMemoryLock for CoreAllocatePoolPages (#4)
- incorporate feedback from Liming (#2, #6)
- add patch to enable the NX memory protection policy for ArmVirtPkg (#7)
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-v4
Ard Biesheuvel (7):
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
ArmVirtPkg/ArmVirt.dsc.inc: enable NX memory protection for all
platforms
ArmPkg/Drivers/CpuDxe/CpuDxe.c | 3 +
ArmPkg/Drivers/CpuDxe/CpuDxe.h | 1 +
ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c | 4 +
ArmVirtPkg/ArmVirt.dsc.inc | 6 +
MdeModulePkg/Core/Dxe/DxeMain.h | 24 ++
MdeModulePkg/Core/Dxe/DxeMain.inf | 1 +
MdeModulePkg/Core/Dxe/Mem/Page.c | 7 +
MdeModulePkg/Core/Dxe/Mem/Pool.c | 65 +++-
MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 371 +++++++++++++++++++-
MdeModulePkg/Core/Pei/Image/Image.c | 23 +-
MdeModulePkg/MdeModulePkg.dec | 32 ++
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 +-
17 files changed, 558 insertions(+), 24 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v4 1/7] ArmPkg/CpuDxe: ignore attribute changes during SyncCacheConfig()
2017-02-27 14:38 [PATCH v4 0/7] MdeModulePkg/DxeCore: increased memory protection Ard Biesheuvel
@ 2017-02-27 14:38 ` Ard Biesheuvel
2017-02-27 15:32 ` Leif Lindholm
2017-02-27 14:38 ` [PATCH v4 2/7] MdeModulePkg/PeiCore: allocate BootServicesCode memory for PE/COFF images Ard Biesheuvel
` (7 subsequent siblings)
8 siblings, 1 reply; 26+ messages in thread
From: Ard Biesheuvel @ 2017-02-27 14:38 UTC (permalink / raw)
To: edk2-devel, afish, leif.lindholm, michael.d.kinney, liming.gao,
jiewen.yao
Cc: lersek, feng.tian, star.zeng, Ard Biesheuvel
To prevent the initial MMU->GCD memory space map synchronization from
stripping permissions attributes [which we cannot use in the GCD memory
space map, unfortunately], implement the same approach as x86, and ignore
SetMemoryAttributes() calls during the time SyncCacheConfig() is in
progress. This is a horrible hack, but is currently the only way we can
implement strict permissions on arbitrary memory regions [as opposed to
PE/COFF text/data sections only]
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
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] 26+ messages in thread
* [PATCH v4 2/7] MdeModulePkg/PeiCore: allocate BootServicesCode memory for PE/COFF images
2017-02-27 14:38 [PATCH v4 0/7] MdeModulePkg/DxeCore: increased memory protection Ard Biesheuvel
2017-02-27 14:38 ` [PATCH v4 1/7] ArmPkg/CpuDxe: ignore attribute changes during SyncCacheConfig() Ard Biesheuvel
@ 2017-02-27 14:38 ` Ard Biesheuvel
2017-02-28 5:42 ` Gao, Liming
2017-02-27 14:38 ` [PATCH v4 3/7] MdeModulePkg/EbcDxe: use EfiBootServicesCode memory for thunks Ard Biesheuvel
` (6 subsequent siblings)
8 siblings, 1 reply; 26+ messages in thread
From: Ard Biesheuvel @ 2017-02-27 14:38 UTC (permalink / raw)
To: edk2-devel, afish, leif.lindholm, michael.d.kinney, liming.gao,
jiewen.yao
Cc: lersek, feng.tian, star.zeng, Ard Biesheuvel
Ensure that any memory allocated for PE/COFF images is identifiable as
a boot services code region, so that we know it requires its executable
permissions to be preserved when we tighten mapping permissions later on.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
---
MdeModulePkg/Core/Pei/Image/Image.c | 23 ++++++++++++--------
1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/MdeModulePkg/Core/Pei/Image/Image.c b/MdeModulePkg/Core/Pei/Image/Image.c
index d659de8b3e64..68e40c027e63 100644
--- a/MdeModulePkg/Core/Pei/Image/Image.c
+++ b/MdeModulePkg/Core/Pei/Image/Image.c
@@ -112,11 +112,12 @@ GetImageReadFunction (
IN PE_COFF_LOADER_IMAGE_CONTEXT *ImageContext
)
{
- PEI_CORE_INSTANCE *Private;
- VOID* MemoryBuffer;
+ PEI_CORE_INSTANCE *Private;
+ EFI_PHYSICAL_ADDRESS MemoryBuffer;
Private = PEI_CORE_INSTANCE_FROM_PS_THIS (GetPeiServicesTablePointer ());
-
+ MemoryBuffer = 0;
+
if (Private->PeiMemoryInstalled && (((Private->HobList.HandoffInformationTable->BootMode != BOOT_ON_S3_RESUME) && PcdGetBool (PcdShadowPeimOnBoot)) ||
((Private->HobList.HandoffInformationTable->BootMode == BOOT_ON_S3_RESUME) && PcdGetBool (PcdShadowPeimOnS3Boot))) &&
(EFI_IMAGE_MACHINE_TYPE_SUPPORTED(EFI_IMAGE_MACHINE_X64) || EFI_IMAGE_MACHINE_TYPE_SUPPORTED(EFI_IMAGE_MACHINE_IA32))) {
@@ -125,9 +126,9 @@ GetImageReadFunction (
// compilers that have been tested
//
if (Private->ShadowedImageRead == NULL) {
- MemoryBuffer = AllocatePages (0x400 / EFI_PAGE_SIZE + 1);
- ASSERT (MemoryBuffer != NULL);
- CopyMem (MemoryBuffer, (CONST VOID *) (UINTN) PeiImageReadForShadow, 0x400);
+ PeiServicesAllocatePages (EfiBootServicesCode, 0x400 / EFI_PAGE_SIZE + 1, &MemoryBuffer);
+ ASSERT (MemoryBuffer != 0);
+ CopyMem ((VOID *)(UINTN)MemoryBuffer, (CONST VOID *) (UINTN) PeiImageReadForShadow, 0x400);
Private->ShadowedImageRead = (PE_COFF_LOADER_READ_FILE) (UINTN) MemoryBuffer;
}
@@ -453,12 +454,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] 26+ messages in thread
* [PATCH v4 3/7] MdeModulePkg/EbcDxe: use EfiBootServicesCode memory for thunks
2017-02-27 14:38 [PATCH v4 0/7] MdeModulePkg/DxeCore: increased memory protection Ard Biesheuvel
2017-02-27 14:38 ` [PATCH v4 1/7] ArmPkg/CpuDxe: ignore attribute changes during SyncCacheConfig() Ard Biesheuvel
2017-02-27 14:38 ` [PATCH v4 2/7] MdeModulePkg/PeiCore: allocate BootServicesCode memory for PE/COFF images Ard Biesheuvel
@ 2017-02-27 14:38 ` Ard Biesheuvel
2017-02-27 14:38 ` [PATCH v4 4/7] MdeModulePkg/DxeCore: use separate lock for pool allocations Ard Biesheuvel
` (5 subsequent siblings)
8 siblings, 0 replies; 26+ messages in thread
From: Ard Biesheuvel @ 2017-02-27 14:38 UTC (permalink / raw)
To: edk2-devel, afish, leif.lindholm, michael.d.kinney, liming.gao,
jiewen.yao
Cc: 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>
Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
---
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] 26+ messages in thread
* [PATCH v4 4/7] MdeModulePkg/DxeCore: use separate lock for pool allocations
2017-02-27 14:38 [PATCH v4 0/7] MdeModulePkg/DxeCore: increased memory protection Ard Biesheuvel
` (2 preceding siblings ...)
2017-02-27 14:38 ` [PATCH v4 3/7] MdeModulePkg/EbcDxe: use EfiBootServicesCode memory for thunks Ard Biesheuvel
@ 2017-02-27 14:38 ` Ard Biesheuvel
2017-02-28 9:32 ` Gao, Liming
2017-02-27 14:38 ` [PATCH v4 5/7] MdeModulePkg: define PCD for DXE memory protection policy Ard Biesheuvel
` (4 subsequent siblings)
8 siblings, 1 reply; 26+ messages in thread
From: Ard Biesheuvel @ 2017-02-27 14:38 UTC (permalink / raw)
To: edk2-devel, afish, leif.lindholm, michael.d.kinney, liming.gao,
jiewen.yao
Cc: lersek, feng.tian, star.zeng, Ard Biesheuvel
In 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 | 58 ++++++++++++++++----
1 file changed, 48 insertions(+), 10 deletions(-)
diff --git a/MdeModulePkg/Core/Dxe/Mem/Pool.c b/MdeModulePkg/Core/Dxe/Mem/Pool.c
index 7afd2d312c1d..ebb2fceedd80 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,28 @@ CoreAllocatePool (
return Status;
}
+STATIC
+VOID *
+CoreAllocatePoolPagesI (
+ IN EFI_MEMORY_TYPE PoolType,
+ IN UINTN NoPages,
+ IN UINTN Granularity
+ )
+{
+ VOID *Buffer;
+ EFI_STATUS Status;
+
+ Status = CoreAcquireLockOrFail (&gMemoryLock);
+ if (EFI_ERROR (Status)) {
+ return NULL;
+ }
+
+ 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 +341,7 @@ CoreAllocatePoolI (
UINTN NoPages;
UINTN Granularity;
- ASSERT_LOCKED (&gMemoryLock);
+ ASSERT_LOCKED (&mPoolMemoryLock);
if (PoolType == EfiACPIReclaimMemory ||
PoolType == EfiACPIMemoryNVS ||
@@ -355,7 +379,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 +407,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 +510,9 @@ CoreInternalFreePool (
return EFI_INVALID_PARAMETER;
}
- CoreAcquireMemoryLock ();
+ CoreAcquireLock (&mPoolMemoryLock);
Status = CoreFreePoolI (Buffer, PoolType);
- CoreReleaseMemoryLock ();
+ CoreReleaseLock (&mPoolMemoryLock);
return Status;
}
@@ -525,6 +549,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 +610,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 +661,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 +717,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] 26+ messages in thread
* [PATCH v4 5/7] MdeModulePkg: define PCD for DXE memory protection policy
2017-02-27 14:38 [PATCH v4 0/7] MdeModulePkg/DxeCore: increased memory protection Ard Biesheuvel
` (3 preceding siblings ...)
2017-02-27 14:38 ` [PATCH v4 4/7] MdeModulePkg/DxeCore: use separate lock for pool allocations Ard Biesheuvel
@ 2017-02-27 14:38 ` Ard Biesheuvel
2017-02-27 14:38 ` [PATCH v4 6/7] MdeModulePkg/DxeCore: implement " Ard Biesheuvel
` (3 subsequent siblings)
8 siblings, 0 replies; 26+ messages in thread
From: Ard Biesheuvel @ 2017-02-27 14:38 UTC (permalink / raw)
To: edk2-devel, afish, leif.lindholm, michael.d.kinney, liming.gao,
jiewen.yao
Cc: lersek, feng.tian, star.zeng, Ard Biesheuvel
Define a new fixed/patchable PCD that sets the DXE memory protection
policy: its primary use is to define which memory types should have
their executable permissions removed. Combined with the image protection
policy, this can be used to implement a strict W^X policy, i.e.. a policy
where no regions exist that are both executable and writable at the same
time.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
MdeModulePkg/MdeModulePkg.dec | 32 ++++++++++++++++++++
1 file changed, 32 insertions(+)
diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index 426634fbbd4d..10f460818915 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -1107,6 +1107,38 @@ [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>
+ # User MUST set the same NX protection for EfiBootServicesData and EfiConventionalMemory. <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.PcdDxeNxMemoryProtectionPolicy|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] 26+ messages in thread
* [PATCH v4 6/7] MdeModulePkg/DxeCore: implement memory protection policy
2017-02-27 14:38 [PATCH v4 0/7] MdeModulePkg/DxeCore: increased memory protection Ard Biesheuvel
` (4 preceding siblings ...)
2017-02-27 14:38 ` [PATCH v4 5/7] MdeModulePkg: define PCD for DXE memory protection policy Ard Biesheuvel
@ 2017-02-27 14:38 ` Ard Biesheuvel
2017-02-28 9:33 ` Gao, Liming
2017-02-27 14:38 ` [PATCH v4 7/7] ArmVirtPkg/ArmVirt.dsc.inc: enable NX memory protection for all platforms Ard Biesheuvel
` (2 subsequent siblings)
8 siblings, 1 reply; 26+ messages in thread
From: Ard Biesheuvel @ 2017-02-27 14:38 UTC (permalink / raw)
To: edk2-devel, afish, leif.lindholm, michael.d.kinney, liming.gao,
jiewen.yao
Cc: lersek, feng.tian, star.zeng, Ard Biesheuvel
This implements a DXE memory protection policy that ensure that regions
that don't require executable permissions are mapped with the non-exec
attribute set.
First of all, it iterates over all entries in the UEFI memory map, and
removes executable permissions according to the configured DXE memory
protection policy, as recorded in PcdDxeMemoryProtectionPolicy.
Secondly, it sets or clears the non-executable attribute when allocating
or freeing pages, both for page based or pool based allocations.
Note that this complements the image protection facility, which applies
strict permissions to BootServicesCode/RuntimeServicesCode regions when
the section alignment allows it. The memory protection configured by this
patch operates on non-code regions only.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
MdeModulePkg/Core/Dxe/DxeMain.h | 24 ++
MdeModulePkg/Core/Dxe/DxeMain.inf | 1 +
MdeModulePkg/Core/Dxe/Mem/Page.c | 7 +
MdeModulePkg/Core/Dxe/Mem/Pool.c | 7 +
MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 371 +++++++++++++++++++-
5 files changed, 409 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..d596db7ad427 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Page.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
@@ -553,6 +553,9 @@ CoreAddMemoryDescriptor (
CoreFreeMemoryMapStack ();
CoreReleaseMemoryLock ();
+ ApplyMemoryProtectionPolicy (EfiMaxMemoryType, Type, Start,
+ EFI_PAGES_TO_SIZE (NumberOfPages));
+
//
// If Loading Module At Fixed Address feature is enabled. try to allocate memory with Runtime code & Boot time code type
//
@@ -1344,6 +1347,8 @@ CoreAllocatePages (
NULL
);
InstallMemoryAttributesTableOnMemoryAllocation (MemoryType);
+ ApplyMemoryProtectionPolicy (EfiConventionalMemory, MemoryType, *Memory,
+ EFI_PAGES_TO_SIZE (NumberOfPages));
}
return Status;
}
@@ -1460,6 +1465,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 ebb2fceedd80..ced64443c77d 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Pool.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Pool.c
@@ -310,6 +310,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;
}
@@ -560,6 +564,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..172d6679857a 100644
--- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
+++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
@@ -64,8 +64,16 @@ 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;
+extern LIST_ENTRY mGcdMemorySpaceMap;
+
/**
Sort code section in image record, based upon CodeSegmentBase from low to high.
@@ -647,6 +655,251 @@ UnprotectUefiImage (
}
/**
+ Return the EFI memory permission attribute associated with memory
+ type 'MemoryType' 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;
+ LIST_ENTRY *Link;
+ EFI_GCD_MAP_ENTRY *Entry;
+
+ //
+ // 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: applying strict permissions to active 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);
+
+ //
+ // Apply the policy for RAM regions that we know are present and
+ // accessible, but have not been added to the UEFI memory map (yet).
+ //
+ if (GetPermissionAttributeForMemoryType (EfiConventionalMemory) != 0) {
+ DEBUG((DEBUG_ERROR,
+ "%a: applying strict permissions to inactive memory regions\n",
+ __FUNCTION__));
+
+ CoreAcquireGcdMemoryLock ();
+
+ Link = mGcdMemorySpaceMap.ForwardLink;
+ while (Link != &mGcdMemorySpaceMap) {
+
+ Entry = CR (Link, EFI_GCD_MAP_ENTRY, Link, EFI_GCD_MAP_SIGNATURE);
+
+ if (Entry->GcdMemoryType == EfiGcdMemoryTypeReserved &&
+ Entry->EndAddress < MAX_ADDRESS &&
+ (Entry->Capabilities & (EFI_MEMORY_PRESENT | EFI_MEMORY_INITIALIZED | EFI_MEMORY_TESTED)) ==
+ (EFI_MEMORY_PRESENT | EFI_MEMORY_INITIALIZED)) {
+
+ Attributes = GetPermissionAttributeForMemoryType (EfiConventionalMemory) |
+ (Entry->Attributes & CACHE_ATTRIBUTE_MASK);
+
+ DEBUG ((DEBUG_INFO,
+ "Untested GCD memory space region: - 0x%016lx - 0x%016lx (0x%016lx)\n",
+ Entry->BaseAddress, Entry->EndAddress - Entry->BaseAddress + 1,
+ Attributes));
+
+ ASSERT(gCpu != NULL);
+ gCpu->SetMemoryAttributes (gCpu, Entry->BaseAddress,
+ Entry->EndAddress - Entry->BaseAddress + 1, Attributes);
+ }
+
+ Link = Link->ForwardLink;
+ }
+ CoreReleaseGcdMemoryLock ();
+ }
+}
+
+
+/**
A notification for CPU_ARCH protocol.
@param[in] Event Event whose notification function is being invoked.
@@ -674,6 +927,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 +1017,19 @@ CoreInitializeMemoryProtection (
mImageProtectionPolicy = PcdGet32(PcdImageProtectionPolicy);
- if (mImageProtectionPolicy != 0) {
+ //
+ // Sanity check the PcdDxeNxMemoryProtectionPolicy setting:
+ // - code regions should have no EFI_MEMORY_XP attribute
+ // - EfiConventionalMemory and EfiBootServicesData should use the
+ // same attribute
+ //
+ ASSERT ((GetPermissionAttributeForMemoryType (EfiBootServicesCode) & EFI_MEMORY_XP) == 0);
+ ASSERT ((GetPermissionAttributeForMemoryType (EfiRuntimeServicesCode) & EFI_MEMORY_XP) == 0);
+ ASSERT ((GetPermissionAttributeForMemoryType (EfiLoaderCode) & EFI_MEMORY_XP) == 0);
+ ASSERT (GetPermissionAttributeForMemoryType (EfiBootServicesData) ==
+ GetPermissionAttributeForMemoryType (EfiConventionalMemory));
+
+ if (mImageProtectionPolicy != 0 || PcdGet64 (PcdDxeNxMemoryProtectionPolicy) != 0) {
Status = CoreCreateEvent (
EVT_NOTIFY_SIGNAL,
TPL_CALLBACK,
@@ -775,3 +1051,96 @@ CoreInitializeMemoryProtection (
}
return ;
}
+
+/**
+ Returns whether we are currently executing in SMM mode
+**/
+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, or
+ // - this is a newly added region (OldType == EfiMaxMemoryType)
+ //
+ NewAttributes = GetPermissionAttributeForMemoryType (NewType);
+
+ if (OldType != EfiMaxMemoryType) {
+ OldAttributes = GetPermissionAttributeForMemoryType (OldType);
+ if (OldAttributes == NewAttributes) {
+ // policy is the same between OldType and NewType
+ return EFI_SUCCESS;
+ }
+ } else if (NewAttributes == 0) {
+ // newly added region of a type that does not require protection
+ return EFI_SUCCESS;
+ }
+
+ return gCpu->SetMemoryAttributes (gCpu, Memory, Length, NewAttributes);
+}
--
2.7.4
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v4 7/7] ArmVirtPkg/ArmVirt.dsc.inc: enable NX memory protection for all platforms
2017-02-27 14:38 [PATCH v4 0/7] MdeModulePkg/DxeCore: increased memory protection Ard Biesheuvel
` (5 preceding siblings ...)
2017-02-27 14:38 ` [PATCH v4 6/7] MdeModulePkg/DxeCore: implement " Ard Biesheuvel
@ 2017-02-27 14:38 ` Ard Biesheuvel
2017-02-28 5:48 ` [PATCH v4 0/7] MdeModulePkg/DxeCore: increased memory protection Yao, Jiewen
2017-02-28 10:46 ` Laszlo Ersek
8 siblings, 0 replies; 26+ messages in thread
From: Ard Biesheuvel @ 2017-02-27 14:38 UTC (permalink / raw)
To: edk2-devel, afish, leif.lindholm, michael.d.kinney, liming.gao,
jiewen.yao
Cc: lersek, feng.tian, star.zeng, Ard Biesheuvel
This sets the recently introduced PCD PcdDxeNxMemoryProtectionPolicy to
a value that protects all memory regions except code regions against
inadvertent execution.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
ArmVirtPkg/ArmVirt.dsc.inc | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
index 151f413f1b2b..c9f20d570049 100644
--- a/ArmVirtPkg/ArmVirt.dsc.inc
+++ b/ArmVirtPkg/ArmVirt.dsc.inc
@@ -383,6 +383,12 @@ [PcdsFixedAtBuild.AARCH64]
#
gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy|0x3
+ #
+ # Enable NX memory protection for all non-code regions, including OEM and OS
+ # reserved ones.
+ #
+ gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy|0xC000000000007FD5
+
[Components.common]
#
# Networking stack
--
2.7.4
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v4 1/7] ArmPkg/CpuDxe: ignore attribute changes during SyncCacheConfig()
2017-02-27 14:38 ` [PATCH v4 1/7] ArmPkg/CpuDxe: ignore attribute changes during SyncCacheConfig() Ard Biesheuvel
@ 2017-02-27 15:32 ` Leif Lindholm
2017-02-27 15:33 ` Ard Biesheuvel
0 siblings, 1 reply; 26+ messages in thread
From: Leif Lindholm @ 2017-02-27 15:32 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: edk2-devel, afish, michael.d.kinney, liming.gao, jiewen.yao,
lersek, feng.tian, star.zeng
On Mon, Feb 27, 2017 at 02:38:05PM +0000, Ard Biesheuvel wrote:
> 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]
Sounds like another excellent argument for why this CpuDxe should be
cosying up with the UefiCpuPkg one longer-term.
> 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;
OK, I am unable to not bikeshed this:
The behaviour you're copying is implemented via a variable called
mIsFlushingGCD. Why change the prefix? Surely we're not looking to
export this variable any further in ARM?
>
> /**
> 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;
Eew ... this suggest we are.
/
Leif
>
> /**
> 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 [flat|nested] 26+ messages in thread
* Re: [PATCH v4 1/7] ArmPkg/CpuDxe: ignore attribute changes during SyncCacheConfig()
2017-02-27 15:32 ` Leif Lindholm
@ 2017-02-27 15:33 ` Ard Biesheuvel
2017-02-27 15:38 ` Leif Lindholm
0 siblings, 1 reply; 26+ messages in thread
From: Ard Biesheuvel @ 2017-02-27 15:33 UTC (permalink / raw)
To: Leif Lindholm
Cc: edk2-devel@lists.01.org, afish@apple.com, Kinney, Michael D,
Gao, Liming, Yao, Jiewen, Laszlo Ersek, Tian, Feng, Zeng, Star
On 27 February 2017 at 15:32, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Mon, Feb 27, 2017 at 02:38:05PM +0000, Ard Biesheuvel wrote:
>> 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]
>
> Sounds like another excellent argument for why this CpuDxe should be
> cosying up with the UefiCpuPkg one longer-term.
>
I suppose so, yes.
>> 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;
>
> OK, I am unable to not bikeshed this:
> The behaviour you're copying is implemented via a variable called
> mIsFlushingGCD. Why change the prefix? Surely we're not looking to
> export this variable any further in ARM?
>
Because it is not local the one compilation unit, that's all. It is
not in a library, so in that sense, it is guaranteed to remain local
to this module, if that is any consolation.
>>
>> /**
>> 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;
>
> Eew ... this suggest we are.
>
> /
> Leif
>
>>
>> /**
>> 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 [flat|nested] 26+ messages in thread
* Re: [PATCH v4 1/7] ArmPkg/CpuDxe: ignore attribute changes during SyncCacheConfig()
2017-02-27 15:33 ` Ard Biesheuvel
@ 2017-02-27 15:38 ` Leif Lindholm
2017-02-27 15:39 ` Ard Biesheuvel
0 siblings, 1 reply; 26+ messages in thread
From: Leif Lindholm @ 2017-02-27 15:38 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: edk2-devel@lists.01.org, afish@apple.com, Kinney, Michael D,
Gao, Liming, Yao, Jiewen, Laszlo Ersek, Tian, Feng, Zeng, Star
On Mon, Feb 27, 2017 at 03:33:56PM +0000, Ard Biesheuvel wrote:
> On 27 February 2017 at 15:32, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > On Mon, Feb 27, 2017 at 02:38:05PM +0000, Ard Biesheuvel wrote:
> >> 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]
> >
> > Sounds like another excellent argument for why this CpuDxe should be
> > cosying up with the UefiCpuPkg one longer-term.
> >
>
> I suppose so, yes.
>
> >> 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;
> >
> > OK, I am unable to not bikeshed this:
> > The behaviour you're copying is implemented via a variable called
> > mIsFlushingGCD. Why change the prefix? Surely we're not looking to
> > export this variable any further in ARM?
> >
>
> Because it is not local the one compilation unit, that's all. It is
> not in a library, so in that sense, it is guaranteed to remain local
> to this module, if that is any consolation.
Ah, excellent.
So, from how I read the coding standards (section 4.4), 'm' would
still be the appropriate prefix:
"A module variable is intended to only be accessed across a small set of
related routines that have strict rules for accessing the data; in
effect, constrained to the set of files described within a single .inf
file."
Regards,
Leif
> >>
> >> /**
> >> 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;
> >
> > Eew ... this suggest we are.
> >
> > /
> > Leif
> >
> >>
> >> /**
> >> 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 [flat|nested] 26+ messages in thread
* Re: [PATCH v4 1/7] ArmPkg/CpuDxe: ignore attribute changes during SyncCacheConfig()
2017-02-27 15:38 ` Leif Lindholm
@ 2017-02-27 15:39 ` Ard Biesheuvel
2017-02-27 15:41 ` Leif Lindholm
0 siblings, 1 reply; 26+ messages in thread
From: Ard Biesheuvel @ 2017-02-27 15:39 UTC (permalink / raw)
To: Leif Lindholm
Cc: edk2-devel@lists.01.org, afish@apple.com, Kinney, Michael D,
Gao, Liming, Yao, Jiewen, Laszlo Ersek, Tian, Feng, Zeng, Star
On 27 February 2017 at 15:38, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Mon, Feb 27, 2017 at 03:33:56PM +0000, Ard Biesheuvel wrote:
>> On 27 February 2017 at 15:32, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>> > On Mon, Feb 27, 2017 at 02:38:05PM +0000, Ard Biesheuvel wrote:
>> >> 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]
>> >
>> > Sounds like another excellent argument for why this CpuDxe should be
>> > cosying up with the UefiCpuPkg one longer-term.
>> >
>>
>> I suppose so, yes.
>>
>> >> 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;
>> >
>> > OK, I am unable to not bikeshed this:
>> > The behaviour you're copying is implemented via a variable called
>> > mIsFlushingGCD. Why change the prefix? Surely we're not looking to
>> > export this variable any further in ARM?
>> >
>>
>> Because it is not local the one compilation unit, that's all. It is
>> not in a library, so in that sense, it is guaranteed to remain local
>> to this module, if that is any consolation.
>
> Ah, excellent.
>
> So, from how I read the coding standards (section 4.4), 'm' would
> still be the appropriate prefix:
> "A module variable is intended to only be accessed across a small set of
> related routines that have strict rules for accessing the data; in
> effect, constrained to the set of files described within a single .inf
> file."
>
Ah, nice, I wasn't aware of that. I will use the m prefix instead, then.
Thanks,
Ard.
>> >>
>> >> /**
>> >> 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;
>> >
>> > Eew ... this suggest we are.
>> >
>> > /
>> > Leif
>> >
>> >>
>> >> /**
>> >> 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 [flat|nested] 26+ messages in thread
* Re: [PATCH v4 1/7] ArmPkg/CpuDxe: ignore attribute changes during SyncCacheConfig()
2017-02-27 15:39 ` Ard Biesheuvel
@ 2017-02-27 15:41 ` Leif Lindholm
0 siblings, 0 replies; 26+ messages in thread
From: Leif Lindholm @ 2017-02-27 15:41 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: edk2-devel@lists.01.org, afish@apple.com, Kinney, Michael D,
Gao, Liming, Yao, Jiewen, Laszlo Ersek, Tian, Feng, Zeng, Star
On Mon, Feb 27, 2017 at 03:39:54PM +0000, Ard Biesheuvel wrote:
> On 27 February 2017 at 15:38, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > On Mon, Feb 27, 2017 at 03:33:56PM +0000, Ard Biesheuvel wrote:
> >> On 27 February 2017 at 15:32, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> >> > On Mon, Feb 27, 2017 at 02:38:05PM +0000, Ard Biesheuvel wrote:
> >> >> 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]
> >> >
> >> > Sounds like another excellent argument for why this CpuDxe should be
> >> > cosying up with the UefiCpuPkg one longer-term.
> >> >
> >>
> >> I suppose so, yes.
> >>
> >> >> 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;
> >> >
> >> > OK, I am unable to not bikeshed this:
> >> > The behaviour you're copying is implemented via a variable called
> >> > mIsFlushingGCD. Why change the prefix? Surely we're not looking to
> >> > export this variable any further in ARM?
> >> >
> >>
> >> Because it is not local the one compilation unit, that's all. It is
> >> not in a library, so in that sense, it is guaranteed to remain local
> >> to this module, if that is any consolation.
> >
> > Ah, excellent.
> >
> > So, from how I read the coding standards (section 4.4), 'm' would
> > still be the appropriate prefix:
> > "A module variable is intended to only be accessed across a small set of
> > related routines that have strict rules for accessing the data; in
> > effect, constrained to the set of files described within a single .inf
> > file."
> >
>
> Ah, nice, I wasn't aware of that. I will use the m prefix instead, then.
In that case,
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> Thanks,
> Ard.
>
>
> >> >>
> >> >> /**
> >> >> 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;
> >> >
> >> > Eew ... this suggest we are.
> >> >
> >> > /
> >> > Leif
> >> >
> >> >>
> >> >> /**
> >> >> 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 [flat|nested] 26+ messages in thread
* Re: [PATCH v4 2/7] MdeModulePkg/PeiCore: allocate BootServicesCode memory for PE/COFF images
2017-02-27 14:38 ` [PATCH v4 2/7] MdeModulePkg/PeiCore: allocate BootServicesCode memory for PE/COFF images Ard Biesheuvel
@ 2017-02-28 5:42 ` Gao, Liming
0 siblings, 0 replies; 26+ messages in thread
From: Gao, Liming @ 2017-02-28 5:42 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel@lists.01.org, afish@apple.com,
leif.lindholm@linaro.org, Kinney, Michael D, Yao, Jiewen
Cc: lersek@redhat.com, Tian, Feng, Zeng, Star
Reviewed-by: Liming Gao <liming.gao@intel.com>
>-----Original Message-----
>From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>Sent: Monday, February 27, 2017 10:38 PM
>To: edk2-devel@lists.01.org; afish@apple.com; leif.lindholm@linaro.org;
>Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
><liming.gao@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
>Cc: lersek@redhat.com; Tian, Feng <feng.tian@intel.com>; Zeng, Star
><star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
>Subject: [PATCH v4 2/7] 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 | 23 ++++++++++++--------
> 1 file changed, 14 insertions(+), 9 deletions(-)
>
>diff --git a/MdeModulePkg/Core/Pei/Image/Image.c
>b/MdeModulePkg/Core/Pei/Image/Image.c
>index d659de8b3e64..68e40c027e63 100644
>--- a/MdeModulePkg/Core/Pei/Image/Image.c
>+++ b/MdeModulePkg/Core/Pei/Image/Image.c
>@@ -112,11 +112,12 @@ GetImageReadFunction (
> IN PE_COFF_LOADER_IMAGE_CONTEXT *ImageContext
> )
> {
>- PEI_CORE_INSTANCE *Private;
>- VOID* MemoryBuffer;
>+ PEI_CORE_INSTANCE *Private;
>+ EFI_PHYSICAL_ADDRESS MemoryBuffer;
>
> Private = PEI_CORE_INSTANCE_FROM_PS_THIS
>(GetPeiServicesTablePointer ());
>-
>+ MemoryBuffer = 0;
>+
> if (Private->PeiMemoryInstalled && (((Private-
>>HobList.HandoffInformationTable->BootMode != BOOT_ON_S3_RESUME)
>&& PcdGetBool (PcdShadowPeimOnBoot)) ||
> ((Private->HobList.HandoffInformationTable->BootMode ==
>BOOT_ON_S3_RESUME) && PcdGetBool (PcdShadowPeimOnS3Boot))) &&
> (EFI_IMAGE_MACHINE_TYPE_SUPPORTED(EFI_IMAGE_MACHINE_X64) ||
>EFI_IMAGE_MACHINE_TYPE_SUPPORTED(EFI_IMAGE_MACHINE_IA32))) {
>@@ -125,9 +126,9 @@ GetImageReadFunction (
> // compilers that have been tested
> //
> if (Private->ShadowedImageRead == NULL) {
>- MemoryBuffer = AllocatePages (0x400 / EFI_PAGE_SIZE + 1);
>- ASSERT (MemoryBuffer != NULL);
>- CopyMem (MemoryBuffer, (CONST VOID *) (UINTN)
>PeiImageReadForShadow, 0x400);
>+ PeiServicesAllocatePages (EfiBootServicesCode, 0x400 / EFI_PAGE_SIZE +
>1, &MemoryBuffer);
>+ ASSERT (MemoryBuffer != 0);
>+ CopyMem ((VOID *)(UINTN)MemoryBuffer, (CONST VOID *) (UINTN)
>PeiImageReadForShadow, 0x400);
> Private->ShadowedImageRead = (PE_COFF_LOADER_READ_FILE) (UINTN)
>MemoryBuffer;
> }
>
>@@ -453,12 +454,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 [flat|nested] 26+ messages in thread
* Re: [PATCH v4 0/7] MdeModulePkg/DxeCore: increased memory protection
2017-02-27 14:38 [PATCH v4 0/7] MdeModulePkg/DxeCore: increased memory protection Ard Biesheuvel
` (6 preceding siblings ...)
2017-02-27 14:38 ` [PATCH v4 7/7] ArmVirtPkg/ArmVirt.dsc.inc: enable NX memory protection for all platforms Ard Biesheuvel
@ 2017-02-28 5:48 ` Yao, Jiewen
2017-02-28 14:59 ` Ard Biesheuvel
2017-02-28 10:46 ` Laszlo Ersek
8 siblings, 1 reply; 26+ messages in thread
From: Yao, Jiewen @ 2017-02-28 5:48 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel@lists.01.org, afish@apple.com,
leif.lindholm@linaro.org, Kinney, Michael D, Gao, Liming
Cc: lersek@redhat.com, Tian, Feng, Zeng, Star
All series reviewed-by: jiewen.yao@intel.com
X86 platform regression tested-by: Jiewen.yao@intel.com
Thank you
Yao Jiewen
> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Monday, February 27, 2017 10:38 PM
> To: edk2-devel@lists.01.org; afish@apple.com; leif.lindholm@linaro.org; Kinney,
> Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>;
> Yao, Jiewen <jiewen.yao@intel.com>
> Cc: lersek@redhat.com; Tian, Feng <feng.tian@intel.com>; Zeng, Star
> <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: [PATCH v4 0/7] MdeModulePkg/DxeCore: increased memory protection
>
> Hello all,
>
> First of all, thanks for the reviews and regression testing. However, I did
> not add the tested-by tags nor some of the R-b's, given the changes in this v4.
>
> This series implements a memory protection policy 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 v3:
> - mandate that the same policy applies to EfiConventionalMemory regions and
> EfiBootServicesData regions: they are unlikely to differ in practice, and
> dealing with that corner case greatly complicates the implementation, given
> the way DxeCore allocates memory for itself in the implementation of the
> page
> and pool allocation routines.
> - apply the EfiConventionalMemory policy to untested RAM regions in the GCD
> memory space map: without this, we may still have a large region of RAM that
> is exploitable, and it also removes the need to apply memory protections in
> PromoteMemoryResource (), which is very difficult to achieve without a major
> restructuring of the code due to the way locking is implemented here.
> - add missing ApplyMemoryProtectionPolicy() call to
> CoreAddMemoryDescriptor()
> - use CoreAcquireLockOrFail() on gMemoryLock for CoreAllocatePoolPages (#4)
> - incorporate feedback from Liming (#2, #6)
> - add patch to enable the NX memory protection policy for ArmVirtPkg (#7)
>
> 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-v4
>
> Ard Biesheuvel (7):
> 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
> ArmVirtPkg/ArmVirt.dsc.inc: enable NX memory protection for all
> platforms
>
> ArmPkg/Drivers/CpuDxe/CpuDxe.c | 3 +
> ArmPkg/Drivers/CpuDxe/CpuDxe.h | 1 +
> ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c | 4 +
> ArmVirtPkg/ArmVirt.dsc.inc | 6 +
> MdeModulePkg/Core/Dxe/DxeMain.h | 24 ++
> MdeModulePkg/Core/Dxe/DxeMain.inf | 1 +
> MdeModulePkg/Core/Dxe/Mem/Page.c | 7 +
> MdeModulePkg/Core/Dxe/Mem/Pool.c | 65 +++-
> MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 371
> +++++++++++++++++++-
> MdeModulePkg/Core/Pei/Image/Image.c | 23 +-
> MdeModulePkg/MdeModulePkg.dec | 32 ++
> 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 +-
> 17 files changed, 558 insertions(+), 24 deletions(-)
>
> --
> 2.7.4
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 4/7] MdeModulePkg/DxeCore: use separate lock for pool allocations
2017-02-27 14:38 ` [PATCH v4 4/7] MdeModulePkg/DxeCore: use separate lock for pool allocations Ard Biesheuvel
@ 2017-02-28 9:32 ` Gao, Liming
0 siblings, 0 replies; 26+ messages in thread
From: Gao, Liming @ 2017-02-28 9:32 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel@lists.01.org, afish@apple.com,
leif.lindholm@linaro.org, Kinney, Michael D, Yao, Jiewen
Cc: lersek@redhat.com, Tian, Feng, Zeng, Star
Reviewed-by: Liming Gao <liming.gao@intel.com>
-----Original Message-----
From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
Sent: Monday, February 27, 2017 10:38 PM
To: edk2-devel@lists.01.org; afish@apple.com; leif.lindholm@linaro.org; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
Cc: lersek@redhat.com; Tian, Feng <feng.tian@intel.com>; Zeng, Star <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
Subject: [PATCH v4 4/7] 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 | 58 ++++++++++++++++----
1 file changed, 48 insertions(+), 10 deletions(-)
diff --git a/MdeModulePkg/Core/Dxe/Mem/Pool.c b/MdeModulePkg/Core/Dxe/Mem/Pool.c
index 7afd2d312c1d..ebb2fceedd80 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,28 @@ CoreAllocatePool (
return Status;
}
+STATIC
+VOID *
+CoreAllocatePoolPagesI (
+ IN EFI_MEMORY_TYPE PoolType,
+ IN UINTN NoPages,
+ IN UINTN Granularity
+ )
+{
+ VOID *Buffer;
+ EFI_STATUS Status;
+
+ Status = CoreAcquireLockOrFail (&gMemoryLock);
+ if (EFI_ERROR (Status)) {
+ return NULL;
+ }
+
+ 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 +341,7 @@ CoreAllocatePoolI (
UINTN NoPages;
UINTN Granularity;
- ASSERT_LOCKED (&gMemoryLock);
+ ASSERT_LOCKED (&mPoolMemoryLock);
if (PoolType == EfiACPIReclaimMemory ||
PoolType == EfiACPIMemoryNVS ||
@@ -355,7 +379,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 +407,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 +510,9 @@ CoreInternalFreePool (
return EFI_INVALID_PARAMETER;
}
- CoreAcquireMemoryLock ();
+ CoreAcquireLock (&mPoolMemoryLock);
Status = CoreFreePoolI (Buffer, PoolType);
- CoreReleaseMemoryLock ();
+ CoreReleaseLock (&mPoolMemoryLock);
return Status;
}
@@ -525,6 +549,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 +610,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 +661,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 +717,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] 26+ messages in thread
* Re: [PATCH v4 6/7] MdeModulePkg/DxeCore: implement memory protection policy
2017-02-27 14:38 ` [PATCH v4 6/7] MdeModulePkg/DxeCore: implement " Ard Biesheuvel
@ 2017-02-28 9:33 ` Gao, Liming
0 siblings, 0 replies; 26+ messages in thread
From: Gao, Liming @ 2017-02-28 9:33 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel@lists.01.org, afish@apple.com,
leif.lindholm@linaro.org, Kinney, Michael D, Yao, Jiewen
Cc: Tian, Feng, lersek@redhat.com, Zeng, Star
Reviewed-by: Liming Gao <liming.gao@intel.com>
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard Biesheuvel
Sent: Monday, February 27, 2017 10:38 PM
To: edk2-devel@lists.01.org; afish@apple.com; leif.lindholm@linaro.org; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
Cc: Tian, Feng <feng.tian@intel.com>; lersek@redhat.com; Zeng, Star <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
Subject: [edk2] [PATCH v4 6/7] 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 | 7 +
MdeModulePkg/Core/Dxe/Mem/Pool.c | 7 +
MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 371 +++++++++++++++++++-
5 files changed, 409 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..d596db7ad427 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Page.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
@@ -553,6 +553,9 @@ CoreAddMemoryDescriptor (
CoreFreeMemoryMapStack ();
CoreReleaseMemoryLock ();
+ ApplyMemoryProtectionPolicy (EfiMaxMemoryType, Type, Start,
+ EFI_PAGES_TO_SIZE (NumberOfPages));
+
//
// If Loading Module At Fixed Address feature is enabled. try to allocate memory with Runtime code & Boot time code type
//
@@ -1344,6 +1347,8 @@ CoreAllocatePages (
NULL
);
InstallMemoryAttributesTableOnMemoryAllocation (MemoryType);
+ ApplyMemoryProtectionPolicy (EfiConventionalMemory, MemoryType, *Memory,
+ EFI_PAGES_TO_SIZE (NumberOfPages));
}
return Status;
}
@@ -1460,6 +1465,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 ebb2fceedd80..ced64443c77d 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Pool.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Pool.c
@@ -310,6 +310,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;
}
@@ -560,6 +564,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..172d6679857a 100644
--- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
+++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
@@ -64,8 +64,16 @@ 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;
+extern LIST_ENTRY mGcdMemorySpaceMap;
+
/**
Sort code section in image record, based upon CodeSegmentBase from low to high.
@@ -647,6 +655,251 @@ UnprotectUefiImage (
}
/**
+ Return the EFI memory permission attribute associated with memory
+ type 'MemoryType' 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;
+ LIST_ENTRY *Link;
+ EFI_GCD_MAP_ENTRY *Entry;
+
+ //
+ // 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: applying strict permissions to active 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);
+
+ //
+ // Apply the policy for RAM regions that we know are present and
+ // accessible, but have not been added to the UEFI memory map (yet).
+ //
+ if (GetPermissionAttributeForMemoryType (EfiConventionalMemory) != 0) {
+ DEBUG((DEBUG_ERROR,
+ "%a: applying strict permissions to inactive memory regions\n",
+ __FUNCTION__));
+
+ CoreAcquireGcdMemoryLock ();
+
+ Link = mGcdMemorySpaceMap.ForwardLink;
+ while (Link != &mGcdMemorySpaceMap) {
+
+ Entry = CR (Link, EFI_GCD_MAP_ENTRY, Link, EFI_GCD_MAP_SIGNATURE);
+
+ if (Entry->GcdMemoryType == EfiGcdMemoryTypeReserved &&
+ Entry->EndAddress < MAX_ADDRESS &&
+ (Entry->Capabilities & (EFI_MEMORY_PRESENT | EFI_MEMORY_INITIALIZED | EFI_MEMORY_TESTED)) ==
+ (EFI_MEMORY_PRESENT | EFI_MEMORY_INITIALIZED)) {
+
+ Attributes = GetPermissionAttributeForMemoryType (EfiConventionalMemory) |
+ (Entry->Attributes & CACHE_ATTRIBUTE_MASK);
+
+ DEBUG ((DEBUG_INFO,
+ "Untested GCD memory space region: - 0x%016lx - 0x%016lx (0x%016lx)\n",
+ Entry->BaseAddress, Entry->EndAddress - Entry->BaseAddress + 1,
+ Attributes));
+
+ ASSERT(gCpu != NULL);
+ gCpu->SetMemoryAttributes (gCpu, Entry->BaseAddress,
+ Entry->EndAddress - Entry->BaseAddress + 1, Attributes);
+ }
+
+ Link = Link->ForwardLink;
+ }
+ CoreReleaseGcdMemoryLock ();
+ }
+}
+
+
+/**
A notification for CPU_ARCH protocol.
@param[in] Event Event whose notification function is being invoked.
@@ -674,6 +927,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 +1017,19 @@ CoreInitializeMemoryProtection (
mImageProtectionPolicy = PcdGet32(PcdImageProtectionPolicy);
- if (mImageProtectionPolicy != 0) {
+ //
+ // Sanity check the PcdDxeNxMemoryProtectionPolicy setting:
+ // - code regions should have no EFI_MEMORY_XP attribute
+ // - EfiConventionalMemory and EfiBootServicesData should use the
+ // same attribute
+ //
+ ASSERT ((GetPermissionAttributeForMemoryType (EfiBootServicesCode) & EFI_MEMORY_XP) == 0);
+ ASSERT ((GetPermissionAttributeForMemoryType (EfiRuntimeServicesCode) & EFI_MEMORY_XP) == 0);
+ ASSERT ((GetPermissionAttributeForMemoryType (EfiLoaderCode) & EFI_MEMORY_XP) == 0);
+ ASSERT (GetPermissionAttributeForMemoryType (EfiBootServicesData) ==
+ GetPermissionAttributeForMemoryType (EfiConventionalMemory));
+
+ if (mImageProtectionPolicy != 0 || PcdGet64 (PcdDxeNxMemoryProtectionPolicy) != 0) {
Status = CoreCreateEvent (
EVT_NOTIFY_SIGNAL,
TPL_CALLBACK,
@@ -775,3 +1051,96 @@ CoreInitializeMemoryProtection (
}
return ;
}
+
+/**
+ Returns whether we are currently executing in SMM mode
+**/
+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, or
+ // - this is a newly added region (OldType == EfiMaxMemoryType)
+ //
+ NewAttributes = GetPermissionAttributeForMemoryType (NewType);
+
+ if (OldType != EfiMaxMemoryType) {
+ OldAttributes = GetPermissionAttributeForMemoryType (OldType);
+ if (OldAttributes == NewAttributes) {
+ // policy is the same between OldType and NewType
+ return EFI_SUCCESS;
+ }
+ } else if (NewAttributes == 0) {
+ // newly added region of a type that does not require protection
+ return EFI_SUCCESS;
+ }
+
+ return gCpu->SetMemoryAttributes (gCpu, Memory, Length, NewAttributes);
+}
--
2.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] 26+ messages in thread
* Re: [PATCH v4 0/7] MdeModulePkg/DxeCore: increased memory protection
2017-02-27 14:38 [PATCH v4 0/7] MdeModulePkg/DxeCore: increased memory protection Ard Biesheuvel
` (7 preceding siblings ...)
2017-02-28 5:48 ` [PATCH v4 0/7] MdeModulePkg/DxeCore: increased memory protection Yao, Jiewen
@ 2017-02-28 10:46 ` Laszlo Ersek
2017-02-28 10:52 ` Ard Biesheuvel
8 siblings, 1 reply; 26+ messages in thread
From: Laszlo Ersek @ 2017-02-28 10:46 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel, afish, leif.lindholm,
michael.d.kinney, liming.gao, jiewen.yao
Cc: feng.tian, star.zeng
On 02/27/17 15:38, Ard Biesheuvel wrote:
> Hello all,
>
> First of all, thanks for the reviews and regression testing. However, I did
> not add the tested-by tags nor some of the R-b's, given the changes in this v4.
>
> This series implements a memory protection policy 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 v3:
> - mandate that the same policy applies to EfiConventionalMemory regions and
> EfiBootServicesData regions: they are unlikely to differ in practice, and
> dealing with that corner case greatly complicates the implementation, given
> the way DxeCore allocates memory for itself in the implementation of the page
> and pool allocation routines.
> - apply the EfiConventionalMemory policy to untested RAM regions in the GCD
> memory space map: without this, we may still have a large region of RAM that
> is exploitable, and it also removes the need to apply memory protections in
> PromoteMemoryResource (), which is very difficult to achieve without a major
> restructuring of the code due to the way locking is implemented here.
> - add missing ApplyMemoryProtectionPolicy() call to CoreAddMemoryDescriptor()
> - use CoreAcquireLockOrFail() on gMemoryLock for CoreAllocatePoolPages (#4)
> - incorporate feedback from Liming (#2, #6)
> - add patch to enable the NX memory protection policy for ArmVirtPkg (#7)
>
> 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-v4
>
> Ard Biesheuvel (7):
> 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
> ArmVirtPkg/ArmVirt.dsc.inc: enable NX memory protection for all
> platforms
>
> ArmPkg/Drivers/CpuDxe/CpuDxe.c | 3 +
> ArmPkg/Drivers/CpuDxe/CpuDxe.h | 1 +
> ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c | 4 +
> ArmVirtPkg/ArmVirt.dsc.inc | 6 +
> MdeModulePkg/Core/Dxe/DxeMain.h | 24 ++
> MdeModulePkg/Core/Dxe/DxeMain.inf | 1 +
> MdeModulePkg/Core/Dxe/Mem/Page.c | 7 +
> MdeModulePkg/Core/Dxe/Mem/Pool.c | 65 +++-
> MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 371 +++++++++++++++++++-
> MdeModulePkg/Core/Pei/Image/Image.c | 23 +-
> MdeModulePkg/MdeModulePkg.dec | 32 ++
> 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 +-
> 17 files changed, 558 insertions(+), 24 deletions(-)
>
I regression-tested this series for x86 / OVMF as under v3, with the zero PCD default, and experienced no issues.
However, v4 breaks booting Fedora 24 on my Mustang (aarch64/KVM):
-----------------
[Bds]Booting Fedora
FSOpen: Open '\EFI\fedora\shim.efi' Success
[Bds] DevicePath expand: HD(1,MBR,0xDB4976D3,0x800,0x64000)/\EFI\fedora\shim.efi -> PciRoot(0x0)/Pci(0x1,0x1)/Pci(0x0,0x0)/Scsi(0x1,0x0)/HD(1,MBR,0xDB4976D3,0x800,0x64000)/\EFI\fedora\shim.efi
[Security] 3rd party image[0] can be loaded after EndOfDxe: PciRoot(0x0)/Pci(0x1,0x1)/Pci(0x0,0x0)/Scsi(0x1,0x0)/HD(1,MBR,0xDB4976D3,0x800,0x64000)/\EFI\fedora\shim.efi.
InstallProtocolInterface: [EfiLoadedImageProtocol] 13A6D2AC0
Loading driver at 0x001382F4000 EntryPoint=0x001382F4148
Loading driver at 0x001382F4000 EntryPoint=0x001382F4148
InstallProtocolInterface: [EfiLoadedImageDevicePathProtocol] 138CDBD98
ProtectUefiImageCommon - 0x3A6D2AC0
- 0x00000001382F4000 - 0x00000000000CBAE0
!!!!!!!! ProtectUefiImageCommon - Section Alignment(0x20) is incorrect !!!!!!!!
FSOpen: Open '\EFI\fedora\grubaa64.efi' Success
Synchronous Exception at 0x00000001380F7400
X0 0x000000013A6EEA98 X1 0x000000013BFF0018 X2 0x00000001380F7400 X3 0x00000000000FD000
X4 0x0000000000000000 X5 0x0000000000000000 X6 0x0000000138362AF4 X7 0x0000000000000000
X8 0x000000013C01F548 X9 0x0000000200000000 X10 0x00000001380F6000 X11 0x00000001382F3FFF
X12 0x0000000000000000 X13 0x0000000000000008 X14 0x0000000000000000 X15 0x0000000000000000
X16 0x000000013EC6ABD0 X17 0x0000000000000000 X18 0x0000000000000000 X19 0x0000000138CDB698
X20 0x000000013A746E18 X21 0x0000000000000000 X22 0x0000000000000000 X23 0x0000000000000000
X24 0x0000000000000000 X25 0x0000000000000000 X26 0x0000000000000000 X27 0x0000000000000000
X28 0x0000000000000000 FP 0x000000013EC6AA50 LR 0x00000001382F80F8
V0 0xAFAFAFAFAFAFAFAF AFAFAFAFAFAFAFAF V1 0x6963702F66666666 6666666666666666
V2 0x697363732F312C31 406567646972622D V3 0x0000000000000000 0000000000000000
V4 0x0000000000000400 0000000000000000 V5 0x4010040140100401 4010040140100401
V6 0x0004000000000000 0004000000000000 V7 0x0000000000000000 0000000000000000
V8 0x0000000000000000 0000000000000000 V9 0x0000000000000000 0000000000000000
V10 0x0000000000000000 0000000000000000 V11 0x0000000000000000 0000000000000000
V12 0x0000000000000000 0000000000000000 V13 0x0000000000000000 0000000000000000
V14 0x0000000000000000 0000000000000000 V15 0x0000000000000000 0000000000000000
V16 0x0000000000000000 0000000000000000 V17 0x0000000000000000 0000000000000000
V18 0x0000000000000000 0000000000000000 V19 0x0000000000000000 0000000000000000
V20 0x0000000000000000 0000000000000000 V21 0x0000000000000000 0000000000000000
V22 0x0000000000000000 0000000000000000 V23 0x0000000000000000 0000000000000000
V24 0x0000000000000000 0000000000000000 V25 0x0000000000000000 0000000000000000
V26 0x0000000000000000 0000000000000000 V27 0x0000000000000000 0000000000000000
V28 0x0000000000000000 0000000000000000 V29 0x0000000000000000 0000000000000000
V30 0x0000000000000000 0000000000000000 V31 0x0000000000000000 0000000000000000
SP 0x000000013EC6AA50 ELR 0x00000001380F7400 SPSR 0x60000205 FPSR 0x00000000
ESR 0x8600000E FAR 0x00000001380F7400
ESR : EC 0x21 IL 0x1 ISS 0x0000000E
Instruction abort: Permission fault, second level
Stack dump:
000013EC6A950: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
000013EC6A970: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
000013EC6A990: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
000013EC6A9B0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
000013EC6A9D0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
000013EC6A9F0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
000013EC6AA10: 0000000000000000 0000000000000000 00000001380F7400 0000000060000205
000013EC6AA30: 0000000000000000 000000008600000E 00000001380F7400 0000000400000800
> 000013EC6AA50: 000000013EC6AB50 00000001382F8198 0000000138367370 000000013A6EEA98
000013EC6AA70: 0000000138CDB698 000FD00000000000 00000001381F5018 0000000000000000
000013EC6AA90: 0000000000000000 00000001382F3D18 0000000000001000 0000000000000000
000013EC6AAB0: 000000013BFF0018 000000013A4BA518 000000013A6D2F98 0000000000000000
000013EC6AAD0: 0000000000000000 0000000000000000 00000001382F4000 00000000000CBAE0
000013EC6AAF0: 0000000200000001 0000000000000000 000000013A6D2AC0 11D295625B1B31A1
000013EC6AB10: 3B7269C9A0003F8E 0000000000000000 00000001382F3F98 000000003EC6AB58
000013EC6AB30: 000000013EC6AB60 800000000000000E 000000013EC6AB80 0000000000000000
ASSERT [ArmCpuDxe] .../ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c(265): ((BOOLEAN)(0==1))
-----------------
The "shim.efi" binary is not built with the required section alignment, but that's not a problem, it only elicits a warning, and that's it. "shim.efi" still proceeds to load "grubaa64.efi".
However, "grubaa64.efi" blows up.
I experience the same with my "RHEL for ARM 7.3" guest.
In my "openSUSE Tumbleweed" guest, "shim.efi" isn't actually used; there the "opensuse" UEFI boot option refers to grub directly. There I even catch the message "Welcome to GRUB!", but then it crashes too:
---------------
[Bds]Booting opensuse
FSOpen: Open '\EFI\opensuse\grubaa64.efi' Success
[Bds] DevicePath expand: HD(1,GPT,3E62269B-A0A1-4F3C-B404-081796D9CDB4,0x800,0x4E000)/\EFI\opensuse\grubaa64.efi -> PciRoot(0x0)/Pci(0x1,0x1)/Pci(0x0,0x0)/Scsi(0x1,0x0)/HD(1,GPT,3E62269B-A0A1-4F3C-B404-081796D9CDB4,0x800,0x4E000)/\EFI\opensuse\grubaa64.efi
[Security] 3rd party image[0] can be loaded after EndOfDxe: PciRoot(0x0)/Pci(0x1,0x1)/Pci(0x0,0x0)/Scsi(0x1,0x0)/HD(1,GPT,3E62269B-A0A1-4F3C-B404-081796D9CDB4,0x800,0x4E000)/\EFI\opensuse\grubaa64.efi.
InstallProtocolInterface: [EfiLoadedImageProtocol] 13A69FD40
Loading driver at 0x00138391000 EntryPoint=0x00138391400
Loading driver at 0x00138391000 EntryPoint=0x00138391400
InstallProtocolInterface: [EfiLoadedImageDevicePathProtocol] 138CD4E98
ProtectUefiImageCommon - 0x3A69FD40
- 0x0000000138391000 - 0x000000000002E600
!!!!!!!! ProtectUefiImageCommon - Section Alignment(0x200) is incorrect !!!!!!!!
Welcome to GRUB!
^M
Synchronous Exception at 0x00000000FFFF1F3C
X0 0x00000000FFFF3720 X1 0x00000000FFFF1F3C X2 0x000000000000000D X3 0x000000013839FA30
X4 0x00000000FFFF1F3C X5 0x00000000FFFF0800 X6 0x00000000FFFF1000 X7 0x0000000000000000
X8 0x00000000FFFF2120 X9 0x000000000000001F X10 0x0000000000000000 X11 0x000000013EC6A880
X12 0x0098989800989898 X13 0x0098989800989898 X14 0x0000000000000001 X15 0x0000000000000003
X16 0x000000013EC6ABD0 X17 0x0000000000000000 X18 0x0000000000000000 X19 0x00000000FFFF3720
X20 0x0000000138399000 X21 0x0000000138399000 X22 0x0000000000000000 X23 0x0000000000000000
X24 0x0000000000000000 X25 0x0000000000000000 X26 0x0000000000000000 X27 0x0000000000000000
X28 0x0000000000000000 FP 0x000000013EC6AB60 LR 0x00000001383980CC
V0 0xAFAFAFAFAFAFAFAF AFAFAFAFAFAFAFAF V1 0x6963702F66666666 6666666666666666
V2 0x697363732F312C31 406567646972622D V3 0x0000000000000000 0000000000000000
V4 0x0000000000000400 0000000000000000 V5 0x4010040140100401 4010040140100401
V6 0x0004000000000000 0004000000000000 V7 0x0000000000000000 0000000000000000
V8 0x0000000000000000 0000000000000000 V9 0x0000000000000000 0000000000000000
V10 0x0000000000000000 0000000000000000 V11 0x0000000000000000 0000000000000000
V12 0x0000000000000000 0000000000000000 V13 0x0000000000000000 0000000000000000
V14 0x0000000000000000 0000000000000000 V15 0x0000000000000000 0000000000000000
V16 0x0000000000000000 0000000000000000 V17 0x0000000000000000 0000000000000000
V18 0x0000000000000000 0000000000000000 V19 0x0000000000000000 0000000000000000
V20 0x0000000000000000 0000000000000000 V21 0x0000000000000000 0000000000000000
V22 0x0000000000000000 0000000000000000 V23 0x0000000000000000 0000000000000000
V24 0x0000000000000000 0000000000000000 V25 0x0000000000000000 0000000000000000
V26 0x0000000000000000 0000000000000000 V27 0x0000000000000000 0000000000000000
V28 0x0000000000000000 0000000000000000 V29 0x0000000000000000 0000000000000000
V30 0x0000000000000000 0000000000000000 V31 0x0000000000000000 0000000000000000
SP 0x000000013EC6AB60 ELR 0x00000000FFFF1F3C SPSR 0x60000205 FPSR 0x00000000
ESR 0x8600000D FAR 0x00000000FFFF1F3C
ESR : EC 0x21 IL 0x1 ISS 0x0000000D
Instruction abort: Permission fault, first level
Stack dump:
000013EC6AA60: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
000013EC6AA80: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
000013EC6AAA0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
000013EC6AAC0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
000013EC6AAE0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
000013EC6AB00: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
000013EC6AB20: 0000000000000000 0000000000000000 00000000FFFF1F3C 0000000060000205
000013EC6AB40: 0000000000000000 000000008600000D 00000000FFFF1F3C 0000000000000000
> 000013EC6AB60: 000000013EC6AB80 0000000138399650 00000001383AAA18 0000000000000000
000013EC6AB80: 000000013EC6ABD0 000000013EC71E50 000000013A732218 000000013A747718
000013EC6ABA0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
000013EC6ABC0: 0000000000000000 000000013A747718 000000013EC6AC50 000000013BA5C62C
000013EC6ABE0: 000000013EC6AC00 0000000138CD4060 0000000138CD4068 000000013A6EE018
000013EC6AC00: 000000013EC6AC30 000000013BA577B4 0000000000000000 0000000000000000
000013EC6AC20: 000000013ECA83D8 0000000000000126 000000013EC6AC50 000000013A69FD18
000013EC6AC40: 000000013A6EE018 0000000000000000 000000013EC6ACF0 000000013BA4AF88
ASSERT [ArmCpuDxe] .../ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c(265): ((BOOLEAN)(0==1))
---------------
If I revert the last patch in the series (leaving ArmVirtQemu's PCD at the default 0), then all three guests boot fine.
This reminds me of the case when Star introduced the non-executable stack for DXE, we turned it on in OVMF, and that killed the grub version shipped with an older Debian release:
https://www.mail-archive.com/edk2-devel@lists.01.org/msg02022.html
So ultimately we had to make the PCD dynamic, and make it default to "off" (see commit ab081a50e565, "OvmfPkg: PlatformPei: take no-exec DXE settings from the QEMU command line", 2015-09-15).
Thanks
Laszlo
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 0/7] MdeModulePkg/DxeCore: increased memory protection
2017-02-28 10:46 ` Laszlo Ersek
@ 2017-02-28 10:52 ` Ard Biesheuvel
2017-02-28 10:59 ` Ard Biesheuvel
0 siblings, 1 reply; 26+ messages in thread
From: Ard Biesheuvel @ 2017-02-28 10:52 UTC (permalink / raw)
To: Laszlo Ersek, Leif Lindholm
Cc: edk2-devel@lists.01.org, afish@apple.com, Kinney, Michael D,
Gao, Liming, Yao, Jiewen, Tian, Feng, Zeng, Star
On 28 February 2017 at 10:46, Laszlo Ersek <lersek@redhat.com> wrote:
> On 02/27/17 15:38, Ard Biesheuvel wrote:
>> Hello all,
>>
>> First of all, thanks for the reviews and regression testing. However, I did
>> not add the tested-by tags nor some of the R-b's, given the changes in this v4.
>>
>> This series implements a memory protection policy 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 v3:
>> - mandate that the same policy applies to EfiConventionalMemory regions and
>> EfiBootServicesData regions: they are unlikely to differ in practice, and
>> dealing with that corner case greatly complicates the implementation, given
>> the way DxeCore allocates memory for itself in the implementation of the page
>> and pool allocation routines.
>> - apply the EfiConventionalMemory policy to untested RAM regions in the GCD
>> memory space map: without this, we may still have a large region of RAM that
>> is exploitable, and it also removes the need to apply memory protections in
>> PromoteMemoryResource (), which is very difficult to achieve without a major
>> restructuring of the code due to the way locking is implemented here.
>> - add missing ApplyMemoryProtectionPolicy() call to CoreAddMemoryDescriptor()
>> - use CoreAcquireLockOrFail() on gMemoryLock for CoreAllocatePoolPages (#4)
>> - incorporate feedback from Liming (#2, #6)
>> - add patch to enable the NX memory protection policy for ArmVirtPkg (#7)
>>
>> 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-v4
>>
>> Ard Biesheuvel (7):
>> 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
>> ArmVirtPkg/ArmVirt.dsc.inc: enable NX memory protection for all
>> platforms
>>
>> ArmPkg/Drivers/CpuDxe/CpuDxe.c | 3 +
>> ArmPkg/Drivers/CpuDxe/CpuDxe.h | 1 +
>> ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c | 4 +
>> ArmVirtPkg/ArmVirt.dsc.inc | 6 +
>> MdeModulePkg/Core/Dxe/DxeMain.h | 24 ++
>> MdeModulePkg/Core/Dxe/DxeMain.inf | 1 +
>> MdeModulePkg/Core/Dxe/Mem/Page.c | 7 +
>> MdeModulePkg/Core/Dxe/Mem/Pool.c | 65 +++-
>> MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 371 +++++++++++++++++++-
>> MdeModulePkg/Core/Pei/Image/Image.c | 23 +-
>> MdeModulePkg/MdeModulePkg.dec | 32 ++
>> 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 +-
>> 17 files changed, 558 insertions(+), 24 deletions(-)
>>
>
> I regression-tested this series for x86 / OVMF as under v3, with the zero PCD default, and experienced no issues.
>
> However, v4 breaks booting Fedora 24 on my Mustang (aarch64/KVM):
>
> -----------------
> [Bds]Booting Fedora
> FSOpen: Open '\EFI\fedora\shim.efi' Success
> [Bds] DevicePath expand: HD(1,MBR,0xDB4976D3,0x800,0x64000)/\EFI\fedora\shim.efi -> PciRoot(0x0)/Pci(0x1,0x1)/Pci(0x0,0x0)/Scsi(0x1,0x0)/HD(1,MBR,0xDB4976D3,0x800,0x64000)/\EFI\fedora\shim.efi
> [Security] 3rd party image[0] can be loaded after EndOfDxe: PciRoot(0x0)/Pci(0x1,0x1)/Pci(0x0,0x0)/Scsi(0x1,0x0)/HD(1,MBR,0xDB4976D3,0x800,0x64000)/\EFI\fedora\shim.efi.
> InstallProtocolInterface: [EfiLoadedImageProtocol] 13A6D2AC0
> Loading driver at 0x001382F4000 EntryPoint=0x001382F4148
> Loading driver at 0x001382F4000 EntryPoint=0x001382F4148
> InstallProtocolInterface: [EfiLoadedImageDevicePathProtocol] 138CDBD98
> ProtectUefiImageCommon - 0x3A6D2AC0
> - 0x00000001382F4000 - 0x00000000000CBAE0
> !!!!!!!! ProtectUefiImageCommon - Section Alignment(0x20) is incorrect !!!!!!!!
> FSOpen: Open '\EFI\fedora\grubaa64.efi' Success
>
>
> Synchronous Exception at 0x00000001380F7400
>
> X0 0x000000013A6EEA98 X1 0x000000013BFF0018 X2 0x00000001380F7400 X3 0x00000000000FD000
> X4 0x0000000000000000 X5 0x0000000000000000 X6 0x0000000138362AF4 X7 0x0000000000000000
> X8 0x000000013C01F548 X9 0x0000000200000000 X10 0x00000001380F6000 X11 0x00000001382F3FFF
> X12 0x0000000000000000 X13 0x0000000000000008 X14 0x0000000000000000 X15 0x0000000000000000
> X16 0x000000013EC6ABD0 X17 0x0000000000000000 X18 0x0000000000000000 X19 0x0000000138CDB698
> X20 0x000000013A746E18 X21 0x0000000000000000 X22 0x0000000000000000 X23 0x0000000000000000
> X24 0x0000000000000000 X25 0x0000000000000000 X26 0x0000000000000000 X27 0x0000000000000000
> X28 0x0000000000000000 FP 0x000000013EC6AA50 LR 0x00000001382F80F8
>
> V0 0xAFAFAFAFAFAFAFAF AFAFAFAFAFAFAFAF V1 0x6963702F66666666 6666666666666666
> V2 0x697363732F312C31 406567646972622D V3 0x0000000000000000 0000000000000000
> V4 0x0000000000000400 0000000000000000 V5 0x4010040140100401 4010040140100401
> V6 0x0004000000000000 0004000000000000 V7 0x0000000000000000 0000000000000000
> V8 0x0000000000000000 0000000000000000 V9 0x0000000000000000 0000000000000000
> V10 0x0000000000000000 0000000000000000 V11 0x0000000000000000 0000000000000000
> V12 0x0000000000000000 0000000000000000 V13 0x0000000000000000 0000000000000000
> V14 0x0000000000000000 0000000000000000 V15 0x0000000000000000 0000000000000000
> V16 0x0000000000000000 0000000000000000 V17 0x0000000000000000 0000000000000000
> V18 0x0000000000000000 0000000000000000 V19 0x0000000000000000 0000000000000000
> V20 0x0000000000000000 0000000000000000 V21 0x0000000000000000 0000000000000000
> V22 0x0000000000000000 0000000000000000 V23 0x0000000000000000 0000000000000000
> V24 0x0000000000000000 0000000000000000 V25 0x0000000000000000 0000000000000000
> V26 0x0000000000000000 0000000000000000 V27 0x0000000000000000 0000000000000000
> V28 0x0000000000000000 0000000000000000 V29 0x0000000000000000 0000000000000000
> V30 0x0000000000000000 0000000000000000 V31 0x0000000000000000 0000000000000000
>
> SP 0x000000013EC6AA50 ELR 0x00000001380F7400 SPSR 0x60000205 FPSR 0x00000000
> ESR 0x8600000E FAR 0x00000001380F7400
>
> ESR : EC 0x21 IL 0x1 ISS 0x0000000E
>
> Instruction abort: Permission fault, second level
>
Hmm, that is disappointing. This is probably due to GRUB's modular
nature, which means it allocates memory and loads executable code into
it, under the assumption that memory is always executable in UEFI.
The short term fix is to remove the NX bit from LoaderData regions,
but in the mean time, I will work with Leif to get this fixed properly
(assuming there is a proper way to fix this)
Thanks a lot for taking the time to test this,
Ard.
> Stack dump:
> 000013EC6A950: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> 000013EC6A970: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> 000013EC6A990: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> 000013EC6A9B0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> 000013EC6A9D0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> 000013EC6A9F0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> 000013EC6AA10: 0000000000000000 0000000000000000 00000001380F7400 0000000060000205
> 000013EC6AA30: 0000000000000000 000000008600000E 00000001380F7400 0000000400000800
>> 000013EC6AA50: 000000013EC6AB50 00000001382F8198 0000000138367370 000000013A6EEA98
> 000013EC6AA70: 0000000138CDB698 000FD00000000000 00000001381F5018 0000000000000000
> 000013EC6AA90: 0000000000000000 00000001382F3D18 0000000000001000 0000000000000000
> 000013EC6AAB0: 000000013BFF0018 000000013A4BA518 000000013A6D2F98 0000000000000000
> 000013EC6AAD0: 0000000000000000 0000000000000000 00000001382F4000 00000000000CBAE0
> 000013EC6AAF0: 0000000200000001 0000000000000000 000000013A6D2AC0 11D295625B1B31A1
> 000013EC6AB10: 3B7269C9A0003F8E 0000000000000000 00000001382F3F98 000000003EC6AB58
> 000013EC6AB30: 000000013EC6AB60 800000000000000E 000000013EC6AB80 0000000000000000
> ASSERT [ArmCpuDxe] .../ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c(265): ((BOOLEAN)(0==1))
> -----------------
>
> The "shim.efi" binary is not built with the required section alignment, but that's not a problem, it only elicits a warning, and that's it. "shim.efi" still proceeds to load "grubaa64.efi".
>
> However, "grubaa64.efi" blows up.
>
> I experience the same with my "RHEL for ARM 7.3" guest.
>
> In my "openSUSE Tumbleweed" guest, "shim.efi" isn't actually used; there the "opensuse" UEFI boot option refers to grub directly. There I even catch the message "Welcome to GRUB!", but then it crashes too:
>
> ---------------
> [Bds]Booting opensuse
> FSOpen: Open '\EFI\opensuse\grubaa64.efi' Success
> [Bds] DevicePath expand: HD(1,GPT,3E62269B-A0A1-4F3C-B404-081796D9CDB4,0x800,0x4E000)/\EFI\opensuse\grubaa64.efi -> PciRoot(0x0)/Pci(0x1,0x1)/Pci(0x0,0x0)/Scsi(0x1,0x0)/HD(1,GPT,3E62269B-A0A1-4F3C-B404-081796D9CDB4,0x800,0x4E000)/\EFI\opensuse\grubaa64.efi
> [Security] 3rd party image[0] can be loaded after EndOfDxe: PciRoot(0x0)/Pci(0x1,0x1)/Pci(0x0,0x0)/Scsi(0x1,0x0)/HD(1,GPT,3E62269B-A0A1-4F3C-B404-081796D9CDB4,0x800,0x4E000)/\EFI\opensuse\grubaa64.efi.
> InstallProtocolInterface: [EfiLoadedImageProtocol] 13A69FD40
> Loading driver at 0x00138391000 EntryPoint=0x00138391400
> Loading driver at 0x00138391000 EntryPoint=0x00138391400
> InstallProtocolInterface: [EfiLoadedImageDevicePathProtocol] 138CD4E98
> ProtectUefiImageCommon - 0x3A69FD40
> - 0x0000000138391000 - 0x000000000002E600
> !!!!!!!! ProtectUefiImageCommon - Section Alignment(0x200) is incorrect !!!!!!!!
> Welcome to GRUB!
>
> ^M
>
> Synchronous Exception at 0x00000000FFFF1F3C
>
> X0 0x00000000FFFF3720 X1 0x00000000FFFF1F3C X2 0x000000000000000D X3 0x000000013839FA30
> X4 0x00000000FFFF1F3C X5 0x00000000FFFF0800 X6 0x00000000FFFF1000 X7 0x0000000000000000
> X8 0x00000000FFFF2120 X9 0x000000000000001F X10 0x0000000000000000 X11 0x000000013EC6A880
> X12 0x0098989800989898 X13 0x0098989800989898 X14 0x0000000000000001 X15 0x0000000000000003
> X16 0x000000013EC6ABD0 X17 0x0000000000000000 X18 0x0000000000000000 X19 0x00000000FFFF3720
> X20 0x0000000138399000 X21 0x0000000138399000 X22 0x0000000000000000 X23 0x0000000000000000
> X24 0x0000000000000000 X25 0x0000000000000000 X26 0x0000000000000000 X27 0x0000000000000000
> X28 0x0000000000000000 FP 0x000000013EC6AB60 LR 0x00000001383980CC
>
> V0 0xAFAFAFAFAFAFAFAF AFAFAFAFAFAFAFAF V1 0x6963702F66666666 6666666666666666
> V2 0x697363732F312C31 406567646972622D V3 0x0000000000000000 0000000000000000
> V4 0x0000000000000400 0000000000000000 V5 0x4010040140100401 4010040140100401
> V6 0x0004000000000000 0004000000000000 V7 0x0000000000000000 0000000000000000
> V8 0x0000000000000000 0000000000000000 V9 0x0000000000000000 0000000000000000
> V10 0x0000000000000000 0000000000000000 V11 0x0000000000000000 0000000000000000
> V12 0x0000000000000000 0000000000000000 V13 0x0000000000000000 0000000000000000
> V14 0x0000000000000000 0000000000000000 V15 0x0000000000000000 0000000000000000
> V16 0x0000000000000000 0000000000000000 V17 0x0000000000000000 0000000000000000
> V18 0x0000000000000000 0000000000000000 V19 0x0000000000000000 0000000000000000
> V20 0x0000000000000000 0000000000000000 V21 0x0000000000000000 0000000000000000
> V22 0x0000000000000000 0000000000000000 V23 0x0000000000000000 0000000000000000
> V24 0x0000000000000000 0000000000000000 V25 0x0000000000000000 0000000000000000
> V26 0x0000000000000000 0000000000000000 V27 0x0000000000000000 0000000000000000
> V28 0x0000000000000000 0000000000000000 V29 0x0000000000000000 0000000000000000
> V30 0x0000000000000000 0000000000000000 V31 0x0000000000000000 0000000000000000
>
> SP 0x000000013EC6AB60 ELR 0x00000000FFFF1F3C SPSR 0x60000205 FPSR 0x00000000
> ESR 0x8600000D FAR 0x00000000FFFF1F3C
>
> ESR : EC 0x21 IL 0x1 ISS 0x0000000D
>
> Instruction abort: Permission fault, first level
>
> Stack dump:
> 000013EC6AA60: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> 000013EC6AA80: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> 000013EC6AAA0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> 000013EC6AAC0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> 000013EC6AAE0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> 000013EC6AB00: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> 000013EC6AB20: 0000000000000000 0000000000000000 00000000FFFF1F3C 0000000060000205
> 000013EC6AB40: 0000000000000000 000000008600000D 00000000FFFF1F3C 0000000000000000
>> 000013EC6AB60: 000000013EC6AB80 0000000138399650 00000001383AAA18 0000000000000000
> 000013EC6AB80: 000000013EC6ABD0 000000013EC71E50 000000013A732218 000000013A747718
> 000013EC6ABA0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> 000013EC6ABC0: 0000000000000000 000000013A747718 000000013EC6AC50 000000013BA5C62C
> 000013EC6ABE0: 000000013EC6AC00 0000000138CD4060 0000000138CD4068 000000013A6EE018
> 000013EC6AC00: 000000013EC6AC30 000000013BA577B4 0000000000000000 0000000000000000
> 000013EC6AC20: 000000013ECA83D8 0000000000000126 000000013EC6AC50 000000013A69FD18
> 000013EC6AC40: 000000013A6EE018 0000000000000000 000000013EC6ACF0 000000013BA4AF88
> ASSERT [ArmCpuDxe] .../ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c(265): ((BOOLEAN)(0==1))
> ---------------
>
> If I revert the last patch in the series (leaving ArmVirtQemu's PCD at the default 0), then all three guests boot fine.
>
> This reminds me of the case when Star introduced the non-executable stack for DXE, we turned it on in OVMF, and that killed the grub version shipped with an older Debian release:
>
> https://www.mail-archive.com/edk2-devel@lists.01.org/msg02022.html
>
> So ultimately we had to make the PCD dynamic, and make it default to "off" (see commit ab081a50e565, "OvmfPkg: PlatformPei: take no-exec DXE settings from the QEMU command line", 2015-09-15).
>
> Thanks
> Laszlo
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 0/7] MdeModulePkg/DxeCore: increased memory protection
2017-02-28 10:52 ` Ard Biesheuvel
@ 2017-02-28 10:59 ` Ard Biesheuvel
2017-02-28 11:47 ` Ard Biesheuvel
2017-02-28 23:46 ` Laszlo Ersek
0 siblings, 2 replies; 26+ messages in thread
From: Ard Biesheuvel @ 2017-02-28 10:59 UTC (permalink / raw)
To: Laszlo Ersek, Leif Lindholm
Cc: edk2-devel@lists.01.org, afish@apple.com, Kinney, Michael D,
Gao, Liming, Yao, Jiewen, Tian, Feng, Zeng, Star
On 28 February 2017 at 10:52, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 28 February 2017 at 10:46, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 02/27/17 15:38, Ard Biesheuvel wrote:
>>> Hello all,
>>>
>>> First of all, thanks for the reviews and regression testing. However, I did
>>> not add the tested-by tags nor some of the R-b's, given the changes in this v4.
>>>
>>> This series implements a memory protection policy 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 v3:
>>> - mandate that the same policy applies to EfiConventionalMemory regions and
>>> EfiBootServicesData regions: they are unlikely to differ in practice, and
>>> dealing with that corner case greatly complicates the implementation, given
>>> the way DxeCore allocates memory for itself in the implementation of the page
>>> and pool allocation routines.
>>> - apply the EfiConventionalMemory policy to untested RAM regions in the GCD
>>> memory space map: without this, we may still have a large region of RAM that
>>> is exploitable, and it also removes the need to apply memory protections in
>>> PromoteMemoryResource (), which is very difficult to achieve without a major
>>> restructuring of the code due to the way locking is implemented here.
>>> - add missing ApplyMemoryProtectionPolicy() call to CoreAddMemoryDescriptor()
>>> - use CoreAcquireLockOrFail() on gMemoryLock for CoreAllocatePoolPages (#4)
>>> - incorporate feedback from Liming (#2, #6)
>>> - add patch to enable the NX memory protection policy for ArmVirtPkg (#7)
>>>
>>> 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-v4
>>>
>>> Ard Biesheuvel (7):
>>> 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
>>> ArmVirtPkg/ArmVirt.dsc.inc: enable NX memory protection for all
>>> platforms
>>>
>>> ArmPkg/Drivers/CpuDxe/CpuDxe.c | 3 +
>>> ArmPkg/Drivers/CpuDxe/CpuDxe.h | 1 +
>>> ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c | 4 +
>>> ArmVirtPkg/ArmVirt.dsc.inc | 6 +
>>> MdeModulePkg/Core/Dxe/DxeMain.h | 24 ++
>>> MdeModulePkg/Core/Dxe/DxeMain.inf | 1 +
>>> MdeModulePkg/Core/Dxe/Mem/Page.c | 7 +
>>> MdeModulePkg/Core/Dxe/Mem/Pool.c | 65 +++-
>>> MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 371 +++++++++++++++++++-
>>> MdeModulePkg/Core/Pei/Image/Image.c | 23 +-
>>> MdeModulePkg/MdeModulePkg.dec | 32 ++
>>> 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 +-
>>> 17 files changed, 558 insertions(+), 24 deletions(-)
>>>
>>
>> I regression-tested this series for x86 / OVMF as under v3, with the zero PCD default, and experienced no issues.
>>
>> However, v4 breaks booting Fedora 24 on my Mustang (aarch64/KVM):
>>
>> -----------------
>> [Bds]Booting Fedora
>> FSOpen: Open '\EFI\fedora\shim.efi' Success
>> [Bds] DevicePath expand: HD(1,MBR,0xDB4976D3,0x800,0x64000)/\EFI\fedora\shim.efi -> PciRoot(0x0)/Pci(0x1,0x1)/Pci(0x0,0x0)/Scsi(0x1,0x0)/HD(1,MBR,0xDB4976D3,0x800,0x64000)/\EFI\fedora\shim.efi
>> [Security] 3rd party image[0] can be loaded after EndOfDxe: PciRoot(0x0)/Pci(0x1,0x1)/Pci(0x0,0x0)/Scsi(0x1,0x0)/HD(1,MBR,0xDB4976D3,0x800,0x64000)/\EFI\fedora\shim.efi.
>> InstallProtocolInterface: [EfiLoadedImageProtocol] 13A6D2AC0
>> Loading driver at 0x001382F4000 EntryPoint=0x001382F4148
>> Loading driver at 0x001382F4000 EntryPoint=0x001382F4148
>> InstallProtocolInterface: [EfiLoadedImageDevicePathProtocol] 138CDBD98
>> ProtectUefiImageCommon - 0x3A6D2AC0
>> - 0x00000001382F4000 - 0x00000000000CBAE0
>> !!!!!!!! ProtectUefiImageCommon - Section Alignment(0x20) is incorrect !!!!!!!!
>> FSOpen: Open '\EFI\fedora\grubaa64.efi' Success
>>
>>
>> Synchronous Exception at 0x00000001380F7400
>>
>> X0 0x000000013A6EEA98 X1 0x000000013BFF0018 X2 0x00000001380F7400 X3 0x00000000000FD000
>> X4 0x0000000000000000 X5 0x0000000000000000 X6 0x0000000138362AF4 X7 0x0000000000000000
>> X8 0x000000013C01F548 X9 0x0000000200000000 X10 0x00000001380F6000 X11 0x00000001382F3FFF
>> X12 0x0000000000000000 X13 0x0000000000000008 X14 0x0000000000000000 X15 0x0000000000000000
>> X16 0x000000013EC6ABD0 X17 0x0000000000000000 X18 0x0000000000000000 X19 0x0000000138CDB698
>> X20 0x000000013A746E18 X21 0x0000000000000000 X22 0x0000000000000000 X23 0x0000000000000000
>> X24 0x0000000000000000 X25 0x0000000000000000 X26 0x0000000000000000 X27 0x0000000000000000
>> X28 0x0000000000000000 FP 0x000000013EC6AA50 LR 0x00000001382F80F8
>>
>> V0 0xAFAFAFAFAFAFAFAF AFAFAFAFAFAFAFAF V1 0x6963702F66666666 6666666666666666
>> V2 0x697363732F312C31 406567646972622D V3 0x0000000000000000 0000000000000000
>> V4 0x0000000000000400 0000000000000000 V5 0x4010040140100401 4010040140100401
>> V6 0x0004000000000000 0004000000000000 V7 0x0000000000000000 0000000000000000
>> V8 0x0000000000000000 0000000000000000 V9 0x0000000000000000 0000000000000000
>> V10 0x0000000000000000 0000000000000000 V11 0x0000000000000000 0000000000000000
>> V12 0x0000000000000000 0000000000000000 V13 0x0000000000000000 0000000000000000
>> V14 0x0000000000000000 0000000000000000 V15 0x0000000000000000 0000000000000000
>> V16 0x0000000000000000 0000000000000000 V17 0x0000000000000000 0000000000000000
>> V18 0x0000000000000000 0000000000000000 V19 0x0000000000000000 0000000000000000
>> V20 0x0000000000000000 0000000000000000 V21 0x0000000000000000 0000000000000000
>> V22 0x0000000000000000 0000000000000000 V23 0x0000000000000000 0000000000000000
>> V24 0x0000000000000000 0000000000000000 V25 0x0000000000000000 0000000000000000
>> V26 0x0000000000000000 0000000000000000 V27 0x0000000000000000 0000000000000000
>> V28 0x0000000000000000 0000000000000000 V29 0x0000000000000000 0000000000000000
>> V30 0x0000000000000000 0000000000000000 V31 0x0000000000000000 0000000000000000
>>
>> SP 0x000000013EC6AA50 ELR 0x00000001380F7400 SPSR 0x60000205 FPSR 0x00000000
>> ESR 0x8600000E FAR 0x00000001380F7400
>>
>> ESR : EC 0x21 IL 0x1 ISS 0x0000000E
>>
>> Instruction abort: Permission fault, second level
>>
>
> Hmm, that is disappointing. This is probably due to GRUB's modular
> nature, which means it allocates memory and loads executable code into
> it, under the assumption that memory is always executable in UEFI.
>
> The short term fix is to remove the NX bit from LoaderData regions,
> but in the mean time, I will work with Leif to get this fixed properly
> (assuming there is a proper way to fix this)
>
Care to have a quick go at using 0xC000000000007FD1 instead? (if you
are not already doing so)
Thanks,
Ard.
>> Stack dump:
>> 000013EC6A950: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
>> 000013EC6A970: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
>> 000013EC6A990: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
>> 000013EC6A9B0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
>> 000013EC6A9D0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
>> 000013EC6A9F0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
>> 000013EC6AA10: 0000000000000000 0000000000000000 00000001380F7400 0000000060000205
>> 000013EC6AA30: 0000000000000000 000000008600000E 00000001380F7400 0000000400000800
>>> 000013EC6AA50: 000000013EC6AB50 00000001382F8198 0000000138367370 000000013A6EEA98
>> 000013EC6AA70: 0000000138CDB698 000FD00000000000 00000001381F5018 0000000000000000
>> 000013EC6AA90: 0000000000000000 00000001382F3D18 0000000000001000 0000000000000000
>> 000013EC6AAB0: 000000013BFF0018 000000013A4BA518 000000013A6D2F98 0000000000000000
>> 000013EC6AAD0: 0000000000000000 0000000000000000 00000001382F4000 00000000000CBAE0
>> 000013EC6AAF0: 0000000200000001 0000000000000000 000000013A6D2AC0 11D295625B1B31A1
>> 000013EC6AB10: 3B7269C9A0003F8E 0000000000000000 00000001382F3F98 000000003EC6AB58
>> 000013EC6AB30: 000000013EC6AB60 800000000000000E 000000013EC6AB80 0000000000000000
>> ASSERT [ArmCpuDxe] .../ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c(265): ((BOOLEAN)(0==1))
>> -----------------
>>
>> The "shim.efi" binary is not built with the required section alignment, but that's not a problem, it only elicits a warning, and that's it. "shim.efi" still proceeds to load "grubaa64.efi".
>>
>> However, "grubaa64.efi" blows up.
>>
>> I experience the same with my "RHEL for ARM 7.3" guest.
>>
>> In my "openSUSE Tumbleweed" guest, "shim.efi" isn't actually used; there the "opensuse" UEFI boot option refers to grub directly. There I even catch the message "Welcome to GRUB!", but then it crashes too:
>>
>> ---------------
>> [Bds]Booting opensuse
>> FSOpen: Open '\EFI\opensuse\grubaa64.efi' Success
>> [Bds] DevicePath expand: HD(1,GPT,3E62269B-A0A1-4F3C-B404-081796D9CDB4,0x800,0x4E000)/\EFI\opensuse\grubaa64.efi -> PciRoot(0x0)/Pci(0x1,0x1)/Pci(0x0,0x0)/Scsi(0x1,0x0)/HD(1,GPT,3E62269B-A0A1-4F3C-B404-081796D9CDB4,0x800,0x4E000)/\EFI\opensuse\grubaa64.efi
>> [Security] 3rd party image[0] can be loaded after EndOfDxe: PciRoot(0x0)/Pci(0x1,0x1)/Pci(0x0,0x0)/Scsi(0x1,0x0)/HD(1,GPT,3E62269B-A0A1-4F3C-B404-081796D9CDB4,0x800,0x4E000)/\EFI\opensuse\grubaa64.efi.
>> InstallProtocolInterface: [EfiLoadedImageProtocol] 13A69FD40
>> Loading driver at 0x00138391000 EntryPoint=0x00138391400
>> Loading driver at 0x00138391000 EntryPoint=0x00138391400
>> InstallProtocolInterface: [EfiLoadedImageDevicePathProtocol] 138CD4E98
>> ProtectUefiImageCommon - 0x3A69FD40
>> - 0x0000000138391000 - 0x000000000002E600
>> !!!!!!!! ProtectUefiImageCommon - Section Alignment(0x200) is incorrect !!!!!!!!
>> Welcome to GRUB!
>>
>> ^M
>>
>> Synchronous Exception at 0x00000000FFFF1F3C
>>
>> X0 0x00000000FFFF3720 X1 0x00000000FFFF1F3C X2 0x000000000000000D X3 0x000000013839FA30
>> X4 0x00000000FFFF1F3C X5 0x00000000FFFF0800 X6 0x00000000FFFF1000 X7 0x0000000000000000
>> X8 0x00000000FFFF2120 X9 0x000000000000001F X10 0x0000000000000000 X11 0x000000013EC6A880
>> X12 0x0098989800989898 X13 0x0098989800989898 X14 0x0000000000000001 X15 0x0000000000000003
>> X16 0x000000013EC6ABD0 X17 0x0000000000000000 X18 0x0000000000000000 X19 0x00000000FFFF3720
>> X20 0x0000000138399000 X21 0x0000000138399000 X22 0x0000000000000000 X23 0x0000000000000000
>> X24 0x0000000000000000 X25 0x0000000000000000 X26 0x0000000000000000 X27 0x0000000000000000
>> X28 0x0000000000000000 FP 0x000000013EC6AB60 LR 0x00000001383980CC
>>
>> V0 0xAFAFAFAFAFAFAFAF AFAFAFAFAFAFAFAF V1 0x6963702F66666666 6666666666666666
>> V2 0x697363732F312C31 406567646972622D V3 0x0000000000000000 0000000000000000
>> V4 0x0000000000000400 0000000000000000 V5 0x4010040140100401 4010040140100401
>> V6 0x0004000000000000 0004000000000000 V7 0x0000000000000000 0000000000000000
>> V8 0x0000000000000000 0000000000000000 V9 0x0000000000000000 0000000000000000
>> V10 0x0000000000000000 0000000000000000 V11 0x0000000000000000 0000000000000000
>> V12 0x0000000000000000 0000000000000000 V13 0x0000000000000000 0000000000000000
>> V14 0x0000000000000000 0000000000000000 V15 0x0000000000000000 0000000000000000
>> V16 0x0000000000000000 0000000000000000 V17 0x0000000000000000 0000000000000000
>> V18 0x0000000000000000 0000000000000000 V19 0x0000000000000000 0000000000000000
>> V20 0x0000000000000000 0000000000000000 V21 0x0000000000000000 0000000000000000
>> V22 0x0000000000000000 0000000000000000 V23 0x0000000000000000 0000000000000000
>> V24 0x0000000000000000 0000000000000000 V25 0x0000000000000000 0000000000000000
>> V26 0x0000000000000000 0000000000000000 V27 0x0000000000000000 0000000000000000
>> V28 0x0000000000000000 0000000000000000 V29 0x0000000000000000 0000000000000000
>> V30 0x0000000000000000 0000000000000000 V31 0x0000000000000000 0000000000000000
>>
>> SP 0x000000013EC6AB60 ELR 0x00000000FFFF1F3C SPSR 0x60000205 FPSR 0x00000000
>> ESR 0x8600000D FAR 0x00000000FFFF1F3C
>>
>> ESR : EC 0x21 IL 0x1 ISS 0x0000000D
>>
>> Instruction abort: Permission fault, first level
>>
>> Stack dump:
>> 000013EC6AA60: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
>> 000013EC6AA80: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
>> 000013EC6AAA0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
>> 000013EC6AAC0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
>> 000013EC6AAE0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
>> 000013EC6AB00: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
>> 000013EC6AB20: 0000000000000000 0000000000000000 00000000FFFF1F3C 0000000060000205
>> 000013EC6AB40: 0000000000000000 000000008600000D 00000000FFFF1F3C 0000000000000000
>>> 000013EC6AB60: 000000013EC6AB80 0000000138399650 00000001383AAA18 0000000000000000
>> 000013EC6AB80: 000000013EC6ABD0 000000013EC71E50 000000013A732218 000000013A747718
>> 000013EC6ABA0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
>> 000013EC6ABC0: 0000000000000000 000000013A747718 000000013EC6AC50 000000013BA5C62C
>> 000013EC6ABE0: 000000013EC6AC00 0000000138CD4060 0000000138CD4068 000000013A6EE018
>> 000013EC6AC00: 000000013EC6AC30 000000013BA577B4 0000000000000000 0000000000000000
>> 000013EC6AC20: 000000013ECA83D8 0000000000000126 000000013EC6AC50 000000013A69FD18
>> 000013EC6AC40: 000000013A6EE018 0000000000000000 000000013EC6ACF0 000000013BA4AF88
>> ASSERT [ArmCpuDxe] .../ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c(265): ((BOOLEAN)(0==1))
>> ---------------
>>
>> If I revert the last patch in the series (leaving ArmVirtQemu's PCD at the default 0), then all three guests boot fine.
>>
>> This reminds me of the case when Star introduced the non-executable stack for DXE, we turned it on in OVMF, and that killed the grub version shipped with an older Debian release:
>>
>> https://www.mail-archive.com/edk2-devel@lists.01.org/msg02022.html
>>
>> So ultimately we had to make the PCD dynamic, and make it default to "off" (see commit ab081a50e565, "OvmfPkg: PlatformPei: take no-exec DXE settings from the QEMU command line", 2015-09-15).
>>
>> Thanks
>> Laszlo
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 0/7] MdeModulePkg/DxeCore: increased memory protection
2017-02-28 10:59 ` Ard Biesheuvel
@ 2017-02-28 11:47 ` Ard Biesheuvel
2017-02-28 23:46 ` Laszlo Ersek
1 sibling, 0 replies; 26+ messages in thread
From: Ard Biesheuvel @ 2017-02-28 11:47 UTC (permalink / raw)
To: Laszlo Ersek, Leif Lindholm
Cc: edk2-devel@lists.01.org, afish@apple.com, Kinney, Michael D,
Gao, Liming, Yao, Jiewen, Tian, Feng, Zeng, Star
On 28 February 2017 at 10:59, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 28 February 2017 at 10:52, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> On 28 February 2017 at 10:46, Laszlo Ersek <lersek@redhat.com> wrote:
>>> On 02/27/17 15:38, Ard Biesheuvel wrote:
>>>> Hello all,
>>>>
>>>> First of all, thanks for the reviews and regression testing. However, I did
>>>> not add the tested-by tags nor some of the R-b's, given the changes in this v4.
>>>>
>>>> This series implements a memory protection policy 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 v3:
>>>> - mandate that the same policy applies to EfiConventionalMemory regions and
>>>> EfiBootServicesData regions: they are unlikely to differ in practice, and
>>>> dealing with that corner case greatly complicates the implementation, given
>>>> the way DxeCore allocates memory for itself in the implementation of the page
>>>> and pool allocation routines.
>>>> - apply the EfiConventionalMemory policy to untested RAM regions in the GCD
>>>> memory space map: without this, we may still have a large region of RAM that
>>>> is exploitable, and it also removes the need to apply memory protections in
>>>> PromoteMemoryResource (), which is very difficult to achieve without a major
>>>> restructuring of the code due to the way locking is implemented here.
>>>> - add missing ApplyMemoryProtectionPolicy() call to CoreAddMemoryDescriptor()
>>>> - use CoreAcquireLockOrFail() on gMemoryLock for CoreAllocatePoolPages (#4)
>>>> - incorporate feedback from Liming (#2, #6)
>>>> - add patch to enable the NX memory protection policy for ArmVirtPkg (#7)
>>>>
>>>> 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-v4
>>>>
>>>> Ard Biesheuvel (7):
>>>> 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
>>>> ArmVirtPkg/ArmVirt.dsc.inc: enable NX memory protection for all
>>>> platforms
>>>>
>>>> ArmPkg/Drivers/CpuDxe/CpuDxe.c | 3 +
>>>> ArmPkg/Drivers/CpuDxe/CpuDxe.h | 1 +
>>>> ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c | 4 +
>>>> ArmVirtPkg/ArmVirt.dsc.inc | 6 +
>>>> MdeModulePkg/Core/Dxe/DxeMain.h | 24 ++
>>>> MdeModulePkg/Core/Dxe/DxeMain.inf | 1 +
>>>> MdeModulePkg/Core/Dxe/Mem/Page.c | 7 +
>>>> MdeModulePkg/Core/Dxe/Mem/Pool.c | 65 +++-
>>>> MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 371 +++++++++++++++++++-
>>>> MdeModulePkg/Core/Pei/Image/Image.c | 23 +-
>>>> MdeModulePkg/MdeModulePkg.dec | 32 ++
>>>> 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 +-
>>>> 17 files changed, 558 insertions(+), 24 deletions(-)
>>>>
>>>
>>> I regression-tested this series for x86 / OVMF as under v3, with the zero PCD default, and experienced no issues.
>>>
>>> However, v4 breaks booting Fedora 24 on my Mustang (aarch64/KVM):
>>>
>>> -----------------
>>> [Bds]Booting Fedora
>>> FSOpen: Open '\EFI\fedora\shim.efi' Success
>>> [Bds] DevicePath expand: HD(1,MBR,0xDB4976D3,0x800,0x64000)/\EFI\fedora\shim.efi -> PciRoot(0x0)/Pci(0x1,0x1)/Pci(0x0,0x0)/Scsi(0x1,0x0)/HD(1,MBR,0xDB4976D3,0x800,0x64000)/\EFI\fedora\shim.efi
>>> [Security] 3rd party image[0] can be loaded after EndOfDxe: PciRoot(0x0)/Pci(0x1,0x1)/Pci(0x0,0x0)/Scsi(0x1,0x0)/HD(1,MBR,0xDB4976D3,0x800,0x64000)/\EFI\fedora\shim.efi.
>>> InstallProtocolInterface: [EfiLoadedImageProtocol] 13A6D2AC0
>>> Loading driver at 0x001382F4000 EntryPoint=0x001382F4148
>>> Loading driver at 0x001382F4000 EntryPoint=0x001382F4148
>>> InstallProtocolInterface: [EfiLoadedImageDevicePathProtocol] 138CDBD98
>>> ProtectUefiImageCommon - 0x3A6D2AC0
>>> - 0x00000001382F4000 - 0x00000000000CBAE0
>>> !!!!!!!! ProtectUefiImageCommon - Section Alignment(0x20) is incorrect !!!!!!!!
>>> FSOpen: Open '\EFI\fedora\grubaa64.efi' Success
>>>
>>>
>>> Synchronous Exception at 0x00000001380F7400
>>>
>>> X0 0x000000013A6EEA98 X1 0x000000013BFF0018 X2 0x00000001380F7400 X3 0x00000000000FD000
>>> X4 0x0000000000000000 X5 0x0000000000000000 X6 0x0000000138362AF4 X7 0x0000000000000000
>>> X8 0x000000013C01F548 X9 0x0000000200000000 X10 0x00000001380F6000 X11 0x00000001382F3FFF
>>> X12 0x0000000000000000 X13 0x0000000000000008 X14 0x0000000000000000 X15 0x0000000000000000
>>> X16 0x000000013EC6ABD0 X17 0x0000000000000000 X18 0x0000000000000000 X19 0x0000000138CDB698
>>> X20 0x000000013A746E18 X21 0x0000000000000000 X22 0x0000000000000000 X23 0x0000000000000000
>>> X24 0x0000000000000000 X25 0x0000000000000000 X26 0x0000000000000000 X27 0x0000000000000000
>>> X28 0x0000000000000000 FP 0x000000013EC6AA50 LR 0x00000001382F80F8
>>>
>>> V0 0xAFAFAFAFAFAFAFAF AFAFAFAFAFAFAFAF V1 0x6963702F66666666 6666666666666666
>>> V2 0x697363732F312C31 406567646972622D V3 0x0000000000000000 0000000000000000
>>> V4 0x0000000000000400 0000000000000000 V5 0x4010040140100401 4010040140100401
>>> V6 0x0004000000000000 0004000000000000 V7 0x0000000000000000 0000000000000000
>>> V8 0x0000000000000000 0000000000000000 V9 0x0000000000000000 0000000000000000
>>> V10 0x0000000000000000 0000000000000000 V11 0x0000000000000000 0000000000000000
>>> V12 0x0000000000000000 0000000000000000 V13 0x0000000000000000 0000000000000000
>>> V14 0x0000000000000000 0000000000000000 V15 0x0000000000000000 0000000000000000
>>> V16 0x0000000000000000 0000000000000000 V17 0x0000000000000000 0000000000000000
>>> V18 0x0000000000000000 0000000000000000 V19 0x0000000000000000 0000000000000000
>>> V20 0x0000000000000000 0000000000000000 V21 0x0000000000000000 0000000000000000
>>> V22 0x0000000000000000 0000000000000000 V23 0x0000000000000000 0000000000000000
>>> V24 0x0000000000000000 0000000000000000 V25 0x0000000000000000 0000000000000000
>>> V26 0x0000000000000000 0000000000000000 V27 0x0000000000000000 0000000000000000
>>> V28 0x0000000000000000 0000000000000000 V29 0x0000000000000000 0000000000000000
>>> V30 0x0000000000000000 0000000000000000 V31 0x0000000000000000 0000000000000000
>>>
>>> SP 0x000000013EC6AA50 ELR 0x00000001380F7400 SPSR 0x60000205 FPSR 0x00000000
>>> ESR 0x8600000E FAR 0x00000001380F7400
>>>
>>> ESR : EC 0x21 IL 0x1 ISS 0x0000000E
>>>
>>> Instruction abort: Permission fault, second level
>>>
>>
>> Hmm, that is disappointing. This is probably due to GRUB's modular
>> nature, which means it allocates memory and loads executable code into
>> it, under the assumption that memory is always executable in UEFI.
>>
>> The short term fix is to remove the NX bit from LoaderData regions,
>> but in the mean time, I will work with Leif to get this fixed properly
>> (assuming there is a proper way to fix this)
>>
>
> Care to have a quick go at using 0xC000000000007FD1 instead? (if you
> are not already doing so)
>
Adding my own data point: running CelloBoard with
gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy set to
0xC000000000007FD5 happily boots straight into the kernel, but crashes
when booting via GRUB. Changing the value to 0xC000000000007FD1 gets
things working again.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 0/7] MdeModulePkg/DxeCore: increased memory protection
2017-02-28 5:48 ` [PATCH v4 0/7] MdeModulePkg/DxeCore: increased memory protection Yao, Jiewen
@ 2017-02-28 14:59 ` Ard Biesheuvel
0 siblings, 0 replies; 26+ messages in thread
From: Ard Biesheuvel @ 2017-02-28 14:59 UTC (permalink / raw)
To: Yao, Jiewen, leif.lindholm@linaro.org, Gao, Liming,
lersek@redhat.com
Cc: edk2-devel@lists.01.org, afish@apple.com, Kinney, Michael D,
Tian, Feng, Zeng, Star
On 28 February 2017 at 05:48, Yao, Jiewen <jiewen.yao@intel.com> wrote:
> All series reviewed-by: jiewen.yao@intel.com
> X86 platform regression tested-by: Jiewen.yao@intel.com
>
Thanks all for the reviews and testing.
I am going to proceed and merge this series, except for the last patch
enabling it for ArmVirtQemu. That allows us to test it more widely on
out of tree platforms.
In the mean time, the recommendation is to disable NX protection for
EfiLoaderData regions, on AArch64 as well as on X86. In the future, we
may want to promote the PCD to one that can be configured as dynamic,
and let the user decide which policy to use. I will follow up with a
separate patch if we intend to go down that road.
Regards,
Ard.
>> -----Original Message-----
>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>> Sent: Monday, February 27, 2017 10:38 PM
>> To: edk2-devel@lists.01.org; afish@apple.com; leif.lindholm@linaro.org; Kinney,
>> Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>;
>> Yao, Jiewen <jiewen.yao@intel.com>
>> Cc: lersek@redhat.com; Tian, Feng <feng.tian@intel.com>; Zeng, Star
>> <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Subject: [PATCH v4 0/7] MdeModulePkg/DxeCore: increased memory protection
>>
>> Hello all,
>>
>> First of all, thanks for the reviews and regression testing. However, I did
>> not add the tested-by tags nor some of the R-b's, given the changes in this v4.
>>
>> This series implements a memory protection policy 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 v3:
>> - mandate that the same policy applies to EfiConventionalMemory regions and
>> EfiBootServicesData regions: they are unlikely to differ in practice, and
>> dealing with that corner case greatly complicates the implementation, given
>> the way DxeCore allocates memory for itself in the implementation of the
>> page
>> and pool allocation routines.
>> - apply the EfiConventionalMemory policy to untested RAM regions in the GCD
>> memory space map: without this, we may still have a large region of RAM that
>> is exploitable, and it also removes the need to apply memory protections in
>> PromoteMemoryResource (), which is very difficult to achieve without a major
>> restructuring of the code due to the way locking is implemented here.
>> - add missing ApplyMemoryProtectionPolicy() call to
>> CoreAddMemoryDescriptor()
>> - use CoreAcquireLockOrFail() on gMemoryLock for CoreAllocatePoolPages (#4)
>> - incorporate feedback from Liming (#2, #6)
>> - add patch to enable the NX memory protection policy for ArmVirtPkg (#7)
>>
>> 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-v4
>>
>> Ard Biesheuvel (7):
>> 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
>> ArmVirtPkg/ArmVirt.dsc.inc: enable NX memory protection for all
>> platforms
>>
>> ArmPkg/Drivers/CpuDxe/CpuDxe.c | 3 +
>> ArmPkg/Drivers/CpuDxe/CpuDxe.h | 1 +
>> ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c | 4 +
>> ArmVirtPkg/ArmVirt.dsc.inc | 6 +
>> MdeModulePkg/Core/Dxe/DxeMain.h | 24 ++
>> MdeModulePkg/Core/Dxe/DxeMain.inf | 1 +
>> MdeModulePkg/Core/Dxe/Mem/Page.c | 7 +
>> MdeModulePkg/Core/Dxe/Mem/Pool.c | 65 +++-
>> MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 371
>> +++++++++++++++++++-
>> MdeModulePkg/Core/Pei/Image/Image.c | 23 +-
>> MdeModulePkg/MdeModulePkg.dec | 32 ++
>> 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 +-
>> 17 files changed, 558 insertions(+), 24 deletions(-)
>>
>> --
>> 2.7.4
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 0/7] MdeModulePkg/DxeCore: increased memory protection
2017-02-28 10:59 ` Ard Biesheuvel
2017-02-28 11:47 ` Ard Biesheuvel
@ 2017-02-28 23:46 ` Laszlo Ersek
2017-03-13 8:43 ` Michael Zimmermann
1 sibling, 1 reply; 26+ messages in thread
From: Laszlo Ersek @ 2017-02-28 23:46 UTC (permalink / raw)
To: Ard Biesheuvel, Leif Lindholm
Cc: edk2-devel@lists.01.org, afish@apple.com, Kinney, Michael D,
Gao, Liming, Yao, Jiewen, Tian, Feng, Zeng, Star
On 02/28/17 11:59, Ard Biesheuvel wrote:
> On 28 February 2017 at 10:52, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> On 28 February 2017 at 10:46, Laszlo Ersek <lersek@redhat.com> wrote:
>>> I regression-tested this series for x86 / OVMF as under v3, with the zero PCD default, and experienced no issues.
>>>
>>> However, v4 breaks booting Fedora 24 on my Mustang (aarch64/KVM):
>> Hmm, that is disappointing. This is probably due to GRUB's modular
>> nature, which means it allocates memory and loads executable code into
>> it, under the assumption that memory is always executable in UEFI.
>>
>> The short term fix is to remove the NX bit from LoaderData regions,
>> but in the mean time, I will work with Leif to get this fixed properly
>> (assuming there is a proper way to fix this)
>>
>
> Care to have a quick go at using 0xC000000000007FD1 instead? (if you
> are not already doing so)
With the following patch on top:
> commit ef6be33275e45045a15201a15a2be26e6fbabcaa
> Author: Laszlo Ersek <lersek@redhat.com>
> Date: Wed Mar 1 00:06:37 2017 +0100
>
> ArmVirtPkg: remove the NX bit from LoaderData regions
>
> msgid <CAKv+Gu8V4o0-s9jhQSM5hFaaC6yppdC001MiuBX830WrXi_VKQ@mail.gmail.com>
>
> Suggested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>
> diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
> index 23b601a199ed..4d3ae5d0bc80 100644
> --- a/ArmVirtPkg/ArmVirt.dsc.inc
> +++ b/ArmVirtPkg/ArmVirt.dsc.inc
> @@ -386,7 +386,7 @@ [PcdsFixedAtBuild.AARCH64]
> # Enable NX memory protection for all non-code regions, including OEM and OS
> # reserved ones.
> #
> - gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy|0xC000000000007FD5
> + gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy|0xC000000000007FD1
>
> [Components.common]
> #
all three guests mentioned previously boot okay.
(I also made sure that the "applying strict permissions..." messages showed up in the firmware log for each.)
Thanks
Laszlo
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 0/7] MdeModulePkg/DxeCore: increased memory protection
2017-02-28 23:46 ` Laszlo Ersek
@ 2017-03-13 8:43 ` Michael Zimmermann
2017-03-13 8:50 ` Ard Biesheuvel
0 siblings, 1 reply; 26+ messages in thread
From: Michael Zimmermann @ 2017-03-13 8:43 UTC (permalink / raw)
To: Laszlo Ersek
Cc: Ard Biesheuvel, Leif Lindholm, Tian, Feng,
edk2-devel@lists.01.org, afish@apple.com, Gao, Liming,
Yao, Jiewen, Kinney, Michael D, Zeng, Star
I fail to get this working on my target. I've enabled the following
Pcd's like in ArmVirt:
gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy|0x3
gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy|0xC000000000007FD1
gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|TRUE
but apparently, DxeCore removes the executable permission from it's own code.
after the BL instruction of the call to gCpu->SetMemoryAttributes I
get an instruction permission fault:
InitializeDxeNxMemoryProtectionPolicy: applying strict permissions to
active memory regions
SetUefiImageMemoryAttributes - 0x0000000080200000 - 0x0000000008C00000
(0x0000000000004000)
SetUefiImageMemoryAttributes - 0x0000000089000000 - 0x0000000004A00000
(0x0000000000004000)
SetUefiImageMemoryAttributes - 0x000000008EC00000 - 0x0000000000400000
(0x0000000000004000)
SetUefiImageMemoryAttributes - 0x000000008F700000 - 0x0000000000700000
(0x0000000000004000)
SetUefiImageMemoryAttributes - 0x000000008FF00000 - 0x000000006E095000
(0x0000000000004000)
SetUefiImageMemoryAttributes - 0x00000000FDFB9000 - 0x0000000000047000
(0x0000000000004000)
SetUefiImageMemoryAttributes - 0x00000000FE400000 - 0x0000000001C00000
(0x0000000000004000)
Prefetch Abort Exception PC at 0xFEEA630E CPSR 0x20000033 nzCveaifT_svc
Build/LittleKernelPkg/DEBUG_GCC5/ARM/MdeModulePkg/Core/Dxe/DxeMain/DEBUG/DxeCore.dll
loaded at 0xFEEA4000 (PE/COFF offset) 0x230E (ELF or Mach-O offset) 0x130E
0xBF00 IT EQ
R0 0x00000000 R1 0x01C00000 R2 0x00000000 R3 0x00000000
R4 0x00000000 R5 0x00026000 R6 0x00000000 R7 0xFE000214
R8 0x80000000 R9 0xFE400000 R10 0xFFFEF000 R11 0x00000004
R12 0x00000002 SP 0xFFFFEBA0 LR 0xFDF98B4D PC 0xFEEA630E
DFSR 0x00000000 DFAR 0x00000000 IFSR 0x0000000D IFAR 0xFEEA630E
Instruction Permission fault on Section at 0xFEEA630E
ASSERT [ArmCpuDxe]
ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c(268):
((BOOLEAN)(0==1))
----
so did I miss anything?
Thanks
Michael
On Wed, Mar 1, 2017 at 12:46 AM, Laszlo Ersek <lersek@redhat.com> wrote:
> On 02/28/17 11:59, Ard Biesheuvel wrote:
>> On 28 February 2017 at 10:52, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>> On 28 February 2017 at 10:46, Laszlo Ersek <lersek@redhat.com> wrote:
>
>>>> I regression-tested this series for x86 / OVMF as under v3, with the zero PCD default, and experienced no issues.
>>>>
>>>> However, v4 breaks booting Fedora 24 on my Mustang (aarch64/KVM):
>
>>> Hmm, that is disappointing. This is probably due to GRUB's modular
>>> nature, which means it allocates memory and loads executable code into
>>> it, under the assumption that memory is always executable in UEFI.
>>>
>>> The short term fix is to remove the NX bit from LoaderData regions,
>>> but in the mean time, I will work with Leif to get this fixed properly
>>> (assuming there is a proper way to fix this)
>>>
>>
>> Care to have a quick go at using 0xC000000000007FD1 instead? (if you
>> are not already doing so)
>
>
> With the following patch on top:
>
>> commit ef6be33275e45045a15201a15a2be26e6fbabcaa
>> Author: Laszlo Ersek <lersek@redhat.com>
>> Date: Wed Mar 1 00:06:37 2017 +0100
>>
>> ArmVirtPkg: remove the NX bit from LoaderData regions
>>
>> msgid <CAKv+Gu8V4o0-s9jhQSM5hFaaC6yppdC001MiuBX830WrXi_VKQ@mail.gmail.com>
>>
>> Suggested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>
>> diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
>> index 23b601a199ed..4d3ae5d0bc80 100644
>> --- a/ArmVirtPkg/ArmVirt.dsc.inc
>> +++ b/ArmVirtPkg/ArmVirt.dsc.inc
>> @@ -386,7 +386,7 @@ [PcdsFixedAtBuild.AARCH64]
>> # Enable NX memory protection for all non-code regions, including OEM and OS
>> # reserved ones.
>> #
>> - gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy|0xC000000000007FD5
>> + gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy|0xC000000000007FD1
>>
>> [Components.common]
>> #
>
> all three guests mentioned previously boot okay.
>
> (I also made sure that the "applying strict permissions..." messages showed up in the firmware log for each.)
>
> Thanks
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 0/7] MdeModulePkg/DxeCore: increased memory protection
2017-03-13 8:43 ` Michael Zimmermann
@ 2017-03-13 8:50 ` Ard Biesheuvel
2017-03-13 8:53 ` Michael Zimmermann
0 siblings, 1 reply; 26+ messages in thread
From: Ard Biesheuvel @ 2017-03-13 8:50 UTC (permalink / raw)
To: Michael Zimmermann
Cc: Laszlo Ersek, Leif Lindholm, Tian, Feng, edk2-devel@lists.01.org,
afish@apple.com, Gao, Liming, Yao, Jiewen, Kinney, Michael D,
Zeng, Star
On 13 March 2017 at 09:43, Michael Zimmermann <sigmaepsilon92@gmail.com> wrote:
> I fail to get this working on my target. I've enabled the following
> Pcd's like in ArmVirt:
> gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy|0x3
> gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy|0xC000000000007FD1
> gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|TRUE
>
> but apparently, DxeCore removes the executable permission from it's own code.
> after the BL instruction of the call to gCpu->SetMemoryAttributes I
> get an instruction permission fault:
>
> InitializeDxeNxMemoryProtectionPolicy: applying strict permissions to
> active memory regions
> SetUefiImageMemoryAttributes - 0x0000000080200000 - 0x0000000008C00000
> (0x0000000000004000)
> SetUefiImageMemoryAttributes - 0x0000000089000000 - 0x0000000004A00000
> (0x0000000000004000)
> SetUefiImageMemoryAttributes - 0x000000008EC00000 - 0x0000000000400000
> (0x0000000000004000)
> SetUefiImageMemoryAttributes - 0x000000008F700000 - 0x0000000000700000
> (0x0000000000004000)
> SetUefiImageMemoryAttributes - 0x000000008FF00000 - 0x000000006E095000
> (0x0000000000004000)
> SetUefiImageMemoryAttributes - 0x00000000FDFB9000 - 0x0000000000047000
> (0x0000000000004000)
> SetUefiImageMemoryAttributes - 0x00000000FE400000 - 0x0000000001C00000
> (0x0000000000004000)
>
> Prefetch Abort Exception PC at 0xFEEA630E CPSR 0x20000033 nzCveaifT_svc
> Build/LittleKernelPkg/DEBUG_GCC5/ARM/MdeModulePkg/Core/Dxe/DxeMain/DEBUG/DxeCore.dll
> loaded at 0xFEEA4000 (PE/COFF offset) 0x230E (ELF or Mach-O offset) 0x130E
> 0xBF00 IT EQ
> R0 0x00000000 R1 0x01C00000 R2 0x00000000 R3 0x00000000
> R4 0x00000000 R5 0x00026000 R6 0x00000000 R7 0xFE000214
> R8 0x80000000 R9 0xFE400000 R10 0xFFFEF000 R11 0x00000004
> R12 0x00000002 SP 0xFFFFEBA0 LR 0xFDF98B4D PC 0xFEEA630E
> DFSR 0x00000000 DFAR 0x00000000 IFSR 0x0000000D IFAR 0xFEEA630E
> Instruction Permission fault on Section at 0xFEEA630E
>
> ASSERT [ArmCpuDxe]
> ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c(268):
> ((BOOLEAN)(0==1))
>
> ----
>
> so did I miss anything?
>
You are using PrePi, right?
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 0/7] MdeModulePkg/DxeCore: increased memory protection
2017-03-13 8:50 ` Ard Biesheuvel
@ 2017-03-13 8:53 ` Michael Zimmermann
0 siblings, 0 replies; 26+ messages in thread
From: Michael Zimmermann @ 2017-03-13 8:53 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Laszlo Ersek, Leif Lindholm, Tian, Feng, edk2-devel@lists.01.org,
afish@apple.com, Gao, Liming, Yao, Jiewen, Kinney, Michael D,
Zeng, Star
> You are using PrePi, right?
yes. Isn't that supported yet? afaik ArmVirtXen uses PrePi too.
On Mon, Mar 13, 2017 at 9:50 AM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> On 13 March 2017 at 09:43, Michael Zimmermann <sigmaepsilon92@gmail.com> wrote:
>> I fail to get this working on my target. I've enabled the following
>> Pcd's like in ArmVirt:
>> gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy|0x3
>> gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy|0xC000000000007FD1
>> gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|TRUE
>>
>> but apparently, DxeCore removes the executable permission from it's own code.
>> after the BL instruction of the call to gCpu->SetMemoryAttributes I
>> get an instruction permission fault:
>>
>> InitializeDxeNxMemoryProtectionPolicy: applying strict permissions to
>> active memory regions
>> SetUefiImageMemoryAttributes - 0x0000000080200000 - 0x0000000008C00000
>> (0x0000000000004000)
>> SetUefiImageMemoryAttributes - 0x0000000089000000 - 0x0000000004A00000
>> (0x0000000000004000)
>> SetUefiImageMemoryAttributes - 0x000000008EC00000 - 0x0000000000400000
>> (0x0000000000004000)
>> SetUefiImageMemoryAttributes - 0x000000008F700000 - 0x0000000000700000
>> (0x0000000000004000)
>> SetUefiImageMemoryAttributes - 0x000000008FF00000 - 0x000000006E095000
>> (0x0000000000004000)
>> SetUefiImageMemoryAttributes - 0x00000000FDFB9000 - 0x0000000000047000
>> (0x0000000000004000)
>> SetUefiImageMemoryAttributes - 0x00000000FE400000 - 0x0000000001C00000
>> (0x0000000000004000)
>>
>> Prefetch Abort Exception PC at 0xFEEA630E CPSR 0x20000033 nzCveaifT_svc
>> Build/LittleKernelPkg/DEBUG_GCC5/ARM/MdeModulePkg/Core/Dxe/DxeMain/DEBUG/DxeCore.dll
>> loaded at 0xFEEA4000 (PE/COFF offset) 0x230E (ELF or Mach-O offset) 0x130E
>> 0xBF00 IT EQ
>> R0 0x00000000 R1 0x01C00000 R2 0x00000000 R3 0x00000000
>> R4 0x00000000 R5 0x00026000 R6 0x00000000 R7 0xFE000214
>> R8 0x80000000 R9 0xFE400000 R10 0xFFFEF000 R11 0x00000004
>> R12 0x00000002 SP 0xFFFFEBA0 LR 0xFDF98B4D PC 0xFEEA630E
>> DFSR 0x00000000 DFAR 0x00000000 IFSR 0x0000000D IFAR 0xFEEA630E
>> Instruction Permission fault on Section at 0xFEEA630E
>>
>> ASSERT [ArmCpuDxe]
>> ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c(268):
>> ((BOOLEAN)(0==1))
>>
>> ----
>>
>> so did I miss anything?
>>
>
> You are using PrePi, right?
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2017-03-13 8:53 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-02-27 14:38 [PATCH v4 0/7] MdeModulePkg/DxeCore: increased memory protection Ard Biesheuvel
2017-02-27 14:38 ` [PATCH v4 1/7] ArmPkg/CpuDxe: ignore attribute changes during SyncCacheConfig() Ard Biesheuvel
2017-02-27 15:32 ` Leif Lindholm
2017-02-27 15:33 ` Ard Biesheuvel
2017-02-27 15:38 ` Leif Lindholm
2017-02-27 15:39 ` Ard Biesheuvel
2017-02-27 15:41 ` Leif Lindholm
2017-02-27 14:38 ` [PATCH v4 2/7] MdeModulePkg/PeiCore: allocate BootServicesCode memory for PE/COFF images Ard Biesheuvel
2017-02-28 5:42 ` Gao, Liming
2017-02-27 14:38 ` [PATCH v4 3/7] MdeModulePkg/EbcDxe: use EfiBootServicesCode memory for thunks Ard Biesheuvel
2017-02-27 14:38 ` [PATCH v4 4/7] MdeModulePkg/DxeCore: use separate lock for pool allocations Ard Biesheuvel
2017-02-28 9:32 ` Gao, Liming
2017-02-27 14:38 ` [PATCH v4 5/7] MdeModulePkg: define PCD for DXE memory protection policy Ard Biesheuvel
2017-02-27 14:38 ` [PATCH v4 6/7] MdeModulePkg/DxeCore: implement " Ard Biesheuvel
2017-02-28 9:33 ` Gao, Liming
2017-02-27 14:38 ` [PATCH v4 7/7] ArmVirtPkg/ArmVirt.dsc.inc: enable NX memory protection for all platforms Ard Biesheuvel
2017-02-28 5:48 ` [PATCH v4 0/7] MdeModulePkg/DxeCore: increased memory protection Yao, Jiewen
2017-02-28 14:59 ` Ard Biesheuvel
2017-02-28 10:46 ` Laszlo Ersek
2017-02-28 10:52 ` Ard Biesheuvel
2017-02-28 10:59 ` Ard Biesheuvel
2017-02-28 11:47 ` Ard Biesheuvel
2017-02-28 23:46 ` Laszlo Ersek
2017-03-13 8:43 ` Michael Zimmermann
2017-03-13 8:50 ` Ard Biesheuvel
2017-03-13 8:53 ` Michael Zimmermann
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox