* [PATCH v4 00/11] ArmPkg: implement EFI memory attributes protocol
@ 2023-02-09 13:59 Ard Biesheuvel
2023-02-09 13:59 ` [PATCH v4 01/11] ArmPkg/ArmMmuLib ARM: Remove half baked large page support Ard Biesheuvel
` (11 more replies)
0 siblings, 12 replies; 18+ messages in thread
From: Ard Biesheuvel @ 2023-02-09 13:59 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
v4:
- major cleanup of the 32-bit ARM code
- add support for EFI_MEMORY_RP using the access flag
- enable stack guard in ArmVirtPkg (which uses EFI_MEMORY_RP)
- incorporate optimization from other series [0] to avoid splitting
block entries unnecessarily
v3:
- fix ARM32 bug in attribute conversion
- add Liming's ack to patch #1
- include draft patch (NOT FOR MERGE) used to test the changes
v2:
- drop patch to bump exposed UEFI revision to v2.10
- add missing permitted return values to protocol definition
[0] https://edk2.groups.io/g/devel/message/99801
Cc: Michael Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Michael Kubacki <michael.kubacki@microsoft.com>
Cc: Sean Brogan <sean.brogan@microsoft.com>
Cc: Rebecca Cran <quic_rcran@quicinc.com>
Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Cc: Taylor Beebe <t@taylorbeebe.com>
Ard Biesheuvel (11):
ArmPkg/ArmMmuLib ARM: Remove half baked large page support
ArmPkg/ArmMmuLib ARM: Split off XN page descriptor bit from type field
ArmPkg/CpuDxe ARM: Fix page-to-section attribute conversion
ArmPkg/ArmMmuLib ARM: Isolate the access flag from AP mask
ArmPkg/ArmMmuLib ARM: Clear individual permission bits
ArmPkg/ArmMmuLib: Implement EFI_MEMORY_RP using access flag
ArmVirtPkg: Enable stack guard
ArmPkg/ArmMmuLib: Avoid splitting block entries if possible
ArmPkg/CpuDxe: Expose unified region-to-EFI attribute conversion
MdePkg: Add Memory Attribute Protocol definition
ArmPkg/CpuDxe: Implement EFI memory attributes protocol
ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c | 25 +-
ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 96 +++++--
ArmPkg/Drivers/CpuDxe/CpuDxe.c | 2 +
ArmPkg/Drivers/CpuDxe/CpuDxe.h | 17 ++
ArmPkg/Drivers/CpuDxe/CpuDxe.inf | 2 +
ArmPkg/Drivers/CpuDxe/MemoryAttribute.c | 271 ++++++++++++++++++++
ArmPkg/Include/Chipset/ArmV7Mmu.h | 88 +++----
ArmPkg/Include/Library/ArmMmuLib.h | 34 +++
ArmPkg/Library/ArmLib/Arm/ArmV7Support.S | 2 +
ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 68 ++++-
ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibConvert.c | 8 +-
ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c | 2 +-
ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c | 173 +++++++++++--
ArmVirtPkg/ArmVirt.dsc.inc | 2 +
MdePkg/Include/Protocol/MemoryAttribute.h | 142 ++++++++++
MdePkg/MdePkg.dec | 3 +
16 files changed, 833 insertions(+), 102 deletions(-)
create mode 100644 ArmPkg/Drivers/CpuDxe/MemoryAttribute.c
create mode 100644 MdePkg/Include/Protocol/MemoryAttribute.h
--
2.39.1
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v4 01/11] ArmPkg/ArmMmuLib ARM: Remove half baked large page support
2023-02-09 13:59 [PATCH v4 00/11] ArmPkg: implement EFI memory attributes protocol Ard Biesheuvel
@ 2023-02-09 13:59 ` Ard Biesheuvel
2023-02-09 13:59 ` [PATCH v4 02/11] ArmPkg/ArmMmuLib ARM: Split off XN page descriptor bit from type field Ard Biesheuvel
` (10 subsequent siblings)
11 siblings, 0 replies; 18+ messages in thread
From: Ard Biesheuvel @ 2023-02-09 13:59 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
Large page support on 32-bit ARM is essentially a glorified contiguous
bit where 16 consecutive entries describing a contiguous range with the
same attributes are presented in a way that permits the TLB to cache its
translation with a single entry.
This was never wired up completely, and does not add a lot of value in
EFI, where the page granularity is 4k and we expect to be able to set RO
and XP permissions on individual pages.
Given that large page support complicates the handling of the XN bit at
the page level (which is in a different place depending on whether the
page is small or large), let's just rip it out.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 8 ++---
ArmPkg/Include/Chipset/ArmV7Mmu.h | 38 ++++++--------------
ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibConvert.c | 7 ++--
ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c | 2 +-
ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c | 2 +-
5 files changed, 19 insertions(+), 38 deletions(-)
diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
index 2daf47ba6fe5..ea856f5cdd26 100644
--- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
+++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
@@ -165,7 +165,7 @@ SyncCacheConfigPage (
// Convert SectionAttributes into PageAttributes
NextPageAttributes =
- TT_DESCRIPTOR_CONVERT_TO_PAGE_CACHE_POLICY (*NextSectionAttributes, 0) |
+ TT_DESCRIPTOR_CONVERT_TO_PAGE_CACHE_POLICY (*NextSectionAttributes) |
TT_DESCRIPTOR_CONVERT_TO_PAGE_AP (*NextSectionAttributes);
// obtain page table base
@@ -212,7 +212,7 @@ SyncCacheConfigPage (
// Convert back PageAttributes into SectionAttributes
*NextSectionAttributes =
- TT_DESCRIPTOR_CONVERT_TO_SECTION_CACHE_POLICY (NextPageAttributes, 0) |
+ TT_DESCRIPTOR_CONVERT_TO_SECTION_CACHE_POLICY (NextPageAttributes) |
TT_DESCRIPTOR_CONVERT_TO_SECTION_AP (NextPageAttributes);
return EFI_SUCCESS;
@@ -399,7 +399,7 @@ GetMemoryRegionPage (
UINT32 PageDescriptor;
// Convert the section attributes into page attributes
- PageAttributes = ConvertSectionAttributesToPageAttributes (*RegionAttributes, 0);
+ PageAttributes = ConvertSectionAttributesToPageAttributes (*RegionAttributes);
// Calculate index into first level translation table for start of modification
TableIndex = ((*BaseAddress) & TT_DESCRIPTOR_PAGE_INDEX_MASK) >> TT_DESCRIPTOR_PAGE_BASE_SHIFT;
@@ -479,7 +479,7 @@ GetMemoryRegion (
ASSERT (PageTableIndex < TRANSLATION_TABLE_PAGE_COUNT);
PageAttributes = PageTable[PageTableIndex] & TT_DESCRIPTOR_PAGE_ATTRIBUTE_MASK;
- *RegionAttributes = TT_DESCRIPTOR_CONVERT_TO_SECTION_CACHE_POLICY (PageAttributes, 0) |
+ *RegionAttributes = TT_DESCRIPTOR_CONVERT_TO_SECTION_CACHE_POLICY (PageAttributes) |
TT_DESCRIPTOR_CONVERT_TO_SECTION_AP (PageAttributes);
}
diff --git a/ArmPkg/Include/Chipset/ArmV7Mmu.h b/ArmPkg/Include/Chipset/ArmV7Mmu.h
index db99527d6efa..7501ebfdf97f 100644
--- a/ArmPkg/Include/Chipset/ArmV7Mmu.h
+++ b/ArmPkg/Include/Chipset/ArmV7Mmu.h
@@ -98,9 +98,8 @@
#define TT_DESCRIPTOR_PAGE_AP_RO_NO ((1UL << 9) | (1UL << 4))
#define TT_DESCRIPTOR_PAGE_AP_RO_RO ((1UL << 9) | (3UL << 4))
-#define TT_DESCRIPTOR_SECTION_XN_MASK (0x1UL << 4)
-#define TT_DESCRIPTOR_PAGE_XN_MASK (0x1UL << 0)
-#define TT_DESCRIPTOR_LARGEPAGE_XN_MASK (0x1UL << 15)
+#define TT_DESCRIPTOR_SECTION_XN_MASK (0x1UL << 4)
+#define TT_DESCRIPTOR_PAGE_XN_MASK (0x1UL << 0)
#define TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK ((3UL << 12) | (1UL << 3) | (1UL << 2))
#define TT_DESCRIPTOR_SECTION_CACHEABLE_MASK (1UL << 3)
@@ -124,30 +123,14 @@
#define TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_BACK_ALLOC ((1UL << 6) | (1UL << 3) | (1UL << 2))
#define TT_DESCRIPTOR_PAGE_CACHE_POLICY_NON_SHAREABLE_DEVICE ((2UL << 6) | (0UL << 3) | (0UL << 2))
-#define TT_DESCRIPTOR_LARGEPAGE_CACHE_POLICY_MASK ((3UL << 12) | (1UL << 3) | (1UL << 2))
-#define TT_DESCRIPTOR_LARGEPAGE_CACHE_POLICY_STRONGLY_ORDERED ((0UL << 12) | (0UL << 3) | (0UL << 2))
-#define TT_DESCRIPTOR_LARGEPAGE_CACHE_POLICY_SHAREABLE_DEVICE ((0UL << 12) | (0UL << 3) | (1UL << 2))
-#define TT_DESCRIPTOR_LARGEPAGE_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC ((0UL << 12) | (1UL << 3) | (0UL << 2))
-#define TT_DESCRIPTOR_LARGEPAGE_CACHE_POLICY_WRITE_BACK_NO_ALLOC ((0UL << 12) | (1UL << 3) | (1UL << 2))
-#define TT_DESCRIPTOR_LARGEPAGE_CACHE_POLICY_NON_CACHEABLE ((1UL << 12) | (0UL << 3) | (0UL << 2))
-#define TT_DESCRIPTOR_LARGEPAGE_CACHE_POLICY_WRITE_BACK_ALLOC ((1UL << 12) | (1UL << 3) | (1UL << 2))
-#define TT_DESCRIPTOR_LARGEPAGE_CACHE_POLICY_NON_SHAREABLE_DEVICE ((2UL << 12) | (0UL << 3) | (0UL << 2))
+#define TT_DESCRIPTOR_CONVERT_TO_PAGE_AP(Desc) ((((Desc) & TT_DESCRIPTOR_SECTION_AP_MASK) >> 6) & TT_DESCRIPTOR_PAGE_AP_MASK)
+#define TT_DESCRIPTOR_CONVERT_TO_PAGE_NG(Desc) ((((Desc) & TT_DESCRIPTOR_SECTION_NG_MASK) >> 6) & TT_DESCRIPTOR_PAGE_NG_MASK)
+#define TT_DESCRIPTOR_CONVERT_TO_PAGE_S(Desc) ((((Desc) & TT_DESCRIPTOR_SECTION_S_MASK) >> 6) & TT_DESCRIPTOR_PAGE_S_MASK)
+#define TT_DESCRIPTOR_CONVERT_TO_PAGE_XN(Desc) ((((Desc) & TT_DESCRIPTOR_SECTION_XN_MASK) >> 4) & TT_DESCRIPTOR_PAGE_XN_MASK)
+#define TT_DESCRIPTOR_CONVERT_TO_PAGE_CACHE_POLICY(Desc) ((((Desc) & (0x3 << 12)) >> 6) | (Desc & (0x3 << 2)))
-#define TT_DESCRIPTOR_CONVERT_TO_PAGE_AP(Desc) ((((Desc) & TT_DESCRIPTOR_SECTION_AP_MASK) >> 6) & TT_DESCRIPTOR_PAGE_AP_MASK)
-#define TT_DESCRIPTOR_CONVERT_TO_PAGE_NG(Desc) ((((Desc) & TT_DESCRIPTOR_SECTION_NG_MASK) >> 6) & TT_DESCRIPTOR_PAGE_NG_MASK)
-#define TT_DESCRIPTOR_CONVERT_TO_PAGE_S(Desc) ((((Desc) & TT_DESCRIPTOR_SECTION_S_MASK) >> 6) & TT_DESCRIPTOR_PAGE_S_MASK)
-#define TT_DESCRIPTOR_CONVERT_TO_PAGE_XN(Desc, IsLargePage) ((IsLargePage)?\
- ((((Desc) & TT_DESCRIPTOR_SECTION_XN_MASK) << 11) & TT_DESCRIPTOR_LARGEPAGE_XN_MASK): \
- ((((Desc) & TT_DESCRIPTOR_SECTION_XN_MASK) >> 4) & TT_DESCRIPTOR_PAGE_XN_MASK))
-#define TT_DESCRIPTOR_CONVERT_TO_PAGE_CACHE_POLICY(Desc, IsLargePage) (IsLargePage? \
- (((Desc) & TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK) & TT_DESCRIPTOR_LARGEPAGE_CACHE_POLICY_MASK): \
- (((((Desc) & (0x3 << 12)) >> 6) | (Desc & (0x3 << 2)))))
-
-#define TT_DESCRIPTOR_CONVERT_TO_SECTION_AP(Desc) ((((Desc) & TT_DESCRIPTOR_PAGE_AP_MASK) << 6) & TT_DESCRIPTOR_SECTION_AP_MASK)
-
-#define TT_DESCRIPTOR_CONVERT_TO_SECTION_CACHE_POLICY(Desc, IsLargePage) (IsLargePage? \
- (((Desc) & TT_DESCRIPTOR_LARGEPAGE_CACHE_POLICY_MASK) & TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK): \
- (((((Desc) & (0x3 << 6)) << 6) | (Desc & (0x3 << 2)))))
+#define TT_DESCRIPTOR_CONVERT_TO_SECTION_AP(Desc) ((((Desc) & TT_DESCRIPTOR_PAGE_AP_MASK) << 6) & TT_DESCRIPTOR_SECTION_AP_MASK)
+#define TT_DESCRIPTOR_CONVERT_TO_SECTION_CACHE_POLICY(Desc) ((((Desc) & (0x3 << 6)) << 6) | (Desc & (0x3 << 2)))
#define TT_DESCRIPTOR_SECTION_ATTRIBUTE_MASK (TT_DESCRIPTOR_SECTION_NS_MASK | TT_DESCRIPTOR_SECTION_NG_MASK | \
TT_DESCRIPTOR_SECTION_S_MASK | TT_DESCRIPTOR_SECTION_AP_MASK | \
@@ -230,8 +213,7 @@ typedef UINT32 ARM_PAGE_TABLE_ENTRY;
UINT32
ConvertSectionAttributesToPageAttributes (
- IN UINT32 SectionAttributes,
- IN BOOLEAN IsLargePage
+ IN UINT32 SectionAttributes
);
#endif // ARMV7_MMU_H_
diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibConvert.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibConvert.c
index bee8ad7028d3..6e2f08a7ce15 100644
--- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibConvert.c
+++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibConvert.c
@@ -15,16 +15,15 @@
UINT32
ConvertSectionAttributesToPageAttributes (
- IN UINT32 SectionAttributes,
- IN BOOLEAN IsLargePage
+ IN UINT32 SectionAttributes
)
{
UINT32 PageAttributes;
PageAttributes = 0;
- PageAttributes |= TT_DESCRIPTOR_CONVERT_TO_PAGE_CACHE_POLICY (SectionAttributes, IsLargePage);
+ PageAttributes |= TT_DESCRIPTOR_CONVERT_TO_PAGE_CACHE_POLICY (SectionAttributes);
PageAttributes |= TT_DESCRIPTOR_CONVERT_TO_PAGE_AP (SectionAttributes);
- PageAttributes |= TT_DESCRIPTOR_CONVERT_TO_PAGE_XN (SectionAttributes, IsLargePage);
+ PageAttributes |= TT_DESCRIPTOR_CONVERT_TO_PAGE_XN (SectionAttributes);
PageAttributes |= TT_DESCRIPTOR_CONVERT_TO_PAGE_NG (SectionAttributes);
PageAttributes |= TT_DESCRIPTOR_CONVERT_TO_PAGE_S (SectionAttributes);
diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
index 9e304ea05e63..28cc9b2fe058 100644
--- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
@@ -145,7 +145,7 @@ PopulateLevel2PageTable (
);
// Translate the Section Descriptor into Page Descriptor
- SectionDescriptor = TT_DESCRIPTOR_PAGE_TYPE_PAGE | ConvertSectionAttributesToPageAttributes (*SectionEntry, FALSE);
+ SectionDescriptor = TT_DESCRIPTOR_PAGE_TYPE_PAGE | ConvertSectionAttributesToPageAttributes (*SectionEntry);
BaseSectionAddress = TT_DESCRIPTOR_SECTION_BASE_ADDRESS (*SectionEntry);
diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c
index b402197ade99..9ca00c976d5f 100644
--- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c
+++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c
@@ -53,7 +53,7 @@ ConvertSectionToPages (
// Get section attributes and convert to page attributes
SectionDescriptor = FirstLevelTable[FirstLevelIdx];
- PageDescriptor = TT_DESCRIPTOR_PAGE_TYPE_PAGE | ConvertSectionAttributesToPageAttributes (SectionDescriptor, FALSE);
+ PageDescriptor = TT_DESCRIPTOR_PAGE_TYPE_PAGE | ConvertSectionAttributesToPageAttributes (SectionDescriptor);
// Allocate a page table for the 4KB entries (we use up a full page even though we only need 1KB)
PageTable = (volatile ARM_PAGE_TABLE_ENTRY *)AllocatePages (1);
--
2.39.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v4 02/11] ArmPkg/ArmMmuLib ARM: Split off XN page descriptor bit from type field
2023-02-09 13:59 [PATCH v4 00/11] ArmPkg: implement EFI memory attributes protocol Ard Biesheuvel
2023-02-09 13:59 ` [PATCH v4 01/11] ArmPkg/ArmMmuLib ARM: Remove half baked large page support Ard Biesheuvel
@ 2023-02-09 13:59 ` Ard Biesheuvel
2023-02-09 13:59 ` [PATCH v4 03/11] ArmPkg/CpuDxe ARM: Fix page-to-section attribute conversion Ard Biesheuvel
` (9 subsequent siblings)
11 siblings, 0 replies; 18+ messages in thread
From: Ard Biesheuvel @ 2023-02-09 13:59 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
With large page support out of the picture, we can treat bits 1 and 0 of
the page descriptor as individual valid and XN bits, instead of treating
XN as a page type. Doing so aligns the handling of the attribute with
the section descriptor layout, as well as the XN handling on AArch64,
and this is beneficial for maintainability.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
ArmPkg/Include/Chipset/ArmV7Mmu.h | 8 +++-----
ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c | 12 ++++++------
2 files changed, 9 insertions(+), 11 deletions(-)
diff --git a/ArmPkg/Include/Chipset/ArmV7Mmu.h b/ArmPkg/Include/Chipset/ArmV7Mmu.h
index 7501ebfdf97f..6a2584ceb303 100644
--- a/ArmPkg/Include/Chipset/ArmV7Mmu.h
+++ b/ArmPkg/Include/Chipset/ArmV7Mmu.h
@@ -54,11 +54,9 @@
#define TT_DESCRIPTOR_SECTION_TYPE_IS_PAGE_TABLE(Desc) (((Desc) & 3UL) == TT_DESCRIPTOR_SECTION_TYPE_PAGE_TABLE)
// Translation table descriptor types
-#define TT_DESCRIPTOR_PAGE_TYPE_MASK (3UL << 0)
-#define TT_DESCRIPTOR_PAGE_TYPE_FAULT (0UL << 0)
-#define TT_DESCRIPTOR_PAGE_TYPE_PAGE (2UL << 0)
-#define TT_DESCRIPTOR_PAGE_TYPE_PAGE_XN (3UL << 0)
-#define TT_DESCRIPTOR_PAGE_TYPE_LARGEPAGE (1UL << 0)
+#define TT_DESCRIPTOR_PAGE_TYPE_MASK (1UL << 1)
+#define TT_DESCRIPTOR_PAGE_TYPE_FAULT (0UL << 1)
+#define TT_DESCRIPTOR_PAGE_TYPE_PAGE (1UL << 1)
// Section descriptor definitions
#define TT_DESCRIPTOR_SECTION_SIZE (0x00100000)
diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c
index 9ca00c976d5f..12d0f4c30f8e 100644
--- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c
+++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c
@@ -104,12 +104,8 @@ UpdatePageEntries (
// EntryMask: bitmask of values to change (1 = change this value, 0 = leave alone)
// EntryValue: values at bit positions specified by EntryMask
- EntryMask = TT_DESCRIPTOR_PAGE_TYPE_MASK | TT_DESCRIPTOR_PAGE_AP_MASK;
- if ((Attributes & EFI_MEMORY_XP) != 0) {
- EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE_XN;
- } else {
- EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE;
- }
+ EntryMask = TT_DESCRIPTOR_PAGE_TYPE_MASK | TT_DESCRIPTOR_PAGE_AP_MASK | TT_DESCRIPTOR_PAGE_XN_MASK;
+ EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE;
// Although the PI spec is unclear on this, the GCD guarantees that only
// one Attribute bit is set at a time, so the order of the conditionals below
@@ -148,6 +144,10 @@ UpdatePageEntries (
EntryValue |= TT_DESCRIPTOR_PAGE_AP_RW_RW;
}
+ if ((Attributes & EFI_MEMORY_XP) != 0) {
+ EntryValue |= TT_DESCRIPTOR_PAGE_XN_MASK;
+ }
+
// Obtain page table base
FirstLevelTable = (ARM_FIRST_LEVEL_DESCRIPTOR *)ArmGetTTBR0BaseAddress ();
--
2.39.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v4 03/11] ArmPkg/CpuDxe ARM: Fix page-to-section attribute conversion
2023-02-09 13:59 [PATCH v4 00/11] ArmPkg: implement EFI memory attributes protocol Ard Biesheuvel
2023-02-09 13:59 ` [PATCH v4 01/11] ArmPkg/ArmMmuLib ARM: Remove half baked large page support Ard Biesheuvel
2023-02-09 13:59 ` [PATCH v4 02/11] ArmPkg/ArmMmuLib ARM: Split off XN page descriptor bit from type field Ard Biesheuvel
@ 2023-02-09 13:59 ` Ard Biesheuvel
2023-02-09 13:59 ` [PATCH v4 04/11] ArmPkg/ArmMmuLib ARM: Isolate the access flag from AP mask Ard Biesheuvel
` (8 subsequent siblings)
11 siblings, 0 replies; 18+ messages in thread
From: Ard Biesheuvel @ 2023-02-09 13:59 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
The section-to-page attribute conversion takes the shareability and
execute-never attributes into account, whereas the page-to-section
counterpart does not. The result is that GetMemoryRegionPage () -which
takes a section attribute argument (via *RegionAttributes) that is
ostensibly based on the first page in the range, but differs from the
actual page attributes when converted back- may return with a
RegionLength of zero. This is incorrect, and confuses code that scans a
region by calling GetMemoryRegion () in sequence.
So fix the conversion, and ASSERT () on a non-zero region length.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 3 +++
ArmPkg/Include/Chipset/ArmV7Mmu.h | 2 ++
2 files changed, 5 insertions(+)
diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
index ea856f5cdd26..8eb1f71395f5 100644
--- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
+++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
@@ -480,6 +480,8 @@ GetMemoryRegion (
PageAttributes = PageTable[PageTableIndex] & TT_DESCRIPTOR_PAGE_ATTRIBUTE_MASK;
*RegionAttributes = TT_DESCRIPTOR_CONVERT_TO_SECTION_CACHE_POLICY (PageAttributes) |
+ TT_DESCRIPTOR_CONVERT_TO_SECTION_S (PageAttributes) |
+ TT_DESCRIPTOR_CONVERT_TO_SECTION_XN (PageAttributes) |
TT_DESCRIPTOR_CONVERT_TO_SECTION_AP (PageAttributes);
}
@@ -494,6 +496,7 @@ GetMemoryRegion (
// Scan the page table to find the end of the region.
Status = GetMemoryRegionPage (PageTable, BaseAddress, RegionLength, RegionAttributes);
+ ASSERT (*RegionLength > 0);
// If we have found the end of the region (Status == EFI_SUCCESS) then we exit the for-loop
if (Status == EFI_SUCCESS) {
diff --git a/ArmPkg/Include/Chipset/ArmV7Mmu.h b/ArmPkg/Include/Chipset/ArmV7Mmu.h
index 6a2584ceb303..e0219747df86 100644
--- a/ArmPkg/Include/Chipset/ArmV7Mmu.h
+++ b/ArmPkg/Include/Chipset/ArmV7Mmu.h
@@ -128,6 +128,8 @@
#define TT_DESCRIPTOR_CONVERT_TO_PAGE_CACHE_POLICY(Desc) ((((Desc) & (0x3 << 12)) >> 6) | (Desc & (0x3 << 2)))
#define TT_DESCRIPTOR_CONVERT_TO_SECTION_AP(Desc) ((((Desc) & TT_DESCRIPTOR_PAGE_AP_MASK) << 6) & TT_DESCRIPTOR_SECTION_AP_MASK)
+#define TT_DESCRIPTOR_CONVERT_TO_SECTION_S(Desc) ((((Desc) & TT_DESCRIPTOR_PAGE_S_MASK) << 6) & TT_DESCRIPTOR_SECTION_S_MASK)
+#define TT_DESCRIPTOR_CONVERT_TO_SECTION_XN(Desc) ((((Desc) & TT_DESCRIPTOR_PAGE_XN_MASK) << 4) & TT_DESCRIPTOR_SECTION_XN_MASK)
#define TT_DESCRIPTOR_CONVERT_TO_SECTION_CACHE_POLICY(Desc) ((((Desc) & (0x3 << 6)) << 6) | (Desc & (0x3 << 2)))
#define TT_DESCRIPTOR_SECTION_ATTRIBUTE_MASK (TT_DESCRIPTOR_SECTION_NS_MASK | TT_DESCRIPTOR_SECTION_NG_MASK | \
--
2.39.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v4 04/11] ArmPkg/ArmMmuLib ARM: Isolate the access flag from AP mask
2023-02-09 13:59 [PATCH v4 00/11] ArmPkg: implement EFI memory attributes protocol Ard Biesheuvel
` (2 preceding siblings ...)
2023-02-09 13:59 ` [PATCH v4 03/11] ArmPkg/CpuDxe ARM: Fix page-to-section attribute conversion Ard Biesheuvel
@ 2023-02-09 13:59 ` Ard Biesheuvel
2023-02-09 13:59 ` [PATCH v4 05/11] ArmPkg/ArmMmuLib ARM: Clear individual permission bits Ard Biesheuvel
` (7 subsequent siblings)
11 siblings, 0 replies; 18+ messages in thread
From: Ard Biesheuvel @ 2023-02-09 13:59 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
Split the ARM permission fields in the short descriptors into an access
flag and AP[2:1] as per the recommendation in the ARM ARM. This makes
the access flag available separately, which allows us to implement
EFI_MEMORY_RP memory analogous to how it will be implemented for
AArch64.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 47 ++++++++++----------
ArmPkg/Include/Chipset/ArmV7Mmu.h | 40 +++++++++++------
ArmPkg/Library/ArmLib/Arm/ArmV7Support.S | 2 +
ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibConvert.c | 1 +
ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c | 12 ++++-
5 files changed, 63 insertions(+), 39 deletions(-)
diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
index 8eb1f71395f5..07faab8216ec 100644
--- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
+++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
@@ -50,30 +50,27 @@ SectionToGcdAttributes (
// determine protection attributes
switch (SectionAttributes & TT_DESCRIPTOR_SECTION_AP_MASK) {
- case TT_DESCRIPTOR_SECTION_AP_NO_NO: // no read, no write
- // *GcdAttributes |= EFI_MEMORY_RO | EFI_MEMORY_RP;
- break;
-
- case TT_DESCRIPTOR_SECTION_AP_RW_NO:
+ case TT_DESCRIPTOR_SECTION_AP_NO_RW:
case TT_DESCRIPTOR_SECTION_AP_RW_RW:
// normal read/write access, do not add additional attributes
break;
// read only cases map to write-protect
- case TT_DESCRIPTOR_SECTION_AP_RO_NO:
+ case TT_DESCRIPTOR_SECTION_AP_NO_RO:
case TT_DESCRIPTOR_SECTION_AP_RO_RO:
*GcdAttributes |= EFI_MEMORY_RO;
break;
-
- default:
- return EFI_UNSUPPORTED;
}
// now process eXectue Never attribute
- if ((SectionAttributes & TT_DESCRIPTOR_SECTION_XN_MASK) != 0 ) {
+ if ((SectionAttributes & TT_DESCRIPTOR_SECTION_XN_MASK) != 0) {
*GcdAttributes |= EFI_MEMORY_XP;
}
+ if ((SectionAttributes & TT_DESCRIPTOR_SECTION_AF) == 0) {
+ *GcdAttributes |= EFI_MEMORY_RP;
+ }
+
return EFI_SUCCESS;
}
@@ -114,30 +111,27 @@ PageToGcdAttributes (
// determine protection attributes
switch (PageAttributes & TT_DESCRIPTOR_PAGE_AP_MASK) {
- case TT_DESCRIPTOR_PAGE_AP_NO_NO: // no read, no write
- // *GcdAttributes |= EFI_MEMORY_RO | EFI_MEMORY_RP;
- break;
-
- case TT_DESCRIPTOR_PAGE_AP_RW_NO:
+ case TT_DESCRIPTOR_PAGE_AP_NO_RW:
case TT_DESCRIPTOR_PAGE_AP_RW_RW:
// normal read/write access, do not add additional attributes
break;
// read only cases map to write-protect
- case TT_DESCRIPTOR_PAGE_AP_RO_NO:
+ case TT_DESCRIPTOR_PAGE_AP_NO_RO:
case TT_DESCRIPTOR_PAGE_AP_RO_RO:
*GcdAttributes |= EFI_MEMORY_RO;
break;
-
- default:
- return EFI_UNSUPPORTED;
}
// now process eXectue Never attribute
- if ((PageAttributes & TT_DESCRIPTOR_PAGE_XN_MASK) != 0 ) {
+ if ((PageAttributes & TT_DESCRIPTOR_PAGE_XN_MASK) != 0) {
*GcdAttributes |= EFI_MEMORY_XP;
}
+ if ((PageAttributes & TT_DESCRIPTOR_PAGE_AF) == 0) {
+ *GcdAttributes |= EFI_MEMORY_RP;
+ }
+
return EFI_SUCCESS;
}
@@ -166,6 +160,7 @@ SyncCacheConfigPage (
// Convert SectionAttributes into PageAttributes
NextPageAttributes =
TT_DESCRIPTOR_CONVERT_TO_PAGE_CACHE_POLICY (*NextSectionAttributes) |
+ TT_DESCRIPTOR_CONVERT_TO_PAGE_AF (*NextSectionAttributes) |
TT_DESCRIPTOR_CONVERT_TO_PAGE_AP (*NextSectionAttributes);
// obtain page table base
@@ -174,7 +169,7 @@ SyncCacheConfigPage (
for (i = 0; i < TRANSLATION_TABLE_PAGE_COUNT; i++) {
if ((SecondLevelTable[i] & TT_DESCRIPTOR_PAGE_TYPE_MASK) == TT_DESCRIPTOR_PAGE_TYPE_PAGE) {
// extract attributes (cacheability and permissions)
- PageAttributes = SecondLevelTable[i] & (TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK | TT_DESCRIPTOR_PAGE_AP_MASK);
+ PageAttributes = SecondLevelTable[i] & (TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK | TT_DESCRIPTOR_PAGE_AP_MASK | TT_DESCRIPTOR_PAGE_AF);
if (NextPageAttributes == 0) {
// start on a new region
@@ -213,6 +208,7 @@ SyncCacheConfigPage (
// Convert back PageAttributes into SectionAttributes
*NextSectionAttributes =
TT_DESCRIPTOR_CONVERT_TO_SECTION_CACHE_POLICY (NextPageAttributes) |
+ TT_DESCRIPTOR_CONVERT_TO_SECTION_AF (NextPageAttributes) |
TT_DESCRIPTOR_CONVERT_TO_SECTION_AP (NextPageAttributes);
return EFI_SUCCESS;
@@ -256,14 +252,14 @@ SyncCacheConfig (
FirstLevelTable = (ARM_FIRST_LEVEL_DESCRIPTOR *)(ArmGetTTBR0BaseAddress ());
// Get the first region
- NextSectionAttributes = FirstLevelTable[0] & (TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK | TT_DESCRIPTOR_SECTION_AP_MASK);
+ NextSectionAttributes = FirstLevelTable[0] & (TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK | TT_DESCRIPTOR_SECTION_AP_MASK | TT_DESCRIPTOR_SECTION_AF);
// iterate through each 1MB descriptor
NextRegionBase = NextRegionLength = 0;
for (i = 0; i < TRANSLATION_TABLE_SECTION_COUNT; i++) {
if ((FirstLevelTable[i] & TT_DESCRIPTOR_SECTION_TYPE_MASK) == TT_DESCRIPTOR_SECTION_TYPE_SECTION) {
// extract attributes (cacheability and permissions)
- SectionAttributes = FirstLevelTable[i] & (TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK | TT_DESCRIPTOR_SECTION_AP_MASK);
+ SectionAttributes = FirstLevelTable[i] & (TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK | TT_DESCRIPTOR_SECTION_AP_MASK | TT_DESCRIPTOR_SECTION_AF);
if (NextSectionAttributes == 0) {
// start on a new region
@@ -383,6 +379,10 @@ EfiAttributeToArmAttribute (
ArmAttributes |= TT_DESCRIPTOR_SECTION_XN_MASK;
}
+ if ((EfiAttributes & EFI_MEMORY_RP) == 0) {
+ ArmAttributes |= TT_DESCRIPTOR_SECTION_AF;
+ }
+
return ArmAttributes;
}
@@ -482,6 +482,7 @@ GetMemoryRegion (
*RegionAttributes = TT_DESCRIPTOR_CONVERT_TO_SECTION_CACHE_POLICY (PageAttributes) |
TT_DESCRIPTOR_CONVERT_TO_SECTION_S (PageAttributes) |
TT_DESCRIPTOR_CONVERT_TO_SECTION_XN (PageAttributes) |
+ TT_DESCRIPTOR_CONVERT_TO_SECTION_AF (PageAttributes) |
TT_DESCRIPTOR_CONVERT_TO_SECTION_AP (PageAttributes);
}
diff --git a/ArmPkg/Include/Chipset/ArmV7Mmu.h b/ArmPkg/Include/Chipset/ArmV7Mmu.h
index e0219747df86..da4f3160f8ff 100644
--- a/ArmPkg/Include/Chipset/ArmV7Mmu.h
+++ b/ArmPkg/Include/Chipset/ArmV7Mmu.h
@@ -80,21 +80,21 @@
#define TT_DESCRIPTOR_PAGE_S_NOT_SHARED (0UL << 10)
#define TT_DESCRIPTOR_PAGE_S_SHARED (1UL << 10)
-#define TT_DESCRIPTOR_SECTION_AP_MASK ((1UL << 15) | (3UL << 10))
-#define TT_DESCRIPTOR_SECTION_AP_NO_NO ((0UL << 15) | (0UL << 10))
-#define TT_DESCRIPTOR_SECTION_AP_RW_NO ((0UL << 15) | (1UL << 10))
-#define TT_DESCRIPTOR_SECTION_AP_RW_RO ((0UL << 15) | (2UL << 10))
-#define TT_DESCRIPTOR_SECTION_AP_RW_RW ((0UL << 15) | (3UL << 10))
-#define TT_DESCRIPTOR_SECTION_AP_RO_NO ((1UL << 15) | (1UL << 10))
-#define TT_DESCRIPTOR_SECTION_AP_RO_RO ((1UL << 15) | (3UL << 10))
+#define TT_DESCRIPTOR_SECTION_AP_MASK ((1UL << 15) | (1UL << 11))
+#define TT_DESCRIPTOR_SECTION_AP_NO_RW ((0UL << 15) | (0UL << 11))
+#define TT_DESCRIPTOR_SECTION_AP_RW_RW ((0UL << 15) | (1UL << 11))
+#define TT_DESCRIPTOR_SECTION_AP_NO_RO ((1UL << 15) | (0UL << 11))
+#define TT_DESCRIPTOR_SECTION_AP_RO_RO ((1UL << 15) | (1UL << 11))
-#define TT_DESCRIPTOR_PAGE_AP_MASK ((1UL << 9) | (3UL << 4))
-#define TT_DESCRIPTOR_PAGE_AP_NO_NO ((0UL << 9) | (0UL << 4))
-#define TT_DESCRIPTOR_PAGE_AP_RW_NO ((0UL << 9) | (1UL << 4))
-#define TT_DESCRIPTOR_PAGE_AP_RW_RO ((0UL << 9) | (2UL << 4))
-#define TT_DESCRIPTOR_PAGE_AP_RW_RW ((0UL << 9) | (3UL << 4))
-#define TT_DESCRIPTOR_PAGE_AP_RO_NO ((1UL << 9) | (1UL << 4))
-#define TT_DESCRIPTOR_PAGE_AP_RO_RO ((1UL << 9) | (3UL << 4))
+#define TT_DESCRIPTOR_SECTION_AF (1UL << 10)
+
+#define TT_DESCRIPTOR_PAGE_AP_MASK ((1UL << 9) | (1UL << 5))
+#define TT_DESCRIPTOR_PAGE_AP_NO_RW ((0UL << 9) | (0UL << 5))
+#define TT_DESCRIPTOR_PAGE_AP_RW_RW ((0UL << 9) | (1UL << 5))
+#define TT_DESCRIPTOR_PAGE_AP_NO_RO ((1UL << 9) | (0UL << 5))
+#define TT_DESCRIPTOR_PAGE_AP_RO_RO ((1UL << 9) | (1UL << 5))
+
+#define TT_DESCRIPTOR_PAGE_AF (1UL << 4)
#define TT_DESCRIPTOR_SECTION_XN_MASK (0x1UL << 4)
#define TT_DESCRIPTOR_PAGE_XN_MASK (0x1UL << 0)
@@ -124,20 +124,24 @@
#define TT_DESCRIPTOR_CONVERT_TO_PAGE_AP(Desc) ((((Desc) & TT_DESCRIPTOR_SECTION_AP_MASK) >> 6) & TT_DESCRIPTOR_PAGE_AP_MASK)
#define TT_DESCRIPTOR_CONVERT_TO_PAGE_NG(Desc) ((((Desc) & TT_DESCRIPTOR_SECTION_NG_MASK) >> 6) & TT_DESCRIPTOR_PAGE_NG_MASK)
#define TT_DESCRIPTOR_CONVERT_TO_PAGE_S(Desc) ((((Desc) & TT_DESCRIPTOR_SECTION_S_MASK) >> 6) & TT_DESCRIPTOR_PAGE_S_MASK)
+#define TT_DESCRIPTOR_CONVERT_TO_PAGE_AF(Desc) ((((Desc) & TT_DESCRIPTOR_SECTION_AF) >> 6) & TT_DESCRIPTOR_PAGE_AF)
#define TT_DESCRIPTOR_CONVERT_TO_PAGE_XN(Desc) ((((Desc) & TT_DESCRIPTOR_SECTION_XN_MASK) >> 4) & TT_DESCRIPTOR_PAGE_XN_MASK)
#define TT_DESCRIPTOR_CONVERT_TO_PAGE_CACHE_POLICY(Desc) ((((Desc) & (0x3 << 12)) >> 6) | (Desc & (0x3 << 2)))
#define TT_DESCRIPTOR_CONVERT_TO_SECTION_AP(Desc) ((((Desc) & TT_DESCRIPTOR_PAGE_AP_MASK) << 6) & TT_DESCRIPTOR_SECTION_AP_MASK)
#define TT_DESCRIPTOR_CONVERT_TO_SECTION_S(Desc) ((((Desc) & TT_DESCRIPTOR_PAGE_S_MASK) << 6) & TT_DESCRIPTOR_SECTION_S_MASK)
+#define TT_DESCRIPTOR_CONVERT_TO_SECTION_AF(Desc) ((((Desc) & TT_DESCRIPTOR_PAGE_AF) << 6) & TT_DESCRIPTOR_SECTION_AF)
#define TT_DESCRIPTOR_CONVERT_TO_SECTION_XN(Desc) ((((Desc) & TT_DESCRIPTOR_PAGE_XN_MASK) << 4) & TT_DESCRIPTOR_SECTION_XN_MASK)
#define TT_DESCRIPTOR_CONVERT_TO_SECTION_CACHE_POLICY(Desc) ((((Desc) & (0x3 << 6)) << 6) | (Desc & (0x3 << 2)))
#define TT_DESCRIPTOR_SECTION_ATTRIBUTE_MASK (TT_DESCRIPTOR_SECTION_NS_MASK | TT_DESCRIPTOR_SECTION_NG_MASK | \
TT_DESCRIPTOR_SECTION_S_MASK | TT_DESCRIPTOR_SECTION_AP_MASK | \
+ TT_DESCRIPTOR_SECTION_AF | \
TT_DESCRIPTOR_SECTION_XN_MASK | TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK)
#define TT_DESCRIPTOR_PAGE_ATTRIBUTE_MASK (TT_DESCRIPTOR_PAGE_NG_MASK | TT_DESCRIPTOR_PAGE_S_MASK | \
TT_DESCRIPTOR_PAGE_AP_MASK | TT_DESCRIPTOR_PAGE_XN_MASK | \
+ TT_DESCRIPTOR_PAGE_AF | \
TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK)
#define TT_DESCRIPTOR_SECTION_DOMAIN_MASK (0x0FUL << 5)
@@ -159,6 +163,7 @@
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) | \
@@ -166,6 +171,7 @@
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) | \
@@ -174,6 +180,7 @@
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) | \
@@ -181,28 +188,33 @@
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_PAGE_WRITE_BACK (TT_DESCRIPTOR_PAGE_TYPE_PAGE | \
TT_DESCRIPTOR_PAGE_NG_GLOBAL | \
TT_DESCRIPTOR_PAGE_S_SHARED | \
TT_DESCRIPTOR_PAGE_AP_RW_RW | \
+ TT_DESCRIPTOR_PAGE_AF | \
TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_BACK_ALLOC)
#define TT_DESCRIPTOR_PAGE_WRITE_THROUGH (TT_DESCRIPTOR_PAGE_TYPE_PAGE | \
TT_DESCRIPTOR_PAGE_NG_GLOBAL | \
TT_DESCRIPTOR_PAGE_S_SHARED | \
TT_DESCRIPTOR_PAGE_AP_RW_RW | \
+ TT_DESCRIPTOR_PAGE_AF | \
TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC)
#define TT_DESCRIPTOR_PAGE_DEVICE (TT_DESCRIPTOR_PAGE_TYPE_PAGE | \
TT_DESCRIPTOR_PAGE_NG_GLOBAL | \
TT_DESCRIPTOR_PAGE_S_NOT_SHARED | \
TT_DESCRIPTOR_PAGE_AP_RW_RW | \
+ TT_DESCRIPTOR_PAGE_AF | \
TT_DESCRIPTOR_PAGE_XN_MASK | \
TT_DESCRIPTOR_PAGE_CACHE_POLICY_SHAREABLE_DEVICE)
#define TT_DESCRIPTOR_PAGE_UNCACHED (TT_DESCRIPTOR_PAGE_TYPE_PAGE | \
TT_DESCRIPTOR_PAGE_NG_GLOBAL | \
TT_DESCRIPTOR_PAGE_S_NOT_SHARED | \
TT_DESCRIPTOR_PAGE_AP_RW_RW | \
+ TT_DESCRIPTOR_PAGE_AF | \
TT_DESCRIPTOR_PAGE_CACHE_POLICY_NON_CACHEABLE)
// First Level Descriptors
diff --git a/ArmPkg/Library/ArmLib/Arm/ArmV7Support.S b/ArmPkg/Library/ArmLib/Arm/ArmV7Support.S
index 4925f6628e1e..1f396adffc11 100644
--- a/ArmPkg/Library/ArmLib/Arm/ArmV7Support.S
+++ b/ArmPkg/Library/ArmLib/Arm/ArmV7Support.S
@@ -16,6 +16,7 @@
.set CTRL_C_BIT, (1 << 2)
.set CTRL_B_BIT, (1 << 7)
.set CTRL_I_BIT, (1 << 12)
+.set CTRL_AFE_BIT,(1 << 29)
ASM_FUNC(ArmInvalidateDataCacheEntryByMVA)
@@ -64,6 +65,7 @@ ASM_FUNC(ArmInvalidateInstructionCache)
ASM_FUNC(ArmEnableMmu)
mrc p15,0,R0,c1,c0,0
orr R0,R0,#1
+ orr R0,R0,#CTRL_AFE_BIT
mcr p15,0,R0,c1,c0,0
dsb
isb
diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibConvert.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibConvert.c
index 6e2f08a7ce15..52dbfd714029 100644
--- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibConvert.c
+++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibConvert.c
@@ -23,6 +23,7 @@ ConvertSectionAttributesToPageAttributes (
PageAttributes = 0;
PageAttributes |= TT_DESCRIPTOR_CONVERT_TO_PAGE_CACHE_POLICY (SectionAttributes);
PageAttributes |= TT_DESCRIPTOR_CONVERT_TO_PAGE_AP (SectionAttributes);
+ PageAttributes |= TT_DESCRIPTOR_CONVERT_TO_PAGE_AF (SectionAttributes);
PageAttributes |= TT_DESCRIPTOR_CONVERT_TO_PAGE_XN (SectionAttributes);
PageAttributes |= TT_DESCRIPTOR_CONVERT_TO_PAGE_NG (SectionAttributes);
PageAttributes |= TT_DESCRIPTOR_CONVERT_TO_PAGE_S (SectionAttributes);
diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c
index 12d0f4c30f8e..484c67476619 100644
--- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c
+++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c
@@ -104,7 +104,7 @@ UpdatePageEntries (
// EntryMask: bitmask of values to change (1 = change this value, 0 = leave alone)
// EntryValue: values at bit positions specified by EntryMask
- EntryMask = TT_DESCRIPTOR_PAGE_TYPE_MASK | TT_DESCRIPTOR_PAGE_AP_MASK | TT_DESCRIPTOR_PAGE_XN_MASK;
+ EntryMask = TT_DESCRIPTOR_PAGE_TYPE_MASK | TT_DESCRIPTOR_PAGE_AP_MASK | TT_DESCRIPTOR_PAGE_XN_MASK | TT_DESCRIPTOR_PAGE_AF;
EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE;
// Although the PI spec is unclear on this, the GCD guarantees that only
@@ -138,6 +138,10 @@ UpdatePageEntries (
return EFI_UNSUPPORTED;
}
+ if ((Attributes & EFI_MEMORY_RP) == 0) {
+ EntryValue |= TT_DESCRIPTOR_PAGE_AF;
+ }
+
if ((Attributes & EFI_MEMORY_RO) != 0) {
EntryValue |= TT_DESCRIPTOR_PAGE_AP_RO_RO;
} else {
@@ -237,7 +241,7 @@ UpdateSectionEntries (
// Make sure we handle a section range that is unmapped
EntryMask = TT_DESCRIPTOR_SECTION_TYPE_MASK | TT_DESCRIPTOR_SECTION_XN_MASK |
- TT_DESCRIPTOR_SECTION_AP_MASK;
+ TT_DESCRIPTOR_SECTION_AP_MASK | TT_DESCRIPTOR_SECTION_AF;
EntryValue = TT_DESCRIPTOR_SECTION_TYPE_SECTION;
// Although the PI spec is unclear on this, the GCD guarantees that only
@@ -281,6 +285,10 @@ UpdateSectionEntries (
EntryValue |= TT_DESCRIPTOR_SECTION_XN_MASK;
}
+ if ((Attributes & EFI_MEMORY_RP) == 0) {
+ EntryValue |= TT_DESCRIPTOR_SECTION_AF;
+ }
+
// obtain page table base
FirstLevelTable = (ARM_FIRST_LEVEL_DESCRIPTOR *)ArmGetTTBR0BaseAddress ();
--
2.39.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v4 05/11] ArmPkg/ArmMmuLib ARM: Clear individual permission bits
2023-02-09 13:59 [PATCH v4 00/11] ArmPkg: implement EFI memory attributes protocol Ard Biesheuvel
` (3 preceding siblings ...)
2023-02-09 13:59 ` [PATCH v4 04/11] ArmPkg/ArmMmuLib ARM: Isolate the access flag from AP mask Ard Biesheuvel
@ 2023-02-09 13:59 ` Ard Biesheuvel
2023-02-09 13:59 ` [PATCH v4 06/11] ArmPkg/ArmMmuLib: Implement EFI_MEMORY_RP using access flag Ard Biesheuvel
` (6 subsequent siblings)
11 siblings, 0 replies; 18+ messages in thread
From: Ard Biesheuvel @ 2023-02-09 13:59 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
Currently, the MMU code that is supposed to clear the RO or XP
attributes from a region just clears both unconditionally. This
approximates the desired behavior to some extent, but it does mean that
setting the RO bit first on a code region, and then clearing the XP bit
results both RO and XP being cleared, and we end up with writable code,
and avoiding that is the point of all these protections.
Once we introduce RP support, this will only get worse, so let's fix
this up, by reshuffling the attribute update code to take the entry mask
from the caller, and use the mask to preserve other attributes when
clearing RO or XP.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c | 94 +++++++++++++++++---
1 file changed, 81 insertions(+), 13 deletions(-)
diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c
index 484c67476619..23f613f5dbb0 100644
--- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c
+++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c
@@ -81,12 +81,12 @@ UpdatePageEntries (
IN EFI_PHYSICAL_ADDRESS BaseAddress,
IN UINT64 Length,
IN UINT64 Attributes,
+ IN UINT32 EntryMask,
OUT BOOLEAN *FlushTlbs OPTIONAL
)
{
EFI_STATUS Status;
UINT32 EntryValue;
- UINT32 EntryMask;
UINT32 FirstLevelIdx;
UINT32 Offset;
UINT32 NumPageEntries;
@@ -104,7 +104,6 @@ UpdatePageEntries (
// EntryMask: bitmask of values to change (1 = change this value, 0 = leave alone)
// EntryValue: values at bit positions specified by EntryMask
- EntryMask = TT_DESCRIPTOR_PAGE_TYPE_MASK | TT_DESCRIPTOR_PAGE_AP_MASK | TT_DESCRIPTOR_PAGE_XN_MASK | TT_DESCRIPTOR_PAGE_AF;
EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE;
// Although the PI spec is unclear on this, the GCD guarantees that only
@@ -220,11 +219,11 @@ EFI_STATUS
UpdateSectionEntries (
IN EFI_PHYSICAL_ADDRESS BaseAddress,
IN UINT64 Length,
- IN UINT64 Attributes
+ IN UINT64 Attributes,
+ IN UINT32 EntryMask
)
{
EFI_STATUS Status;
- UINT32 EntryMask;
UINT32 EntryValue;
UINT32 FirstLevelIdx;
UINT32 NumSections;
@@ -240,8 +239,6 @@ UpdateSectionEntries (
// EntryValue: values at bit positions specified by EntryMask
// Make sure we handle a section range that is unmapped
- EntryMask = TT_DESCRIPTOR_SECTION_TYPE_MASK | TT_DESCRIPTOR_SECTION_XN_MASK |
- TT_DESCRIPTOR_SECTION_AP_MASK | TT_DESCRIPTOR_SECTION_AF;
EntryValue = TT_DESCRIPTOR_SECTION_TYPE_SECTION;
// Although the PI spec is unclear on this, the GCD guarantees that only
@@ -310,6 +307,7 @@ UpdateSectionEntries (
(FirstLevelIdx + i) << TT_DESCRIPTOR_SECTION_BASE_SHIFT,
TT_DESCRIPTOR_SECTION_SIZE,
Attributes,
+ ConvertSectionAttributesToPageAttributes (EntryMask),
NULL
);
} else {
@@ -340,11 +338,26 @@ UpdateSectionEntries (
return Status;
}
+/**
+ Update the permission or memory type attributes on a range of memory.
+
+ @param BaseAddress The start of the region.
+ @param Length The size of the region.
+ @param Attributes A mask of EFI_MEMORY_xx constants.
+ @param SectionMask A mask of short descriptor section attributes
+ describing which descriptor bits to update.
+
+ @retval EFI_SUCCESS The attributes were set successfully.
+ @retval EFI_OUT_OF_RESOURCES The operation failed due to insufficient memory.
+
+**/
+STATIC
EFI_STATUS
-ArmSetMemoryAttributes (
+SetMemoryAttributes (
IN EFI_PHYSICAL_ADDRESS BaseAddress,
IN UINT64 Length,
- IN UINT64 Attributes
+ IN UINT64 Attributes,
+ IN UINT32 SectionMask
)
{
EFI_STATUS Status;
@@ -375,7 +388,12 @@ ArmSetMemoryAttributes (
Attributes
));
- Status = UpdateSectionEntries (BaseAddress, ChunkLength, Attributes);
+ Status = UpdateSectionEntries (
+ BaseAddress,
+ ChunkLength,
+ Attributes,
+ SectionMask
+ );
FlushTlbs = TRUE;
} else {
@@ -401,6 +419,7 @@ ArmSetMemoryAttributes (
BaseAddress,
ChunkLength,
Attributes,
+ ConvertSectionAttributesToPageAttributes (SectionMask),
&FlushTlbs
);
}
@@ -420,13 +439,47 @@ ArmSetMemoryAttributes (
return Status;
}
+/**
+ Update the permission or memory type attributes on a range of memory.
+
+ @param BaseAddress The start of the region.
+ @param Length The size of the region.
+ @param Attributes A mask of EFI_MEMORY_xx constants.
+
+ @retval EFI_SUCCESS The attributes were set successfully.
+ @retval EFI_OUT_OF_RESOURCES The operation failed due to insufficient memory.
+
+**/
+EFI_STATUS
+ArmSetMemoryAttributes (
+ IN EFI_PHYSICAL_ADDRESS BaseAddress,
+ IN UINT64 Length,
+ IN UINT64 Attributes
+ )
+{
+ return SetMemoryAttributes (
+ BaseAddress,
+ Length,
+ Attributes,
+ TT_DESCRIPTOR_SECTION_TYPE_MASK |
+ TT_DESCRIPTOR_SECTION_XN_MASK |
+ TT_DESCRIPTOR_SECTION_AP_MASK |
+ TT_DESCRIPTOR_SECTION_AF
+ );
+}
+
EFI_STATUS
ArmSetMemoryRegionNoExec (
IN EFI_PHYSICAL_ADDRESS BaseAddress,
IN UINT64 Length
)
{
- return ArmSetMemoryAttributes (BaseAddress, Length, EFI_MEMORY_XP);
+ return SetMemoryAttributes (
+ BaseAddress,
+ Length,
+ EFI_MEMORY_XP,
+ TT_DESCRIPTOR_SECTION_XN_MASK
+ );
}
EFI_STATUS
@@ -435,7 +488,12 @@ ArmClearMemoryRegionNoExec (
IN UINT64 Length
)
{
- return ArmSetMemoryAttributes (BaseAddress, Length, __EFI_MEMORY_RWX);
+ return SetMemoryAttributes (
+ BaseAddress,
+ Length,
+ 0,
+ TT_DESCRIPTOR_SECTION_XN_MASK
+ );
}
EFI_STATUS
@@ -444,7 +502,12 @@ ArmSetMemoryRegionReadOnly (
IN UINT64 Length
)
{
- return ArmSetMemoryAttributes (BaseAddress, Length, EFI_MEMORY_RO);
+ return SetMemoryAttributes (
+ BaseAddress,
+ Length,
+ EFI_MEMORY_RO,
+ TT_DESCRIPTOR_SECTION_AP_MASK
+ );
}
EFI_STATUS
@@ -453,5 +516,10 @@ ArmClearMemoryRegionReadOnly (
IN UINT64 Length
)
{
- return ArmSetMemoryAttributes (BaseAddress, Length, __EFI_MEMORY_RWX);
+ return SetMemoryAttributes (
+ BaseAddress,
+ Length,
+ 0,
+ TT_DESCRIPTOR_SECTION_AP_MASK
+ );
}
--
2.39.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v4 06/11] ArmPkg/ArmMmuLib: Implement EFI_MEMORY_RP using access flag
2023-02-09 13:59 [PATCH v4 00/11] ArmPkg: implement EFI memory attributes protocol Ard Biesheuvel
` (4 preceding siblings ...)
2023-02-09 13:59 ` [PATCH v4 05/11] ArmPkg/ArmMmuLib ARM: Clear individual permission bits Ard Biesheuvel
@ 2023-02-09 13:59 ` Ard Biesheuvel
2023-02-09 13:59 ` [PATCH v4 07/11] ArmVirtPkg: Enable stack guard Ard Biesheuvel
` (5 subsequent siblings)
11 siblings, 0 replies; 18+ messages in thread
From: Ard Biesheuvel @ 2023-02-09 13:59 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
Implement support for read-protected memory by wiring it up to the
access flag in the page table descriptor. The resulting mapping is
implicitly non-writable and non-executable as well, but this is good
enough for implementing this attribute, as we never rely on write or
execute permissions without read permissions.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c | 8 ++-
ArmPkg/Include/Library/ArmMmuLib.h | 34 ++++++++++++
ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 58 +++++++++++++++++++-
ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c | 48 ++++++++++++++++
4 files changed, 144 insertions(+), 4 deletions(-)
diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
index 8bb33046e707..8bda11f08a30 100644
--- a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
+++ b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
@@ -64,6 +64,10 @@ PageAttributeToGcdAttribute (
}
// Determine protection attributes
+ if ((PageAttributes & TT_AF) == 0) {
+ GcdAttributes |= EFI_MEMORY_RP;
+ }
+
if (((PageAttributes & TT_AP_MASK) == TT_AP_NO_RO) ||
((PageAttributes & TT_AP_MASK) == TT_AP_RO_RO))
{
@@ -301,7 +305,9 @@ EfiAttributeToArmAttribute (
}
// Set the access flag to match the block attributes
- ArmAttributes |= TT_AF;
+ if ((EfiAttributes & EFI_MEMORY_RP) == 0) {
+ ArmAttributes |= TT_AF;
+ }
// Determine protection attributes
if ((EfiAttributes & EFI_MEMORY_RO) != 0) {
diff --git a/ArmPkg/Include/Library/ArmMmuLib.h b/ArmPkg/Include/Library/ArmMmuLib.h
index b745e2230e7e..4cf59a1e376b 100644
--- a/ArmPkg/Include/Library/ArmMmuLib.h
+++ b/ArmPkg/Include/Library/ArmMmuLib.h
@@ -21,6 +21,40 @@ ArmConfigureMmu (
OUT UINTN *TranslationTableSize OPTIONAL
);
+/**
+ Convert a region of memory to read-protected, by clearing the access flag.
+
+ @param BaseAddress The start of the region.
+ @param Length The size of the region.
+
+ @retval EFI_SUCCESS The attributes were set successfully.
+ @retval EFI_OUT_OF_RESOURCES The operation failed due to insufficient memory.
+
+**/
+EFI_STATUS
+EFIAPI
+ArmSetMemoryRegionNoAccess (
+ IN EFI_PHYSICAL_ADDRESS BaseAddress,
+ IN UINT64 Length
+ );
+
+/**
+ Convert a region of memory to read-enabled, by setting the access flag.
+
+ @param BaseAddress The start of the region.
+ @param Length The size of the region.
+
+ @retval EFI_SUCCESS The attributes were set successfully.
+ @retval EFI_OUT_OF_RESOURCES The operation failed due to insufficient memory.
+
+**/
+EFI_STATUS
+EFIAPI
+ArmClearMemoryRegionNoAccess (
+ IN EFI_PHYSICAL_ADDRESS BaseAddress,
+ IN UINT64 Length
+ );
+
EFI_STATUS
EFIAPI
ArmSetMemoryRegionNoExec (
diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index 764c7d362e2e..6d21a2e41dd1 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -438,7 +438,11 @@ GcdAttributeToPageAttribute (
PageAttributes |= TT_AP_NO_RO;
}
- return PageAttributes | TT_AF;
+ if ((GcdAttributes & EFI_MEMORY_RP) == 0) {
+ PageAttributes |= TT_AF;
+ }
+
+ return PageAttributes;
}
EFI_STATUS
@@ -459,9 +463,9 @@ ArmSetMemoryAttributes (
// No memory type was set in Attributes, so we are going to update the
// permissions only.
//
- PageAttributes &= TT_AP_MASK | TT_UXN_MASK | TT_PXN_MASK;
+ PageAttributes &= TT_AP_MASK | TT_UXN_MASK | TT_PXN_MASK | TT_AF;
PageAttributeMask = ~(TT_ADDRESS_MASK_BLOCK_ENTRY | TT_AP_MASK |
- TT_PXN_MASK | TT_XN_MASK);
+ TT_PXN_MASK | TT_XN_MASK | TT_AF);
}
return UpdateRegionMapping (
@@ -534,6 +538,54 @@ ArmClearMemoryRegionNoExec (
);
}
+/**
+ Convert a region of memory to read-protected, by clearing the access flag.
+
+ @param BaseAddress The start of the region.
+ @param Length The size of the region.
+
+ @retval EFI_SUCCESS The attributes were set successfully.
+ @retval EFI_OUT_OF_RESOURCES The operation failed due to insufficient memory.
+
+**/
+EFI_STATUS
+ArmSetMemoryRegionNoAccess (
+ IN EFI_PHYSICAL_ADDRESS BaseAddress,
+ IN UINT64 Length
+ )
+{
+ return SetMemoryRegionAttribute (
+ BaseAddress,
+ Length,
+ 0,
+ ~(TT_ADDRESS_MASK_BLOCK_ENTRY | TT_AF)
+ );
+}
+
+/**
+ Convert a region of memory to read-enabled, by setting the access flag.
+
+ @param BaseAddress The start of the region.
+ @param Length The size of the region.
+
+ @retval EFI_SUCCESS The attributes were set successfully.
+ @retval EFI_OUT_OF_RESOURCES The operation failed due to insufficient memory.
+
+**/
+EFI_STATUS
+ArmClearMemoryRegionNoAccess (
+ IN EFI_PHYSICAL_ADDRESS BaseAddress,
+ IN UINT64 Length
+ )
+{
+ return SetMemoryRegionAttribute (
+ BaseAddress,
+ Length,
+ TT_AF,
+ ~TT_ADDRESS_MASK_BLOCK_ENTRY
+ );
+}
+
EFI_STATUS
ArmSetMemoryRegionReadOnly (
IN EFI_PHYSICAL_ADDRESS BaseAddress,
diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c
index 23f613f5dbb0..247cf87bf3d3 100644
--- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c
+++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c
@@ -523,3 +523,51 @@ ArmClearMemoryRegionReadOnly (
TT_DESCRIPTOR_SECTION_AP_MASK
);
}
+
+/**
+ Convert a region of memory to read-protected, by clearing the access flag.
+
+ @param BaseAddress The start of the region.
+ @param Length The size of the region.
+
+ @retval EFI_SUCCESS The attributes were set successfully.
+ @retval EFI_OUT_OF_RESOURCES The operation failed due to insufficient memory.
+
+**/
+EFI_STATUS
+ArmSetMemoryRegionNoAccess (
+ IN EFI_PHYSICAL_ADDRESS BaseAddress,
+ IN UINT64 Length
+ )
+{
+ return SetMemoryAttributes (
+ BaseAddress,
+ Length,
+ EFI_MEMORY_RP,
+ TT_DESCRIPTOR_SECTION_AF
+ );
+}
+
+/**
+ Convert a region of memory to read-enabled, by setting the access flag.
+
+ @param BaseAddress The start of the region.
+ @param Length The size of the region.
+
+ @retval EFI_SUCCESS The attributes were set successfully.
+ @retval EFI_OUT_OF_RESOURCES The operation failed due to insufficient memory.
+
+**/
+EFI_STATUS
+ArmClearMemoryRegionNoAccess (
+ IN EFI_PHYSICAL_ADDRESS BaseAddress,
+ IN UINT64 Length
+ )
+{
+ return SetMemoryAttributes (
+ BaseAddress,
+ Length,
+ 0,
+ TT_DESCRIPTOR_SECTION_AF
+ );
+}
--
2.39.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v4 07/11] ArmVirtPkg: Enable stack guard
2023-02-09 13:59 [PATCH v4 00/11] ArmPkg: implement EFI memory attributes protocol Ard Biesheuvel
` (5 preceding siblings ...)
2023-02-09 13:59 ` [PATCH v4 06/11] ArmPkg/ArmMmuLib: Implement EFI_MEMORY_RP using access flag Ard Biesheuvel
@ 2023-02-09 13:59 ` Ard Biesheuvel
2023-02-09 13:59 ` [PATCH v4 08/11] ArmPkg/ArmMmuLib: Avoid splitting block entries if possible Ard Biesheuvel
` (4 subsequent siblings)
11 siblings, 0 replies; 18+ messages in thread
From: Ard Biesheuvel @ 2023-02-09 13:59 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
Enable the stack guard in ArmVirtPkg builds, so that stack overflows are
caught as they occur, rather than when they happen to hit a read-only
memory region.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
ArmVirtPkg/ArmVirt.dsc.inc | 2 ++
1 file changed, 2 insertions(+)
diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
index 74d98e6314c4..5b18184be263 100644
--- a/ArmVirtPkg/ArmVirt.dsc.inc
+++ b/ArmVirtPkg/ArmVirt.dsc.inc
@@ -363,6 +363,8 @@ [PcdsFixedAtBuild.common]
#
gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy|0xC000000000007FD5
+ gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard|TRUE
+
[Components.common]
#
# Ramdisk support
--
2.39.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v4 08/11] ArmPkg/ArmMmuLib: Avoid splitting block entries if possible
2023-02-09 13:59 [PATCH v4 00/11] ArmPkg: implement EFI memory attributes protocol Ard Biesheuvel
` (6 preceding siblings ...)
2023-02-09 13:59 ` [PATCH v4 07/11] ArmVirtPkg: Enable stack guard Ard Biesheuvel
@ 2023-02-09 13:59 ` Ard Biesheuvel
2023-02-09 13:59 ` [PATCH v4 09/11] ArmPkg/CpuDxe: Expose unified region-to-EFI attribute conversion Ard Biesheuvel
` (3 subsequent siblings)
11 siblings, 0 replies; 18+ messages in thread
From: Ard Biesheuvel @ 2023-02-09 13:59 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
Currently, the ARM MMU page table logic will break down any block entry
that overlaps with the region being mapped, even if the block entry in
question is using the same attributes as the new region.
This means that creating a non-executable mapping inside a region that
is already mapped non-executable at a coarser granularity may trigger a
call to AllocatePages (), which may recurse back into the page table
code to update the attributes on the newly allocated page tables.
Let's avoid this, by preserving the block entry if it already covers the
region being mapped with the correct attributes.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 10 ++++++++++
ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c | 11 +++++++++++
2 files changed, 21 insertions(+)
diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index 6d21a2e41dd1..1ce200c43c72 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -251,6 +251,16 @@ UpdateRegionMappingRecursive (
ASSERT (Level < 3);
if (!IsTableEntry (*Entry, Level)) {
+ //
+ // If the region we are trying to map is already covered by a block
+ // entry with the right attributes, don't bother splitting it up.
+ //
+ if (IsBlockEntry (*Entry, Level) &&
+ ((*Entry & TT_ATTRIBUTES_MASK & ~AttributeClearMask) == AttributeSetMask))
+ {
+ continue;
+ }
+
//
// No table entry exists yet, so we need to allocate a page table
// for the next level.
diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c
index 247cf87bf3d3..299d38ad07e8 100644
--- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c
+++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c
@@ -170,6 +170,17 @@ UpdatePageEntries (
// Does this descriptor need to be converted from section entry to 4K pages?
if (!TT_DESCRIPTOR_SECTION_TYPE_IS_PAGE_TABLE (Descriptor)) {
+ //
+ // If the section mapping covers the requested region with the expected
+ // attributes, splitting it is unnecessary, and should be avoided as it
+ // may result in unbounded recursion when using a strict NX policy.
+ //
+ if ((EntryValue & ~TT_DESCRIPTOR_PAGE_TYPE_MASK & EntryMask) ==
+ (ConvertSectionAttributesToPageAttributes (Descriptor) & EntryMask))
+ {
+ continue;
+ }
+
Status = ConvertSectionToPages (FirstLevelIdx << TT_DESCRIPTOR_SECTION_BASE_SHIFT);
if (EFI_ERROR (Status)) {
// Exit for loop
--
2.39.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v4 09/11] ArmPkg/CpuDxe: Expose unified region-to-EFI attribute conversion
2023-02-09 13:59 [PATCH v4 00/11] ArmPkg: implement EFI memory attributes protocol Ard Biesheuvel
` (7 preceding siblings ...)
2023-02-09 13:59 ` [PATCH v4 08/11] ArmPkg/ArmMmuLib: Avoid splitting block entries if possible Ard Biesheuvel
@ 2023-02-09 13:59 ` Ard Biesheuvel
2023-02-09 13:59 ` [PATCH v4 10/11] MdePkg: Add Memory Attribute Protocol definition Ard Biesheuvel
` (2 subsequent siblings)
11 siblings, 0 replies; 18+ messages in thread
From: Ard Biesheuvel @ 2023-02-09 13:59 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
In preparation for introducing an implementation of the EFI memory
attributes protocol that is shared between ARM and AArch64, unify the
existing code that converts a page table descriptor into a
EFI_MEMORY_xxx bitfield, so it can be called from the generic code.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c | 17 +++++++++
ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 38 ++++++++++++++++++++
ArmPkg/Drivers/CpuDxe/CpuDxe.h | 14 ++++++++
3 files changed, 69 insertions(+)
diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
index 8bda11f08a30..4a416743fb8a 100644
--- a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
+++ b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
@@ -83,6 +83,23 @@ PageAttributeToGcdAttribute (
return GcdAttributes;
}
+/**
+ Convert a arch specific set of page attributes into a mask
+ of EFI_MEMORY_xx constants.
+
+ @param PageAttributes The set of page attributes.
+
+ @retval The mask of EFI_MEMORY_xx constants.
+
+**/
+UINT64
+RegionAttributeToGcdAttribute (
+ IN UINTN PageAttributes
+ )
+{
+ return PageAttributeToGcdAttribute (PageAttributes);
+}
+
STATIC
UINT64
GetFirstPageAttribute (
diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
index 07faab8216ec..8e0dd5d2aaca 100644
--- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
+++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
@@ -13,6 +13,15 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include <Library/MemoryAllocationLib.h>
#include "CpuDxe.h"
+/**
+ Convert a set of ARM short descriptor section attributes into a mask
+ of EFI_MEMORY_xx constants.
+
+ @param SectionAttributes The set of page attributes.
+ @param GcdAttributes Pointer to the return value.
+
+**/
+STATIC
EFI_STATUS
SectionToGcdAttributes (
IN UINT32 SectionAttributes,
@@ -74,6 +83,35 @@ SectionToGcdAttributes (
return EFI_SUCCESS;
}
+/**
+ Convert a arch specific set of page attributes into a mask
+ of EFI_MEMORY_xx constants.
+
+ @param PageAttributes The set of page attributes.
+
+ @retval The mask of EFI_MEMORY_xx constants.
+
+**/
+UINT64
+RegionAttributeToGcdAttribute (
+ IN UINTN PageAttributes
+ )
+{
+ UINT64 Result;
+
+ SectionToGcdAttributes (PageAttributes, &Result);
+ return Result;
+}
+
+/**
+ Convert a set of ARM short descriptor page attributes into a mask
+ of EFI_MEMORY_xx constants.
+
+ @param PageAttributes The set of page attributes.
+ @param GcdAttributes Pointer to the return value.
+
+**/
+STATIC
EFI_STATUS
PageToGcdAttributes (
IN UINT32 PageAttributes,
diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.h b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
index ff672390ce51..8cb105dcc841 100644
--- a/ArmPkg/Drivers/CpuDxe/CpuDxe.h
+++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
@@ -126,4 +126,18 @@ SetGcdMemorySpaceAttributes (
IN UINT64 Attributes
);
+/**
+ Convert a arch specific set of page attributes into a mask
+ of EFI_MEMORY_xx constants.
+
+ @param PageAttributes The set of page attributes.
+
+ @retval The mask of EFI_MEMORY_xx constants.
+
+**/
+UINT64
+RegionAttributeToGcdAttribute (
+ IN UINTN PageAttributes
+ );
+
#endif // CPU_DXE_H_
--
2.39.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v4 10/11] MdePkg: Add Memory Attribute Protocol definition
2023-02-09 13:59 [PATCH v4 00/11] ArmPkg: implement EFI memory attributes protocol Ard Biesheuvel
` (8 preceding siblings ...)
2023-02-09 13:59 ` [PATCH v4 09/11] ArmPkg/CpuDxe: Expose unified region-to-EFI attribute conversion Ard Biesheuvel
@ 2023-02-09 13:59 ` Ard Biesheuvel
2023-02-09 13:59 ` [PATCH v4 11/11] ArmPkg/CpuDxe: Implement EFI memory attributes protocol Ard Biesheuvel
2023-02-11 0:56 ` [PATCH v4 00/11] ArmPkg: implement " Taylor Beebe
11 siblings, 0 replies; 18+ messages in thread
From: Ard Biesheuvel @ 2023-02-09 13:59 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
Add the Memory Attribute Protocol definition, which was adopted and
included in version 2.10 of the UEFI specification.
Link: https://bugzilla.tianocore.org/show_bug.cgi?id=3519
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
---
MdePkg/Include/Protocol/MemoryAttribute.h | 142 ++++++++++++++++++++
MdePkg/MdePkg.dec | 3 +
2 files changed, 145 insertions(+)
diff --git a/MdePkg/Include/Protocol/MemoryAttribute.h b/MdePkg/Include/Protocol/MemoryAttribute.h
new file mode 100644
index 000000000000..5c6b7badb589
--- /dev/null
+++ b/MdePkg/Include/Protocol/MemoryAttribute.h
@@ -0,0 +1,142 @@
+/** @file
+
+ EFI Memory Attribute Protocol provides retrieval and update service
+ for memory attributes in EFI environment.
+
+ Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) 2023, Google LLC. All rights reserved.<BR>
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#ifndef EFI_MEMORY_ATTRIBUTE_H_
+#define EFI_MEMORY_ATTRIBUTE_H_
+
+#define EFI_MEMORY_ATTRIBUTE_PROTOCOL_GUID \
+ { \
+ 0xf4560cf6, 0x40ec, 0x4b4a, { 0xa1, 0x92, 0xbf, 0x1d, 0x57, 0xd0, 0xb1, 0x89 } \
+ }
+
+typedef struct _EFI_MEMORY_ATTRIBUTE_PROTOCOL EFI_MEMORY_ATTRIBUTE_PROTOCOL;
+
+/**
+ This function set given attributes of the memory region specified by
+ BaseAddress and Length.
+
+ The valid Attributes is EFI_MEMORY_RP, EFI_MEMORY_XP, and EFI_MEMORY_RO.
+
+ @param This The EFI_MEMORY_ATTRIBUTE_PROTOCOL instance.
+ @param BaseAddress The physical address that is the start address of
+ a memory region.
+ @param Length The size in bytes of the memory region.
+ @param Attributes The bit mask of attributes to set for the memory
+ region.
+
+ @retval EFI_SUCCESS The attributes were set for the memory region.
+ @retval EFI_INVALID_PARAMETER Length is zero.
+ Attributes specified an illegal combination of
+ attributes that cannot be set together.
+ @retval EFI_UNSUPPORTED The processor does not support one or more
+ bytes of the memory resource range specified
+ by BaseAddress and Length.
+ The bit mask of attributes is not supported for
+ the memory resource range specified by
+ BaseAddress and Length.
+ @retval EFI_OUT_OF_RESOURCES Requested attributes cannot be applied due to
+ lack of system resources.
+ @retval EFI_ACCESS_DENIED Attributes for the requested memory region are
+ controlled by system firmware and cannot be
+ updated via the protocol.
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI *EFI_SET_MEMORY_ATTRIBUTES)(
+ IN EFI_MEMORY_ATTRIBUTE_PROTOCOL *This,
+ IN EFI_PHYSICAL_ADDRESS BaseAddress,
+ IN UINT64 Length,
+ IN UINT64 Attributes
+ );
+
+/**
+ This function clears given attributes of the memory region specified by
+ BaseAddress and Length.
+
+ The valid Attributes is EFI_MEMORY_RP, EFI_MEMORY_XP, and EFI_MEMORY_RO.
+
+ @param This The EFI_MEMORY_ATTRIBUTE_PROTOCOL instance.
+ @param BaseAddress The physical address that is the start address of
+ a memory region.
+ @param Length The size in bytes of the memory region.
+ @param Attributes The bit mask of attributes to clear for the memory
+ region.
+
+ @retval EFI_SUCCESS The attributes were cleared for the memory region.
+ @retval EFI_INVALID_PARAMETER Length is zero.
+ Attributes specified an illegal combination of
+ attributes that cannot be cleared together.
+ @retval EFI_UNSUPPORTED The processor does not support one or more
+ bytes of the memory resource range specified
+ by BaseAddress and Length.
+ The bit mask of attributes is not supported for
+ the memory resource range specified by
+ BaseAddress and Length.
+ @retval EFI_OUT_OF_RESOURCES Requested attributes cannot be applied due to
+ lack of system resources.
+ @retval EFI_ACCESS_DENIED Attributes for the requested memory region are
+ controlled by system firmware and cannot be
+ updated via the protocol.
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI *EFI_CLEAR_MEMORY_ATTRIBUTES)(
+ IN EFI_MEMORY_ATTRIBUTE_PROTOCOL *This,
+ IN EFI_PHYSICAL_ADDRESS BaseAddress,
+ IN UINT64 Length,
+ IN UINT64 Attributes
+ );
+
+/**
+ This function retrieves the attributes of the memory region specified by
+ BaseAddress and Length. If different attributes are got from different part
+ of the memory region, EFI_NO_MAPPING will be returned.
+
+ @param This The EFI_MEMORY_ATTRIBUTE_PROTOCOL instance.
+ @param BaseAddress The physical address that is the start address of
+ a memory region.
+ @param Length The size in bytes of the memory region.
+ @param Attributes Pointer to attributes returned.
+
+ @retval EFI_SUCCESS The attributes got for the memory region.
+ @retval EFI_INVALID_PARAMETER Length is zero.
+ Attributes is NULL.
+ @retval EFI_NO_MAPPING Attributes are not consistent cross the memory
+ region.
+ @retval EFI_UNSUPPORTED The processor does not support one or more
+ bytes of the memory resource range specified
+ by BaseAddress and Length.
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI *EFI_GET_MEMORY_ATTRIBUTES)(
+ IN EFI_MEMORY_ATTRIBUTE_PROTOCOL *This,
+ IN EFI_PHYSICAL_ADDRESS BaseAddress,
+ IN UINT64 Length,
+ OUT UINT64 *Attributes
+ );
+
+///
+/// EFI Memory Attribute Protocol provides services to retrieve or update
+/// attribute of memory in the EFI environment.
+///
+struct _EFI_MEMORY_ATTRIBUTE_PROTOCOL {
+ EFI_GET_MEMORY_ATTRIBUTES GetMemoryAttributes;
+ EFI_SET_MEMORY_ATTRIBUTES SetMemoryAttributes;
+ EFI_CLEAR_MEMORY_ATTRIBUTES ClearMemoryAttributes;
+};
+
+extern EFI_GUID gEfiMemoryAttributeProtocolGuid;
+
+#endif
diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
index 3d08f20d15b0..a8658403c8fd 100644
--- a/MdePkg/MdePkg.dec
+++ b/MdePkg/MdePkg.dec
@@ -1915,6 +1915,9 @@ [Protocols]
## Include/Protocol/RedfishDiscover.h
gEfiRedfishDiscoverProtocolGuid = { 0x5db12509, 0x4550, 0x4347, { 0x96, 0xb3, 0x73, 0xc0, 0xff, 0x6e, 0x86, 0x9f }}
+ ## Include/Protocol/MemoryAttribute.h
+ gEfiMemoryAttributeProtocolGuid = { 0xf4560cf6, 0x40ec, 0x4b4a, { 0xa1, 0x92, 0xbf, 0x1d, 0x57, 0xd0, 0xb1, 0x89 }}
+
#
# Protocols defined in Shell2.0
#
--
2.39.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v4 11/11] ArmPkg/CpuDxe: Implement EFI memory attributes protocol
2023-02-09 13:59 [PATCH v4 00/11] ArmPkg: implement EFI memory attributes protocol Ard Biesheuvel
` (9 preceding siblings ...)
2023-02-09 13:59 ` [PATCH v4 10/11] MdePkg: Add Memory Attribute Protocol definition Ard Biesheuvel
@ 2023-02-09 13:59 ` Ard Biesheuvel
2023-02-11 0:56 ` [PATCH v4 00/11] ArmPkg: implement " Taylor Beebe
11 siblings, 0 replies; 18+ messages in thread
From: Ard Biesheuvel @ 2023-02-09 13:59 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
Expose the protocol introduced in v2.10 that permits the caller to
manage mapping permissions in the page tables.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
ArmPkg/Drivers/CpuDxe/CpuDxe.c | 2 +
ArmPkg/Drivers/CpuDxe/CpuDxe.h | 3 +
ArmPkg/Drivers/CpuDxe/CpuDxe.inf | 2 +
ArmPkg/Drivers/CpuDxe/MemoryAttribute.c | 271 ++++++++++++++++++++
4 files changed, 278 insertions(+)
diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.c b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
index e6742f0a25fc..d04958e79e52 100644
--- a/ArmPkg/Drivers/CpuDxe/CpuDxe.c
+++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
@@ -244,6 +244,8 @@ CpuDxeInitialize (
&mCpuHandle,
&gEfiCpuArchProtocolGuid,
&mCpu,
+ &gEfiMemoryAttributeProtocolGuid,
+ &mMemoryAttribute,
NULL
);
diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.h b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
index 8cb105dcc841..ce2981361aca 100644
--- a/ArmPkg/Drivers/CpuDxe/CpuDxe.h
+++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
@@ -30,9 +30,12 @@
#include <Protocol/Cpu.h>
#include <Protocol/DebugSupport.h>
#include <Protocol/LoadedImage.h>
+#include <Protocol/MemoryAttribute.h>
extern BOOLEAN mIsFlushingGCD;
+extern EFI_MEMORY_ATTRIBUTE_PROTOCOL mMemoryAttribute;
+
/**
This function registers and enables the handler specified by InterruptHandler for a processor
interrupt or exception type specified by InterruptType. If InterruptHandler is NULL, then the
diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.inf b/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
index 10792b393fc8..e732e21cb94a 100644
--- a/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
+++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.inf
@@ -23,6 +23,7 @@ [Sources.Common]
CpuDxe.h
CpuMmuCommon.c
Exception.c
+ MemoryAttribute.c
[Sources.ARM]
Arm/Mmu.c
@@ -53,6 +54,7 @@ [LibraryClasses]
[Protocols]
gEfiCpuArchProtocolGuid
+ gEfiMemoryAttributeProtocolGuid
[Guids]
gEfiDebugImageInfoTableGuid
diff --git a/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c b/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c
new file mode 100644
index 000000000000..b47464c0269e
--- /dev/null
+++ b/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c
@@ -0,0 +1,271 @@
+/** @file
+
+ Copyright (c) 2023, Google LLC. All rights reserved.
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include "CpuDxe.h"
+
+/**
+ This function retrieves the attributes of the memory region specified by
+ BaseAddress and Length. If different attributes are got from different part
+ of the memory region, EFI_NO_MAPPING will be returned.
+
+ @param This The EFI_MEMORY_ATTRIBUTE_PROTOCOL instance.
+ @param BaseAddress The physical address that is the start address of
+ a memory region.
+ @param Length The size in bytes of the memory region.
+ @param Attributes Pointer to attributes returned.
+
+ @retval EFI_SUCCESS The attributes got for the memory region.
+ @retval EFI_INVALID_PARAMETER Length is zero.
+ Attributes is NULL.
+ @retval EFI_NO_MAPPING Attributes are not consistent cross the memory
+ region.
+ @retval EFI_UNSUPPORTED The processor does not support one or more
+ bytes of the memory resource range specified
+ by BaseAddress and Length.
+
+**/
+STATIC
+EFI_STATUS
+GetMemoryAttributes (
+ IN EFI_MEMORY_ATTRIBUTE_PROTOCOL *This,
+ IN EFI_PHYSICAL_ADDRESS BaseAddress,
+ IN UINT64 Length,
+ OUT UINT64 *Attributes
+ )
+{
+ UINTN RegionAddress;
+ UINTN RegionLength;
+ UINTN RegionAttributes;
+ UINTN Union;
+ UINTN Intersection;
+ EFI_STATUS Status;
+
+ if ((Length == 0) || (Attributes == NULL)) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ DEBUG ((
+ DEBUG_VERBOSE,
+ "%a: BaseAddress == 0x%lx, Length == 0x%lx\n",
+ __FUNCTION__,
+ BaseAddress,
+ Length
+ ));
+
+ Union = 0;
+ Intersection = MAX_UINTN;
+
+ for (RegionAddress = (UINTN)BaseAddress;
+ RegionAddress < (UINTN)(BaseAddress + Length);
+ RegionAddress += RegionLength)
+ {
+ Status = GetMemoryRegion (
+ &RegionAddress,
+ &RegionLength,
+ &RegionAttributes
+ );
+
+ DEBUG ((
+ DEBUG_VERBOSE,
+ "%a: RegionAddress == 0x%lx, RegionLength == 0x%lx, RegionAttributes == 0x%lx\n",
+ __FUNCTION__,
+ (UINT64)RegionAddress,
+ (UINT64)RegionLength,
+ (UINT64)RegionAttributes
+ ));
+
+ if (EFI_ERROR (Status)) {
+ return EFI_NO_MAPPING;
+ }
+
+ Union |= RegionAttributes;
+ Intersection &= RegionAttributes;
+ }
+
+ DEBUG ((
+ DEBUG_VERBOSE,
+ "%a: Union == %lx, Intersection == %lx\n",
+ __FUNCTION__,
+ (UINT64)Union,
+ (UINT64)Intersection
+ ));
+
+ if (Union != Intersection) {
+ return EFI_NO_MAPPING;
+ }
+
+ *Attributes = RegionAttributeToGcdAttribute (Union);
+ *Attributes &= EFI_MEMORY_RP | EFI_MEMORY_RO | EFI_MEMORY_XP;
+ return EFI_SUCCESS;
+}
+
+/**
+ This function set given attributes of the memory region specified by
+ BaseAddress and Length.
+
+ The valid Attributes is EFI_MEMORY_RP, EFI_MEMORY_XP, and EFI_MEMORY_RO.
+
+ @param This The EFI_MEMORY_ATTRIBUTE_PROTOCOL instance.
+ @param BaseAddress The physical address that is the start address of
+ a memory region.
+ @param Length The size in bytes of the memory region.
+ @param Attributes The bit mask of attributes to set for the memory
+ region.
+
+ @retval EFI_SUCCESS The attributes were set for the memory region.
+ @retval EFI_INVALID_PARAMETER Length is zero.
+ Attributes specified an illegal combination of
+ attributes that cannot be set together.
+ @retval EFI_UNSUPPORTED The processor does not support one or more
+ bytes of the memory resource range specified
+ by BaseAddress and Length.
+ The bit mask of attributes is not supported for
+ the memory resource range specified by
+ BaseAddress and Length.
+ @retval EFI_OUT_OF_RESOURCES Requested attributes cannot be applied due to
+ lack of system resources.
+ @retval EFI_ACCESS_DENIED Attributes for the requested memory region are
+ controlled by system firmware and cannot be
+ updated via the protocol.
+
+**/
+STATIC
+EFI_STATUS
+SetMemoryAttributes (
+ IN EFI_MEMORY_ATTRIBUTE_PROTOCOL *This,
+ IN EFI_PHYSICAL_ADDRESS BaseAddress,
+ IN UINT64 Length,
+ IN UINT64 Attributes
+ )
+{
+ EFI_STATUS Status;
+
+ DEBUG ((
+ DEBUG_INFO,
+ "%a: BaseAddress == 0x%lx, Length == 0x%lx, Attributes == 0x%lx\n",
+ __FUNCTION__,
+ (UINTN)BaseAddress,
+ (UINTN)Length,
+ (UINTN)Attributes
+ ));
+
+ if ((Length == 0) ||
+ ((Attributes & ~(EFI_MEMORY_RO | EFI_MEMORY_RP | EFI_MEMORY_XP)) != 0))
+ {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ if ((Attributes & EFI_MEMORY_RP) != 0) {
+ Status = ArmSetMemoryRegionNoAccess (BaseAddress, Length);
+ if (EFI_ERROR (Status)) {
+ return EFI_UNSUPPORTED;
+ }
+ }
+
+ if ((Attributes & EFI_MEMORY_RO) != 0) {
+ Status = ArmSetMemoryRegionReadOnly (BaseAddress, Length);
+ if (EFI_ERROR (Status)) {
+ return EFI_UNSUPPORTED;
+ }
+ }
+
+ if ((Attributes & EFI_MEMORY_XP) != 0) {
+ Status = ArmSetMemoryRegionNoExec (BaseAddress, Length);
+ if (EFI_ERROR (Status)) {
+ return EFI_UNSUPPORTED;
+ }
+ }
+
+ return EFI_SUCCESS;
+}
+
+/**
+ This function clears given attributes of the memory region specified by
+ BaseAddress and Length.
+
+ The valid Attributes is EFI_MEMORY_RP, EFI_MEMORY_XP, and EFI_MEMORY_RO.
+
+ @param This The EFI_MEMORY_ATTRIBUTE_PROTOCOL instance.
+ @param BaseAddress The physical address that is the start address of
+ a memory region.
+ @param Length The size in bytes of the memory region.
+ @param Attributes The bit mask of attributes to clear for the memory
+ region.
+
+ @retval EFI_SUCCESS The attributes were cleared for the memory region.
+ @retval EFI_INVALID_PARAMETER Length is zero.
+ Attributes specified an illegal combination of
+ attributes that cannot be cleared together.
+ @retval EFI_UNSUPPORTED The processor does not support one or more
+ bytes of the memory resource range specified
+ by BaseAddress and Length.
+ The bit mask of attributes is not supported for
+ the memory resource range specified by
+ BaseAddress and Length.
+ @retval EFI_OUT_OF_RESOURCES Requested attributes cannot be applied due to
+ lack of system resources.
+ @retval EFI_ACCESS_DENIED Attributes for the requested memory region are
+ controlled by system firmware and cannot be
+ updated via the protocol.
+
+**/
+STATIC
+EFI_STATUS
+ClearMemoryAttributes (
+ IN EFI_MEMORY_ATTRIBUTE_PROTOCOL *This,
+ IN EFI_PHYSICAL_ADDRESS BaseAddress,
+ IN UINT64 Length,
+ IN UINT64 Attributes
+ )
+{
+ EFI_STATUS Status;
+
+ DEBUG ((
+ DEBUG_INFO,
+ "%a: BaseAddress == 0x%lx, Length == 0x%lx, Attributes == 0x%lx\n",
+ __FUNCTION__,
+ (UINTN)BaseAddress,
+ (UINTN)Length,
+ (UINTN)Attributes
+ ));
+
+ if ((Length == 0) ||
+ ((Attributes & ~(EFI_MEMORY_RO | EFI_MEMORY_RP | EFI_MEMORY_XP)) != 0))
+ {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ if ((Attributes & EFI_MEMORY_RP) != 0) {
+ Status = ArmClearMemoryRegionNoAccess (BaseAddress, Length);
+ if (EFI_ERROR (Status)) {
+ return EFI_UNSUPPORTED;
+ }
+ }
+
+ if ((Attributes & EFI_MEMORY_RO) != 0) {
+ Status = ArmClearMemoryRegionReadOnly (BaseAddress, Length);
+ if (EFI_ERROR (Status)) {
+ return EFI_UNSUPPORTED;
+ }
+ }
+
+ if ((Attributes & EFI_MEMORY_XP) != 0) {
+ Status = ArmClearMemoryRegionNoExec (BaseAddress, Length);
+ if (EFI_ERROR (Status)) {
+ return EFI_UNSUPPORTED;
+ }
+ }
+
+ return EFI_SUCCESS;
+}
+
+EFI_MEMORY_ATTRIBUTE_PROTOCOL mMemoryAttribute = {
+ GetMemoryAttributes,
+ SetMemoryAttributes,
+ ClearMemoryAttributes
+};
--
2.39.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v4 00/11] ArmPkg: implement EFI memory attributes protocol
2023-02-09 13:59 [PATCH v4 00/11] ArmPkg: implement EFI memory attributes protocol Ard Biesheuvel
` (10 preceding siblings ...)
2023-02-09 13:59 ` [PATCH v4 11/11] ArmPkg/CpuDxe: Implement EFI memory attributes protocol Ard Biesheuvel
@ 2023-02-11 0:56 ` Taylor Beebe
2023-02-11 10:05 ` Ard Biesheuvel
11 siblings, 1 reply; 18+ messages in thread
From: Taylor Beebe @ 2023-02-11 0:56 UTC (permalink / raw)
To: Ard Biesheuvel, devel
Cc: Michael Kinney, Liming Gao, Jiewen Yao, Michael Kubacki,
Sean Brogan, Rebecca Cran, Leif Lindholm, Sami Mujawar
Hey Ard,
Once the Memory Attribute Protocol is made available, Windows will have
some expectations about its functionality. Can you run this test app
created by me and Jiewen to ensure it meets the Windows requirements?
Part of the test needed an AARCH64 implementation which I just added -
let me know if it doesn't work.
Thanks :)
https://github.com/TaylorBeebe/mu_basecore/tree/update_map_test_app/MdePkg/Test/ShellTest/MemoryAttributeProtocolFuncTestApp
On 2/9/2023 5:59 AM, Ard Biesheuvel wrote:
> v4:
>
> - major cleanup of the 32-bit ARM code
>
> - add support for EFI_MEMORY_RP using the access flag
>
> - enable stack guard in ArmVirtPkg (which uses EFI_MEMORY_RP)
>
> - incorporate optimization from other series [0] to avoid splitting
>
> block entries unnecessarily
>
>
>
> v3:
>
> - fix ARM32 bug in attribute conversion
>
> - add Liming's ack to patch #1
>
> - include draft patch (NOT FOR MERGE) used to test the changes
>
>
>
> v2:
>
> - drop patch to bump exposed UEFI revision to v2.10
>
> - add missing permitted return values to protocol definition
>
>
>
> [0] https://edk2.groups.io/g/devel/message/99801
>
>
>
> Cc: Michael Kinney <michael.d.kinney@intel.com>
>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
>
> Cc: Michael Kubacki <michael.kubacki@microsoft.com>
>
> Cc: Sean Brogan <sean.brogan@microsoft.com>
>
> Cc: Rebecca Cran <quic_rcran@quicinc.com>
>
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
>
> Cc: Taylor Beebe <t@taylorbeebe.com>
>
>
>
> Ard Biesheuvel (11):
>
> ArmPkg/ArmMmuLib ARM: Remove half baked large page support
>
> ArmPkg/ArmMmuLib ARM: Split off XN page descriptor bit from type field
>
> ArmPkg/CpuDxe ARM: Fix page-to-section attribute conversion
>
> ArmPkg/ArmMmuLib ARM: Isolate the access flag from AP mask
>
> ArmPkg/ArmMmuLib ARM: Clear individual permission bits
>
> ArmPkg/ArmMmuLib: Implement EFI_MEMORY_RP using access flag
>
> ArmVirtPkg: Enable stack guard
>
> ArmPkg/ArmMmuLib: Avoid splitting block entries if possible
>
> ArmPkg/CpuDxe: Expose unified region-to-EFI attribute conversion
>
> MdePkg: Add Memory Attribute Protocol definition
>
> ArmPkg/CpuDxe: Implement EFI memory attributes protocol
>
>
>
> ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c | 25 +-
>
> ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 96 +++++--
>
> ArmPkg/Drivers/CpuDxe/CpuDxe.c | 2 +
>
> ArmPkg/Drivers/CpuDxe/CpuDxe.h | 17 ++
>
> ArmPkg/Drivers/CpuDxe/CpuDxe.inf | 2 +
>
> ArmPkg/Drivers/CpuDxe/MemoryAttribute.c | 271 ++++++++++++++++++++
>
> ArmPkg/Include/Chipset/ArmV7Mmu.h | 88 +++----
>
> ArmPkg/Include/Library/ArmMmuLib.h | 34 +++
>
> ArmPkg/Library/ArmLib/Arm/ArmV7Support.S | 2 +
>
> ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 68 ++++-
>
> ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibConvert.c | 8 +-
>
> ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c | 2 +-
>
> ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c | 173 +++++++++++--
>
> ArmVirtPkg/ArmVirt.dsc.inc | 2 +
>
> MdePkg/Include/Protocol/MemoryAttribute.h | 142 ++++++++++
>
> MdePkg/MdePkg.dec | 3 +
>
> 16 files changed, 833 insertions(+), 102 deletions(-)
>
> create mode 100644 ArmPkg/Drivers/CpuDxe/MemoryAttribute.c
>
> create mode 100644 MdePkg/Include/Protocol/MemoryAttribute.h
>
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 00/11] ArmPkg: implement EFI memory attributes protocol
2023-02-11 0:56 ` [PATCH v4 00/11] ArmPkg: implement " Taylor Beebe
@ 2023-02-11 10:05 ` Ard Biesheuvel
2023-03-01 20:43 ` Taylor Beebe
0 siblings, 1 reply; 18+ messages in thread
From: Ard Biesheuvel @ 2023-02-11 10:05 UTC (permalink / raw)
To: Taylor Beebe
Cc: devel, Michael Kinney, Liming Gao, Jiewen Yao, Michael Kubacki,
Sean Brogan, Rebecca Cran, Leif Lindholm, Sami Mujawar
On Sat, 11 Feb 2023 at 01:56, Taylor Beebe <t@taylorbeebe.com> wrote:
>
> Hey Ard,
>
> Once the Memory Attribute Protocol is made available, Windows will have
> some expectations about its functionality. Can you run this test app
> created by me and Jiewen to ensure it meets the Windows requirements?
> Part of the test needed an AARCH64 implementation which I just added -
> let me know if it doesn't work.
>
Thanks, this is rather helpful.
There appears to be an issue related to
DEBUG_PROPERTY_DEBUG_CLEAR_MEMORY_ENABLED so I had to disable that to
run these tests, as otherwise, the DXE core tries to clear freed pages
before restoring the memory attributes.
With that out of the way, the only test that fails is 'New
EfiLoaderCode buffer attributes expected' because this firmware build
maps loader code RWX, as existing boot stages for Linux are relying on
this (including the kernel itself at this point)
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 00/11] ArmPkg: implement EFI memory attributes protocol
2023-02-11 10:05 ` Ard Biesheuvel
@ 2023-03-01 20:43 ` Taylor Beebe
2023-03-01 21:57 ` Ard Biesheuvel
0 siblings, 1 reply; 18+ messages in thread
From: Taylor Beebe @ 2023-03-01 20:43 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: devel, Michael Kinney, Liming Gao, Jiewen Yao, Michael Kubacki,
Sean Brogan, Rebecca Cran, Leif Lindholm, Sami Mujawar
On 2/11/2023 2:05 AM, Ard Biesheuvel wrote:
> On Sat, 11 Feb 2023 at 01:56, Taylor Beebe <t@taylorbeebe.com> wrote:
>>
>> Hey Ard,
>>
>> Once the Memory Attribute Protocol is made available, Windows will have
>> some expectations about its functionality. Can you run this test app
>> created by me and Jiewen to ensure it meets the Windows requirements?
>> Part of the test needed an AARCH64 implementation which I just added -
>> let me know if it doesn't work.
>>
>
> Thanks, this is rather helpful.
>
> There appears to be an issue related to
> DEBUG_PROPERTY_DEBUG_CLEAR_MEMORY_ENABLED so I had to disable that to
> run these tests, as otherwise, the DXE core tries to clear freed pages
> before restoring the memory attributes.
>
> With that out of the way, the only test that fails is 'New
> EfiLoaderCode buffer attributes expected' because this firmware build
> maps loader code RWX, as existing boot stages for Linux are relying on
> this (including the kernel itself at this point)
It makes sense that the NewEfiLoaderCode test fails, but I am surprised
the FreePagesWithProtectionAttributesTestCase passes. The test ensures
that a page with EFI_MEMORY_RP and/or EFI_MEMORY_RO has those attributes
cleared before attempting to free the page within the FreePage routine
and is related to the concern Marvin had.
Did you make a change to the core or is there an execution path I'm not
seeing which allows that test to pass?
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 00/11] ArmPkg: implement EFI memory attributes protocol
2023-03-01 20:43 ` Taylor Beebe
@ 2023-03-01 21:57 ` Ard Biesheuvel
2023-03-08 17:24 ` Taylor Beebe
0 siblings, 1 reply; 18+ messages in thread
From: Ard Biesheuvel @ 2023-03-01 21:57 UTC (permalink / raw)
To: Taylor Beebe
Cc: devel, Michael Kinney, Liming Gao, Jiewen Yao, Michael Kubacki,
Sean Brogan, Rebecca Cran, Leif Lindholm, Sami Mujawar
On Wed, 1 Mar 2023 at 21:43, Taylor Beebe <t@taylorbeebe.com> wrote:
>
>
>
> On 2/11/2023 2:05 AM, Ard Biesheuvel wrote:
> > On Sat, 11 Feb 2023 at 01:56, Taylor Beebe <t@taylorbeebe.com> wrote:
> >>
> >> Hey Ard,
> >>
> >> Once the Memory Attribute Protocol is made available, Windows will have
> >> some expectations about its functionality. Can you run this test app
> >> created by me and Jiewen to ensure it meets the Windows requirements?
> >> Part of the test needed an AARCH64 implementation which I just added -
> >> let me know if it doesn't work.
> >>
> >
> > Thanks, this is rather helpful.
> >
> > There appears to be an issue related to
> > DEBUG_PROPERTY_DEBUG_CLEAR_MEMORY_ENABLED so I had to disable that to
> > run these tests, as otherwise, the DXE core tries to clear freed pages
> > before restoring the memory attributes.
> >
> > With that out of the way, the only test that fails is 'New
> > EfiLoaderCode buffer attributes expected' because this firmware build
> > maps loader code RWX, as existing boot stages for Linux are relying on
> > this (including the kernel itself at this point)
>
> It makes sense that the NewEfiLoaderCode test fails, but I am surprised
> the FreePagesWithProtectionAttributesTestCase passes. The test ensures
> that a page with EFI_MEMORY_RP and/or EFI_MEMORY_RO has those attributes
> cleared before attempting to free the page within the FreePage routine
> and is related to the concern Marvin had.
>
> Did you make a change to the core or is there an execution path I'm not
> seeing which allows that test to pass?
No, I didn't make any additional changes to the core afair.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 00/11] ArmPkg: implement EFI memory attributes protocol
2023-03-01 21:57 ` Ard Biesheuvel
@ 2023-03-08 17:24 ` Taylor Beebe
2023-03-13 14:23 ` Ard Biesheuvel
0 siblings, 1 reply; 18+ messages in thread
From: Taylor Beebe @ 2023-03-08 17:24 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: devel, Michael Kinney, Liming Gao, Jiewen Yao, Michael Kubacki,
Sean Brogan, Rebecca Cran, Leif Lindholm, Sami Mujawar
My mistake - the DEBUG_PROPERTY_DEBUG_CLEAR_MEMORY_ENABLED feature is
why FreePagesWithProtectionAttributesTestCase might fail.
To make the clear memory feature more compatible with the memory
attribute protocol, can you add a check to DebugClearMemoryEnabled() on
free and clear EFI_MEMORY_RP and EFI_MEMORY_RO if they're set?
-Taylor
On 3/1/2023 1:57 PM, Ard Biesheuvel wrote:
> On Wed, 1 Mar 2023 at 21:43, Taylor Beebe <t@taylorbeebe.com> wrote:
>>
>>
>>
>> On 2/11/2023 2:05 AM, Ard Biesheuvel wrote:
>>> On Sat, 11 Feb 2023 at 01:56, Taylor Beebe <t@taylorbeebe.com> wrote:
>>>>
>>>> Hey Ard,
>>>>
>>>> Once the Memory Attribute Protocol is made available, Windows will have
>>>> some expectations about its functionality. Can you run this test app
>>>> created by me and Jiewen to ensure it meets the Windows requirements?
>>>> Part of the test needed an AARCH64 implementation which I just added -
>>>> let me know if it doesn't work.
>>>>
>>>
>>> Thanks, this is rather helpful.
>>>
>>> There appears to be an issue related to
>>> DEBUG_PROPERTY_DEBUG_CLEAR_MEMORY_ENABLED so I had to disable that to
>>> run these tests, as otherwise, the DXE core tries to clear freed pages
>>> before restoring the memory attributes.
>>>
>>> With that out of the way, the only test that fails is 'New
>>> EfiLoaderCode buffer attributes expected' because this firmware build
>>> maps loader code RWX, as existing boot stages for Linux are relying on
>>> this (including the kernel itself at this point)
>>
>> It makes sense that the NewEfiLoaderCode test fails, but I am surprised
>> the FreePagesWithProtectionAttributesTestCase passes. The test ensures
>> that a page with EFI_MEMORY_RP and/or EFI_MEMORY_RO has those attributes
>> cleared before attempting to free the page within the FreePage routine
>> and is related to the concern Marvin had.
>>
>> Did you make a change to the core or is there an execution path I'm not
>> seeing which allows that test to pass?
>
> No, I didn't make any additional changes to the core afair.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 00/11] ArmPkg: implement EFI memory attributes protocol
2023-03-08 17:24 ` Taylor Beebe
@ 2023-03-13 14:23 ` Ard Biesheuvel
0 siblings, 0 replies; 18+ messages in thread
From: Ard Biesheuvel @ 2023-03-13 14:23 UTC (permalink / raw)
To: Taylor Beebe
Cc: devel, Michael Kinney, Liming Gao, Jiewen Yao, Michael Kubacki,
Sean Brogan, Rebecca Cran, Leif Lindholm, Sami Mujawar
On Wed, 8 Mar 2023 at 18:24, Taylor Beebe <t@taylorbeebe.com> wrote:
>
> My mistake - the DEBUG_PROPERTY_DEBUG_CLEAR_MEMORY_ENABLED feature is
> why FreePagesWithProtectionAttributesTestCase might fail.
>
> To make the clear memory feature more compatible with the memory
> attribute protocol, can you add a check to DebugClearMemoryEnabled() on
> free and clear EFI_MEMORY_RP and EFI_MEMORY_RO if they're set?
>
I think the best strategy here is to simply apply the default
permissions first, and only then actually free the pages, clear them
etc etc That also fixes the existing issue where we may remap fewer
pages than what we actually freed. I'll add a patch to my series for
this.
>
> On 3/1/2023 1:57 PM, Ard Biesheuvel wrote:
> > On Wed, 1 Mar 2023 at 21:43, Taylor Beebe <t@taylorbeebe.com> wrote:
> >>
> >>
> >>
> >> On 2/11/2023 2:05 AM, Ard Biesheuvel wrote:
> >>> On Sat, 11 Feb 2023 at 01:56, Taylor Beebe <t@taylorbeebe.com> wrote:
> >>>>
> >>>> Hey Ard,
> >>>>
> >>>> Once the Memory Attribute Protocol is made available, Windows will have
> >>>> some expectations about its functionality. Can you run this test app
> >>>> created by me and Jiewen to ensure it meets the Windows requirements?
> >>>> Part of the test needed an AARCH64 implementation which I just added -
> >>>> let me know if it doesn't work.
> >>>>
> >>>
> >>> Thanks, this is rather helpful.
> >>>
> >>> There appears to be an issue related to
> >>> DEBUG_PROPERTY_DEBUG_CLEAR_MEMORY_ENABLED so I had to disable that to
> >>> run these tests, as otherwise, the DXE core tries to clear freed pages
> >>> before restoring the memory attributes.
> >>>
> >>> With that out of the way, the only test that fails is 'New
> >>> EfiLoaderCode buffer attributes expected' because this firmware build
> >>> maps loader code RWX, as existing boot stages for Linux are relying on
> >>> this (including the kernel itself at this point)
> >>
> >> It makes sense that the NewEfiLoaderCode test fails, but I am surprised
> >> the FreePagesWithProtectionAttributesTestCase passes. The test ensures
> >> that a page with EFI_MEMORY_RP and/or EFI_MEMORY_RO has those attributes
> >> cleared before attempting to free the page within the FreePage routine
> >> and is related to the concern Marvin had.
> >>
> >> Did you make a change to the core or is there an execution path I'm not
> >> seeing which allows that test to pass?
> >
> > No, I didn't make any additional changes to the core afair.
>
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2023-03-13 14:24 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-02-09 13:59 [PATCH v4 00/11] ArmPkg: implement EFI memory attributes protocol Ard Biesheuvel
2023-02-09 13:59 ` [PATCH v4 01/11] ArmPkg/ArmMmuLib ARM: Remove half baked large page support Ard Biesheuvel
2023-02-09 13:59 ` [PATCH v4 02/11] ArmPkg/ArmMmuLib ARM: Split off XN page descriptor bit from type field Ard Biesheuvel
2023-02-09 13:59 ` [PATCH v4 03/11] ArmPkg/CpuDxe ARM: Fix page-to-section attribute conversion Ard Biesheuvel
2023-02-09 13:59 ` [PATCH v4 04/11] ArmPkg/ArmMmuLib ARM: Isolate the access flag from AP mask Ard Biesheuvel
2023-02-09 13:59 ` [PATCH v4 05/11] ArmPkg/ArmMmuLib ARM: Clear individual permission bits Ard Biesheuvel
2023-02-09 13:59 ` [PATCH v4 06/11] ArmPkg/ArmMmuLib: Implement EFI_MEMORY_RP using access flag Ard Biesheuvel
2023-02-09 13:59 ` [PATCH v4 07/11] ArmVirtPkg: Enable stack guard Ard Biesheuvel
2023-02-09 13:59 ` [PATCH v4 08/11] ArmPkg/ArmMmuLib: Avoid splitting block entries if possible Ard Biesheuvel
2023-02-09 13:59 ` [PATCH v4 09/11] ArmPkg/CpuDxe: Expose unified region-to-EFI attribute conversion Ard Biesheuvel
2023-02-09 13:59 ` [PATCH v4 10/11] MdePkg: Add Memory Attribute Protocol definition Ard Biesheuvel
2023-02-09 13:59 ` [PATCH v4 11/11] ArmPkg/CpuDxe: Implement EFI memory attributes protocol Ard Biesheuvel
2023-02-11 0:56 ` [PATCH v4 00/11] ArmPkg: implement " Taylor Beebe
2023-02-11 10:05 ` Ard Biesheuvel
2023-03-01 20:43 ` Taylor Beebe
2023-03-01 21:57 ` Ard Biesheuvel
2023-03-08 17:24 ` Taylor Beebe
2023-03-13 14:23 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox