* [RFC 01/13] ArmPkg/Mmu: Remove handling of NONSECURE memory regions
2023-02-13 15:17 [RFC 00/13] Hardware enforced W^X memory protections Ard Biesheuvel
@ 2023-02-13 15:17 ` Ard Biesheuvel
2023-02-13 15:17 ` [RFC 02/13] ArmPkg/ArmMmuLib: Introduce region types for RO/XP WB cached memory Ard Biesheuvel
` (11 subsequent siblings)
12 siblings, 0 replies; 21+ messages in thread
From: Ard Biesheuvel @ 2023-02-13 15:17 UTC (permalink / raw)
To: devel
Cc: Ard Biesheuvel, Michael Kinney, Liming Gao, Jiewen Yao,
Michael Kubacki, Sean Brogan, Rebecca Cran, Leif Lindholm,
Sami Mujawar, Taylor Beebe, Matthew Garrett, Peter Jones,
Kees Cook
Non-secure memory is a distinction that only matters when executing code
in the secure world that reasons about the secure vs non-secure address
spaces. EDK2 was not designed for that, and the AArch64 version of the
MMU handling library already treats them as identical, so let's just
drop the ARM memory region types that mark memory as 'non-secure'
explicitly.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
ArmPkg/Include/Chipset/ArmV7Mmu.h | 51 +++++++-------------
ArmPkg/Include/Library/ArmLib.h | 11 -----
ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 5 --
ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c | 33 +++----------
4 files changed, 24 insertions(+), 76 deletions(-)
diff --git a/ArmPkg/Include/Chipset/ArmV7Mmu.h b/ArmPkg/Include/Chipset/ArmV7Mmu.h
index da4f3160f8ff..89b81e33d004 100644
--- a/ArmPkg/Include/Chipset/ArmV7Mmu.h
+++ b/ArmPkg/Include/Chipset/ArmV7Mmu.h
@@ -157,39 +157,24 @@
#define TT_DESCRIPTOR_PAGE_BASE_ADDRESS(a) ((a) & TT_DESCRIPTOR_PAGE_BASE_ADDRESS_MASK)
#define TT_DESCRIPTOR_PAGE_BASE_SHIFT 12
-#define TT_DESCRIPTOR_SECTION_WRITE_BACK(NonSecure) (TT_DESCRIPTOR_SECTION_TYPE_SECTION | \
- ((NonSecure) ? TT_DESCRIPTOR_SECTION_NS : 0) | \
- TT_DESCRIPTOR_SECTION_NG_GLOBAL | \
- TT_DESCRIPTOR_SECTION_S_SHARED | \
- TT_DESCRIPTOR_SECTION_DOMAIN(0) | \
- TT_DESCRIPTOR_SECTION_AP_RW_RW | \
- TT_DESCRIPTOR_SECTION_AF | \
- TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_BACK_ALLOC)
-#define TT_DESCRIPTOR_SECTION_WRITE_THROUGH(NonSecure) (TT_DESCRIPTOR_SECTION_TYPE_SECTION | \
- ((NonSecure) ? TT_DESCRIPTOR_SECTION_NS : 0) | \
- TT_DESCRIPTOR_SECTION_NG_GLOBAL | \
- TT_DESCRIPTOR_SECTION_S_SHARED | \
- TT_DESCRIPTOR_SECTION_DOMAIN(0) | \
- TT_DESCRIPTOR_SECTION_AP_RW_RW | \
- TT_DESCRIPTOR_SECTION_AF | \
- TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC)
-#define TT_DESCRIPTOR_SECTION_DEVICE(NonSecure) (TT_DESCRIPTOR_SECTION_TYPE_SECTION | \
- ((NonSecure) ? TT_DESCRIPTOR_SECTION_NS : 0) | \
- TT_DESCRIPTOR_SECTION_NG_GLOBAL | \
- TT_DESCRIPTOR_SECTION_S_NOT_SHARED | \
- TT_DESCRIPTOR_SECTION_DOMAIN(0) | \
- TT_DESCRIPTOR_SECTION_AP_RW_RW | \
- TT_DESCRIPTOR_SECTION_XN_MASK | \
- TT_DESCRIPTOR_SECTION_AF | \
- TT_DESCRIPTOR_SECTION_CACHE_POLICY_SHAREABLE_DEVICE)
-#define TT_DESCRIPTOR_SECTION_UNCACHED(NonSecure) (TT_DESCRIPTOR_SECTION_TYPE_SECTION | \
- ((NonSecure) ? TT_DESCRIPTOR_SECTION_NS : 0) | \
- TT_DESCRIPTOR_SECTION_NG_GLOBAL | \
- TT_DESCRIPTOR_SECTION_S_NOT_SHARED | \
- TT_DESCRIPTOR_SECTION_DOMAIN(0) | \
- TT_DESCRIPTOR_SECTION_AP_RW_RW | \
- TT_DESCRIPTOR_SECTION_AF | \
- TT_DESCRIPTOR_SECTION_CACHE_POLICY_NON_CACHEABLE)
+#define TT_DESCRIPTOR_SECTION_DEFAULT (TT_DESCRIPTOR_SECTION_TYPE_SECTION | \
+ TT_DESCRIPTOR_SECTION_NG_GLOBAL | \
+ TT_DESCRIPTOR_SECTION_S_SHARED | \
+ TT_DESCRIPTOR_SECTION_DOMAIN(0) | \
+ TT_DESCRIPTOR_SECTION_AP_RW_RW | \
+ TT_DESCRIPTOR_SECTION_AF)
+
+#define TT_DESCRIPTOR_SECTION_WRITE_BACK (TT_DESCRIPTOR_SECTION_DEFAULT | \
+ TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_BACK_ALLOC)
+
+#define TT_DESCRIPTOR_SECTION_WRITE_THROUGH (TT_DESCRIPTOR_SECTION_DEFAULT | \
+ TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC)
+
+#define TT_DESCRIPTOR_SECTION_DEVICE (TT_DESCRIPTOR_SECTION_DEFAULT | \
+ TT_DESCRIPTOR_SECTION_CACHE_POLICY_SHAREABLE_DEVICE)
+
+#define TT_DESCRIPTOR_SECTION_UNCACHED (TT_DESCRIPTOR_SECTION_DEFAULT | \
+ TT_DESCRIPTOR_SECTION_CACHE_POLICY_NON_CACHEABLE)
#define TT_DESCRIPTOR_PAGE_WRITE_BACK (TT_DESCRIPTOR_PAGE_TYPE_PAGE | \
TT_DESCRIPTOR_PAGE_NG_GLOBAL | \
diff --git a/ArmPkg/Include/Library/ArmLib.h b/ArmPkg/Include/Library/ArmLib.h
index fa605f128bfd..a53f60d98852 100644
--- a/ArmPkg/Include/Library/ArmLib.h
+++ b/ArmPkg/Include/Library/ArmLib.h
@@ -25,29 +25,18 @@
EFI_MEMORY_WT | EFI_MEMORY_WB | \
EFI_MEMORY_UCE)
-/**
- * The UEFI firmware must not use the ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_* attributes.
- *
- * The Non Secure memory attribute (ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_*) should only
- * be used in Secure World to distinguished Secure to Non-Secure memory.
- */
typedef enum {
ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED = 0,
- ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_UNCACHED_UNBUFFERED,
ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK,
- ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_BACK,
// On some platforms, memory mapped flash region is designed as not supporting
// shareable attribute, so WRITE_BACK_NONSHAREABLE is added for such special
// need.
// Do NOT use below two attributes if you are not sure.
ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK_NONSHAREABLE,
- ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_BACK_NONSHAREABLE,
ARM_MEMORY_REGION_ATTRIBUTE_WRITE_THROUGH,
- ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_THROUGH,
ARM_MEMORY_REGION_ATTRIBUTE_DEVICE,
- ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_DEVICE
} ARM_MEMORY_REGION_ATTRIBUTES;
#define IS_ARM_MEMORY_REGION_ATTRIBUTES_SECURE(attr) ((UINT32)(attr) & 1)
diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index 1ce200c43c72..ee4c5c995ce8 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -39,26 +39,21 @@ ArmMemoryAttributeToPageAttribute (
{
switch (Attributes) {
case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK_NONSHAREABLE:
- case ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_BACK_NONSHAREABLE:
return TT_ATTR_INDX_MEMORY_WRITE_BACK;
case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK:
- case ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_BACK:
return TT_ATTR_INDX_MEMORY_WRITE_BACK | TT_SH_INNER_SHAREABLE;
case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_THROUGH:
- case ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_THROUGH:
return TT_ATTR_INDX_MEMORY_WRITE_THROUGH | TT_SH_INNER_SHAREABLE;
// Uncached and device mappings are treated as outer shareable by default,
case ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED:
- case ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_UNCACHED_UNBUFFERED:
return TT_ATTR_INDX_MEMORY_NON_CACHEABLE;
default:
ASSERT (0);
case ARM_MEMORY_REGION_ATTRIBUTE_DEVICE:
- case ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_DEVICE:
if (ArmReadCurrentEL () == AARCH64_EL2) {
return TT_ATTR_INDX_DEVICE_MEMORY | TT_XN_MASK;
} else {
diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
index 28cc9b2fe058..154298357460 100644
--- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
@@ -100,24 +100,19 @@ PopulateLevel2PageTable (
switch (Attributes) {
case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK:
- case ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_BACK:
PageAttributes = TT_DESCRIPTOR_PAGE_WRITE_BACK;
break;
case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK_NONSHAREABLE:
- case ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_BACK_NONSHAREABLE:
PageAttributes = TT_DESCRIPTOR_PAGE_WRITE_BACK;
PageAttributes &= ~TT_DESCRIPTOR_PAGE_S_SHARED;
break;
case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_THROUGH:
- case ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_THROUGH:
PageAttributes = TT_DESCRIPTOR_PAGE_WRITE_THROUGH;
break;
case ARM_MEMORY_REGION_ATTRIBUTE_DEVICE:
- case ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_DEVICE:
PageAttributes = TT_DESCRIPTOR_PAGE_DEVICE;
break;
case ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED:
- case ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_UNCACHED_UNBUFFERED:
PageAttributes = TT_DESCRIPTOR_PAGE_UNCACHED;
break;
default:
@@ -239,39 +234,23 @@ FillTranslationTable (
switch (MemoryRegion->Attributes) {
case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK:
- Attributes = TT_DESCRIPTOR_SECTION_WRITE_BACK (0);
+ Attributes = TT_DESCRIPTOR_SECTION_WRITE_BACK;
break;
case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK_NONSHAREABLE:
- Attributes = TT_DESCRIPTOR_SECTION_WRITE_BACK (0);
+ Attributes = TT_DESCRIPTOR_SECTION_WRITE_BACK;
Attributes &= ~TT_DESCRIPTOR_SECTION_S_SHARED;
break;
case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_THROUGH:
- Attributes = TT_DESCRIPTOR_SECTION_WRITE_THROUGH (0);
+ Attributes = TT_DESCRIPTOR_SECTION_WRITE_THROUGH;
break;
case ARM_MEMORY_REGION_ATTRIBUTE_DEVICE:
- Attributes = TT_DESCRIPTOR_SECTION_DEVICE (0);
+ Attributes = TT_DESCRIPTOR_SECTION_DEVICE;
break;
case ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED:
- Attributes = TT_DESCRIPTOR_SECTION_UNCACHED (0);
- break;
- case ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_BACK:
- Attributes = TT_DESCRIPTOR_SECTION_WRITE_BACK (1);
- break;
- case ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_BACK_NONSHAREABLE:
- Attributes = TT_DESCRIPTOR_SECTION_WRITE_BACK (1);
- Attributes &= ~TT_DESCRIPTOR_SECTION_S_SHARED;
- break;
- case ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_THROUGH:
- Attributes = TT_DESCRIPTOR_SECTION_WRITE_THROUGH (1);
- break;
- case ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_DEVICE:
- Attributes = TT_DESCRIPTOR_SECTION_DEVICE (1);
- break;
- case ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_UNCACHED_UNBUFFERED:
- Attributes = TT_DESCRIPTOR_SECTION_UNCACHED (1);
+ Attributes = TT_DESCRIPTOR_SECTION_UNCACHED;
break;
default:
- Attributes = TT_DESCRIPTOR_SECTION_UNCACHED (0);
+ Attributes = TT_DESCRIPTOR_SECTION_UNCACHED;
break;
}
--
2.39.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [RFC 02/13] ArmPkg/ArmMmuLib: Introduce region types for RO/XP WB cached memory
2023-02-13 15:17 [RFC 00/13] Hardware enforced W^X memory protections Ard Biesheuvel
2023-02-13 15:17 ` [RFC 01/13] ArmPkg/Mmu: Remove handling of NONSECURE memory regions Ard Biesheuvel
@ 2023-02-13 15:17 ` Ard Biesheuvel
2023-02-13 15:18 ` [RFC 03/13] MdePkg/BasePeCoffLib: Add API to keep track of relocation range Ard Biesheuvel
` (10 subsequent siblings)
12 siblings, 0 replies; 21+ messages in thread
From: Ard Biesheuvel @ 2023-02-13 15:17 UTC (permalink / raw)
To: devel
Cc: Ard Biesheuvel, Michael Kinney, Liming Gao, Jiewen Yao,
Michael Kubacki, Sean Brogan, Rebecca Cran, Leif Lindholm,
Sami Mujawar, Taylor Beebe, Matthew Garrett, Peter Jones,
Kees Cook
To prepare for the enablement of booting EFI with the SCTLR.WXN control
enabled, which makes all writeable memory regions non-executable by
default, introduce a memory type that we will use to describe the flash
region that carries the SEC and PEIM modules that execute in place. Even
if these are implicitly read-only due to the ROM nature, they need to be
mapped with read-only attributes in the page tables to be able to
execute from them.
Also add the XP counterpart which will be used for all normal DRAM right
at the outset.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
ArmPkg/Include/Library/ArmLib.h | 6 ++++
ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 29 ++++++++++++++++----
ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c | 16 +++++++++++
3 files changed, 45 insertions(+), 6 deletions(-)
diff --git a/ArmPkg/Include/Library/ArmLib.h b/ArmPkg/Include/Library/ArmLib.h
index a53f60d98852..fb1ae57b3522 100644
--- a/ArmPkg/Include/Library/ArmLib.h
+++ b/ArmPkg/Include/Library/ArmLib.h
@@ -35,6 +35,12 @@ typedef enum {
// Do NOT use below two attributes if you are not sure.
ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK_NONSHAREABLE,
+ // Special region types for memory that must be mapped with read-only or
+ // non-execute permissions from the very start, e.g., to support the use
+ // of the WXN virtual memory control.
+ ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK_RO,
+ ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK_XP,
+
ARM_MEMORY_REGION_ATTRIBUTE_WRITE_THROUGH,
ARM_MEMORY_REGION_ATTRIBUTE_DEVICE,
} ARM_MEMORY_REGION_ATTRIBUTES;
diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index ee4c5c995ce8..9cdaa8b32c62 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -37,12 +37,33 @@ ArmMemoryAttributeToPageAttribute (
IN ARM_MEMORY_REGION_ATTRIBUTES Attributes
)
{
+ UINT64 Permissions = 0;
+
+ switch (Attributes) {
+ case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK_RO:
+ Permissions = TT_AP_NO_RO;
+ break;
+
+ case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK_XP:
+ case ARM_MEMORY_REGION_ATTRIBUTE_DEVICE:
+ if (ArmReadCurrentEL () == AARCH64_EL2) {
+ Permissions = TT_XN_MASK;
+ } else {
+ Permissions = TT_UXN_MASK | TT_PXN_MASK;
+ }
+ break;
+ default:
+ break;
+ }
+
switch (Attributes) {
case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK_NONSHAREABLE:
return TT_ATTR_INDX_MEMORY_WRITE_BACK;
case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK:
- return TT_ATTR_INDX_MEMORY_WRITE_BACK | TT_SH_INNER_SHAREABLE;
+ case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK_RO:
+ case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK_XP:
+ return TT_ATTR_INDX_MEMORY_WRITE_BACK | TT_SH_INNER_SHAREABLE | Permissions;
case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_THROUGH:
return TT_ATTR_INDX_MEMORY_WRITE_THROUGH | TT_SH_INNER_SHAREABLE;
@@ -54,11 +75,7 @@ ArmMemoryAttributeToPageAttribute (
default:
ASSERT (0);
case ARM_MEMORY_REGION_ATTRIBUTE_DEVICE:
- if (ArmReadCurrentEL () == AARCH64_EL2) {
- return TT_ATTR_INDX_DEVICE_MEMORY | TT_XN_MASK;
- } else {
- return TT_ATTR_INDX_DEVICE_MEMORY | TT_UXN_MASK | TT_PXN_MASK;
- }
+ return TT_ATTR_INDX_DEVICE_MEMORY | Permissions;
}
}
diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
index 154298357460..00c5f42cd91a 100644
--- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
@@ -106,6 +106,14 @@ PopulateLevel2PageTable (
PageAttributes = TT_DESCRIPTOR_PAGE_WRITE_BACK;
PageAttributes &= ~TT_DESCRIPTOR_PAGE_S_SHARED;
break;
+ case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK_RO:
+ PageAttributes = TT_DESCRIPTOR_PAGE_WRITE_BACK;
+ PageAttributes |= TT_DESCRIPTOR_PAGE_AP_NO_RO;
+ break;
+ case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK_XP:
+ PageAttributes = TT_DESCRIPTOR_PAGE_WRITE_BACK;
+ PageAttributes |= TT_DESCRIPTOR_PAGE_XN_MASK;
+ break;
case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_THROUGH:
PageAttributes = TT_DESCRIPTOR_PAGE_WRITE_THROUGH;
break;
@@ -240,6 +248,14 @@ FillTranslationTable (
Attributes = TT_DESCRIPTOR_SECTION_WRITE_BACK;
Attributes &= ~TT_DESCRIPTOR_SECTION_S_SHARED;
break;
+ case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK_RO:
+ Attributes = TT_DESCRIPTOR_SECTION_WRITE_BACK;
+ Attributes |= TT_DESCRIPTOR_SECTION_AP_NO_RO;
+ break;
+ case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK_XP:
+ Attributes = TT_DESCRIPTOR_SECTION_WRITE_BACK;
+ Attributes |= TT_DESCRIPTOR_SECTION_XN_MASK;
+ break;
case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_THROUGH:
Attributes = TT_DESCRIPTOR_SECTION_WRITE_THROUGH;
break;
--
2.39.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [RFC 03/13] MdePkg/BasePeCoffLib: Add API to keep track of relocation range
2023-02-13 15:17 [RFC 00/13] Hardware enforced W^X memory protections Ard Biesheuvel
2023-02-13 15:17 ` [RFC 01/13] ArmPkg/Mmu: Remove handling of NONSECURE memory regions Ard Biesheuvel
2023-02-13 15:17 ` [RFC 02/13] ArmPkg/ArmMmuLib: Introduce region types for RO/XP WB cached memory Ard Biesheuvel
@ 2023-02-13 15:18 ` Ard Biesheuvel
2023-02-13 15:18 ` [RFC 04/13] MdeModulePkg/DxeIpl: Avoid shadowing IPL PEIM by default Ard Biesheuvel
` (9 subsequent siblings)
12 siblings, 0 replies; 21+ messages in thread
From: Ard Biesheuvel @ 2023-02-13 15:18 UTC (permalink / raw)
To: devel
Cc: Ard Biesheuvel, Michael Kinney, Liming Gao, Jiewen Yao,
Michael Kubacki, Sean Brogan, Rebecca Cran, Leif Lindholm,
Sami Mujawar, Taylor Beebe, Matthew Garrett, Peter Jones,
Kees Cook
Add a library call to obtain the start and end of the region covered by
relocation fixups. This will be used in a future patch to limit the
range of memory that needs to be remapped with read-write-execute
permissions at ExitBootServices() time.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
MdePkg/Include/Library/PeCoffLib.h | 25 ++++++
MdePkg/Library/BasePeCoffLib/BasePeCoff.c | 83 +++++++++++++++++++-
2 files changed, 107 insertions(+), 1 deletion(-)
diff --git a/MdePkg/Include/Library/PeCoffLib.h b/MdePkg/Include/Library/PeCoffLib.h
index b45879453785..3706c8a4e858 100644
--- a/MdePkg/Include/Library/PeCoffLib.h
+++ b/MdePkg/Include/Library/PeCoffLib.h
@@ -382,4 +382,29 @@ PeCoffLoaderUnloadImage (
IN OUT PE_COFF_LOADER_IMAGE_CONTEXT *ImageContext
);
+/**
+ Retrieve the range subject to relocation fixups from the recorded fixup data
+ of a runtime image
+
+ @param ImageBase The base address of a PE/COFF image that has been loaded
+ and relocated into system memory.
+ @param VirtImageBase The request virtual address that the PE/COFF image is to
+ be fixed up for.
+ @param ImageSize The size, in bytes, of the PE/COFF image.
+ @param RelocationData A pointer to the relocation data that was collected when the
+ PE/COFF image was relocated using PeCoffLoaderRelocateImage().
+ @param[out] RelocationRangeMin The start of the relocated range.
+ @param[out] RelocationRangeMax The end of the relocated range.
+
+**/
+VOID
+EFIAPI
+PeCoffLoaderGetRelocationRange (
+ IN PHYSICAL_ADDRESS ImageBase,
+ IN UINTN ImageSize,
+ IN VOID *RelocationData,
+ OUT PHYSICAL_ADDRESS *RelocationRangeMin,
+ OUT PHYSICAL_ADDRESS *RelocationRangeMax
+ );
+
#endif
diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
index 97a8aaf8c73d..10f3d04d2490 100644
--- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
+++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
@@ -936,6 +936,8 @@ PeCoffLoaderRelocateImage (
PHYSICAL_ADDRESS BaseAddress;
UINT32 NumberOfRvaAndSizes;
UINT32 TeStrippedOffset;
+ PHYSICAL_ADDRESS *RelocRangeStart;
+ PHYSICAL_ADDRESS *RelocRangeEnd;
ASSERT (ImageContext != NULL);
@@ -1043,6 +1045,21 @@ PeCoffLoaderRelocateImage (
// Run the relocation information and apply the fixups
//
FixupData = ImageContext->FixupData;
+ if (FixupData != NULL) {
+ FixupData = ALIGN_POINTER (FixupData, sizeof (PHYSICAL_ADDRESS));
+
+ //
+ // Use the first two UINT64s in the fixup data to keep track of the start
+ // and end of the region that is subject to relocation fixups.
+ //
+ RelocRangeStart = (PHYSICAL_ADDRESS *)FixupData;
+ RelocRangeEnd = RelocRangeStart + 1;
+ FixupData += 2 * sizeof (PHYSICAL_ADDRESS);
+
+ *RelocRangeStart = MAX_UINT64;
+ *RelocRangeEnd = 0;
+ }
+
while ((UINTN)RelocBase < (UINTN)RelocBaseEnd) {
Reloc = (UINT16 *)((CHAR8 *)RelocBase + sizeof (EFI_IMAGE_BASE_RELOCATION));
//
@@ -1070,6 +1087,14 @@ PeCoffLoaderRelocateImage (
return RETURN_LOAD_ERROR;
}
+ //
+ // Capture this page in the recorded relocation range
+ //
+ if (FixupData != NULL) {
+ *RelocRangeStart = MIN (*RelocRangeStart, (UINTN)FixupBase);
+ *RelocRangeEnd = MAX (*RelocRangeEnd, (UINTN)FixupBase + SIZE_4KB);
+ }
+
//
// Run this relocation record
//
@@ -1470,6 +1495,9 @@ PeCoffLoaderLoadImage (
//
ImageContext->FixupData = NULL;
+ // Add two UINT64s at the start to carry the min/max of the relocated range
+ ImageContext->FixupDataSize += 2 * sizeof (PHYSICAL_ADDRESS);
+
//
// Load the Codeview information if present
//
@@ -1824,7 +1852,8 @@ PeCoffLoaderRelocateImageForRuntime (
// by code will not be fixed up, since that would set them back to
// defaults.
//
- FixupData = RelocationData;
+ FixupData = ALIGN_POINTER (RelocationData, sizeof (PHYSICAL_ADDRESS));
+ FixupData += 2 * sizeof (PHYSICAL_ADDRESS);
RelocBaseOrig = RelocBase;
while ((UINTN)RelocBase < (UINTN)RelocBaseEnd) {
//
@@ -1994,3 +2023,55 @@ PeCoffLoaderUnloadImage (
PeCoffLoaderUnloadImageExtraAction (ImageContext);
return RETURN_SUCCESS;
}
+
+/**
+ Retrieve the range subject to relocation fixups from the recorded fixup data
+ of a runtime image
+
+ @param ImageBase The base address of a PE/COFF image that has been loaded
+ and relocated into system memory.
+ @param VirtImageBase The request virtual address that the PE/COFF image is to
+ be fixed up for.
+ @param ImageSize The size, in bytes, of the PE/COFF image.
+ @param RelocationData A pointer to the relocation data that was collected when the
+ PE/COFF image was relocated using PeCoffLoaderRelocateImage().
+ @param[out] RelocationRangeMin The start of the relocated range.
+ @param[out] RelocationRangeMax The end of the relocated range.
+
+**/
+VOID
+EFIAPI
+PeCoffLoaderGetRelocationRange (
+ IN PHYSICAL_ADDRESS ImageBase,
+ IN UINTN ImageSize,
+ IN VOID *RelocationData,
+ OUT PHYSICAL_ADDRESS *RelocationRangeMin,
+ OUT PHYSICAL_ADDRESS *RelocationRangeMax
+ )
+{
+ PHYSICAL_ADDRESS *FixupData;
+
+ if ((RelocationData == NULL) || (ImageBase == 0x0)) {
+ return;
+ }
+
+ FixupData = ALIGN_POINTER (RelocationData, sizeof (PHYSICAL_ADDRESS));
+
+ if ((FixupData[0] == MAX_UINT64) && (FixupData[1] == 0)) {
+ // No fixups recorded
+ *RelocationRangeMin = ImageBase;
+ *RelocationRangeMax = ImageBase;
+ return;
+ }
+
+ if ((FixupData[0] < ImageBase) ||
+ (FixupData[1] > (ImageBase + ImageSize))) {
+ ASSERT (FALSE);
+ *RelocationRangeMin = ImageBase;
+ *RelocationRangeMax = ImageBase + ImageSize;
+ return;
+ }
+
+ *RelocationRangeMin = FixupData[0];
+ *RelocationRangeMax = FixupData[1];
+}
--
2.39.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [RFC 04/13] MdeModulePkg/DxeIpl: Avoid shadowing IPL PEIM by default
2023-02-13 15:17 [RFC 00/13] Hardware enforced W^X memory protections Ard Biesheuvel
` (2 preceding siblings ...)
2023-02-13 15:18 ` [RFC 03/13] MdePkg/BasePeCoffLib: Add API to keep track of relocation range Ard Biesheuvel
@ 2023-02-13 15:18 ` Ard Biesheuvel
2023-02-13 15:18 ` [RFC 05/13] MdeModulePkg/DxeIpl AARCH64: Remap DXE core code section before launch Ard Biesheuvel
` (8 subsequent siblings)
12 siblings, 0 replies; 21+ messages in thread
From: Ard Biesheuvel @ 2023-02-13 15:18 UTC (permalink / raw)
To: devel
Cc: Ard Biesheuvel, Michael Kinney, Liming Gao, Jiewen Yao,
Michael Kubacki, Sean Brogan, Rebecca Cran, Leif Lindholm,
Sami Mujawar, Taylor Beebe, Matthew Garrett, Peter Jones,
Kees Cook
Currently, the DXE IPL relies on permanent memory being available, but
does not DEPEX on the associated PPI. Instead, it registers for PEIM
shadowing, and only proceeds when running shadowed, and this implies
that permanent memory has been installed.
While PEIM shadowing is typically good for performance, there are
reasons why we might prefer to avoid it, e.g., when running under
virtualization in a mode where the write protection of the ROM is an
advantage from a safety PoV, and where the performance is identical.
This is especially true when code executing from ordinary RAM needs some
additional work to be executable, like when enabling WXN on ARM, which
only permits execution from memory that is mapped read-only.
So permit DXE IPL to run unshadowed, based on the existing PCD that
decides whether or not shadowing is preferred. While making this
behavior depend on this PCD is strictly redundant (as the IPL PEIM will
be shadowed anyway, even if RegisterForShadow() is not called), let's
test it anyway to avoid modifying the behavior on existing platforms.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf | 5 +++-
MdeModulePkg/Core/DxeIplPeim/DxeLoad.c | 24 +++++++++++---------
2 files changed, 17 insertions(+), 12 deletions(-)
diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
index 052ea0ec1a6f..62821477d012 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
@@ -112,6 +112,9 @@ [FeaturePcd.X64]
[FeaturePcd]
gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSupportUefiDecompress ## CONSUMES
+[Pcd]
+ gEfiMdeModulePkgTokenSpaceGuid.PcdShadowPeimOnBoot ## CONSUMES
+
[Pcd.IA32,Pcd.X64]
gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable ## SOMETIMES_CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask ## CONSUMES
@@ -128,7 +131,7 @@ [Pcd.IA32,Pcd.X64,Pcd.ARM,Pcd.AARCH64]
gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy ## SOMETIMES_CONSUMES
[Depex]
- gEfiPeiLoadFilePpiGuid AND gEfiPeiMasterBootModePpiGuid
+ gEfiPeiLoadFilePpiGuid AND gEfiPeiMasterBootModePpiGuid AND gEfiPeiMemoryDiscoveredPpiGuid
#
# [BootMode]
diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c b/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
index 2c19f1a507ba..228d39a618d3 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c
@@ -77,18 +77,20 @@ PeimInitializeDxeIpl (
BootMode = GetBootModeHob ();
if (BootMode != BOOT_ON_S3_RESUME) {
- Status = PeiServicesRegisterForShadow (FileHandle);
- if (Status == EFI_SUCCESS) {
- //
- // EFI_SUCESS means it is the first time to call register for shadow.
- //
- return Status;
- }
+ if (PcdGetBool (PcdShadowPeimOnBoot)) {
+ Status = PeiServicesRegisterForShadow (FileHandle);
+ if (Status == EFI_SUCCESS) {
+ //
+ // EFI_SUCESS means it is the first time to call register for shadow.
+ //
+ return Status;
+ }
- //
- // Ensure that DXE IPL is shadowed to permanent memory.
- //
- ASSERT (Status == EFI_ALREADY_STARTED);
+ //
+ // Ensure that DXE IPL is shadowed to permanent memory.
+ //
+ ASSERT (Status == EFI_ALREADY_STARTED);
+ }
//
// DXE core load requires permanent memory.
--
2.39.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [RFC 05/13] MdeModulePkg/DxeIpl AARCH64: Remap DXE core code section before launch
2023-02-13 15:17 [RFC 00/13] Hardware enforced W^X memory protections Ard Biesheuvel
` (3 preceding siblings ...)
2023-02-13 15:18 ` [RFC 04/13] MdeModulePkg/DxeIpl: Avoid shadowing IPL PEIM by default Ard Biesheuvel
@ 2023-02-13 15:18 ` Ard Biesheuvel
2023-02-13 15:18 ` [RFC 06/13] MdeModulePkg/DxeCore: Reduce range of W+X remaps at EBS time Ard Biesheuvel
` (7 subsequent siblings)
12 siblings, 0 replies; 21+ messages in thread
From: Ard Biesheuvel @ 2023-02-13 15:18 UTC (permalink / raw)
To: devel
Cc: Ard Biesheuvel, Michael Kinney, Liming Gao, Jiewen Yao,
Michael Kubacki, Sean Brogan, Rebecca Cran, Leif Lindholm,
Sami Mujawar, Taylor Beebe, Matthew Garrett, Peter Jones,
Kees Cook
To permit the platform to adopt a stricter policy when it comes to
memory protections, and map all memory XP by default, add the necessary
handling to the DXE IPL PEIM to ensure that the DXE core code section is
mapped executable before invoking the DXE core.
It is up to the DXE core itself to manage the executable permissions on
other DXE and UEFI drivers and applications that it dispatches.
Note that this requires that the DXE IPL executes non-shadowed from a FV
that is mapped executable.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c | 69 ++++++++++++++++++++
MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf | 1 +
2 files changed, 70 insertions(+)
diff --git a/MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c b/MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c
index f62b6dcb38a7..21eac2851554 100644
--- a/MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c
+++ b/MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c
@@ -11,6 +11,69 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include "DxeIpl.h"
#include <Library/ArmMmuLib.h>
+#include <Library/PeCoffLib.h>
+
+STATIC
+VOID
+RemapDxeCoreCodeReadOnly (
+ IN EFI_PHYSICAL_ADDRESS DxeCoreEntryPoint,
+ IN EFI_PEI_HOB_POINTERS HobList
+ )
+{
+ EFI_PEI_HOB_POINTERS Hob;
+ EFI_HOB_MEMORY_ALLOCATION *ModuleHob;
+ PE_COFF_LOADER_IMAGE_CONTEXT ImageContext;
+ RETURN_STATUS Status;
+ EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION Hdr;
+ EFI_IMAGE_SECTION_HEADER *Section;
+ UINTN Index;
+
+ ImageContext.ImageRead = PeCoffLoaderImageReadFromMemory;
+ ImageContext.Handle = NULL;
+
+ //
+ // Find the module HOB for the DXE core
+ //
+ for (Hob.Raw = HobList.Raw; !END_OF_HOB_LIST (Hob); Hob.Raw = GET_NEXT_HOB (Hob)) {
+ if (GET_HOB_TYPE (Hob) == EFI_HOB_TYPE_MEMORY_ALLOCATION) {
+ ModuleHob = Hob.MemoryAllocation;
+ if ((ModuleHob->AllocDescriptor.MemoryBaseAddress <= DxeCoreEntryPoint &&
+ ((ModuleHob->AllocDescriptor.MemoryBaseAddress + ModuleHob->AllocDescriptor.MemoryLength) > DxeCoreEntryPoint)))
+ {
+ ImageContext.Handle = (VOID *)(UINTN)ModuleHob->AllocDescriptor.MemoryBaseAddress;
+ break;
+ }
+ }
+ }
+
+ ASSERT (ImageContext.Handle != NULL);
+
+ Status = PeCoffLoaderGetImageInfo (&ImageContext);
+ ASSERT_RETURN_ERROR (Status);
+
+ Hdr.Union = (EFI_IMAGE_OPTIONAL_HEADER_UNION *)((UINT8 *)ImageContext.Handle +
+ ImageContext.PeCoffHeaderOffset);
+ ASSERT (Hdr.Pe32->Signature == EFI_IMAGE_NT_SIGNATURE);
+
+ Section = (EFI_IMAGE_SECTION_HEADER *)((UINT8 *)Hdr.Union + sizeof (UINT32) +
+ sizeof (EFI_IMAGE_FILE_HEADER) +
+ Hdr.Pe32->FileHeader.SizeOfOptionalHeader
+ );
+
+ for (Index = 0; Index < Hdr.Pe32->FileHeader.NumberOfSections; Index++) {
+ if ((Section[Index].Characteristics & EFI_IMAGE_SCN_CNT_CODE) != 0) {
+ ArmSetMemoryRegionReadOnly (
+ (UINTN)((UINT8 *)ImageContext.Handle + Section[Index].VirtualAddress),
+ Section[Index].Misc.VirtualSize
+ );
+
+ ArmClearMemoryRegionNoExec (
+ (UINTN)((UINT8 *)ImageContext.Handle + Section[Index].VirtualAddress),
+ Section[Index].Misc.VirtualSize
+ );
+ }
+ }
+}
/**
Transfers control to DxeCore.
@@ -33,6 +96,12 @@ HandOffToDxeCore (
VOID *TopOfStack;
EFI_STATUS Status;
+ //
+ // DRAM may be mapped with non-executable permissions by default, so
+ // we'll need to map the DXE core code region executable explicitly.
+ //
+ RemapDxeCoreCodeReadOnly (DxeCoreEntryPoint, HobList);
+
//
// Allocate 128KB for the Stack
//
diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
index 62821477d012..d85ca79dc0c3 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
@@ -82,6 +82,7 @@ [LibraryClasses]
[LibraryClasses.ARM, LibraryClasses.AARCH64]
ArmMmuLib
+ PeCoffLib
[Ppis]
gEfiDxeIplPpiGuid ## PRODUCES
--
2.39.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [RFC 06/13] MdeModulePkg/DxeCore: Reduce range of W+X remaps at EBS time
2023-02-13 15:17 [RFC 00/13] Hardware enforced W^X memory protections Ard Biesheuvel
` (4 preceding siblings ...)
2023-02-13 15:18 ` [RFC 05/13] MdeModulePkg/DxeIpl AARCH64: Remap DXE core code section before launch Ard Biesheuvel
@ 2023-02-13 15:18 ` Ard Biesheuvel
2023-02-13 15:18 ` [RFC 07/13] MdeModulePkg/DxeCore: Permit preliminary CPU arch fallback Ard Biesheuvel
` (6 subsequent siblings)
12 siblings, 0 replies; 21+ messages in thread
From: Ard Biesheuvel @ 2023-02-13 15:18 UTC (permalink / raw)
To: devel
Cc: Ard Biesheuvel, Michael Kinney, Liming Gao, Jiewen Yao,
Michael Kubacki, Sean Brogan, Rebecca Cran, Leif Lindholm,
Sami Mujawar, Taylor Beebe, Matthew Garrett, Peter Jones,
Kees Cook
Instead of remapping all DXE runtime drivers with read-write-execute
permissions entirely when ExitBootServices() is called, remap only the
parts of those images that require writable access for applying
relocation fixups at SetVirtualAddressMap() time.
As illustrated below, this greatly reduces the footprint of those
regions, which is important for safe execution. And given that the most
important ISAs and toolchains split executable code from relocatable
quantities, the remapped pages in question are generally not the ones
that contain code as well.
On a ArmVirtQemu build, the footprint of those RWX pages is shown below.
As future work, we might investigate whether we can find a way to
guarantee in general that images are built in a way where executable
code and relocatable data never share a 4 KiB page, in which case we
could apply EFI_MEMORY_XP permissions here instead of allowing RWX.
Before:
SetUefiImageMemoryAttributes - 0x0000000047600000 - 0x0000000000050000 (0x0000000000000008)
SetUefiImageMemoryAttributes - 0x0000000044290000 - 0x0000000000050000 (0x0000000000000008)
SetUefiImageMemoryAttributes - 0x0000000044230000 - 0x0000000000050000 (0x0000000000000008)
SetUefiImageMemoryAttributes - 0x00000000441D0000 - 0x0000000000050000 (0x0000000000000008)
SetUefiImageMemoryAttributes - 0x00000000440D0000 - 0x0000000000050000 (0x0000000000000008)
SetUefiImageMemoryAttributes - 0x0000000043F90000 - 0x0000000000040000 (0x0000000000000008)
SetUefiImageMemoryAttributes - 0x0000000043F40000 - 0x0000000000040000 (0x0000000000000008)
SetUefiImageMemoryAttributes - 0x0000000043EF0000 - 0x0000000000040000 (0x0000000000000008)
After:
SetUefiImageMemoryAttributes - 0x0000000047630000 - 0x0000000000001000 (0x0000000000000008)
SetUefiImageMemoryAttributes - 0x00000000442C0000 - 0x0000000000001000 (0x0000000000000008)
SetUefiImageMemoryAttributes - 0x0000000044260000 - 0x0000000000001000 (0x0000000000000008)
SetUefiImageMemoryAttributes - 0x0000000044200000 - 0x0000000000001000 (0x0000000000000008)
SetUefiImageMemoryAttributes - 0x0000000044100000 - 0x0000000000001000 (0x0000000000000008)
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 19 ++++++++++++++++++-
1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
index 5a82eee80781..854651556de4 100644
--- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
+++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
@@ -1060,6 +1060,8 @@ MemoryProtectionExitBootServicesCallback (
{
EFI_RUNTIME_IMAGE_ENTRY *RuntimeImage;
LIST_ENTRY *Link;
+ PHYSICAL_ADDRESS RelocationRangeStart;
+ PHYSICAL_ADDRESS RelocationRangeEnd;
//
// We need remove the RT protection, because RT relocation need write code segment
@@ -1073,7 +1075,22 @@ MemoryProtectionExitBootServicesCallback (
if (mImageProtectionPolicy != 0) {
for (Link = gRuntime->ImageHead.ForwardLink; Link != &gRuntime->ImageHead; Link = Link->ForwardLink) {
RuntimeImage = BASE_CR (Link, EFI_RUNTIME_IMAGE_ENTRY, Link);
- SetUefiImageMemoryAttributes ((UINT64)(UINTN)RuntimeImage->ImageBase, ALIGN_VALUE (RuntimeImage->ImageSize, EFI_PAGE_SIZE), 0);
+
+ PeCoffLoaderGetRelocationRange (
+ (PHYSICAL_ADDRESS)(UINTN)RuntimeImage->ImageBase,
+ ALIGN_VALUE (RuntimeImage->ImageSize, EFI_PAGE_SIZE),
+ RuntimeImage->RelocationData,
+ &RelocationRangeStart,
+ &RelocationRangeEnd
+ );
+
+ if (RelocationRangeEnd > RelocationRangeStart) {
+ SetUefiImageMemoryAttributes (
+ RelocationRangeStart,
+ (UINTN)(RelocationRangeEnd - RelocationRangeStart),
+ 0
+ );
+ }
}
}
}
--
2.39.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [RFC 07/13] MdeModulePkg/DxeCore: Permit preliminary CPU arch fallback
2023-02-13 15:17 [RFC 00/13] Hardware enforced W^X memory protections Ard Biesheuvel
` (5 preceding siblings ...)
2023-02-13 15:18 ` [RFC 06/13] MdeModulePkg/DxeCore: Reduce range of W+X remaps at EBS time Ard Biesheuvel
@ 2023-02-13 15:18 ` Ard Biesheuvel
2023-02-13 21:32 ` [edk2-devel] " Marvin Häuser
2023-02-13 15:18 ` [RFC 08/13] ArmPkg: Implement ArmSetMemoryOverrideLib Ard Biesheuvel
` (5 subsequent siblings)
12 siblings, 1 reply; 21+ messages in thread
From: Ard Biesheuvel @ 2023-02-13 15:18 UTC (permalink / raw)
To: devel
Cc: Ard Biesheuvel, Michael Kinney, Liming Gao, Jiewen Yao,
Michael Kubacki, Sean Brogan, Rebecca Cran, Leif Lindholm,
Sami Mujawar, Taylor Beebe, Matthew Garrett, Peter Jones,
Kees Cook
Store the address of the SetMemoryAttributes() member of the CPU arch
protocol in a global variable, and invoke it via this variable. This
by itself should have not result in functional changes, but it permits
platforms to provide an preliminary implementation of this member at
link time, allowing the DXE core to enforce strict memory permissions
even before dispatching the CPU arch protocol driver itself.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
index 854651556de4..c29985ad3116 100644
--- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
+++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
@@ -66,6 +66,8 @@ extern LIST_ENTRY mGcdMemorySpaceMap;
STATIC LIST_ENTRY mProtectedImageRecordList;
+EFI_CPU_SET_MEMORY_ATTRIBUTES gCpuSetMemoryAttributes;
+
/**
Sort code section in image record, based upon CodeSegmentBase from low to high.
@@ -224,8 +226,8 @@ SetUefiImageMemoryAttributes (
DEBUG ((DEBUG_INFO, "SetUefiImageMemoryAttributes - 0x%016lx - 0x%016lx (0x%016lx)\n", BaseAddress, Length, FinalAttributes));
- ASSERT (gCpu != NULL);
- gCpu->SetMemoryAttributes (gCpu, BaseAddress, Length, FinalAttributes);
+ ASSERT (gCpuSetMemoryAttributes != NULL);
+ gCpuSetMemoryAttributes (gCpu, BaseAddress, Length, FinalAttributes);
}
/**
@@ -408,7 +410,7 @@ ProtectUefiImage (
DEBUG ((DEBUG_INFO, "ProtectUefiImageCommon - 0x%x\n", LoadedImage));
DEBUG ((DEBUG_INFO, " - 0x%016lx - 0x%016lx\n", (EFI_PHYSICAL_ADDRESS)(UINTN)LoadedImage->ImageBase, LoadedImage->ImageSize));
- if (gCpu == NULL) {
+ if (gCpuSetMemoryAttributes == NULL) {
return;
}
@@ -995,6 +997,8 @@ MemoryProtectionCpuArchProtocolNotify (
goto Done;
}
+ gCpuSetMemoryAttributes = gCpu->SetMemoryAttributes;
+
//
// Apply the memory protection policy on non-BScode/RTcode regions.
//
@@ -1278,7 +1282,7 @@ ApplyMemoryProtectionPolicy (
// permission attributes, and it is the job of the driver that installs this
// protocol to set the permissions on existing allocations.
//
- if (gCpu == NULL) {
+ if (gCpuSetMemoryAttributes == NULL) {
return EFI_SUCCESS;
}
@@ -1318,5 +1322,5 @@ ApplyMemoryProtectionPolicy (
//
NewAttributes = GetPermissionAttributeForMemoryType (NewType);
- return gCpu->SetMemoryAttributes (gCpu, Memory, Length, NewAttributes);
+ return gCpuSetMemoryAttributes (gCpu, Memory, Length, NewAttributes);
}
--
2.39.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [edk2-devel] [RFC 07/13] MdeModulePkg/DxeCore: Permit preliminary CPU arch fallback
2023-02-13 15:18 ` [RFC 07/13] MdeModulePkg/DxeCore: Permit preliminary CPU arch fallback Ard Biesheuvel
@ 2023-02-13 21:32 ` Marvin Häuser
2023-02-13 22:07 ` Ard Biesheuvel
0 siblings, 1 reply; 21+ messages in thread
From: Marvin Häuser @ 2023-02-13 21:32 UTC (permalink / raw)
To: Ard Biesheuvel, devel
[-- Attachment #1: Type: text/plain, Size: 830 bytes --]
Without wanting to blow up your RFC with another one - I discussed this with various people, including Bret when he was still at Project Mu, and there was a consensus among them that integrating the whole CPU arch code right into DxeCore would be a good idea. This would especially remove the hack that queues permission applications till CpuDxe loads for good, rather than requiring pro-active consumption of a library that proves this "fallback". For most of the architectural protocols, especially SecurityStubDxe, I never got the gist why you would want them to be separate from DxeCore. Obviously there should be a level of customizability for IBVs and OEMs, though that can be done statically-linked as well.
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3223
What's your take on this?
Best regards,
Marvin
[-- Attachment #2: Type: text/html, Size: 929 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [edk2-devel] [RFC 07/13] MdeModulePkg/DxeCore: Permit preliminary CPU arch fallback
2023-02-13 21:32 ` [edk2-devel] " Marvin Häuser
@ 2023-02-13 22:07 ` Ard Biesheuvel
2023-02-13 22:24 ` Marvin Häuser
0 siblings, 1 reply; 21+ messages in thread
From: Ard Biesheuvel @ 2023-02-13 22:07 UTC (permalink / raw)
To: Marvin Häuser; +Cc: devel
On Mon, 13 Feb 2023 at 22:32, Marvin Häuser <mhaeuser@posteo.de> wrote:
>
> Without wanting to blow up your RFC with another one - I discussed this with various people, including Bret when he was still at Project Mu, and there was a consensus among them that integrating the whole CPU arch code right into DxeCore would be a good idea. This would especially remove the hack that queues permission applications till CpuDxe loads for good, rather than requiring pro-active consumption of a library that proves this "fallback". For most of the architectural protocols, especially SecurityStubDxe, I never got the gist why you would want them to be separate from DxeCore. Obviously there should be a level of customizability for IBVs and OEMs, though that can be done statically-linked as well.
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3223
>
> What's your take on this?
>
My take is that page table manipulation should not be part of the CPU
arch protocol. The DXE core loads images and dispatches them, which
also involves cache maintenance, for instance, as code pages need to
be invalidated from the I-cache before you can execute a newly loaded
image. I think it makes perfect sense for the DXE core to be in charge
of updating the page table descriptors as well.
In my approach, the library version is only used before the CPU arch
protocol appears, so it addresses some of the concerns regarding
multiple owners. But I'd prefer to see this removed from the CPU arch
protocol entirely, or at least for the CPU arch protocol interface to
be deprecated, and redefined in terms of a new DXE services, for
instance, that is implemented by the DXE core itself. That way, all
existing users would still see the same protocols, but the way they
depend on each other would be inverted.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [edk2-devel] [RFC 07/13] MdeModulePkg/DxeCore: Permit preliminary CPU arch fallback
2023-02-13 22:07 ` Ard Biesheuvel
@ 2023-02-13 22:24 ` Marvin Häuser
0 siblings, 0 replies; 21+ messages in thread
From: Marvin Häuser @ 2023-02-13 22:24 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: devel
Sounds good to me, thanks!
Best regards,
Marvin
> On 13. Feb 2023, at 23:07, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Mon, 13 Feb 2023 at 22:32, Marvin Häuser <mhaeuser@posteo.de> wrote:
>>
>> Without wanting to blow up your RFC with another one - I discussed this with various people, including Bret when he was still at Project Mu, and there was a consensus among them that integrating the whole CPU arch code right into DxeCore would be a good idea. This would especially remove the hack that queues permission applications till CpuDxe loads for good, rather than requiring pro-active consumption of a library that proves this "fallback". For most of the architectural protocols, especially SecurityStubDxe, I never got the gist why you would want them to be separate from DxeCore. Obviously there should be a level of customizability for IBVs and OEMs, though that can be done statically-linked as well.
>>
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3223
>>
>> What's your take on this?
>>
>
> My take is that page table manipulation should not be part of the CPU
> arch protocol. The DXE core loads images and dispatches them, which
> also involves cache maintenance, for instance, as code pages need to
> be invalidated from the I-cache before you can execute a newly loaded
> image. I think it makes perfect sense for the DXE core to be in charge
> of updating the page table descriptors as well.
>
> In my approach, the library version is only used before the CPU arch
> protocol appears, so it addresses some of the concerns regarding
> multiple owners. But I'd prefer to see this removed from the CPU arch
> protocol entirely, or at least for the CPU arch protocol interface to
> be deprecated, and redefined in terms of a new DXE services, for
> instance, that is implemented by the DXE core itself. That way, all
> existing users would still see the same protocols, but the way they
> depend on each other would be inverted.
^ permalink raw reply [flat|nested] 21+ messages in thread
* [RFC 08/13] ArmPkg: Implement ArmSetMemoryOverrideLib
2023-02-13 15:17 [RFC 00/13] Hardware enforced W^X memory protections Ard Biesheuvel
` (6 preceding siblings ...)
2023-02-13 15:18 ` [RFC 07/13] MdeModulePkg/DxeCore: Permit preliminary CPU arch fallback Ard Biesheuvel
@ 2023-02-13 15:18 ` Ard Biesheuvel
2023-02-13 15:18 ` [RFC 09/13] ArmVirtPkg/ArmVirtQemu: Use XP memory mappings by default Ard Biesheuvel
` (4 subsequent siblings)
12 siblings, 0 replies; 21+ messages in thread
From: Ard Biesheuvel @ 2023-02-13 15:18 UTC (permalink / raw)
To: devel
Cc: Ard Biesheuvel, Michael Kinney, Liming Gao, Jiewen Yao,
Michael Kubacki, Sean Brogan, Rebecca Cran, Leif Lindholm,
Sami Mujawar, Taylor Beebe, Matthew Garrett, Peter Jones,
Kees Cook
Implement the ARM version of a NULL class library that can be overlaid
on top of the DXE core to equip it right from its launch with an
implementation of the CPU arch protocol member that sets type and
permission attributes on memory regions.
This bridges the gap between dispatch of DXE core and dispatch of the
DXE driver that implements the CPU arch protocol, removing the need to
rely on memory mappings that are writable and executable at the same
time.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
ArmPkg/Library/ArmSetMemoryOverrideLib/ArmSetMemoryOverrideLib.c | 56 ++++++++++++++++++++
ArmPkg/Library/ArmSetMemoryOverrideLib/ArmSetMemoryOverrideLib.inf | 25 +++++++++
2 files changed, 81 insertions(+)
diff --git a/ArmPkg/Library/ArmSetMemoryOverrideLib/ArmSetMemoryOverrideLib.c b/ArmPkg/Library/ArmSetMemoryOverrideLib/ArmSetMemoryOverrideLib.c
new file mode 100644
index 000000000000..d2a9bc96be35
--- /dev/null
+++ b/ArmPkg/Library/ArmSetMemoryOverrideLib/ArmSetMemoryOverrideLib.c
@@ -0,0 +1,56 @@
+/** @file
+ Copyright (c) 2023, Google LLC. All rights reserved.
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#include <PiDxe.h>
+
+#include <Library/ArmMmuLib.h>
+#include <Library/DebugLib.h>
+#include <Protocol/Cpu.h>
+
+extern EFI_CPU_SET_MEMORY_ATTRIBUTES gCpuSetMemoryAttributes;
+
+STATIC UINTN mRecursionLevel;
+
+STATIC
+EFI_STATUS
+EFIAPI
+EarlyArmSetMemoryAttributes (
+ IN EFI_CPU_ARCH_PROTOCOL *This,
+ IN EFI_PHYSICAL_ADDRESS BaseAddress,
+ IN UINT64 Length,
+ IN UINT64 Attributes
+ )
+{
+ EFI_STATUS Status;
+
+ // There are cases where the use of strict memory permissions may trigger
+ // unbounded recursion in the page table code. This happens when setting
+ // memory permissions results in a page table split and therefore a page
+ // allocation, which could trigger a recursive invocation of this function.
+ ASSERT (mRecursionLevel < 2);
+
+ mRecursionLevel++;
+
+ Status = ArmSetMemoryAttributes (
+ BaseAddress,
+ Length,
+ Attributes
+ );
+
+ mRecursionLevel--;
+ return Status;
+}
+
+RETURN_STATUS
+EFIAPI
+ArmSetMemoryOverrideLibConstructor (
+ VOID
+ )
+{
+ gCpuSetMemoryAttributes = EarlyArmSetMemoryAttributes;
+
+ return RETURN_SUCCESS;
+}
diff --git a/ArmPkg/Library/ArmSetMemoryOverrideLib/ArmSetMemoryOverrideLib.inf b/ArmPkg/Library/ArmSetMemoryOverrideLib/ArmSetMemoryOverrideLib.inf
new file mode 100644
index 000000000000..f07da3dd2d15
--- /dev/null
+++ b/ArmPkg/Library/ArmSetMemoryOverrideLib/ArmSetMemoryOverrideLib.inf
@@ -0,0 +1,25 @@
+#/** @file
+# Copyright (c) 2023, Google LLC. All rights reserved.
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#**/
+
+[Defines]
+ INF_VERSION = 1.29
+ BASE_NAME = ArmSetMemoryOverrideLib
+ FILE_GUID = 849a43c0-6ad9-428e-8a5a-e090f7853bd3
+ MODULE_TYPE = BASE
+ VERSION_STRING = 1.0
+ LIBRARY_CLASS = NULL|DXE_CORE
+ CONSTRUCTOR = ArmSetMemoryOverrideLibConstructor
+
+[Sources.common]
+ ArmSetMemoryOverrideLib.c
+
+[Packages]
+ ArmPkg/ArmPkg.dec
+ MdePkg/MdePkg.dec
+
+[LibraryClasses]
+ ArmMmuLib
+ DebugLib
--
2.39.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [RFC 09/13] ArmVirtPkg/ArmVirtQemu: Use XP memory mappings by default
2023-02-13 15:17 [RFC 00/13] Hardware enforced W^X memory protections Ard Biesheuvel
` (7 preceding siblings ...)
2023-02-13 15:18 ` [RFC 08/13] ArmPkg: Implement ArmSetMemoryOverrideLib Ard Biesheuvel
@ 2023-02-13 15:18 ` Ard Biesheuvel
2023-02-13 15:18 ` [RFC 10/13] ArmVirtPkg/ArmVirtQemu: Use PEI flavor of ArmMmuLib for all PEIMs Ard Biesheuvel
` (3 subsequent siblings)
12 siblings, 0 replies; 21+ messages in thread
From: Ard Biesheuvel @ 2023-02-13 15:18 UTC (permalink / raw)
To: devel
Cc: Ard Biesheuvel, Michael Kinney, Liming Gao, Jiewen Yao,
Michael Kubacki, Sean Brogan, Rebecca Cran, Leif Lindholm,
Sami Mujawar, Taylor Beebe, Matthew Garrett, Peter Jones,
Kees Cook
Now that all the plumbing is in place, we can switch to a default policy
of XP for all memory mappings straight out of reset. This reduces the
risk of running with memory ranges mapped as both writable and
executable at the same time.
Note this this requires the overlay library to be added to the DXE core,
as otherwise, it will not be able to dispatch the CPU arch protocol DXE
driver (or any other DXE driver for that matter), as it would lack the
ability to grant executable permissions to those executables.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
ArmVirtPkg/ArmVirtQemu.dsc | 1 +
ArmVirtPkg/ArmVirtQemuKernel.dsc | 1 +
ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c | 2 +-
3 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index 0f1c6395488a..dd4c84ae6eb9 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -372,6 +372,7 @@ [Components.common]
#
MdeModulePkg/Core/Dxe/DxeMain.inf {
<LibraryClasses>
+ NULL|ArmPkg/Library/ArmSetMemoryOverrideLib/ArmSetMemoryOverrideLib.inf
NULL|MdeModulePkg/Library/DxeCrc32GuidedSectionExtractLib/DxeCrc32GuidedSectionExtractLib.inf
DevicePathLib|MdePkg/Library/UefiDevicePathLib/UefiDevicePathLib.inf
}
diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
index 807c85d48285..1ea49fd32e9c 100644
--- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
+++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
@@ -278,6 +278,7 @@ [Components.common]
#
MdeModulePkg/Core/Dxe/DxeMain.inf {
<LibraryClasses>
+ NULL|ArmPkg/Library/ArmSetMemoryOverrideLib/ArmSetMemoryOverrideLib.inf
NULL|MdeModulePkg/Library/DxeCrc32GuidedSectionExtractLib/DxeCrc32GuidedSectionExtractLib.inf
DevicePathLib|MdePkg/Library/UefiDevicePathLib/UefiDevicePathLib.inf
}
diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
index 9cf43f06c073..aa083cec2082 100644
--- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
+++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
@@ -91,7 +91,7 @@ ArmVirtGetMemoryMap (
VirtualMemoryTable[0].PhysicalBase = PcdGet64 (PcdSystemMemoryBase);
VirtualMemoryTable[0].VirtualBase = VirtualMemoryTable[0].PhysicalBase;
VirtualMemoryTable[0].Length = *(UINT64 *)GET_GUID_HOB_DATA (MemorySizeHob);
- VirtualMemoryTable[0].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
+ VirtualMemoryTable[0].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK_XP;
DEBUG ((
DEBUG_INFO,
--
2.39.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [RFC 10/13] ArmVirtPkg/ArmVirtQemu: Use PEI flavor of ArmMmuLib for all PEIMs
2023-02-13 15:17 [RFC 00/13] Hardware enforced W^X memory protections Ard Biesheuvel
` (8 preceding siblings ...)
2023-02-13 15:18 ` [RFC 09/13] ArmVirtPkg/ArmVirtQemu: Use XP memory mappings by default Ard Biesheuvel
@ 2023-02-13 15:18 ` Ard Biesheuvel
2023-02-13 15:18 ` [RFC 11/13] ArmVirtPkg/ArmVirtQemu: Use read-only memory region type for code flash Ard Biesheuvel
` (2 subsequent siblings)
12 siblings, 0 replies; 21+ messages in thread
From: Ard Biesheuvel @ 2023-02-13 15:18 UTC (permalink / raw)
To: devel
Cc: Ard Biesheuvel, Michael Kinney, Liming Gao, Jiewen Yao,
Michael Kubacki, Sean Brogan, Rebecca Cran, Leif Lindholm,
Sami Mujawar, Taylor Beebe, Matthew Garrett, Peter Jones,
Kees Cook
The PEI flavor of the ArmMmuLib will install a HOB that exposes its
implementation of the special helper routine that is used to update live
entries, so that other instantiations of ArmMmuLib can invoke it. This
is needed to ensure that splitting page tables using break-before-make
(BBM) does not unmap the code that is performing the split.
However, the BASE variety of ArmMmuLib discovers the HOB and sets a
global pointer to refer to it, which is not possible in PEIMs, and so
all PEIMs must use the PEI variety of this library if one does.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
ArmVirtPkg/ArmVirtQemu.dsc | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index dd4c84ae6eb9..69e51d19300d 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -107,6 +107,9 @@ [LibraryClasses.common.PEIM]
Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
!endif
+[LibraryClasses.AARCH64.PEIM]
+ ArmMmuLib|ArmPkg/Library/ArmMmuLib/ArmMmuPeiLib.inf
+
[LibraryClasses.common.DXE_DRIVER]
ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
@@ -333,12 +336,7 @@ [Components.common]
ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf
MdeModulePkg/Core/Pei/PeiMain.inf
ArmPlatformPkg/PlatformPei/PlatformPeim.inf
- ArmVirtPkg/MemoryInitPei/MemoryInitPeim.inf {
- <LibraryClasses>
-!if $(ARCH) == AARCH64
- ArmMmuLib|ArmPkg/Library/ArmMmuLib/ArmMmuPeiLib.inf
-!endif
- }
+ ArmVirtPkg/MemoryInitPei/MemoryInitPeim.inf
ArmPkg/Drivers/CpuPei/CpuPei.inf
!if $(TPM2_ENABLE) == TRUE
--
2.39.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [RFC 11/13] ArmVirtPkg/ArmVirtQemu: Use read-only memory region type for code flash
2023-02-13 15:17 [RFC 00/13] Hardware enforced W^X memory protections Ard Biesheuvel
` (9 preceding siblings ...)
2023-02-13 15:18 ` [RFC 10/13] ArmVirtPkg/ArmVirtQemu: Use PEI flavor of ArmMmuLib for all PEIMs Ard Biesheuvel
@ 2023-02-13 15:18 ` Ard Biesheuvel
2023-02-13 15:18 ` [RFC 12/13] BaseTools/GccBase AARCH64: Avoid page sharing between code and data Ard Biesheuvel
2023-02-13 15:18 ` [RFC 13/13] ArmVirtPkg/ArmVirtQemu: Enable hardware enforced W^X memory permissions Ard Biesheuvel
12 siblings, 0 replies; 21+ messages in thread
From: Ard Biesheuvel @ 2023-02-13 15:18 UTC (permalink / raw)
To: devel
Cc: Ard Biesheuvel, Michael Kinney, Liming Gao, Jiewen Yao,
Michael Kubacki, Sean Brogan, Rebecca Cran, Leif Lindholm,
Sami Mujawar, Taylor Beebe, Matthew Garrett, Peter Jones,
Kees Cook
Map the code flash with read-only attributes so we can execute from it
even under a memory protection regime that enables WXN, making all
writable memory regions non-executable by default.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
index aa083cec2082..a5324b1e4eed 100644
--- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
+++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c
@@ -115,7 +115,7 @@ ArmVirtGetMemoryMap (
VirtualMemoryTable[2].PhysicalBase = PcdGet64 (PcdFvBaseAddress);
VirtualMemoryTable[2].VirtualBase = VirtualMemoryTable[2].PhysicalBase;
VirtualMemoryTable[2].Length = FixedPcdGet32 (PcdFvSize);
- VirtualMemoryTable[2].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
+ VirtualMemoryTable[2].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK_RO;
// End of Table
ZeroMem (&VirtualMemoryTable[3], sizeof (ARM_MEMORY_REGION_DESCRIPTOR));
--
2.39.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [RFC 12/13] BaseTools/GccBase AARCH64: Avoid page sharing between code and data
2023-02-13 15:17 [RFC 00/13] Hardware enforced W^X memory protections Ard Biesheuvel
` (10 preceding siblings ...)
2023-02-13 15:18 ` [RFC 11/13] ArmVirtPkg/ArmVirtQemu: Use read-only memory region type for code flash Ard Biesheuvel
@ 2023-02-13 15:18 ` Ard Biesheuvel
2023-02-13 15:18 ` [RFC 13/13] ArmVirtPkg/ArmVirtQemu: Enable hardware enforced W^X memory permissions Ard Biesheuvel
12 siblings, 0 replies; 21+ messages in thread
From: Ard Biesheuvel @ 2023-02-13 15:18 UTC (permalink / raw)
To: devel
Cc: Ard Biesheuvel, Michael Kinney, Liming Gao, Jiewen Yao,
Michael Kubacki, Sean Brogan, Rebecca Cran, Leif Lindholm,
Sami Mujawar, Taylor Beebe, Matthew Garrett, Peter Jones,
Kees Cook
The AArch64 ARM architecture supports a hardware enforcement mode for
mutual exclusion between code and data: any page that is mapped writable
is implicitly non-executable as well.
This means that remapping part of a runtime image for reapplying
relocation fixups may result in any code sharing the same page to lose
its executable permissions.
Let's avoid this, by moving all quantities that are subject to
relocation fixups to a separate page if the build is using 64k section
alignment, which is only the case when building a runtime driver for
AArch64.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
BaseTools/Scripts/GccBase.lds | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/BaseTools/Scripts/GccBase.lds b/BaseTools/Scripts/GccBase.lds
index 83cebd29d599..63e097e0727c 100644
--- a/BaseTools/Scripts/GccBase.lds
+++ b/BaseTools/Scripts/GccBase.lds
@@ -21,9 +21,8 @@ SECTIONS {
. = PECOFF_HEADER_SIZE;
.text : ALIGN(CONSTANT(COMMONPAGESIZE)) {
- *(.text .text.* .stub .gnu.linkonce.t.*)
+ *(.text .text.* .stub .gnu.linkonce.t.* .plt)
*(.rodata .rodata.* .gnu.linkonce.r.*)
- *(.got .got.*)
/*
* The contents of AutoGen.c files are mostly constant from the POV of the
@@ -34,6 +33,16 @@ SECTIONS {
* emitted GUIDs here.
*/
*:AutoGen.obj(.data.g*Guid)
+
+ /*
+ * AArch64 runtime drivers use 64k alignment, and may run in a mode where
+ * mutual exclusion of RO and XP mappings are hardware enforced. In such
+ * cases, the input sections below, which carry any quantities that are
+ * subject to relocation fixups at runtime, must not share a 4 KiB page
+ * with any code content.
+ */
+ . = ALIGN(CONSTANT(COMMONPAGESIZE) > 0x1000 ? 0x1000 : 0x20);
+ *(.got .got.* .data.rel.ro)
}
/*
--
2.39.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [RFC 13/13] ArmVirtPkg/ArmVirtQemu: Enable hardware enforced W^X memory permissions
2023-02-13 15:17 [RFC 00/13] Hardware enforced W^X memory protections Ard Biesheuvel
` (11 preceding siblings ...)
2023-02-13 15:18 ` [RFC 12/13] BaseTools/GccBase AARCH64: Avoid page sharing between code and data Ard Biesheuvel
@ 2023-02-13 15:18 ` Ard Biesheuvel
2023-02-13 21:16 ` [edk2-devel] " Marvin Häuser
12 siblings, 1 reply; 21+ messages in thread
From: Ard Biesheuvel @ 2023-02-13 15:18 UTC (permalink / raw)
To: devel
Cc: Ard Biesheuvel, Michael Kinney, Liming Gao, Jiewen Yao,
Michael Kubacki, Sean Brogan, Rebecca Cran, Leif Lindholm,
Sami Mujawar, Taylor Beebe, Matthew Garrett, Peter Jones,
Kees Cook
Enable the WXN system control bit straight out of reset when running in
EL1 with the initial ID map from flash. This setting will be inherited
by the page table code after it sets up the permanent boot time page
tables, resulting in all memory mappings that are not explicitly mapped
as read-only to be non-executable.
Note that this requires runtime drivers to be built with position
independent codegen, to ensure that all absolute symbol references are
moved into a separate section in the binary. Otherwise, unmapping the
pages that are subject to relocation fixups at runtime (during the
invocation of SetVirtualAddressMap()) could result in code mappings
losing their executable permissions.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
ArmVirtPkg/ArmVirt.dsc.inc | 1 +
ArmVirtPkg/Library/ArmPlatformLibQemu/AArch64/ArmPlatformHelper.S | 2 +-
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
index 5b18184be263..928dd6330edb 100644
--- a/ArmVirtPkg/ArmVirt.dsc.inc
+++ b/ArmVirtPkg/ArmVirt.dsc.inc
@@ -31,6 +31,7 @@ [BuildOptions.common.EDKII.DXE_CORE,BuildOptions.common.EDKII.DXE_DRIVER,BuildOp
[BuildOptions.common.EDKII.DXE_RUNTIME_DRIVER]
GCC:*_*_ARM_DLINK_FLAGS = -z common-page-size=0x1000
+ GCC:*_*_AARCH64_CC_FLAGS = -fpie
GCC:*_*_AARCH64_DLINK_FLAGS = -z common-page-size=0x10000
[LibraryClasses.common]
diff --git a/ArmVirtPkg/Library/ArmPlatformLibQemu/AArch64/ArmPlatformHelper.S b/ArmVirtPkg/Library/ArmPlatformLibQemu/AArch64/ArmPlatformHelper.S
index 5ac7c732f6ec..51c089a45ffc 100644
--- a/ArmVirtPkg/Library/ArmPlatformLibQemu/AArch64/ArmPlatformHelper.S
+++ b/ArmVirtPkg/Library/ArmPlatformLibQemu/AArch64/ArmPlatformHelper.S
@@ -38,7 +38,7 @@
.set SCTLR_EL1_ITD, 0x1 << 7
.set SCTLR_EL1_RES1, (0x1 << 11) | (0x1 << 20) | (0x1 << 22) | (0x1 << 28) | (0x1 << 29)
.set sctlrval, SCTLR_ELx_M | SCTLR_ELx_C | SCTLR_ELx_SA | SCTLR_EL1_ITD | SCTLR_EL1_SED
- .set sctlrval, sctlrval | SCTLR_ELx_I | SCTLR_EL1_SPAN | SCTLR_EL1_RES1
+ .set sctlrval, sctlrval | SCTLR_ELx_I | SCTLR_EL1_SPAN | SCTLR_EL1_RES1 | SCTLR_EL1_WXN
ASM_FUNC(ArmPlatformPeiBootAction)
--
2.39.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [edk2-devel] [RFC 13/13] ArmVirtPkg/ArmVirtQemu: Enable hardware enforced W^X memory permissions
2023-02-13 15:18 ` [RFC 13/13] ArmVirtPkg/ArmVirtQemu: Enable hardware enforced W^X memory permissions Ard Biesheuvel
@ 2023-02-13 21:16 ` Marvin Häuser
2023-02-13 21:59 ` Ard Biesheuvel
0 siblings, 1 reply; 21+ messages in thread
From: Marvin Häuser @ 2023-02-13 21:16 UTC (permalink / raw)
To: Ard Biesheuvel, devel
[-- Attachment #1: Type: text/plain, Size: 4294 bytes --]
Hey Ard,
*Praise* to you for this series. Comments inline.
On Mon, Feb 13, 2023 at 07:19 AM, Ard Biesheuvel wrote:
>
> Enable the WXN system control bit straight out of reset when running in
> EL1 with the initial ID map from flash. This setting will be inherited
> by the page table code after it sets up the permanent boot time page
> tables, resulting in all memory mappings that are not explicitly mapped
> as read-only to be non-executable.
>
> Note that this requires runtime drivers to be built with position
> independent codegen, to ensure that all absolute symbol references are
> moved into a separate section in the binary. Otherwise, unmapping the
> pages that are subject to relocation fixups at runtime (during the
> invocation of SetVirtualAddressMap()) could result in code mappings
> losing their executable permissions.
I never actually thought about this. SetVirtualAddressMap() will have to relocate its own parent binary, causing issues for software W^X when .text relocs are present (like with MSVC builds). :(
>
>
> Signed-off-by: Ard Biesheuvel <ardb@...>
> ---
> ArmVirtPkg/ArmVirt.dsc.inc | 1 +
> ArmVirtPkg/Library/ArmPlatformLibQemu/AArch64/ArmPlatformHelper.S | 2 +-
> 2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
> index 5b18184be263..928dd6330edb 100644
> --- a/ArmVirtPkg/ArmVirt.dsc.inc
> +++ b/ArmVirtPkg/ArmVirt.dsc.inc
> @@ -31,6 +31,7 @@
> [BuildOptions.common.EDKII.DXE_CORE,BuildOptions.common.E=
> DKII.DXE_DRIVER,BuildOp
> =0D
> [BuildOptions.common.EDKII.DXE_RUNTIME_DRIVER]=0D
> GCC:*_*_ARM_DLINK_FLAGS =3D -z common-page-size=3D0x1000=0D
> + GCC:*_*_AARCH64_CC_FLAGS =3D -fpie=0D
Doesn't this mean -pie must be passed to the linker? I saw in the previous patch that .plt was added to the linker script, was there a particular reason -fno-plt wasn't used here? I just read it may have some unexpected side-effects, but I thought it would be safe for our statically-linked UEFI environment.
On another (related) matter, I've been spending my last two days looking into the whole ELF-to-PE process, because GenFw has been becoming unbearable to us downstream. I went through a bunch of old commits which deal with PIE and saw it was usually disabled but for X64. The funny thing with X64 (even currently) is, that -fpie is combined with -q (a.k.a. --emit-relocs), yielding both object file relocs (.rela.sectname) and PIE-related relative relocs (.rela) in the same binary (as documented in GenFw, they may overlap!). It's my understanding that GenFw currently processes exclusively the -q relocs and not the -fpie relocs (which should be safe as done for X64, I have no experience with ARM whatsoever). However, when PIE is involved anyway, it makes most sense to me to use its related relocs for the translation over a dance with the object file relocs. This change will cause the same behaviour for AARCH64 RT drivers now, right?
In an ideal world, I suppose all architectures but IA32 (due to lacking efficient pcrel addressing) should be using PIE, as most (often all with X64) GOT references can be relaxed, as we strictly deal with local symbols. Though I have to wonder how unideal the world really is. :)
Best regards,
Marvin
>
> GCC:*_*_AARCH64_DLINK_FLAGS =3D -z common-page-size=3D0x10000=0D
> =0D
> [LibraryClasses.common]=0D
> diff --git
> a/ArmVirtPkg/Library/ArmPlatformLibQemu/AArch64/ArmPlatformHelpe=
> r.S b/ArmVirtPkg/Library/ArmPlatformLibQemu/AArch64/ArmPlatformHelper.S
> index 5ac7c732f6ec..51c089a45ffc 100644
> --- a/ArmVirtPkg/Library/ArmPlatformLibQemu/AArch64/ArmPlatformHelper.S
> +++ b/ArmVirtPkg/Library/ArmPlatformLibQemu/AArch64/ArmPlatformHelper.S
> @@ -38,7 +38,7 @@
> .set SCTLR_EL1_ITD, 0x1 << 7=0D
> .set SCTLR_EL1_RES1, (0x1 << 11) | (0x1 << 20) | (0x1 << 22) | (0=
> x1 << 28) | (0x1 << 29)=0D
> .set sctlrval, SCTLR_ELx_M | SCTLR_ELx_C | SCTLR_ELx_SA | SCTLR_EL1_IT=
> D | SCTLR_EL1_SED=0D
> - .set sctlrval, sctlrval | SCTLR_ELx_I | SCTLR_EL1_SPAN | SCTLR_EL1_RES=
> 1=0D
> + .set sctlrval, sctlrval | SCTLR_ELx_I | SCTLR_EL1_SPAN | SCTLR_EL1_RES=
> 1 | SCTLR_EL1_WXN=0D
> =0D
> =0D
> ASM_FUNC(ArmPlatformPeiBootAction)=0D
> --=20
> 2.39.1
[-- Attachment #2: Type: text/html, Size: 4549 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [edk2-devel] [RFC 13/13] ArmVirtPkg/ArmVirtQemu: Enable hardware enforced W^X memory permissions
2023-02-13 21:16 ` [edk2-devel] " Marvin Häuser
@ 2023-02-13 21:59 ` Ard Biesheuvel
2023-02-13 22:23 ` Marvin Häuser
0 siblings, 1 reply; 21+ messages in thread
From: Ard Biesheuvel @ 2023-02-13 21:59 UTC (permalink / raw)
To: Marvin Häuser; +Cc: devel
On Mon, 13 Feb 2023 at 22:16, Marvin Häuser <mhaeuser@posteo.de> wrote:
>
> Hey Ard,
>
> *Praise* to you for this series. Comments inline.
>
Thanks :-)
> On Mon, Feb 13, 2023 at 07:19 AM, Ard Biesheuvel wrote:
>
> Enable the WXN system control bit straight out of reset when running in
> EL1 with the initial ID map from flash. This setting will be inherited
> by the page table code after it sets up the permanent boot time page
> tables, resulting in all memory mappings that are not explicitly mapped
> as read-only to be non-executable.
>
> Note that this requires runtime drivers to be built with position
> independent codegen, to ensure that all absolute symbol references are
> moved into a separate section in the binary. Otherwise, unmapping the
> pages that are subject to relocation fixups at runtime (during the
> invocation of SetVirtualAddressMap()) could result in code mappings
> losing their executable permissions.
>
> I never actually thought about this. SetVirtualAddressMap() will have to relocate its own parent binary, causing issues for software W^X when .text relocs are present (like with MSVC builds). :(
>
>
> Signed-off-by: Ard Biesheuvel <ardb@...>
> ---
> ArmVirtPkg/ArmVirt.dsc.inc | 1 +
> ArmVirtPkg/Library/ArmPlatformLibQemu/AArch64/ArmPlatformHelper.S | 2 +-
> 2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
> index 5b18184be263..928dd6330edb 100644
> --- a/ArmVirtPkg/ArmVirt.dsc.inc
> +++ b/ArmVirtPkg/ArmVirt.dsc.inc
> @@ -31,6 +31,7 @@ [BuildOptions.common.EDKII.DXE_CORE,BuildOptions.common.E=
> DKII.DXE_DRIVER,BuildOp
> =0D
> [BuildOptions.common.EDKII.DXE_RUNTIME_DRIVER]=0D
> GCC:*_*_ARM_DLINK_FLAGS =3D -z common-page-size=3D0x1000=0D
> + GCC:*_*_AARCH64_CC_FLAGS =3D -fpie=0D
>
> Doesn't this mean -pie must be passed to the linker? I saw in the previous patch that .plt was added to the linker script, was there a particular reason -fno-plt wasn't used here? I just read it may have some unexpected side-effects, but I thought it would be safe for our statically-linked UEFI environment.
>
No, the only reason for adding -fpie here is to ensure that statically
initialized CONST pointers are emitted into .data.rel.ro and not into
.rodata, as this is under the control of the compiler. Although,
thinking about this, I wonder if we need to pass this to the linker
for codegen under LTO as well. But the PIE link itself should be
unnecessary here.
> On another (related) matter, I've been spending my last two days looking into the whole ELF-to-PE process, because GenFw has been becoming unbearable to us downstream. I went through a bunch of old commits which deal with PIE and saw it was usually disabled but for X64. The funny thing with X64 (even currently) is, that -fpie is combined with -q (a.k.a. --emit-relocs), yielding both object file relocs (.rela.sectname) and PIE-related relative relocs (.rela) in the same binary (as documented in GenFw, they may overlap!). It's my understanding that GenFw currently processes exclusively the -q relocs and not the -fpie relocs (which should be safe as done for X64, I have no experience with ARM whatsoever). However, when PIE is involved anyway, it makes most sense to me to use its related relocs for the translation over a dance with the object file relocs. This change will cause the same behaviour for AARCH64 RT drivers now, right?
>
It will if you pass -pie to the linker, which is why I would prefer to
avoid that. The main issue IIRC is that the emit-relocs section does
not cover the entries in the GOT table that also require relocation,
and are only covered by the PIE .rela section. For AArch64, I added
relaxation logic to GenFw to actually patch the instructions instead,
which is always possible given the absence of dynamic linking.
(d2687f23c909475d80cef32cdf9a5d121f0a9ae6,
7b8f69d7e10628d473dd225224d8c2122d25a38d)
This means that we don't have to care about compiler generated symbol
references, and so the relocs emitted by emit-relocs are sufficient,
and the additional ones emitted into .rela are unused anyway. The only
remaining absolute references are the ones resulting from statically
initialized globals, and those will either be in .data or in
.data.rel.ro (if -fpie is being used)
But I agree that not using --emit-relocs and only relying on the .rela
section to populate the PE/COFF reloc section would be far cleaner.
> In an ideal world, I suppose all architectures but IA32 (due to lacking efficient pcrel addressing) should be using PIE, as most (often all with X64) GOT references can be relaxed, as we strictly deal with local symbols. Though I have to wonder how unideal the world really is. :)
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [edk2-devel] [RFC 13/13] ArmVirtPkg/ArmVirtQemu: Enable hardware enforced W^X memory permissions
2023-02-13 21:59 ` Ard Biesheuvel
@ 2023-02-13 22:23 ` Marvin Häuser
2023-02-13 22:37 ` Ard Biesheuvel
0 siblings, 1 reply; 21+ messages in thread
From: Marvin Häuser @ 2023-02-13 22:23 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel-groups-io
[-- Attachment #1: Type: text/plain, Size: 2188 bytes --]
> On 13. Feb 2023, at 22:59, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> No, the only reason for adding -fpie here is to ensure that statically
> initialized CONST pointers are emitted into .data.rel.ro and not into
> .rodata, as this is under the control of the compiler. Although,
> thinking about this, I wonder if we need to pass this to the linker
> for codegen under LTO as well. But the PIE link itself should be
> unnecessary here.
Oh, what fun. For some reason I thought it would be unsafe to specify -fpie but not -pie, but considering PIE relocs are ignored either way, this actually makes perfect sense. Sorry! About that last part, the docs say: "It is recommended that you compile all the files participating in the same link with the same options and also specify those options at link time." [1], so good catch!
[1] https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html#Optimize-Options
But what about -fno-plt?
>
> It will if you pass -pie to the linker, which is why I would prefer to
> avoid that. The main issue IIRC is that the emit-relocs section does
> not cover the entries in the GOT table that also require relocation,
> and are only covered by the PIE .rela section. For AArch64, I added
> relaxation logic to GenFw to actually patch the instructions instead,
> which is always possible given the absence of dynamic linking.
> (d2687f23c909475d80cef32cdf9a5d121f0a9ae6,
> 7b8f69d7e10628d473dd225224d8c2122d25a38d)
Yes, seen, very nice. I do wonder though why GOT entries are generated in the first place when symbols are all local and data is within the PC-addressable range. Just today, for a X64 build, I actually saw Clang relax a GOT reference to __stack_chk_guard itself.
>
> This means that we don't have to care about compiler generated symbol
> references, and so the relocs emitted by emit-relocs are sufficient,
> and the additional ones emitted into .rela are unused anyway. The only
> remaining absolute references are the ones resulting from statically
> initialized globals, and those will either be in .data or in
> .data.rel.ro (if -fpie is being used)
Right. thank you.
Best regards,
Marvin
[-- Attachment #2: Type: text/html, Size: 2832 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [edk2-devel] [RFC 13/13] ArmVirtPkg/ArmVirtQemu: Enable hardware enforced W^X memory permissions
2023-02-13 22:23 ` Marvin Häuser
@ 2023-02-13 22:37 ` Ard Biesheuvel
0 siblings, 0 replies; 21+ messages in thread
From: Ard Biesheuvel @ 2023-02-13 22:37 UTC (permalink / raw)
To: devel, mhaeuser
On Mon, 13 Feb 2023 at 23:23, Marvin Häuser <mhaeuser@posteo.de> wrote:
>
>
> On 13. Feb 2023, at 22:59, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> No, the only reason for adding -fpie here is to ensure that statically
> initialized CONST pointers are emitted into .data.rel.ro and not into
> .rodata, as this is under the control of the compiler. Although,
> thinking about this, I wonder if we need to pass this to the linker
> for codegen under LTO as well. But the PIE link itself should be
> unnecessary here.
>
>
> Oh, what fun. For some reason I thought it would be unsafe to specify -fpie but not -pie, but considering PIE relocs are ignored either way, this actually makes perfect sense. Sorry! About that last part, the docs say: "It is recommended that you compile all the files participating in the same link with the same options and also specify those options at link time." [1], so good catch!
>
> [1] https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html#Optimize-Options
>
Yeah, but come to think of it, we pass the CC flags to the linker, so
this should be covered.
> But what about -fno-plt?
>
That doesn't make a difference in codegen on AArch64 - the same BL
instruction is emitted along with the same type of relocation, and it
is left up to the linker whether or not a PLT entry is emitted, and I
don't think it will ever do that when generating a fully linked binary
without -pie.
>
> It will if you pass -pie to the linker, which is why I would prefer to
> avoid that. The main issue IIRC is that the emit-relocs section does
> not cover the entries in the GOT table that also require relocation,
> and are only covered by the PIE .rela section. For AArch64, I added
> relaxation logic to GenFw to actually patch the instructions instead,
> which is always possible given the absence of dynamic linking.
> (d2687f23c909475d80cef32cdf9a5d121f0a9ae6,
> 7b8f69d7e10628d473dd225224d8c2122d25a38d)
>
>
> Yes, seen, very nice. I do wonder though why GOT entries are generated in the first place when symbols are all local and data is within the PC-addressable range. Just today, for a X64 build, I actually saw Clang relax a GOT reference to __stack_chk_guard itself.
>
Yeah there are some strange corner cases, but in general, all symbol
references are relative - it is precisely the 'special' symbol
references that the compiler generates internally that sometimes get
this wrong.
>
> This means that we don't have to care about compiler generated symbol
> references, and so the relocs emitted by emit-relocs are sufficient,
> and the additional ones emitted into .rela are unused anyway. The only
> remaining absolute references are the ones resulting from statically
> initialized globals, and those will either be in .data or in
> .data.rel.ro (if -fpie is being used)
>
>
> Right. thank you.
>
> Best regards,
> Marvin
>
^ permalink raw reply [flat|nested] 21+ messages in thread