* [RFC PATCH 01/10] ArmPkg/ArmMmuLib: Extend API to manage memory permissions better
2023-05-25 14:30 [RFC PATCH 00/10] Add PPI to manage PEI phase memory attributes Ard Biesheuvel
@ 2023-05-25 14:30 ` Ard Biesheuvel
2023-05-25 14:30 ` [RFC PATCH 02/10] ArmPkg/CpuDxe: Simplify memory attributes protocol implementation Ard Biesheuvel
` (9 subsequent siblings)
10 siblings, 0 replies; 31+ messages in thread
From: Ard Biesheuvel @ 2023-05-25 14:30 UTC (permalink / raw)
To: devel
Cc: Ard Biesheuvel, Ray Ni, Jiewen Yao, Gerd Hoffmann, Taylor Beebe,
Oliver Smith-Denny, Dandan Bi, Liming Gao, Kinney, Michael D,
Leif Lindholm, Sunil V L, Andrei Warkentin
Currently, ArmSetMemoryAttributes () takes a combination of
EFI_MEMORY_xx constants describing the memory type and permission
attributes that should be set on a region of memory. In cases where the
memory type is omitted, we assume that the memory permissions being set
are final, and that existing memory permissions can be discarded.
This is problematic, because we aim to map memory non-executable
(EFI_MEMORY_XP) by default, and only relax this requirement for code
regions that are mapped read-only (EFI_MEMORY_RO). Currently, setting
one permission clears the other, and so code managing these permissions
has to be aware of the existing permissions in order to be able to
preserve them, and this is not always tractable (e.g., the UEFI memory
attribute protocol implements an abstraction that promises to preserve
memory permissions that it is not operating on explicitly).
So let's add an AttributeMask parameter to ArmSetMemoryAttributes(),
which is permitted to be non-zero if no memory type is being provided,
in which case only memory permission attributes covered in the mask will
be affected by the update.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c | 2 +-
ArmPkg/Include/Library/ArmMmuLib.h | 36 +++++++-
ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 52 +++++++++++-
ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c | 88 +++++++++++++++++---
ArmPkg/Library/OpteeLib/Optee.c | 2 +-
5 files changed, 165 insertions(+), 15 deletions(-)
diff --git a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
index 2e73719dce04ceb5..2d60c7d24dc05ee9 100644
--- a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
+++ b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
@@ -217,7 +217,7 @@ CpuSetMemoryAttributes (
if (EFI_ERROR (Status) || (RegionArmAttributes != ArmAttributes) ||
((BaseAddress + Length) > (RegionBaseAddress + RegionLength)))
{
- return ArmSetMemoryAttributes (BaseAddress, Length, EfiAttributes);
+ return ArmSetMemoryAttributes (BaseAddress, Length, EfiAttributes, 0);
} else {
return EFI_SUCCESS;
}
diff --git a/ArmPkg/Include/Library/ArmMmuLib.h b/ArmPkg/Include/Library/ArmMmuLib.h
index 4cf59a1e376b123c..91d112314fdf4859 100644
--- a/ArmPkg/Include/Library/ArmMmuLib.h
+++ b/ArmPkg/Include/Library/ArmMmuLib.h
@@ -92,11 +92,45 @@ ArmReplaceLiveTranslationEntry (
IN BOOLEAN DisableMmu
);
+/**
+ Set the requested memory permission attributes on a region of memory.
+
+ BaseAddress and Length must be aligned to EFI_PAGE_SIZE.
+
+ If Attributes contains a memory type attribute (EFI_MEMORY_UC/WC/WT/WB), the
+ region is mapped according to this memory type, and additional memory
+ permission attributes (EFI_MEMORY_RP/RO/XP) are taken into account as well,
+ discarding any permission attributes that are currently set for the region.
+ AttributeMask is ignored in this case, and must be set to 0x0.
+
+ If Attributes contains only a combination of memory permission attributes
+ (EFI_MEMORY_RP/RO/XP), each page in the region will retain its existing
+ memory type, even if it is not uniformly set across the region. In this case,
+ AttributesMask may be set to a mask of permission attributes, and memory
+ permissions omitted from this mask will not be updated for any page in the
+ region. All attributes appearing in Attributes must appear in AttributeMask
+ as well. (Attributes & ~AttributeMask must produce 0x0)
+
+ @param[in] BaseAddress The physical address that is the start address of
+ a memory region.
+ @param[in] Length The size in bytes of the memory region.
+ @param[in] Attributes Mask of memory attributes to set.
+ @param[in] AttributeMask Mask of memory attributes to take into account.
+
+ @retval EFI_SUCCESS The attributes were set for the memory region.
+ @retval EFI_INVALID_PARAMETER BaseAddress or Length is not suitably aligned.
+ Invalid combination of Attributes and
+ AttributeMask.
+ @retval EFI_OUT_OF_RESOURCES Requested attributes cannot be applied due to
+ lack of system resources.
+
+**/
EFI_STATUS
ArmSetMemoryAttributes (
IN EFI_PHYSICAL_ADDRESS BaseAddress,
IN UINT64 Length,
- IN UINT64 Attributes
+ IN UINT64 Attributes,
+ IN UINT64 AttributeMask
);
#endif // ARM_MMU_LIB_H_
diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index 7ed758fbbc699732..22623572b9cb931c 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -469,11 +469,45 @@ GcdAttributeToPageAttribute (
return PageAttributes;
}
+/**
+ Set the requested memory permission attributes on a region of memory.
+
+ BaseAddress and Length must be aligned to EFI_PAGE_SIZE.
+
+ If Attributes contains a memory type attribute (EFI_MEMORY_UC/WC/WT/WB), the
+ region is mapped according to this memory type, and additional memory
+ permission attributes (EFI_MEMORY_RP/RO/XP) are taken into account as well,
+ discarding any permission attributes that are currently set for the region.
+ AttributeMask is ignored in this case, and must be set to 0x0.
+
+ If Attributes contains only a combination of memory permission attributes
+ (EFI_MEMORY_RP/RO/XP), each page in the region will retain its existing
+ memory type, even if it is not uniformly set across the region. In this case,
+ AttributesMask may be set to a mask of permission attributes, and memory
+ permissions omitted from this mask will not be updated for any page in the
+ region. All attributes appearing in Attributes must appear in AttributeMask
+ as well. (Attributes & ~AttributeMask must produce 0x0)
+
+ @param[in] BaseAddress The physical address that is the start address of
+ a memory region.
+ @param[in] Length The size in bytes of the memory region.
+ @param[in] Attributes Mask of memory attributes to set.
+ @param[in] AttributeMask Mask of memory attributes to take into account.
+
+ @retval EFI_SUCCESS The attributes were set for the memory region.
+ @retval EFI_INVALID_PARAMETER BaseAddress or Length is not suitably aligned.
+ Invalid combination of Attributes and
+ AttributeMask.
+ @retval EFI_OUT_OF_RESOURCES Requested attributes cannot be applied due to
+ lack of system resources.
+
+**/
EFI_STATUS
ArmSetMemoryAttributes (
IN EFI_PHYSICAL_ADDRESS BaseAddress,
IN UINT64 Length,
- IN UINT64 Attributes
+ IN UINT64 Attributes,
+ IN UINT64 AttributeMask
)
{
UINT64 PageAttributes;
@@ -490,6 +524,22 @@ ArmSetMemoryAttributes (
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_AF);
+ if (AttributeMask != 0) {
+ if (((AttributeMask & ~(UINT64)(EFI_MEMORY_RP|EFI_MEMORY_RO|EFI_MEMORY_XP)) != 0) ||
+ ((Attributes & ~AttributeMask) != 0))
+ {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ // Add attributes omitted from AttributeMask to the set of attributes to preserve
+ PageAttributeMask |= GcdAttributeToPageAttribute (~AttributeMask) &
+ (TT_AP_MASK | TT_UXN_MASK | TT_PXN_MASK | TT_AF);
+ }
+ } else {
+ ASSERT (AttributeMask == 0);
+ if (AttributeMask != 0) {
+ return EFI_INVALID_PARAMETER;
+ }
}
return UpdateRegionMapping (
diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c
index 299d38ad07e85059..61405965a73eaeb8 100644
--- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c
+++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c
@@ -10,6 +10,7 @@
#include <Uefi.h>
#include <Library/ArmLib.h>
+#include <Library/ArmMmuLib.h>
#include <Library/BaseLib.h>
#include <Library/BaseMemoryLib.h>
#include <Library/DebugLib.h>
@@ -451,31 +452,96 @@ SetMemoryAttributes (
}
/**
- Update the permission or memory type attributes on a range of memory.
+ Set the requested memory permission attributes on a region 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.
+ BaseAddress and Length must be aligned to EFI_PAGE_SIZE.
- @retval EFI_SUCCESS The attributes were set successfully.
- @retval EFI_OUT_OF_RESOURCES The operation failed due to insufficient memory.
+ If Attributes contains a memory type attribute (EFI_MEMORY_UC/WC/WT/WB), the
+ region is mapped according to this memory type, and additional memory
+ permission attributes (EFI_MEMORY_RP/RO/XP) are taken into account as well,
+ discarding any permission attributes that are currently set for the region.
+ AttributeMask is ignored in this case, and must be set to 0x0.
+
+ If Attributes contains only a combination of memory permission attributes
+ (EFI_MEMORY_RP/RO/XP), each page in the region will retain its existing
+ memory type, even if it is not uniformly set across the region. In this case,
+ AttributesMask may be set to a mask of permission attributes, and memory
+ permissions omitted from this mask will not be updated for any page in the
+ region. All attributes appearing in Attributes must appear in AttributeMask
+ as well. (Attributes & ~AttributeMask must produce 0x0)
+
+ @param[in] BaseAddress The physical address that is the start address of
+ a memory region.
+ @param[in] Length The size in bytes of the memory region.
+ @param[in] Attributes Mask of memory attributes to set.
+ @param[in] AttributeMask Mask of memory attributes to take into account.
+
+ @retval EFI_SUCCESS The attributes were set for the memory region.
+ @retval EFI_INVALID_PARAMETER BaseAddress or Length is not suitably aligned.
+ Invalid combination of Attributes and
+ AttributeMask.
+ @retval EFI_OUT_OF_RESOURCES Requested attributes cannot be applied due to
+ lack of system resources.
**/
EFI_STATUS
ArmSetMemoryAttributes (
IN EFI_PHYSICAL_ADDRESS BaseAddress,
IN UINT64 Length,
- IN UINT64 Attributes
+ IN UINT64 Attributes,
+ IN UINT64 AttributeMask
)
{
+ UINT32 TtEntryMask;
+
+ if (((BaseAddress | Length) & EFI_PAGE_MASK) != 0) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ if ((Attributes & EFI_MEMORY_CACHETYPE_MASK) == 0) {
+ //
+ // No memory type was set in Attributes, so we are going to update the
+ // permissions only.
+ //
+ if (AttributeMask != 0) {
+ if (((AttributeMask & ~(UINT64)(EFI_MEMORY_RP|EFI_MEMORY_RO|EFI_MEMORY_XP)) != 0) ||
+ ((Attributes & ~AttributeMask) != 0))
+ {
+ return EFI_INVALID_PARAMETER;
+ }
+ } else {
+ AttributeMask = EFI_MEMORY_RP | EFI_MEMORY_RO | EFI_MEMORY_XP;
+ }
+
+ TtEntryMask = 0;
+ if ((AttributeMask & EFI_MEMORY_RP) != 0) {
+ TtEntryMask |= TT_DESCRIPTOR_SECTION_AF;
+ }
+
+ if ((AttributeMask & EFI_MEMORY_RO) != 0) {
+ TtEntryMask |= TT_DESCRIPTOR_SECTION_AP_MASK;
+ }
+
+ if ((AttributeMask & EFI_MEMORY_XP) != 0) {
+ TtEntryMask |= TT_DESCRIPTOR_SECTION_XN_MASK;
+ }
+ } else {
+ ASSERT (AttributeMask == 0);
+ if (AttributeMask != 0) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ TtEntryMask = TT_DESCRIPTOR_SECTION_TYPE_MASK |
+ TT_DESCRIPTOR_SECTION_XN_MASK |
+ TT_DESCRIPTOR_SECTION_AP_MASK |
+ TT_DESCRIPTOR_SECTION_AF;
+ }
+
return SetMemoryAttributes (
BaseAddress,
Length,
Attributes,
- TT_DESCRIPTOR_SECTION_TYPE_MASK |
- TT_DESCRIPTOR_SECTION_XN_MASK |
- TT_DESCRIPTOR_SECTION_AP_MASK |
- TT_DESCRIPTOR_SECTION_AF
+ TtEntryMask
);
}
diff --git a/ArmPkg/Library/OpteeLib/Optee.c b/ArmPkg/Library/OpteeLib/Optee.c
index 48e33cb3d5ee4ab6..46464f17ef06653e 100644
--- a/ArmPkg/Library/OpteeLib/Optee.c
+++ b/ArmPkg/Library/OpteeLib/Optee.c
@@ -86,7 +86,7 @@ OpteeSharedMemoryRemap (
return EFI_BUFFER_TOO_SMALL;
}
- Status = ArmSetMemoryAttributes (PhysicalAddress, Size, EFI_MEMORY_WB);
+ Status = ArmSetMemoryAttributes (PhysicalAddress, Size, EFI_MEMORY_WB, 0);
if (EFI_ERROR (Status)) {
return Status;
}
--
2.39.2
^ permalink raw reply related [flat|nested] 31+ messages in thread* [RFC PATCH 02/10] ArmPkg/CpuDxe: Simplify memory attributes protocol implementation
2023-05-25 14:30 [RFC PATCH 00/10] Add PPI to manage PEI phase memory attributes Ard Biesheuvel
2023-05-25 14:30 ` [RFC PATCH 01/10] ArmPkg/ArmMmuLib: Extend API to manage memory permissions better Ard Biesheuvel
@ 2023-05-25 14:30 ` Ard Biesheuvel
2023-05-25 14:30 ` [RFC PATCH 03/10] ArmPkg/CpuPei: Drop bogus DEPEX on PEI permanent memory Ard Biesheuvel
` (8 subsequent siblings)
10 siblings, 0 replies; 31+ messages in thread
From: Ard Biesheuvel @ 2023-05-25 14:30 UTC (permalink / raw)
To: devel
Cc: Ard Biesheuvel, Ray Ni, Jiewen Yao, Gerd Hoffmann, Taylor Beebe,
Oliver Smith-Denny, Dandan Bi, Liming Gao, Kinney, Michael D,
Leif Lindholm, Sunil V L, Andrei Warkentin
Now that ArmSetMemoryAttributes() permits a mask to be provided, we can
simplify the implementation the UEFI memory attribute protocol
substantially, and just pass on the requested mask to be set or cleared
directly.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
ArmPkg/Drivers/CpuDxe/MemoryAttribute.c | 50 +-------------------
1 file changed, 2 insertions(+), 48 deletions(-)
diff --git a/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c b/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c
index 61ba8fbbae4ee795..16cc4ef474f9772b 100644
--- a/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c
+++ b/ArmPkg/Drivers/CpuDxe/MemoryAttribute.c
@@ -183,8 +183,6 @@ SetMemoryAttributes (
IN UINT64 Attributes
)
{
- EFI_STATUS Status;
-
DEBUG ((
DEBUG_INFO,
"%a: BaseAddress == 0x%lx, Length == 0x%lx, Attributes == 0x%lx\n",
@@ -204,28 +202,7 @@ SetMemoryAttributes (
return EFI_UNSUPPORTED;
}
- 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;
+ return ArmSetMemoryAttributes (BaseAddress, Length, Attributes, Attributes);
}
/**
@@ -267,8 +244,6 @@ ClearMemoryAttributes (
IN UINT64 Attributes
)
{
- EFI_STATUS Status;
-
DEBUG ((
DEBUG_INFO,
"%a: BaseAddress == 0x%lx, Length == 0x%lx, Attributes == 0x%lx\n",
@@ -288,28 +263,7 @@ ClearMemoryAttributes (
return EFI_UNSUPPORTED;
}
- 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;
+ return ArmSetMemoryAttributes (BaseAddress, Length, 0, Attributes);
}
EFI_MEMORY_ATTRIBUTE_PROTOCOL mMemoryAttribute = {
--
2.39.2
^ permalink raw reply related [flat|nested] 31+ messages in thread* [RFC PATCH 03/10] ArmPkg/CpuPei: Drop bogus DEPEX on PEI permanent memory
2023-05-25 14:30 [RFC PATCH 00/10] Add PPI to manage PEI phase memory attributes Ard Biesheuvel
2023-05-25 14:30 ` [RFC PATCH 01/10] ArmPkg/ArmMmuLib: Extend API to manage memory permissions better Ard Biesheuvel
2023-05-25 14:30 ` [RFC PATCH 02/10] ArmPkg/CpuDxe: Simplify memory attributes protocol implementation Ard Biesheuvel
@ 2023-05-25 14:30 ` Ard Biesheuvel
2023-05-25 14:30 ` [RFC PATCH 04/10] OvmfPkg/RiscVVirt: Remove unimplemented NxForStack configuration Ard Biesheuvel
` (7 subsequent siblings)
10 siblings, 0 replies; 31+ messages in thread
From: Ard Biesheuvel @ 2023-05-25 14:30 UTC (permalink / raw)
To: devel
Cc: Ard Biesheuvel, Ray Ni, Jiewen Yao, Gerd Hoffmann, Taylor Beebe,
Oliver Smith-Denny, Dandan Bi, Liming Gao, Kinney, Michael D,
Leif Lindholm, Sunil V L, Andrei Warkentin
Currently, ARM's CPU PEIM depexes on PEI permanent memory being
installed, but functionally, it does not actually depend on that at all.
So let's drop the DEPEX.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
ArmPkg/Drivers/CpuPei/CpuPei.inf | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/ArmPkg/Drivers/CpuPei/CpuPei.inf b/ArmPkg/Drivers/CpuPei/CpuPei.inf
index a9f85cbc68b1c52e..648f27adf9402435 100644
--- a/ArmPkg/Drivers/CpuPei/CpuPei.inf
+++ b/ArmPkg/Drivers/CpuPei/CpuPei.inf
@@ -48,5 +48,4 @@ [FixedPcd]
gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize
[Depex]
- gEfiPeiMemoryDiscoveredPpiGuid
-
+ TRUE
--
2.39.2
^ permalink raw reply related [flat|nested] 31+ messages in thread* [RFC PATCH 04/10] OvmfPkg/RiscVVirt: Remove unimplemented NxForStack configuration
2023-05-25 14:30 [RFC PATCH 00/10] Add PPI to manage PEI phase memory attributes Ard Biesheuvel
` (2 preceding siblings ...)
2023-05-25 14:30 ` [RFC PATCH 03/10] ArmPkg/CpuPei: Drop bogus DEPEX on PEI permanent memory Ard Biesheuvel
@ 2023-05-25 14:30 ` Ard Biesheuvel
2023-05-29 12:50 ` Sunil V L
2023-05-25 14:30 ` [RFC PATCH 05/10] MdeModulePkg: Define memory attribute PPI Ard Biesheuvel
` (6 subsequent siblings)
10 siblings, 1 reply; 31+ messages in thread
From: Ard Biesheuvel @ 2023-05-25 14:30 UTC (permalink / raw)
To: devel
Cc: Ard Biesheuvel, Ray Ni, Jiewen Yao, Gerd Hoffmann, Taylor Beebe,
Oliver Smith-Denny, Dandan Bi, Liming Gao, Kinney, Michael D,
Leif Lindholm, Sunil V L, Andrei Warkentin
The RISC-V version of the DXE IPL does not implement setting the stack
NX, so before switching to an implementation that will ASSERT() on the
missing support, drop the PCD setting that enables it.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc | 6 ------
1 file changed, 6 deletions(-)
diff --git a/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc b/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc
index 28d9af4d79b9cc35..414d186179fb16e8 100644
--- a/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc
+++ b/OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc
@@ -172,12 +172,6 @@ [PcdsFixedAtBuild.common]
gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE
gEfiMdeModulePkgTokenSpaceGuid.PcdBootManagerMenuFile|{ 0x21, 0xaa, 0x2c, 0x46, 0x14, 0x76, 0x03, 0x45, 0x83, 0x6e, 0x8a, 0xb6, 0xf4, 0x66, 0x23, 0x31 }
-
- #
- # Enable the non-executable DXE stack. (This gets set up by DxeIpl)
- #
- gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|TRUE
-
!if $(SECURE_BOOT_ENABLE) == TRUE
# override the default values from SecurityPkg to ensure images from all sources are verified in secure boot
gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x04
--
2.39.2
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [RFC PATCH 04/10] OvmfPkg/RiscVVirt: Remove unimplemented NxForStack configuration
2023-05-25 14:30 ` [RFC PATCH 04/10] OvmfPkg/RiscVVirt: Remove unimplemented NxForStack configuration Ard Biesheuvel
@ 2023-05-29 12:50 ` Sunil V L
0 siblings, 0 replies; 31+ messages in thread
From: Sunil V L @ 2023-05-29 12:50 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: devel, Ray Ni, Jiewen Yao, Gerd Hoffmann, Taylor Beebe,
Oliver Smith-Denny, Dandan Bi, Liming Gao, Kinney, Michael D,
Leif Lindholm, Andrei Warkentin
On Thu, May 25, 2023 at 04:30:35PM +0200, Ard Biesheuvel wrote:
> The RISC-V version of the DXE IPL does not implement setting the stack
> NX, so before switching to an implementation that will ASSERT() on the
> missing support, drop the PCD setting that enables it.
>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
Reviewed-by: Sunil V L <sunilvl@ventanamicro.com>
Thanks!
Sunil
^ permalink raw reply [flat|nested] 31+ messages in thread
* [RFC PATCH 05/10] MdeModulePkg: Define memory attribute PPI
2023-05-25 14:30 [RFC PATCH 00/10] Add PPI to manage PEI phase memory attributes Ard Biesheuvel
` (3 preceding siblings ...)
2023-05-25 14:30 ` [RFC PATCH 04/10] OvmfPkg/RiscVVirt: Remove unimplemented NxForStack configuration Ard Biesheuvel
@ 2023-05-25 14:30 ` Ard Biesheuvel
2023-05-30 7:15 ` Ni, Ray
2023-05-25 14:30 ` [RFC PATCH 06/10] ArmPkg/CpuPei: Implement the memory attributes PPI Ard Biesheuvel
` (5 subsequent siblings)
10 siblings, 1 reply; 31+ messages in thread
From: Ard Biesheuvel @ 2023-05-25 14:30 UTC (permalink / raw)
To: devel
Cc: Ard Biesheuvel, Ray Ni, Jiewen Yao, Gerd Hoffmann, Taylor Beebe,
Oliver Smith-Denny, Dandan Bi, Liming Gao, Kinney, Michael D,
Leif Lindholm, Sunil V L, Andrei Warkentin
Define a PPI interface that may be used by the PEI core or other PEIMs
to manage permissions on memory ranges. This is primarily intended for
restricting permissions to what is actually needed for correct execution
by the code in question, and for limiting the use of memory mappings
that are both writable and executable at the same time.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
MdeModulePkg/Include/Ppi/MemoryAttribute.h | 78 ++++++++++++++++++++
MdeModulePkg/MdeModulePkg.dec | 3 +
2 files changed, 81 insertions(+)
diff --git a/MdeModulePkg/Include/Ppi/MemoryAttribute.h b/MdeModulePkg/Include/Ppi/MemoryAttribute.h
new file mode 100644
index 0000000000000000..5ff31185ab4183f8
--- /dev/null
+++ b/MdeModulePkg/Include/Ppi/MemoryAttribute.h
@@ -0,0 +1,78 @@
+/** @file
+
+Copyright (c) 2023, Google LLC. All rights reserved.<BR>
+
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#ifndef EDKII_MEMORY_ATTRIBUTE_PPI_H_
+#define EDKII_MEMORY_ATTRIBUTE_PPI_H_
+
+#include <Uefi/UefiSpec.h>
+
+///
+/// Global ID for the EDKII_MEMORY_ATTRIBUTE_PPI.
+///
+#define EDKII_MEMORY_ATTRIBUTE_PPI_GUID \
+ { \
+ 0x1be840de, 0x2d92, 0x41ec, { 0xb6, 0xd3, 0x19, 0x64, 0x13, 0x50, 0x51, 0xfb } \
+ }
+
+///
+/// Forward declaration for the EDKII_MEMORY_ATTRIBUTE_PPI.
+///
+typedef struct _EDKII_MEMORY_ATTRIBUTE_PPI EDKII_MEMORY_ATTRIBUTE_PPI;
+
+/**
+ Set the requested memory permission attributes on a region of memory.
+
+ BaseAddress and Length must be aligned to EFI_PAGE_SIZE.
+
+ Both SetMask and ClearMask may contain any combination of EFI_MEMORY_RP,
+ EFI_MEMORY_RO and EFI_MEMORY_XP, with the following restrictions:
+ - each constant may appear in either SetMask or ClearMask, but not in both;
+ - SetMask or ClearMask may be 0x0, but not both.
+
+ @param[in] This The protocol instance pointer.
+ @param[in] BaseAddress The physical address that is the start address of
+ a memory region.
+ @param[in] Length The size in bytes of the memory region.
+ @param[in] SetMask Mask of memory attributes to set.
+ @param[in] ClearMask Mask of memory attributes to clear.
+
+ @retval EFI_SUCCESS The attributes were set for the memory region.
+ @retval EFI_INVALID_PARAMETER Length is zero.
+ Invalid combination of SetMask and ClearMask.
+ BaseAddress or Length is not suitably aligned.
+ @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.
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI *EDKII_MEMORY_ATTRIBUTE_SET_PERMISSIONS)(
+ IN EDKII_MEMORY_ATTRIBUTE_PPI *This,
+ IN EFI_PHYSICAL_ADDRESS BaseAddress,
+ IN UINT64 Length,
+ IN UINT64 SetMask,
+ IN UINT64 ClearMask
+ );
+
+///
+/// This PPI contains a set of services to manage memory permission attributes.
+///
+struct _EDKII_MEMORY_ATTRIBUTE_PPI {
+ EDKII_MEMORY_ATTRIBUTE_SET_PERMISSIONS SetPermissions;
+};
+
+extern EFI_GUID gEdkiiMemoryAttributePpiGuid;
+
+#endif
+
diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index 95dd077e19b3a901..d65dae18aa81e569 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -528,6 +528,9 @@ [Ppis]
gEdkiiPeiCapsuleOnDiskPpiGuid = { 0x71a9ea61, 0x5a35, 0x4a5d, { 0xac, 0xef, 0x9c, 0xf8, 0x6d, 0x6d, 0x67, 0xe0 } }
gEdkiiPeiBootInCapsuleOnDiskModePpiGuid = { 0xb08a11e4, 0xe2b7, 0x4b75, { 0xb5, 0x15, 0xaf, 0x61, 0x6, 0x68, 0xbf, 0xd1 } }
+ ## Include/Ppi/MemoryAttribute.h
+ gEdkiiMemoryAttributePpiGuid = { 0x1be840de, 0x2d92, 0x41ec, { 0xb6, 0xd3, 0x19, 0x64, 0x13, 0x50, 0x51, 0xfb } }
+
[Protocols]
## Load File protocol provides capability to load and unload EFI image into memory and execute it.
# Include/Protocol/LoadPe32Image.h
--
2.39.2
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [RFC PATCH 05/10] MdeModulePkg: Define memory attribute PPI
2023-05-25 14:30 ` [RFC PATCH 05/10] MdeModulePkg: Define memory attribute PPI Ard Biesheuvel
@ 2023-05-30 7:15 ` Ni, Ray
2023-05-30 7:32 ` Ard Biesheuvel
0 siblings, 1 reply; 31+ messages in thread
From: Ni, Ray @ 2023-05-30 7:15 UTC (permalink / raw)
To: Ard Biesheuvel, devel@edk2.groups.io
Cc: Yao, Jiewen, Gerd Hoffmann, Taylor Beebe, Oliver Smith-Denny,
Bi, Dandan, Gao, Liming, Kinney, Michael D, Leif Lindholm,
Sunil V L, Warkentin, Andrei
1.
The PPI interface supports to set and clear any attributes with single invocation.
That's much better than the existing UEFI protocol prototype which requires caller to call the interfaces
twice to set and clear some attributes.
So far I see two patterns for attributes setting:
*. The patten in this patch: SetMask/ClearMask
*. The pattern I used in PageTableLib: Attribute/Mask.
I think from caller side, they provide the same capabilities.
The difference is SetMask/ClearMask expects callers to not set the same BIT in both
SetMask/ClearMask.
(I thought there would be similar existing interfaces as pattern 2. But I didn't find any now.)
Do you mind to align to pattern #2?
2.
When a memory region is marked from not-present to present, PageTableLib expects
caller to supply all memory attributes (including RW, NX, etc.) as the lib implementation doesn't
want to carry any default attributes..
Do you think the MemoryAttribute PPI should expect the same to caller?
Thanks,
Ray
> -----Original Message-----
> From: Ard Biesheuvel <ardb@kernel.org>
> Sent: Thursday, May 25, 2023 10:31 PM
> To: devel@edk2.groups.io
> Cc: Ard Biesheuvel <ardb@kernel.org>; Ni, Ray <ray.ni@intel.com>; Yao, Jiewen
> <jiewen.yao@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Taylor Beebe
> <t@taylorbeebe.com>; Oliver Smith-Denny <osd@smith-denny.com>; Bi, Dandan
> <dandan.bi@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Kinney,
> Michael D <michael.d.kinney@intel.com>; Leif Lindholm
> <quic_llindhol@quicinc.com>; Sunil V L <sunilvl@ventanamicro.com>; Warkentin,
> Andrei <andrei.warkentin@intel.com>
> Subject: [RFC PATCH 05/10] MdeModulePkg: Define memory attribute PPI
>
> Define a PPI interface that may be used by the PEI core or other PEIMs
> to manage permissions on memory ranges. This is primarily intended for
> restricting permissions to what is actually needed for correct execution
> by the code in question, and for limiting the use of memory mappings
> that are both writable and executable at the same time.
>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
> MdeModulePkg/Include/Ppi/MemoryAttribute.h | 78 ++++++++++++++++++++
> MdeModulePkg/MdeModulePkg.dec | 3 +
> 2 files changed, 81 insertions(+)
>
> diff --git a/MdeModulePkg/Include/Ppi/MemoryAttribute.h
> b/MdeModulePkg/Include/Ppi/MemoryAttribute.h
> new file mode 100644
> index 0000000000000000..5ff31185ab4183f8
> --- /dev/null
> +++ b/MdeModulePkg/Include/Ppi/MemoryAttribute.h
> @@ -0,0 +1,78 @@
> +/** @file
>
> +
>
> +Copyright (c) 2023, Google LLC. All rights reserved.<BR>
>
> +
>
> +SPDX-License-Identifier: BSD-2-Clause-Patent
>
> +
>
> +**/
>
> +
>
> +#ifndef EDKII_MEMORY_ATTRIBUTE_PPI_H_
>
> +#define EDKII_MEMORY_ATTRIBUTE_PPI_H_
>
> +
>
> +#include <Uefi/UefiSpec.h>
>
> +
>
> +///
>
> +/// Global ID for the EDKII_MEMORY_ATTRIBUTE_PPI.
>
> +///
>
> +#define EDKII_MEMORY_ATTRIBUTE_PPI_GUID \
>
> + { \
>
> + 0x1be840de, 0x2d92, 0x41ec, { 0xb6, 0xd3, 0x19, 0x64, 0x13, 0x50, 0x51,
> 0xfb } \
>
> + }
>
> +
>
> +///
>
> +/// Forward declaration for the EDKII_MEMORY_ATTRIBUTE_PPI.
>
> +///
>
> +typedef struct _EDKII_MEMORY_ATTRIBUTE_PPI
> EDKII_MEMORY_ATTRIBUTE_PPI;
>
> +
>
> +/**
>
> + Set the requested memory permission attributes on a region of memory.
>
> +
>
> + BaseAddress and Length must be aligned to EFI_PAGE_SIZE.
>
> +
>
> + Both SetMask and ClearMask may contain any combination of
> EFI_MEMORY_RP,
>
> + EFI_MEMORY_RO and EFI_MEMORY_XP, with the following restrictions:
>
> + - each constant may appear in either SetMask or ClearMask, but not in both;
>
> + - SetMask or ClearMask may be 0x0, but not both.
>
> +
>
> + @param[in] This The protocol instance pointer.
>
> + @param[in] BaseAddress The physical address that is the start address of
>
> + a memory region.
>
> + @param[in] Length The size in bytes of the memory region.
>
> + @param[in] SetMask Mask of memory attributes to set.
>
> + @param[in] ClearMask Mask of memory attributes to clear.
>
> +
>
> + @retval EFI_SUCCESS The attributes were set for the memory region.
>
> + @retval EFI_INVALID_PARAMETER Length is zero.
>
> + Invalid combination of SetMask and ClearMask.
>
> + BaseAddress or Length is not suitably aligned.
>
> + @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.
>
> +
>
> +**/
>
> +typedef
>
> +EFI_STATUS
>
> +(EFIAPI *EDKII_MEMORY_ATTRIBUTE_SET_PERMISSIONS)(
>
> + IN EDKII_MEMORY_ATTRIBUTE_PPI *This,
>
> + IN EFI_PHYSICAL_ADDRESS BaseAddress,
>
> + IN UINT64 Length,
>
> + IN UINT64 SetMask,
>
> + IN UINT64 ClearMask
>
> + );
>
> +
>
> +///
>
> +/// This PPI contains a set of services to manage memory permission attributes.
>
> +///
>
> +struct _EDKII_MEMORY_ATTRIBUTE_PPI {
>
> + EDKII_MEMORY_ATTRIBUTE_SET_PERMISSIONS SetPermissions;
>
> +};
>
> +
>
> +extern EFI_GUID gEdkiiMemoryAttributePpiGuid;
>
> +
>
> +#endif
>
> +
>
> diff --git a/MdeModulePkg/MdeModulePkg.dec
> b/MdeModulePkg/MdeModulePkg.dec
> index 95dd077e19b3a901..d65dae18aa81e569 100644
> --- a/MdeModulePkg/MdeModulePkg.dec
> +++ b/MdeModulePkg/MdeModulePkg.dec
> @@ -528,6 +528,9 @@ [Ppis]
> gEdkiiPeiCapsuleOnDiskPpiGuid = { 0x71a9ea61, 0x5a35, 0x4a5d, { 0xac,
> 0xef, 0x9c, 0xf8, 0x6d, 0x6d, 0x67, 0xe0 } }
>
> gEdkiiPeiBootInCapsuleOnDiskModePpiGuid = { 0xb08a11e4, 0xe2b7, 0x4b75,
> { 0xb5, 0x15, 0xaf, 0x61, 0x6, 0x68, 0xbf, 0xd1 } }
>
>
>
> + ## Include/Ppi/MemoryAttribute.h
>
> + gEdkiiMemoryAttributePpiGuid = { 0x1be840de, 0x2d92, 0x41ec, { 0xb6,
> 0xd3, 0x19, 0x64, 0x13, 0x50, 0x51, 0xfb } }
>
> +
>
> [Protocols]
>
> ## Load File protocol provides capability to load and unload EFI image into
> memory and execute it.
>
> # Include/Protocol/LoadPe32Image.h
>
> --
> 2.39.2
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [RFC PATCH 05/10] MdeModulePkg: Define memory attribute PPI
2023-05-30 7:15 ` Ni, Ray
@ 2023-05-30 7:32 ` Ard Biesheuvel
2023-05-31 7:33 ` Ni, Ray
0 siblings, 1 reply; 31+ messages in thread
From: Ard Biesheuvel @ 2023-05-30 7:32 UTC (permalink / raw)
To: Ni, Ray
Cc: devel@edk2.groups.io, Yao, Jiewen, Gerd Hoffmann, Taylor Beebe,
Oliver Smith-Denny, Bi, Dandan, Gao, Liming, Kinney, Michael D,
Leif Lindholm, Sunil V L, Warkentin, Andrei
On Tue, 30 May 2023 at 09:15, Ni, Ray <ray.ni@intel.com> wrote:
>
> 1.
> The PPI interface supports to set and clear any attributes with single invocation.
> That's much better than the existing UEFI protocol prototype which requires caller to call the interfaces
> twice to set and clear some attributes.
>
Agree, I think that was a mistake to define the UEFI memory attributes
protocol like that, as it requires two traversals of the page tables
for the most common case of converting RO -> XP or vice versa.
> So far I see two patterns for attributes setting:
> *. The patten in this patch: SetMask/ClearMask
> *. The pattern I used in PageTableLib: Attribute/Mask.
>
> I think from caller side, they provide the same capabilities.
> The difference is SetMask/ClearMask expects callers to not set the same BIT in both
> SetMask/ClearMask.
>
> (I thought there would be similar existing interfaces as pattern 2. But I didn't find any now.)
> Do you mind to align to pattern #2?
>
That is fine - I actually prefer that (and this is what ArmMmuLib
implements internally) but I did not want to deviate from the UEFI
protocol too much.
>
> 2.
> When a memory region is marked from not-present to present, PageTableLib expects
> caller to supply all memory attributes (including RW, NX, etc.) as the lib implementation doesn't
> want to carry any default attributes..
> Do you think the MemoryAttribute PPI should expect the same to caller?
>
I'm not sure I follow.
The PPI (as well as the UEFI protocol) can only operate on valid
mapping, and can only be used to manipulate RP/RO/XP. It cannot be
used to create mappings from scratch.
Do you think this capability should be added? If so, I think it is
reasonable to require the caller to provide all attributes, and on ARM
this would have to include the memory cacheability type as we should
not provide a default for that either.
Thanks,
Ard.
>
>
> > -----Original Message-----
> > From: Ard Biesheuvel <ardb@kernel.org>
> > Sent: Thursday, May 25, 2023 10:31 PM
> > To: devel@edk2.groups.io
> > Cc: Ard Biesheuvel <ardb@kernel.org>; Ni, Ray <ray.ni@intel.com>; Yao, Jiewen
> > <jiewen.yao@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Taylor Beebe
> > <t@taylorbeebe.com>; Oliver Smith-Denny <osd@smith-denny.com>; Bi, Dandan
> > <dandan.bi@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Kinney,
> > Michael D <michael.d.kinney@intel.com>; Leif Lindholm
> > <quic_llindhol@quicinc.com>; Sunil V L <sunilvl@ventanamicro.com>; Warkentin,
> > Andrei <andrei.warkentin@intel.com>
> > Subject: [RFC PATCH 05/10] MdeModulePkg: Define memory attribute PPI
> >
> > Define a PPI interface that may be used by the PEI core or other PEIMs
> > to manage permissions on memory ranges. This is primarily intended for
> > restricting permissions to what is actually needed for correct execution
> > by the code in question, and for limiting the use of memory mappings
> > that are both writable and executable at the same time.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> > MdeModulePkg/Include/Ppi/MemoryAttribute.h | 78 ++++++++++++++++++++
> > MdeModulePkg/MdeModulePkg.dec | 3 +
> > 2 files changed, 81 insertions(+)
> >
> > diff --git a/MdeModulePkg/Include/Ppi/MemoryAttribute.h
> > b/MdeModulePkg/Include/Ppi/MemoryAttribute.h
> > new file mode 100644
> > index 0000000000000000..5ff31185ab4183f8
> > --- /dev/null
> > +++ b/MdeModulePkg/Include/Ppi/MemoryAttribute.h
> > @@ -0,0 +1,78 @@
> > +/** @file
> >
> > +
> >
> > +Copyright (c) 2023, Google LLC. All rights reserved.<BR>
> >
> > +
> >
> > +SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> > +
> >
> > +**/
> >
> > +
> >
> > +#ifndef EDKII_MEMORY_ATTRIBUTE_PPI_H_
> >
> > +#define EDKII_MEMORY_ATTRIBUTE_PPI_H_
> >
> > +
> >
> > +#include <Uefi/UefiSpec.h>
> >
> > +
> >
> > +///
> >
> > +/// Global ID for the EDKII_MEMORY_ATTRIBUTE_PPI.
> >
> > +///
> >
> > +#define EDKII_MEMORY_ATTRIBUTE_PPI_GUID \
> >
> > + { \
> >
> > + 0x1be840de, 0x2d92, 0x41ec, { 0xb6, 0xd3, 0x19, 0x64, 0x13, 0x50, 0x51,
> > 0xfb } \
> >
> > + }
> >
> > +
> >
> > +///
> >
> > +/// Forward declaration for the EDKII_MEMORY_ATTRIBUTE_PPI.
> >
> > +///
> >
> > +typedef struct _EDKII_MEMORY_ATTRIBUTE_PPI
> > EDKII_MEMORY_ATTRIBUTE_PPI;
> >
> > +
> >
> > +/**
> >
> > + Set the requested memory permission attributes on a region of memory.
> >
> > +
> >
> > + BaseAddress and Length must be aligned to EFI_PAGE_SIZE.
> >
> > +
> >
> > + Both SetMask and ClearMask may contain any combination of
> > EFI_MEMORY_RP,
> >
> > + EFI_MEMORY_RO and EFI_MEMORY_XP, with the following restrictions:
> >
> > + - each constant may appear in either SetMask or ClearMask, but not in both;
> >
> > + - SetMask or ClearMask may be 0x0, but not both.
> >
> > +
> >
> > + @param[in] This The protocol instance pointer.
> >
> > + @param[in] BaseAddress The physical address that is the start address of
> >
> > + a memory region.
> >
> > + @param[in] Length The size in bytes of the memory region.
> >
> > + @param[in] SetMask Mask of memory attributes to set.
> >
> > + @param[in] ClearMask Mask of memory attributes to clear.
> >
> > +
> >
> > + @retval EFI_SUCCESS The attributes were set for the memory region.
> >
> > + @retval EFI_INVALID_PARAMETER Length is zero.
> >
> > + Invalid combination of SetMask and ClearMask.
> >
> > + BaseAddress or Length is not suitably aligned.
> >
> > + @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.
> >
> > +
> >
> > +**/
> >
> > +typedef
> >
> > +EFI_STATUS
> >
> > +(EFIAPI *EDKII_MEMORY_ATTRIBUTE_SET_PERMISSIONS)(
> >
> > + IN EDKII_MEMORY_ATTRIBUTE_PPI *This,
> >
> > + IN EFI_PHYSICAL_ADDRESS BaseAddress,
> >
> > + IN UINT64 Length,
> >
> > + IN UINT64 SetMask,
> >
> > + IN UINT64 ClearMask
> >
> > + );
> >
> > +
> >
> > +///
> >
> > +/// This PPI contains a set of services to manage memory permission attributes.
> >
> > +///
> >
> > +struct _EDKII_MEMORY_ATTRIBUTE_PPI {
> >
> > + EDKII_MEMORY_ATTRIBUTE_SET_PERMISSIONS SetPermissions;
> >
> > +};
> >
> > +
> >
> > +extern EFI_GUID gEdkiiMemoryAttributePpiGuid;
> >
> > +
> >
> > +#endif
> >
> > +
> >
> > diff --git a/MdeModulePkg/MdeModulePkg.dec
> > b/MdeModulePkg/MdeModulePkg.dec
> > index 95dd077e19b3a901..d65dae18aa81e569 100644
> > --- a/MdeModulePkg/MdeModulePkg.dec
> > +++ b/MdeModulePkg/MdeModulePkg.dec
> > @@ -528,6 +528,9 @@ [Ppis]
> > gEdkiiPeiCapsuleOnDiskPpiGuid = { 0x71a9ea61, 0x5a35, 0x4a5d, { 0xac,
> > 0xef, 0x9c, 0xf8, 0x6d, 0x6d, 0x67, 0xe0 } }
> >
> > gEdkiiPeiBootInCapsuleOnDiskModePpiGuid = { 0xb08a11e4, 0xe2b7, 0x4b75,
> > { 0xb5, 0x15, 0xaf, 0x61, 0x6, 0x68, 0xbf, 0xd1 } }
> >
> >
> >
> > + ## Include/Ppi/MemoryAttribute.h
> >
> > + gEdkiiMemoryAttributePpiGuid = { 0x1be840de, 0x2d92, 0x41ec, { 0xb6,
> > 0xd3, 0x19, 0x64, 0x13, 0x50, 0x51, 0xfb } }
> >
> > +
> >
> > [Protocols]
> >
> > ## Load File protocol provides capability to load and unload EFI image into
> > memory and execute it.
> >
> > # Include/Protocol/LoadPe32Image.h
> >
> > --
> > 2.39.2
>
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [RFC PATCH 05/10] MdeModulePkg: Define memory attribute PPI
2023-05-30 7:32 ` Ard Biesheuvel
@ 2023-05-31 7:33 ` Ni, Ray
2023-05-31 7:53 ` Ard Biesheuvel
0 siblings, 1 reply; 31+ messages in thread
From: Ni, Ray @ 2023-05-31 7:33 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: devel@edk2.groups.io, Yao, Jiewen, Gerd Hoffmann, Taylor Beebe,
Oliver Smith-Denny, Bi, Dandan, Gao, Liming, Kinney, Michael D,
Leif Lindholm, Sunil V L, Warkentin, Andrei
> -----Original Message-----
> From: Ard Biesheuvel <ardb@kernel.org>
> Sent: Tuesday, May 30, 2023 3:32 PM
> To: Ni, Ray <ray.ni@intel.com>
> Cc: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>; Gerd
> Hoffmann <kraxel@redhat.com>; Taylor Beebe <t@taylorbeebe.com>; Oliver
> Smith-Denny <osd@smith-denny.com>; Bi, Dandan <dandan.bi@intel.com>;
> Gao, Liming <gaoliming@byosoft.com.cn>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Leif Lindholm <quic_llindhol@quicinc.com>;
> Sunil V L <sunilvl@ventanamicro.com>; Warkentin, Andrei
> <andrei.warkentin@intel.com>
> Subject: Re: [RFC PATCH 05/10] MdeModulePkg: Define memory attribute PPI
>
> On Tue, 30 May 2023 at 09:15, Ni, Ray <ray.ni@intel.com> wrote:
> >
> > 1.
> > The PPI interface supports to set and clear any attributes with single
> invocation.
> > That's much better than the existing UEFI protocol prototype which requires
> caller to call the interfaces
> > twice to set and clear some attributes.
> >
>
> Agree, I think that was a mistake to define the UEFI memory attributes
> protocol like that, as it requires two traversals of the page tables
> for the most common case of converting RO -> XP or vice versa.
>
> > So far I see two patterns for attributes setting:
> > *. The patten in this patch: SetMask/ClearMask
> > *. The pattern I used in PageTableLib: Attribute/Mask.
> >
> > I think from caller side, they provide the same capabilities.
> > The difference is SetMask/ClearMask expects callers to not set the same BIT
> in both
> > SetMask/ClearMask.
> >
> > (I thought there would be similar existing interfaces as pattern 2. But I didn't
> find any now.)
> > Do you mind to align to pattern #2?
> >
>
> That is fine - I actually prefer that (and this is what ArmMmuLib
> implements internally) but I did not want to deviate from the UEFI
> protocol too much.
By adding "ClearMask", you already made something different😊
Good to know you prefer pattern #2.
>
> >
> > 2.
> > When a memory region is marked from not-present to present, PageTableLib
> expects
> > caller to supply all memory attributes (including RW, NX, etc.) as the lib
> implementation doesn't
> > want to carry any default attributes..
> > Do you think the MemoryAttribute PPI should expect the same to caller?
> >
>
> I'm not sure I follow.
>
> The PPI (as well as the UEFI protocol) can only operate on valid
> mapping, and can only be used to manipulate RP/RO/XP. It cannot be
> used to create mappings from scratch.
When a range of memory is marked as "RP", X86 page table clears the
"Present" bit for that range memory.
"Present" bit is a master bit in X86 page table. When that bit is clear, all
other bits ("Writable", "Non-Execution", etc.) are ignored by CPU.
So, if caller clears the "RP" bit (setting "Present" bit in page table), that's an
operation to map back some memory.
X86 CpuPageTableLib requires all attributes be provided for mapping back
some memory.
>
> Do you think this capability should be added? If so, I think it is
> reasonable to require the caller to provide all attributes, and on ARM
> this would have to include the memory cacheability type as we should
> not provide a default for that either.
Yes. I think this is required. Having this rule can help caller write robust code
instead of depending on some default attributes in PPI/Protocol implementation.
>
> Thanks,
> Ard.
>
>
> >
> >
> > > -----Original Message-----
> > > From: Ard Biesheuvel <ardb@kernel.org>
> > > Sent: Thursday, May 25, 2023 10:31 PM
> > > To: devel@edk2.groups.io
> > > Cc: Ard Biesheuvel <ardb@kernel.org>; Ni, Ray <ray.ni@intel.com>; Yao,
> Jiewen
> > > <jiewen.yao@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Taylor
> Beebe
> > > <t@taylorbeebe.com>; Oliver Smith-Denny <osd@smith-denny.com>; Bi,
> Dandan
> > > <dandan.bi@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>;
> Kinney,
> > > Michael D <michael.d.kinney@intel.com>; Leif Lindholm
> > > <quic_llindhol@quicinc.com>; Sunil V L <sunilvl@ventanamicro.com>;
> Warkentin,
> > > Andrei <andrei.warkentin@intel.com>
> > > Subject: [RFC PATCH 05/10] MdeModulePkg: Define memory attribute PPI
> > >
> > > Define a PPI interface that may be used by the PEI core or other PEIMs
> > > to manage permissions on memory ranges. This is primarily intended for
> > > restricting permissions to what is actually needed for correct execution
> > > by the code in question, and for limiting the use of memory mappings
> > > that are both writable and executable at the same time.
> > >
> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > > ---
> > > MdeModulePkg/Include/Ppi/MemoryAttribute.h | 78
> ++++++++++++++++++++
> > > MdeModulePkg/MdeModulePkg.dec | 3 +
> > > 2 files changed, 81 insertions(+)
> > >
> > > diff --git a/MdeModulePkg/Include/Ppi/MemoryAttribute.h
> > > b/MdeModulePkg/Include/Ppi/MemoryAttribute.h
> > > new file mode 100644
> > > index 0000000000000000..5ff31185ab4183f8
> > > --- /dev/null
> > > +++ b/MdeModulePkg/Include/Ppi/MemoryAttribute.h
> > > @@ -0,0 +1,78 @@
> > > +/** @file
> > >
> > > +
> > >
> > > +Copyright (c) 2023, Google LLC. All rights reserved.<BR>
> > >
> > > +
> > >
> > > +SPDX-License-Identifier: BSD-2-Clause-Patent
> > >
> > > +
> > >
> > > +**/
> > >
> > > +
> > >
> > > +#ifndef EDKII_MEMORY_ATTRIBUTE_PPI_H_
> > >
> > > +#define EDKII_MEMORY_ATTRIBUTE_PPI_H_
> > >
> > > +
> > >
> > > +#include <Uefi/UefiSpec.h>
> > >
> > > +
> > >
> > > +///
> > >
> > > +/// Global ID for the EDKII_MEMORY_ATTRIBUTE_PPI.
> > >
> > > +///
> > >
> > > +#define EDKII_MEMORY_ATTRIBUTE_PPI_GUID \
> > >
> > > + { \
> > >
> > > + 0x1be840de, 0x2d92, 0x41ec, { 0xb6, 0xd3, 0x19, 0x64, 0x13, 0x50,
> 0x51,
> > > 0xfb } \
> > >
> > > + }
> > >
> > > +
> > >
> > > +///
> > >
> > > +/// Forward declaration for the EDKII_MEMORY_ATTRIBUTE_PPI.
> > >
> > > +///
> > >
> > > +typedef struct _EDKII_MEMORY_ATTRIBUTE_PPI
> > > EDKII_MEMORY_ATTRIBUTE_PPI;
> > >
> > > +
> > >
> > > +/**
> > >
> > > + Set the requested memory permission attributes on a region of memory.
> > >
> > > +
> > >
> > > + BaseAddress and Length must be aligned to EFI_PAGE_SIZE.
> > >
> > > +
> > >
> > > + Both SetMask and ClearMask may contain any combination of
> > > EFI_MEMORY_RP,
> > >
> > > + EFI_MEMORY_RO and EFI_MEMORY_XP, with the following restrictions:
> > >
> > > + - each constant may appear in either SetMask or ClearMask, but not in
> both;
> > >
> > > + - SetMask or ClearMask may be 0x0, but not both.
> > >
> > > +
> > >
> > > + @param[in] This The protocol instance pointer.
> > >
> > > + @param[in] BaseAddress The physical address that is the start
> address of
> > >
> > > + a memory region.
> > >
> > > + @param[in] Length The size in bytes of the memory region.
> > >
> > > + @param[in] SetMask Mask of memory attributes to set.
> > >
> > > + @param[in] ClearMask Mask of memory attributes to clear.
> > >
> > > +
> > >
> > > + @retval EFI_SUCCESS The attributes were set for the memory
> region.
> > >
> > > + @retval EFI_INVALID_PARAMETER Length is zero.
> > >
> > > + Invalid combination of SetMask and ClearMask.
> > >
> > > + BaseAddress or Length is not suitably aligned.
> > >
> > > + @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.
> > >
> > > +
> > >
> > > +**/
> > >
> > > +typedef
> > >
> > > +EFI_STATUS
> > >
> > > +(EFIAPI *EDKII_MEMORY_ATTRIBUTE_SET_PERMISSIONS)(
> > >
> > > + IN EDKII_MEMORY_ATTRIBUTE_PPI *This,
> > >
> > > + IN EFI_PHYSICAL_ADDRESS BaseAddress,
> > >
> > > + IN UINT64 Length,
> > >
> > > + IN UINT64 SetMask,
> > >
> > > + IN UINT64 ClearMask
> > >
> > > + );
> > >
> > > +
> > >
> > > +///
> > >
> > > +/// This PPI contains a set of services to manage memory permission
> attributes.
> > >
> > > +///
> > >
> > > +struct _EDKII_MEMORY_ATTRIBUTE_PPI {
> > >
> > > + EDKII_MEMORY_ATTRIBUTE_SET_PERMISSIONS SetPermissions;
> > >
> > > +};
> > >
> > > +
> > >
> > > +extern EFI_GUID gEdkiiMemoryAttributePpiGuid;
> > >
> > > +
> > >
> > > +#endif
> > >
> > > +
> > >
> > > diff --git a/MdeModulePkg/MdeModulePkg.dec
> > > b/MdeModulePkg/MdeModulePkg.dec
> > > index 95dd077e19b3a901..d65dae18aa81e569 100644
> > > --- a/MdeModulePkg/MdeModulePkg.dec
> > > +++ b/MdeModulePkg/MdeModulePkg.dec
> > > @@ -528,6 +528,9 @@ [Ppis]
> > > gEdkiiPeiCapsuleOnDiskPpiGuid = { 0x71a9ea61, 0x5a35, 0x4a5d,
> { 0xac,
> > > 0xef, 0x9c, 0xf8, 0x6d, 0x6d, 0x67, 0xe0 } }
> > >
> > > gEdkiiPeiBootInCapsuleOnDiskModePpiGuid = { 0xb08a11e4, 0xe2b7,
> 0x4b75,
> > > { 0xb5, 0x15, 0xaf, 0x61, 0x6, 0x68, 0xbf, 0xd1 } }
> > >
> > >
> > >
> > > + ## Include/Ppi/MemoryAttribute.h
> > >
> > > + gEdkiiMemoryAttributePpiGuid = { 0x1be840de, 0x2d92, 0x41ec,
> { 0xb6,
> > > 0xd3, 0x19, 0x64, 0x13, 0x50, 0x51, 0xfb } }
> > >
> > > +
> > >
> > > [Protocols]
> > >
> > > ## Load File protocol provides capability to load and unload EFI image into
> > > memory and execute it.
> > >
> > > # Include/Protocol/LoadPe32Image.h
> > >
> > > --
> > > 2.39.2
> >
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [RFC PATCH 05/10] MdeModulePkg: Define memory attribute PPI
2023-05-31 7:33 ` Ni, Ray
@ 2023-05-31 7:53 ` Ard Biesheuvel
2023-05-31 8:56 ` [edk2-devel] " Ni, Ray
0 siblings, 1 reply; 31+ messages in thread
From: Ard Biesheuvel @ 2023-05-31 7:53 UTC (permalink / raw)
To: Ni, Ray
Cc: devel@edk2.groups.io, Yao, Jiewen, Gerd Hoffmann, Taylor Beebe,
Oliver Smith-Denny, Bi, Dandan, Gao, Liming, Kinney, Michael D,
Leif Lindholm, Sunil V L, Warkentin, Andrei
On Wed, 31 May 2023 at 09:34, Ni, Ray <ray.ni@intel.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Ard Biesheuvel <ardb@kernel.org>
> > Sent: Tuesday, May 30, 2023 3:32 PM
> > To: Ni, Ray <ray.ni@intel.com>
> > Cc: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>; Gerd
> > Hoffmann <kraxel@redhat.com>; Taylor Beebe <t@taylorbeebe.com>; Oliver
> > Smith-Denny <osd@smith-denny.com>; Bi, Dandan <dandan.bi@intel.com>;
> > Gao, Liming <gaoliming@byosoft.com.cn>; Kinney, Michael D
> > <michael.d.kinney@intel.com>; Leif Lindholm <quic_llindhol@quicinc.com>;
> > Sunil V L <sunilvl@ventanamicro.com>; Warkentin, Andrei
> > <andrei.warkentin@intel.com>
> > Subject: Re: [RFC PATCH 05/10] MdeModulePkg: Define memory attribute PPI
> >
> > On Tue, 30 May 2023 at 09:15, Ni, Ray <ray.ni@intel.com> wrote:
> > >
> > > 1.
> > > The PPI interface supports to set and clear any attributes with single
> > invocation.
> > > That's much better than the existing UEFI protocol prototype which requires
> > caller to call the interfaces
> > > twice to set and clear some attributes.
> > >
> >
> > Agree, I think that was a mistake to define the UEFI memory attributes
> > protocol like that, as it requires two traversals of the page tables
> > for the most common case of converting RO -> XP or vice versa.
> >
> > > So far I see two patterns for attributes setting:
> > > *. The patten in this patch: SetMask/ClearMask
> > > *. The pattern I used in PageTableLib: Attribute/Mask.
> > >
> > > I think from caller side, they provide the same capabilities.
> > > The difference is SetMask/ClearMask expects callers to not set the same BIT
> > in both
> > > SetMask/ClearMask.
> > >
> > > (I thought there would be similar existing interfaces as pattern 2. But I didn't
> > find any now.)
> > > Do you mind to align to pattern #2?
> > >
> >
> > That is fine - I actually prefer that (and this is what ArmMmuLib
> > implements internally) but I did not want to deviate from the UEFI
> > protocol too much.
>
> By adding "ClearMask", you already made something different😊
> Good to know you prefer pattern #2.
>
Yeah :-)
> >
> > >
> > > 2.
> > > When a memory region is marked from not-present to present, PageTableLib
> > expects
> > > caller to supply all memory attributes (including RW, NX, etc.) as the lib
> > implementation doesn't
> > > want to carry any default attributes..
> > > Do you think the MemoryAttribute PPI should expect the same to caller?
> > >
> >
> > I'm not sure I follow.
> >
> > The PPI (as well as the UEFI protocol) can only operate on valid
> > mapping, and can only be used to manipulate RP/RO/XP. It cannot be
> > used to create mappings from scratch.
> When a range of memory is marked as "RP", X86 page table clears the
> "Present" bit for that range memory.
> "Present" bit is a master bit in X86 page table. When that bit is clear, all
> other bits ("Writable", "Non-Execution", etc.) are ignored by CPU.
>
> So, if caller clears the "RP" bit (setting "Present" bit in page table), that's an
> operation to map back some memory.
> X86 CpuPageTableLib requires all attributes be provided for mapping back
> some memory.
>
> >
> > Do you think this capability should be added? If so, I think it is
> > reasonable to require the caller to provide all attributes, and on ARM
> > this would have to include the memory cacheability type as we should
> > not provide a default for that either.
>
> Yes. I think this is required. Having this rule can help caller write robust code
> instead of depending on some default attributes in PPI/Protocol implementation.
>
I still don't follow. How does that work in the context of the
attribute mask? Can you given some examples?
Creating new memory mappings from scratch is a totally different use
case, so perhaps this should be a separate PPI method.
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [edk2-devel] [RFC PATCH 05/10] MdeModulePkg: Define memory attribute PPI
2023-05-31 7:53 ` Ard Biesheuvel
@ 2023-05-31 8:56 ` Ni, Ray
2023-05-31 9:24 ` Ard Biesheuvel
0 siblings, 1 reply; 31+ messages in thread
From: Ni, Ray @ 2023-05-31 8:56 UTC (permalink / raw)
To: devel@edk2.groups.io, ardb@kernel.org
Cc: Yao, Jiewen, Gerd Hoffmann, Taylor Beebe, Oliver Smith-Denny,
Bi, Dandan, Gao, Liming, Kinney, Michael D, Leif Lindholm,
Sunil V L, Warkentin, Andrei
> > > > 2.
> > > > When a memory region is marked from not-present to present,
> PageTableLib
> > > expects
> > > > caller to supply all memory attributes (including RW, NX, etc.) as the lib
> > > implementation doesn't
> > > > want to carry any default attributes..
> > > > Do you think the MemoryAttribute PPI should expect the same to caller?
> > > >
> > >
> > > I'm not sure I follow.
> > >
> > > The PPI (as well as the UEFI protocol) can only operate on valid
> > > mapping, and can only be used to manipulate RP/RO/XP. It cannot be
> > > used to create mappings from scratch.
> > When a range of memory is marked as "RP", X86 page table clears the
> > "Present" bit for that range memory.
> > "Present" bit is a master bit in X86 page table. When that bit is clear, all
> > other bits ("Writable", "Non-Execution", etc.) are ignored by CPU.
> >
> > So, if caller clears the "RP" bit (setting "Present" bit in page table), that's an
> > operation to map back some memory.
> > X86 CpuPageTableLib requires all attributes be provided for mapping back
> > some memory.
> >
> > >
> > > Do you think this capability should be added? If so, I think it is
> > > reasonable to require the caller to provide all attributes, and on ARM
> > > this would have to include the memory cacheability type as we should
> > > not provide a default for that either.
> >
> > Yes. I think this is required. Having this rule can help caller write robust code
> > instead of depending on some default attributes in PPI/Protocol
> implementation.
> >
>
> I still don't follow. How does that work in the context of the
> attribute mask? Can you given some examples?
OK. Let's reset the discussion.
For example, one caller's code as below:
// mark 0-4k as not-present
MemoryAttrributePpi->SetMemoryAttribute (0, 4K, RP, RP); // Use Attribute/Mask pattern to set RP
Another caller's code:
// mark 0-4k as present
* MemoryAttributePpi->SetMemoryAttribute (0, 4K, 0, RP); // Use Attribute/Mask pattern to clear RP
Q1: Does the PPI support this usage?
Q2: If it supports, what're the other attributes of 0-4k memory? Is XP set? Is RO set?
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [edk2-devel] [RFC PATCH 05/10] MdeModulePkg: Define memory attribute PPI
2023-05-31 8:56 ` [edk2-devel] " Ni, Ray
@ 2023-05-31 9:24 ` Ard Biesheuvel
0 siblings, 0 replies; 31+ messages in thread
From: Ard Biesheuvel @ 2023-05-31 9:24 UTC (permalink / raw)
To: Ni, Ray
Cc: devel@edk2.groups.io, Yao, Jiewen, Gerd Hoffmann, Taylor Beebe,
Oliver Smith-Denny, Bi, Dandan, Gao, Liming, Kinney, Michael D,
Leif Lindholm, Sunil V L, Warkentin, Andrei
On Wed, 31 May 2023 at 10:56, Ni, Ray <ray.ni@intel.com> wrote:
>
> > > > > 2.
> > > > > When a memory region is marked from not-present to present,
> > PageTableLib
> > > > expects
> > > > > caller to supply all memory attributes (including RW, NX, etc.) as the lib
> > > > implementation doesn't
> > > > > want to carry any default attributes..
> > > > > Do you think the MemoryAttribute PPI should expect the same to caller?
> > > > >
> > > >
> > > > I'm not sure I follow.
> > > >
> > > > The PPI (as well as the UEFI protocol) can only operate on valid
> > > > mapping, and can only be used to manipulate RP/RO/XP. It cannot be
> > > > used to create mappings from scratch.
> > > When a range of memory is marked as "RP", X86 page table clears the
> > > "Present" bit for that range memory.
> > > "Present" bit is a master bit in X86 page table. When that bit is clear, all
> > > other bits ("Writable", "Non-Execution", etc.) are ignored by CPU.
> > >
> > > So, if caller clears the "RP" bit (setting "Present" bit in page table), that's an
> > > operation to map back some memory.
> > > X86 CpuPageTableLib requires all attributes be provided for mapping back
> > > some memory.
> > >
> > > >
> > > > Do you think this capability should be added? If so, I think it is
> > > > reasonable to require the caller to provide all attributes, and on ARM
> > > > this would have to include the memory cacheability type as we should
> > > > not provide a default for that either.
> > >
> > > Yes. I think this is required. Having this rule can help caller write robust code
> > > instead of depending on some default attributes in PPI/Protocol
> > implementation.
> > >
> >
> > I still don't follow. How does that work in the context of the
> > attribute mask? Can you given some examples?
>
> OK. Let's reset the discussion.
> For example, one caller's code as below:
> // mark 0-4k as not-present
> MemoryAttrributePpi->SetMemoryAttribute (0, 4K, RP, RP); // Use Attribute/Mask pattern to set RP
>
> Another caller's code:
> // mark 0-4k as present
> * MemoryAttributePpi->SetMemoryAttribute (0, 4K, 0, RP); // Use Attribute/Mask pattern to clear RP
>
OK, I see what you mean now.
> Q1: Does the PPI support this usage?
My current implementation for ARM does not support this, because the
RP flag is not tied to the present bit but to the access flag. This
means that setting and clearing RP will not create a valid region.
> Q2: If it supports, what're the other attributes of 0-4k memory? Is XP set? Is RO set?
>
I am leaning towards not supporting this, at least initially, as
updating permissions and creating mappings are two different things,
and I am not convinced a generic PPI for creating mappings from
scratch is needed at this point.
This is a major difference between ARM and x86, as on x86, it is quite
common to create mappings for regions that may have no memory at all.
On ARM, this is not permited, and so we carefully create mappings only
where we detect DRAM.
However, I understand that it is not trivial to implement this
restriction on x86, unless we remove RP from the set of attributes
that this API can manipulate.
^ permalink raw reply [flat|nested] 31+ messages in thread
* [RFC PATCH 06/10] ArmPkg/CpuPei: Implement the memory attributes PPI
2023-05-25 14:30 [RFC PATCH 00/10] Add PPI to manage PEI phase memory attributes Ard Biesheuvel
` (4 preceding siblings ...)
2023-05-25 14:30 ` [RFC PATCH 05/10] MdeModulePkg: Define memory attribute PPI Ard Biesheuvel
@ 2023-05-25 14:30 ` Ard Biesheuvel
2023-05-25 14:30 ` [RFC PATCH 07/10] MdeModulePkg/PeiCore: Apply restricted permissions in image loader Ard Biesheuvel
` (4 subsequent siblings)
10 siblings, 0 replies; 31+ messages in thread
From: Ard Biesheuvel @ 2023-05-25 14:30 UTC (permalink / raw)
To: devel
Cc: Ard Biesheuvel, Ray Ni, Jiewen Yao, Gerd Hoffmann, Taylor Beebe,
Oliver Smith-Denny, Dandan Bi, Liming Gao, Kinney, Michael D,
Leif Lindholm, Sunil V L, Andrei Warkentin
Implement the newly defined PPI that permits the PEI core and DXE IPL to
manage memory permissions on ranges of DRAM, for doing things like
mapping the stack non-executable, or granting executable permissions to
shadowed PEIMs.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
ArmPkg/Drivers/CpuPei/CpuPei.c | 78 ++++++++++++++++++--
ArmPkg/Drivers/CpuPei/CpuPei.inf | 4 +
2 files changed, 74 insertions(+), 8 deletions(-)
diff --git a/ArmPkg/Drivers/CpuPei/CpuPei.c b/ArmPkg/Drivers/CpuPei/CpuPei.c
index 85ef5ec07b9fdafa..d5996673260544c8 100644
--- a/ArmPkg/Drivers/CpuPei/CpuPei.c
+++ b/ArmPkg/Drivers/CpuPei/CpuPei.c
@@ -3,17 +3,10 @@
Copyright (c) 2006, Intel Corporation. All rights reserved.<BR>
Copyright (c) 2011 Hewlett Packard Corporation. All rights reserved.<BR>
Copyright (c) 2011-2013, ARM Limited. All rights reserved.<BR>
+Copyright (c) 2023, Google, LLC. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
-Module Name:
-
- MemoryInit.c
-
-Abstract:
-
- PEIM to provide fake memory init
-
**/
//
@@ -24,6 +17,7 @@ Module Name:
// The protocols, PPI and GUID definitions for this module
//
#include <Ppi/ArmMpCoreInfo.h>
+#include <Ppi/MemoryAttribute.h>
//
// The Library classes this module consumes
@@ -34,6 +28,71 @@ Module Name:
#include <Library/PcdLib.h>
#include <Library/HobLib.h>
#include <Library/ArmLib.h>
+#include <Library/ArmMmuLib.h>
+
+/**
+ Set the requested memory permission attributes on a region of memory.
+
+ BaseAddress and Length must be aligned to EFI_PAGE_SIZE.
+
+ Both SetMask and ClearMask may contain any combination of EFI_MEMORY_RP,
+ EFI_MEMORY_RO and EFI_MEMORY_XP, with the following restrictions:
+ - each constant may appear in either SetMask or ClearMask, but not in both;
+ - SetMask or ClearMask may be 0x0, but not both.
+
+ @param[in] This The protocol instance pointer.
+ @param[in] BaseAddress The physical address that is the start address of
+ a memory region.
+ @param[in] Length The size in bytes of the memory region.
+ @param[in] SetMask Mask of memory attributes to set.
+ @param[in] ClearMask Mask of memory attributes to clear.
+
+ @retval EFI_SUCCESS The attributes were set for the memory region.
+ @retval EFI_INVALID_PARAMETER Length is zero.
+ Invalid combination of SetMask and ClearMask.
+ BaseAddress or Length is not suitably aligned.
+ @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.
+
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+SetMemoryPermissions (
+ IN EDKII_MEMORY_ATTRIBUTE_PPI *This,
+ IN EFI_PHYSICAL_ADDRESS BaseAddress,
+ IN UINT64 Length,
+ IN UINT64 SetMask,
+ IN UINT64 ClearMask
+ )
+{
+ if ((Length == 0) ||
+ ((SetMask & ClearMask) != 0) ||
+ (((SetMask | ClearMask) & (EFI_MEMORY_RP | EFI_MEMORY_RO | EFI_MEMORY_XP)) == 0) ||
+ (((SetMask | ClearMask) & ~(UINT64)(EFI_MEMORY_RP | EFI_MEMORY_RO | EFI_MEMORY_XP)) != 0) ||
+ (((BaseAddress | Length) & EFI_PAGE_MASK) != 0))
+ {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ return ArmSetMemoryAttributes (BaseAddress, Length, SetMask, SetMask | ClearMask);
+}
+
+STATIC CONST EDKII_MEMORY_ATTRIBUTE_PPI mMemoryAttributePpi = {
+ SetMemoryPermissions
+};
+
+STATIC CONST EFI_PEI_PPI_DESCRIPTOR mMemoryAttributePpiDesc = {
+ (EFI_PEI_PPI_DESCRIPTOR_PPI | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST),
+ &gEdkiiMemoryAttributePpiGuid,
+ (VOID *)&mMemoryAttributePpi
+};
/*++
@@ -79,5 +138,8 @@ InitializeCpuPeim (
}
}
+ Status = PeiServicesInstallPpi (&mMemoryAttributePpiDesc);
+ ASSERT_EFI_ERROR (Status);
+
return EFI_SUCCESS;
}
diff --git a/ArmPkg/Drivers/CpuPei/CpuPei.inf b/ArmPkg/Drivers/CpuPei/CpuPei.inf
index 648f27adf9402435..2ca4f795c62de394 100644
--- a/ArmPkg/Drivers/CpuPei/CpuPei.inf
+++ b/ArmPkg/Drivers/CpuPei/CpuPei.inf
@@ -3,6 +3,7 @@
#
# This module provides platform specific function to detect boot mode.
# Copyright (c) 2006 - 2010, Intel Corporation. All rights reserved.<BR>
+# Copyright (c) 2023, Google, LLC. All rights reserved.<BR>
#
# SPDX-License-Identifier: BSD-2-Clause-Patent
#
@@ -28,6 +29,7 @@ [Sources]
CpuPei.c
[Packages]
+ MdeModulePkg/MdeModulePkg.dec
MdePkg/MdePkg.dec
EmbeddedPkg/EmbeddedPkg.dec
ArmPkg/ArmPkg.dec
@@ -37,9 +39,11 @@ [LibraryClasses]
DebugLib
HobLib
ArmLib
+ ArmMmuLib
[Ppis]
gArmMpCoreInfoPpiGuid
+ gEdkiiMemoryAttributePpiGuid
[Guids]
gArmMpCoreInfoGuid
--
2.39.2
^ permalink raw reply related [flat|nested] 31+ messages in thread* [RFC PATCH 07/10] MdeModulePkg/PeiCore: Apply restricted permissions in image loader
2023-05-25 14:30 [RFC PATCH 00/10] Add PPI to manage PEI phase memory attributes Ard Biesheuvel
` (5 preceding siblings ...)
2023-05-25 14:30 ` [RFC PATCH 06/10] ArmPkg/CpuPei: Implement the memory attributes PPI Ard Biesheuvel
@ 2023-05-25 14:30 ` Ard Biesheuvel
2023-05-25 17:21 ` [edk2-devel] " Oliver Smith-Denny
2023-05-25 14:30 ` [RFC PATCH 08/10] MdeModulePkg/DxeIpl: Merge EBC, RISCV64 and LOONGARCH code Ard Biesheuvel
` (3 subsequent siblings)
10 siblings, 1 reply; 31+ messages in thread
From: Ard Biesheuvel @ 2023-05-25 14:30 UTC (permalink / raw)
To: devel
Cc: Ard Biesheuvel, Ray Ni, Jiewen Yao, Gerd Hoffmann, Taylor Beebe,
Oliver Smith-Denny, Dandan Bi, Liming Gao, Kinney, Michael D,
Leif Lindholm, Sunil V L, Andrei Warkentin
Add a notification callback to the PEI core to grab a reference to the
memory attributes PPI as soon as it is registered, and use it in the
image loader to set restricted memory permissions after loading the
image if the image was loaded into memory.
There are two use cases for this:
- when the DXE IPL loads the DXE core using the PEI image services, its
mappings will be set according to the PE section permission attributes
if the image was built with 4k section alignment; this means DXE core
will never run with mappings that are both writable and executable.
- when PEIMs are shadowed to memory, they will not only be mapped
read-only, but any non-exec permissions will also be removed. (Note
that this requires the component that installs PEI permanent memory to
depex on the memory attributes PPI, to ensure that it is available to
manage permissions on permanent memory before it is used to load
images)
With this logic in place *, there is no longer a need for system memory
to be mapped with both write and execute permissions out of reset.
Instead, all memory can be mapped with non-executable permissions by
default, which means that the stack and other allocations used in PEI or
early in DXE will no longer need to be mapped non-exec explicitly.
* the DXE core will also need its own method for clearing non-exec
permissions on memory ranges, but this is being addressed in a
separate series.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
MdeModulePkg/Core/Pei/Image/Image.c | 160 ++++++++++++++++++++
MdeModulePkg/Core/Pei/PeiMain.h | 6 +
MdeModulePkg/Core/Pei/PeiMain.inf | 1 +
3 files changed, 167 insertions(+)
diff --git a/MdeModulePkg/Core/Pei/Image/Image.c b/MdeModulePkg/Core/Pei/Image/Image.c
index cee9f09c6ea61e31..3a7de45014b8f772 100644
--- a/MdeModulePkg/Core/Pei/Image/Image.c
+++ b/MdeModulePkg/Core/Pei/Image/Image.c
@@ -18,6 +18,50 @@ EFI_PEI_PPI_DESCRIPTOR gPpiLoadFilePpiList = {
&mPeiLoadImagePpi
};
+/**
+ Provide a callback for when the memory attributes PPI is installed.
+
+ @param PeiServices An indirect pointer to the EFI_PEI_SERVICES table
+ published by the PEI Foundation.
+ @param NotifyDescriptor The descriptor for the notification event.
+ @param Ppi Pointer to the PPI in question.
+
+ @return Always success
+
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+MemoryAttributePpiNotifyCallback (
+ IN EFI_PEI_SERVICES **PeiServices,
+ IN EFI_PEI_NOTIFY_DESCRIPTOR *NotifyDescriptor,
+ IN VOID *Ppi
+ )
+{
+ PEI_CORE_INSTANCE *PrivateData;
+
+ //
+ // Get PEI Core private data
+ //
+ PrivateData = PEI_CORE_INSTANCE_FROM_PS_THIS (PeiServices);
+
+ //
+ // If there isn't a memory attribute PPI installed, use the one from
+ // notification
+ //
+ if (PrivateData->MemoryAttributePpi == NULL) {
+ PrivateData->MemoryAttributePpi = (EDKII_MEMORY_ATTRIBUTE_PPI *)Ppi;
+ }
+
+ return EFI_SUCCESS;
+}
+
+STATIC CONST EFI_PEI_NOTIFY_DESCRIPTOR mNotifyList = {
+ EFI_PEI_PPI_DESCRIPTOR_NOTIFY_DISPATCH | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST,
+ &gEdkiiMemoryAttributePpiGuid,
+ MemoryAttributePpiNotifyCallback
+};
+
/**
Support routine for the PE/COFF Loader that reads a buffer from a PE/COFF file.
@@ -243,6 +287,106 @@ GetPeCoffImageFixLoadingAssignedAddress (
return Status;
}
+/**
+ Remap the memory region covering a loaded image so it can be executed.
+
+ @param ImageContext Pointer to the image context structure that describes the
+ PE/COFF image that needs to be examined by this function.
+ @param FileType The FFS file type of the image
+ @param ImageAddress The start of the memory region covering the image
+ @param ImageSize The size of the memory region covering the image
+
+ @retval EFI_SUCCESS The image is ready to be executed
+ @retval EFI_OUT_OF_RESOURCES Not enough memory available to update the memory
+ mapping
+
+**/
+STATIC
+EFI_STATUS
+RemapLoadedImageForExecution (
+ IN CONST PE_COFF_LOADER_IMAGE_CONTEXT *ImageContext,
+ IN EFI_FV_FILETYPE FileType,
+ IN PHYSICAL_ADDRESS ImageAddress,
+ IN UINT64 ImageSize
+ )
+{
+ PEI_CORE_INSTANCE *Private;
+ EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION Hdr;
+ EFI_IMAGE_SECTION_HEADER *Section;
+ PHYSICAL_ADDRESS SectionAddress;
+ EFI_STATUS Status;
+ UINT64 Permissions;
+ UINTN Index;
+
+ Private = PEI_CORE_INSTANCE_FROM_PS_THIS (GetPeiServicesTablePointer ());
+
+ if (Private->MemoryAttributePpi == NULL) {
+ return EFI_SUCCESS;
+ }
+
+ //
+ // PEI phase executables must be able to execute in place from read-only NOR
+ // flash, and so they can be mapped read-only in their entirety.
+ //
+ if ((FileType == EFI_FV_FILETYPE_PEI_CORE) ||
+ (FileType == EFI_FV_FILETYPE_PEIM) ||
+ (FileType == EFI_FV_FILETYPE_COMBINED_PEIM_DRIVER))
+ {
+ return Private->MemoryAttributePpi->SetPermissions (
+ Private->MemoryAttributePpi,
+ ImageAddress,
+ ImageSize,
+ EFI_MEMORY_RO,
+ EFI_MEMORY_XP
+ );
+ }
+
+ //
+ // Only PE images with minimum 4k section alignment can be remapped with
+ // restricted permissions.
+ //
+ if (ImageContext->IsTeImage ||
+ (ImageContext->SectionAlignment < EFI_PAGE_SIZE))
+ {
+ return EFI_UNSUPPORTED;
+ }
+
+ 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++) {
+ SectionAddress = ImageContext->ImageAddress + Section[Index].VirtualAddress;
+ Permissions = 0;
+
+ if ((Section[Index].Characteristics & EFI_IMAGE_SCN_MEM_WRITE) == 0) {
+ Permissions |= EFI_MEMORY_RO;
+ }
+
+ if ((Section[Index].Characteristics & EFI_IMAGE_SCN_MEM_EXECUTE) == 0) {
+ Permissions |= EFI_MEMORY_XP;
+ }
+
+ Status = Private->MemoryAttributePpi->SetPermissions (
+ Private->MemoryAttributePpi,
+ SectionAddress,
+ Section[Index].Misc.VirtualSize,
+ Permissions,
+ Permissions ^ EFI_MEMORY_RO ^ EFI_MEMORY_XP
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+ }
+
+ return EFI_SUCCESS;
+}
+
/**
Loads and relocates a PE/COFF image into memory.
@@ -456,9 +600,24 @@ LoadAndRelocatePeCoffImage (
//
// Flush the instruction cache so the image data is written before we execute it
+ // Also ensure that the pages are mapped for execution
//
if (ImageContext.ImageAddress != (EFI_PHYSICAL_ADDRESS)(UINTN)Pe32Data) {
InvalidateInstructionCacheRange ((VOID *)(UINTN)ImageContext.ImageAddress, (UINTN)ImageContext.ImageSize);
+
+ Status = RemapLoadedImageForExecution (
+ &ImageContext,
+ FileInfo.FileType,
+ ImageContext.ImageAddress & ~(UINT64)EFI_PAGE_MASK,
+ ALIGN_VALUE (
+ AlignImageSize + (ImageContext.ImageAddress & EFI_PAGE_MASK),
+ EFI_PAGE_SIZE
+ )
+ );
+ if (EFI_ERROR (Status)) {
+ ASSERT_EFI_ERROR (Status);
+ return Status;
+ }
}
*ImageAddress = ImageContext.ImageAddress;
@@ -972,6 +1131,7 @@ InitializeImageServices (
//
PrivateData->XipLoadFile = &gPpiLoadFilePpiList;
PeiServicesInstallPpi (PrivateData->XipLoadFile);
+ PeiServicesNotifyPpi (&mNotifyList);
} else {
//
// 2nd time we are running from memory so replace the XIP version with the
diff --git a/MdeModulePkg/Core/Pei/PeiMain.h b/MdeModulePkg/Core/Pei/PeiMain.h
index 556beddad533989f..5499d53b0bbaf641 100644
--- a/MdeModulePkg/Core/Pei/PeiMain.h
+++ b/MdeModulePkg/Core/Pei/PeiMain.h
@@ -26,6 +26,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include <Ppi/TemporaryRamDone.h>
#include <Ppi/SecHobData.h>
#include <Ppi/PeiCoreFvLocation.h>
+#include <Ppi/MemoryAttribute.h>
#include <Library/DebugLib.h>
#include <Library/PeiCoreEntryPoint.h>
#include <Library/BaseLib.h>
@@ -302,6 +303,11 @@ struct _PEI_CORE_INSTANCE {
//
EFI_GUID *TempFileGuid;
+ //
+ // Pointer to the memory attribute PPI
+ //
+ EDKII_MEMORY_ATTRIBUTE_PPI *MemoryAttributePpi;
+
//
// Temp Memory Range is not covered by PeiTempMem and Stack.
// Those Memory Range will be migrated into physical memory.
diff --git a/MdeModulePkg/Core/Pei/PeiMain.inf b/MdeModulePkg/Core/Pei/PeiMain.inf
index 0cf357371a16d872..55d8eb3e862d6418 100644
--- a/MdeModulePkg/Core/Pei/PeiMain.inf
+++ b/MdeModulePkg/Core/Pei/PeiMain.inf
@@ -100,6 +100,7 @@ [Ppis]
gEfiPeiReset2PpiGuid ## SOMETIMES_CONSUMES
gEfiSecHobDataPpiGuid ## SOMETIMES_CONSUMES
gEfiPeiCoreFvLocationPpiGuid ## SOMETIMES_CONSUMES
+ gEdkiiMemoryAttributePpiGuid ## SOMETIMES_CONSUMES
[Pcd]
gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxPeiStackSize ## CONSUMES
--
2.39.2
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [edk2-devel] [RFC PATCH 07/10] MdeModulePkg/PeiCore: Apply restricted permissions in image loader
2023-05-25 14:30 ` [RFC PATCH 07/10] MdeModulePkg/PeiCore: Apply restricted permissions in image loader Ard Biesheuvel
@ 2023-05-25 17:21 ` Oliver Smith-Denny
2023-05-25 21:29 ` Ard Biesheuvel
0 siblings, 1 reply; 31+ messages in thread
From: Oliver Smith-Denny @ 2023-05-25 17:21 UTC (permalink / raw)
To: devel, ardb
Cc: Ray Ni, Jiewen Yao, Gerd Hoffmann, Taylor Beebe,
Oliver Smith-Denny, Dandan Bi, Liming Gao, Kinney, Michael D,
Leif Lindholm, Sunil V L, Andrei Warkentin
On 5/25/2023 7:30 AM, Ard Biesheuvel wrote:
> Add a notification callback to the PEI core to grab a reference to the
> memory attributes PPI as soon as it is registered, and use it in the
> image loader to set restricted memory permissions after loading the
> image if the image was loaded into memory.
>
> There are two use cases for this:
> - when the DXE IPL loads the DXE core using the PEI image services, its
> mappings will be set according to the PE section permission attributes
> if the image was built with 4k section alignment; this means DXE core
> will never run with mappings that are both writable and executable.
> - when PEIMs are shadowed to memory, they will not only be mapped
> read-only, but any non-exec permissions will also be removed. (Note
> that this requires the component that installs PEI permanent memory to
> depex on the memory attributes PPI, to ensure that it is available to
> manage permissions on permanent memory before it is used to load
> images)
>
> With this logic in place *, there is no longer a need for system memory
> to be mapped with both write and execute permissions out of reset.
> Instead, all memory can be mapped with non-executable permissions by
> default, which means that the stack and other allocations used in PEI or
> early in DXE will no longer need to be mapped non-exec explicitly.
>
> * the DXE core will also need its own method for clearing non-exec
> permissions on memory ranges, but this is being addressed in a
> separate series.
>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
> MdeModulePkg/Core/Pei/Image/Image.c | 160 ++++++++++++++++++++
> MdeModulePkg/Core/Pei/PeiMain.h | 6 +
> MdeModulePkg/Core/Pei/PeiMain.inf | 1 +
> 3 files changed, 167 insertions(+)
>
> diff --git a/MdeModulePkg/Core/Pei/Image/Image.c b/MdeModulePkg/Core/Pei/Image/Image.c
> index cee9f09c6ea61e31..3a7de45014b8f772 100644
> --- a/MdeModulePkg/Core/Pei/Image/Image.c
> +++ b/MdeModulePkg/Core/Pei/Image/Image.c
> @@ -18,6 +18,50 @@ EFI_PEI_PPI_DESCRIPTOR gPpiLoadFilePpiList = {
> &mPeiLoadImagePpi
>
> };
>
>
>
> +/**
>
> + Provide a callback for when the memory attributes PPI is installed.
>
> +
>
> + @param PeiServices An indirect pointer to the EFI_PEI_SERVICES table
>
> + published by the PEI Foundation.
>
> + @param NotifyDescriptor The descriptor for the notification event.
>
> + @param Ppi Pointer to the PPI in question.
>
> +
>
> + @return Always success
>
> +
>
> +**/
>
> +STATIC
>
> +EFI_STATUS
>
> +EFIAPI
>
> +MemoryAttributePpiNotifyCallback (
>
> + IN EFI_PEI_SERVICES **PeiServices,
>
> + IN EFI_PEI_NOTIFY_DESCRIPTOR *NotifyDescriptor,
>
> + IN VOID *Ppi
>
> + )
>
> +{
>
> + PEI_CORE_INSTANCE *PrivateData;
>
> +
>
> + //
>
> + // Get PEI Core private data
>
> + //
>
> + PrivateData = PEI_CORE_INSTANCE_FROM_PS_THIS (PeiServices);
>
> +
>
> + //
>
> + // If there isn't a memory attribute PPI installed, use the one from
>
> + // notification
>
> + //
>
> + if (PrivateData->MemoryAttributePpi == NULL) {
>
> + PrivateData->MemoryAttributePpi = (EDKII_MEMORY_ATTRIBUTE_PPI *)Ppi;
>
> + }
>
> +
>
> + return EFI_SUCCESS;
>
> +}
>
> +
>
> +STATIC CONST EFI_PEI_NOTIFY_DESCRIPTOR mNotifyList = {
>
> + EFI_PEI_PPI_DESCRIPTOR_NOTIFY_DISPATCH | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST,
>
> + &gEdkiiMemoryAttributePpiGuid,
>
> + MemoryAttributePpiNotifyCallback
>
> +};
>
> +
>
> /**
>
>
>
> Support routine for the PE/COFF Loader that reads a buffer from a PE/COFF file.
>
> @@ -243,6 +287,106 @@ GetPeCoffImageFixLoadingAssignedAddress (
> return Status;
>
> }
>
>
>
> +/**
>
> + Remap the memory region covering a loaded image so it can be executed.
>
> +
>
> + @param ImageContext Pointer to the image context structure that describes the
>
> + PE/COFF image that needs to be examined by this function.
>
> + @param FileType The FFS file type of the image
>
> + @param ImageAddress The start of the memory region covering the image
>
> + @param ImageSize The size of the memory region covering the image
>
> +
>
> + @retval EFI_SUCCESS The image is ready to be executed
>
> + @retval EFI_OUT_OF_RESOURCES Not enough memory available to update the memory
>
> + mapping
>
> +
>
> +**/
>
> +STATIC
>
> +EFI_STATUS
>
> +RemapLoadedImageForExecution (
>
> + IN CONST PE_COFF_LOADER_IMAGE_CONTEXT *ImageContext,
>
> + IN EFI_FV_FILETYPE FileType,
>
> + IN PHYSICAL_ADDRESS ImageAddress,
>
> + IN UINT64 ImageSize
>
> + )
>
> +{
>
> + PEI_CORE_INSTANCE *Private;
>
> + EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION Hdr;
>
> + EFI_IMAGE_SECTION_HEADER *Section;
>
> + PHYSICAL_ADDRESS SectionAddress;
>
> + EFI_STATUS Status;
>
> + UINT64 Permissions;
>
> + UINTN Index;
>
> +
>
> + Private = PEI_CORE_INSTANCE_FROM_PS_THIS (GetPeiServicesTablePointer ());
>
> +
>
> + if (Private->MemoryAttributePpi == NULL) {
>
> + return EFI_SUCCESS;
We will have a gap here before the MemoryAttributePpi is installed,
obviously, when CpuPei is installed. Is the expectation that only
dependencies for CpuPei will be launched before and everything else
will have a dependency on CpuPei?
Or, is it that shadowed PEIMs won't happen before CpuPei? I am curious
how big or small of a gap this really is.
>
> + }
>
> +
>
> + //
>
> + // PEI phase executables must be able to execute in place from read-only NOR
>
> + // flash, and so they can be mapped read-only in their entirety.
>
> + //
>
> + if ((FileType == EFI_FV_FILETYPE_PEI_CORE) ||
>
> + (FileType == EFI_FV_FILETYPE_PEIM) ||
>
> + (FileType == EFI_FV_FILETYPE_COMBINED_PEIM_DRIVER))
>
> + {
We are calling this from PEI Core, will we have more images of type
PEI Core? I understand if this is for completeness, but just making
sure I understand the flow.
Thanks,
Oliver
>
> + return Private->MemoryAttributePpi->SetPermissions (
>
> + Private->MemoryAttributePpi,
>
> + ImageAddress,
>
> + ImageSize,
>
> + EFI_MEMORY_RO,
>
> + EFI_MEMORY_XP
>
> + );
>
> + }
>
> +
>
> + //
>
> + // Only PE images with minimum 4k section alignment can be remapped with
>
> + // restricted permissions.
>
> + //
>
> + if (ImageContext->IsTeImage ||
>
> + (ImageContext->SectionAlignment < EFI_PAGE_SIZE))
>
> + {
>
> + return EFI_UNSUPPORTED;
>
> + }
>
> +
>
> + 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++) {
>
> + SectionAddress = ImageContext->ImageAddress + Section[Index].VirtualAddress;
>
> + Permissions = 0;
>
> +
>
> + if ((Section[Index].Characteristics & EFI_IMAGE_SCN_MEM_WRITE) == 0) {
>
> + Permissions |= EFI_MEMORY_RO;
>
> + }
>
> +
>
> + if ((Section[Index].Characteristics & EFI_IMAGE_SCN_MEM_EXECUTE) == 0) {
>
> + Permissions |= EFI_MEMORY_XP;
>
> + }
>
> +
>
> + Status = Private->MemoryAttributePpi->SetPermissions (
>
> + Private->MemoryAttributePpi,
>
> + SectionAddress,
>
> + Section[Index].Misc.VirtualSize,
>
> + Permissions,
>
> + Permissions ^ EFI_MEMORY_RO ^ EFI_MEMORY_XP
>
> + );
>
> + if (EFI_ERROR (Status)) {
>
> + return Status;
>
> + }
>
> + }
>
> +
>
> + return EFI_SUCCESS;
>
> +}
>
> +
>
> /**
>
>
>
> Loads and relocates a PE/COFF image into memory.
>
> @@ -456,9 +600,24 @@ LoadAndRelocatePeCoffImage (
>
>
> //
>
> // Flush the instruction cache so the image data is written before we execute it
>
> + // Also ensure that the pages are mapped for execution
>
> //
>
> if (ImageContext.ImageAddress != (EFI_PHYSICAL_ADDRESS)(UINTN)Pe32Data) {
>
> InvalidateInstructionCacheRange ((VOID *)(UINTN)ImageContext.ImageAddress, (UINTN)ImageContext.ImageSize);
>
> +
>
> + Status = RemapLoadedImageForExecution (
>
> + &ImageContext,
>
> + FileInfo.FileType,
>
> + ImageContext.ImageAddress & ~(UINT64)EFI_PAGE_MASK,
>
> + ALIGN_VALUE (
>
> + AlignImageSize + (ImageContext.ImageAddress & EFI_PAGE_MASK),
>
> + EFI_PAGE_SIZE
>
> + )
>
> + );
>
> + if (EFI_ERROR (Status)) {
>
> + ASSERT_EFI_ERROR (Status);
>
> + return Status;
>
> + }
>
> }
>
>
>
> *ImageAddress = ImageContext.ImageAddress;
>
> @@ -972,6 +1131,7 @@ InitializeImageServices (
> //
>
> PrivateData->XipLoadFile = &gPpiLoadFilePpiList;
>
> PeiServicesInstallPpi (PrivateData->XipLoadFile);
>
> + PeiServicesNotifyPpi (&mNotifyList);
>
> } else {
>
> //
>
> // 2nd time we are running from memory so replace the XIP version with the
>
> diff --git a/MdeModulePkg/Core/Pei/PeiMain.h b/MdeModulePkg/Core/Pei/PeiMain.h
> index 556beddad533989f..5499d53b0bbaf641 100644
> --- a/MdeModulePkg/Core/Pei/PeiMain.h
> +++ b/MdeModulePkg/Core/Pei/PeiMain.h
> @@ -26,6 +26,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> #include <Ppi/TemporaryRamDone.h>
>
> #include <Ppi/SecHobData.h>
>
> #include <Ppi/PeiCoreFvLocation.h>
>
> +#include <Ppi/MemoryAttribute.h>
>
> #include <Library/DebugLib.h>
>
> #include <Library/PeiCoreEntryPoint.h>
>
> #include <Library/BaseLib.h>
>
> @@ -302,6 +303,11 @@ struct _PEI_CORE_INSTANCE {
> //
>
> EFI_GUID *TempFileGuid;
>
>
>
> + //
>
> + // Pointer to the memory attribute PPI
>
> + //
>
> + EDKII_MEMORY_ATTRIBUTE_PPI *MemoryAttributePpi;
>
> +
>
> //
>
> // Temp Memory Range is not covered by PeiTempMem and Stack.
>
> // Those Memory Range will be migrated into physical memory.
>
> diff --git a/MdeModulePkg/Core/Pei/PeiMain.inf b/MdeModulePkg/Core/Pei/PeiMain.inf
> index 0cf357371a16d872..55d8eb3e862d6418 100644
> --- a/MdeModulePkg/Core/Pei/PeiMain.inf
> +++ b/MdeModulePkg/Core/Pei/PeiMain.inf
> @@ -100,6 +100,7 @@ [Ppis]
> gEfiPeiReset2PpiGuid ## SOMETIMES_CONSUMES
>
> gEfiSecHobDataPpiGuid ## SOMETIMES_CONSUMES
>
> gEfiPeiCoreFvLocationPpiGuid ## SOMETIMES_CONSUMES
>
> + gEdkiiMemoryAttributePpiGuid ## SOMETIMES_CONSUMES
>
>
>
> [Pcd]
>
> gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxPeiStackSize ## CONSUMES
>
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [edk2-devel] [RFC PATCH 07/10] MdeModulePkg/PeiCore: Apply restricted permissions in image loader
2023-05-25 17:21 ` [edk2-devel] " Oliver Smith-Denny
@ 2023-05-25 21:29 ` Ard Biesheuvel
2023-05-30 16:51 ` Oliver Smith-Denny
0 siblings, 1 reply; 31+ messages in thread
From: Ard Biesheuvel @ 2023-05-25 21:29 UTC (permalink / raw)
To: Oliver Smith-Denny
Cc: devel, Ray Ni, Jiewen Yao, Gerd Hoffmann, Taylor Beebe,
Oliver Smith-Denny, Dandan Bi, Liming Gao, Kinney, Michael D,
Leif Lindholm, Sunil V L, Andrei Warkentin
On Thu, 25 May 2023 at 19:21, Oliver Smith-Denny
<osde@linux.microsoft.com> wrote:
>
> On 5/25/2023 7:30 AM, Ard Biesheuvel wrote:
> > Add a notification callback to the PEI core to grab a reference to the
> > memory attributes PPI as soon as it is registered, and use it in the
> > image loader to set restricted memory permissions after loading the
> > image if the image was loaded into memory.
> >
> > There are two use cases for this:
> > - when the DXE IPL loads the DXE core using the PEI image services, its
> > mappings will be set according to the PE section permission attributes
> > if the image was built with 4k section alignment; this means DXE core
> > will never run with mappings that are both writable and executable.
> > - when PEIMs are shadowed to memory, they will not only be mapped
> > read-only, but any non-exec permissions will also be removed. (Note
> > that this requires the component that installs PEI permanent memory to
> > depex on the memory attributes PPI, to ensure that it is available to
> > manage permissions on permanent memory before it is used to load
> > images)
> >
> > With this logic in place *, there is no longer a need for system memory
> > to be mapped with both write and execute permissions out of reset.
> > Instead, all memory can be mapped with non-executable permissions by
> > default, which means that the stack and other allocations used in PEI or
> > early in DXE will no longer need to be mapped non-exec explicitly.
> >
> > * the DXE core will also need its own method for clearing non-exec
> > permissions on memory ranges, but this is being addressed in a
> > separate series.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> > MdeModulePkg/Core/Pei/Image/Image.c | 160 ++++++++++++++++++++
> > MdeModulePkg/Core/Pei/PeiMain.h | 6 +
> > MdeModulePkg/Core/Pei/PeiMain.inf | 1 +
> > 3 files changed, 167 insertions(+)
> >
> > diff --git a/MdeModulePkg/Core/Pei/Image/Image.c b/MdeModulePkg/Core/Pei/Image/Image.c
> > index cee9f09c6ea61e31..3a7de45014b8f772 100644
> > --- a/MdeModulePkg/Core/Pei/Image/Image.c
> > +++ b/MdeModulePkg/Core/Pei/Image/Image.c
> > @@ -18,6 +18,50 @@ EFI_PEI_PPI_DESCRIPTOR gPpiLoadFilePpiList = {
> > &mPeiLoadImagePpi
> >
> > };
> >
> >
> >
> > +/**
> >
> > + Provide a callback for when the memory attributes PPI is installed.
> >
> > +
> >
> > + @param PeiServices An indirect pointer to the EFI_PEI_SERVICES table
> >
> > + published by the PEI Foundation.
> >
> > + @param NotifyDescriptor The descriptor for the notification event.
> >
> > + @param Ppi Pointer to the PPI in question.
> >
> > +
> >
> > + @return Always success
> >
> > +
> >
> > +**/
> >
> > +STATIC
> >
> > +EFI_STATUS
> >
> > +EFIAPI
> >
> > +MemoryAttributePpiNotifyCallback (
> >
> > + IN EFI_PEI_SERVICES **PeiServices,
> >
> > + IN EFI_PEI_NOTIFY_DESCRIPTOR *NotifyDescriptor,
> >
> > + IN VOID *Ppi
> >
> > + )
> >
> > +{
> >
> > + PEI_CORE_INSTANCE *PrivateData;
> >
> > +
> >
> > + //
> >
> > + // Get PEI Core private data
> >
> > + //
> >
> > + PrivateData = PEI_CORE_INSTANCE_FROM_PS_THIS (PeiServices);
> >
> > +
> >
> > + //
> >
> > + // If there isn't a memory attribute PPI installed, use the one from
> >
> > + // notification
> >
> > + //
> >
> > + if (PrivateData->MemoryAttributePpi == NULL) {
> >
> > + PrivateData->MemoryAttributePpi = (EDKII_MEMORY_ATTRIBUTE_PPI *)Ppi;
> >
> > + }
> >
> > +
> >
> > + return EFI_SUCCESS;
> >
> > +}
> >
> > +
> >
> > +STATIC CONST EFI_PEI_NOTIFY_DESCRIPTOR mNotifyList = {
> >
> > + EFI_PEI_PPI_DESCRIPTOR_NOTIFY_DISPATCH | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST,
> >
> > + &gEdkiiMemoryAttributePpiGuid,
> >
> > + MemoryAttributePpiNotifyCallback
> >
> > +};
> >
> > +
> >
> > /**
> >
> >
> >
> > Support routine for the PE/COFF Loader that reads a buffer from a PE/COFF file.
> >
> > @@ -243,6 +287,106 @@ GetPeCoffImageFixLoadingAssignedAddress (
> > return Status;
> >
> > }
> >
> >
> >
> > +/**
> >
> > + Remap the memory region covering a loaded image so it can be executed.
> >
> > +
> >
> > + @param ImageContext Pointer to the image context structure that describes the
> >
> > + PE/COFF image that needs to be examined by this function.
> >
> > + @param FileType The FFS file type of the image
> >
> > + @param ImageAddress The start of the memory region covering the image
> >
> > + @param ImageSize The size of the memory region covering the image
> >
> > +
> >
> > + @retval EFI_SUCCESS The image is ready to be executed
> >
> > + @retval EFI_OUT_OF_RESOURCES Not enough memory available to update the memory
> >
> > + mapping
> >
> > +
> >
> > +**/
> >
> > +STATIC
> >
> > +EFI_STATUS
> >
> > +RemapLoadedImageForExecution (
> >
> > + IN CONST PE_COFF_LOADER_IMAGE_CONTEXT *ImageContext,
> >
> > + IN EFI_FV_FILETYPE FileType,
> >
> > + IN PHYSICAL_ADDRESS ImageAddress,
> >
> > + IN UINT64 ImageSize
> >
> > + )
> >
> > +{
> >
> > + PEI_CORE_INSTANCE *Private;
> >
> > + EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION Hdr;
> >
> > + EFI_IMAGE_SECTION_HEADER *Section;
> >
> > + PHYSICAL_ADDRESS SectionAddress;
> >
> > + EFI_STATUS Status;
> >
> > + UINT64 Permissions;
> >
> > + UINTN Index;
> >
> > +
> >
> > + Private = PEI_CORE_INSTANCE_FROM_PS_THIS (GetPeiServicesTablePointer ());
> >
> > +
> >
> > + if (Private->MemoryAttributePpi == NULL) {
> >
> > + return EFI_SUCCESS;
>
> We will have a gap here before the MemoryAttributePpi is installed,
> obviously, when CpuPei is installed. Is the expectation that only
> dependencies for CpuPei will be launched before and everything else
> will have a dependency on CpuPei?
>
> Or, is it that shadowed PEIMs won't happen before CpuPei? I am curious
> how big or small of a gap this really is.
>
There are two different cases to consider here:
- First, there is the DxeIpl, which will rely on the PPI (via the
image loader and directly) to map the DXE core and the NX stack. The
DxeIpl will not proceed with that until all PEIMs are dispatched, so
if the PPI is going to be exposed, it will be available by that time.
- Then there are the shadowed PEIMs, and given that shadowed PEIMs
implicitly depend on PEI permanent memory having been installed, the
only requirement here is that, if the platform needs/wants this for
shadowed PEIMs as well, the PEIM that installs the PEI permanent
memory should depex on the PPI.
> >
> > + }
> >
> > +
> >
> > + //
> >
> > + // PEI phase executables must be able to execute in place from read-only NOR
> >
> > + // flash, and so they can be mapped read-only in their entirety.
> >
> > + //
> >
> > + if ((FileType == EFI_FV_FILETYPE_PEI_CORE) ||
> >
> > + (FileType == EFI_FV_FILETYPE_PEIM) ||
> >
> > + (FileType == EFI_FV_FILETYPE_COMBINED_PEIM_DRIVER))
> >
> > + {
>
> We are calling this from PEI Core, will we have more images of type
> PEI Core? I understand if this is for completeness, but just making
> sure I understand the flow.
>
PEI core itself may be shadowed as well, and will be mapped read-only
as well if it is included here.
--
Ard.
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [edk2-devel] [RFC PATCH 07/10] MdeModulePkg/PeiCore: Apply restricted permissions in image loader
2023-05-25 21:29 ` Ard Biesheuvel
@ 2023-05-30 16:51 ` Oliver Smith-Denny
2023-05-30 20:51 ` Ard Biesheuvel
0 siblings, 1 reply; 31+ messages in thread
From: Oliver Smith-Denny @ 2023-05-30 16:51 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: devel, Ray Ni, Jiewen Yao, Gerd Hoffmann, Taylor Beebe,
Oliver Smith-Denny, Dandan Bi, Liming Gao, Kinney, Michael D,
Leif Lindholm, Sunil V L, Andrei Warkentin
On 5/25/2023 2:29 PM, Ard Biesheuvel wrote:
> On Thu, 25 May 2023 at 19:21, Oliver Smith-Denny
> <osde@linux.microsoft.com> wrote:
>>
>> On 5/25/2023 7:30 AM, Ard Biesheuvel wrote:
>>> + if (Private->MemoryAttributePpi == NULL) {
>>>
>>> + return EFI_SUCCESS;
>>
>> We will have a gap here before the MemoryAttributePpi is installed,
>> obviously, when CpuPei is installed. Is the expectation that only
>> dependencies for CpuPei will be launched before and everything else
>> will have a dependency on CpuPei?
>>
>> Or, is it that shadowed PEIMs won't happen before CpuPei? I am curious
>> how big or small of a gap this really is.
>>
>
> There are two different cases to consider here:
> - First, there is the DxeIpl, which will rely on the PPI (via the
> image loader and directly) to map the DXE core and the NX stack. The
> DxeIpl will not proceed with that until all PEIMs are dispatched, so
> if the PPI is going to be exposed, it will be available by that time.
> - Then there are the shadowed PEIMs, and given that shadowed PEIMs
> implicitly depend on PEI permanent memory having been installed, the
> only requirement here is that, if the platform needs/wants this for
> shadowed PEIMs as well, the PEIM that installs the PEI permanent
> memory should depex on the PPI.
>
I see, thanks for the explanation. While this is not the main point
of this patchset, I do think that the core should own the memory
protections of the shadowed PEIMs, instead of relying on the platform
to do the right thing (which it never does :). Sure, this is gated by
PCDs that the platform sets, but that is a simpler interface than
requiring a new depex.
Perhaps this is not trivial, as I assume the permanent memory
installation happens in a platform specific PEIM? And as such would
require a PPI notification in the core for the memory attributes PPI,
which would lead to additional undesirable complexity.
>>>
>>> + }
>>>
>>> +
>>>
>>> + //
>>>
>>> + // PEI phase executables must be able to execute in place from read-only NOR
>>>
>>> + // flash, and so they can be mapped read-only in their entirety.
>>>
>>> + //
>>>
>>> + if ((FileType == EFI_FV_FILETYPE_PEI_CORE) ||
>>>
>>> + (FileType == EFI_FV_FILETYPE_PEIM) ||
>>>
>>> + (FileType == EFI_FV_FILETYPE_COMBINED_PEIM_DRIVER))
>>>
>>> + {
>>
>> We are calling this from PEI Core, will we have more images of type
>> PEI Core? I understand if this is for completeness, but just making
>> sure I understand the flow.
>>
>
> PEI core itself may be shadowed as well, and will be mapped read-only
> as well if it is included here.
>
Gotcha, thanks.
Oliver
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [edk2-devel] [RFC PATCH 07/10] MdeModulePkg/PeiCore: Apply restricted permissions in image loader
2023-05-30 16:51 ` Oliver Smith-Denny
@ 2023-05-30 20:51 ` Ard Biesheuvel
0 siblings, 0 replies; 31+ messages in thread
From: Ard Biesheuvel @ 2023-05-30 20:51 UTC (permalink / raw)
To: devel, osde
Cc: Ray Ni, Jiewen Yao, Gerd Hoffmann, Taylor Beebe,
Oliver Smith-Denny, Dandan Bi, Liming Gao, Kinney, Michael D,
Leif Lindholm, Sunil V L, Andrei Warkentin
On Tue, 30 May 2023 at 18:51, Oliver Smith-Denny
<osde@linux.microsoft.com> wrote:
>
> On 5/25/2023 2:29 PM, Ard Biesheuvel wrote:
> > On Thu, 25 May 2023 at 19:21, Oliver Smith-Denny
> > <osde@linux.microsoft.com> wrote:
> >>
> >> On 5/25/2023 7:30 AM, Ard Biesheuvel wrote:
> >>> + if (Private->MemoryAttributePpi == NULL) {
> >>>
> >>> + return EFI_SUCCESS;
> >>
> >> We will have a gap here before the MemoryAttributePpi is installed,
> >> obviously, when CpuPei is installed. Is the expectation that only
> >> dependencies for CpuPei will be launched before and everything else
> >> will have a dependency on CpuPei?
> >>
> >> Or, is it that shadowed PEIMs won't happen before CpuPei? I am curious
> >> how big or small of a gap this really is.
> >>
> >
> > There are two different cases to consider here:
> > - First, there is the DxeIpl, which will rely on the PPI (via the
> > image loader and directly) to map the DXE core and the NX stack. The
> > DxeIpl will not proceed with that until all PEIMs are dispatched, so
> > if the PPI is going to be exposed, it will be available by that time.
> > - Then there are the shadowed PEIMs, and given that shadowed PEIMs
> > implicitly depend on PEI permanent memory having been installed, the
> > only requirement here is that, if the platform needs/wants this for
> > shadowed PEIMs as well, the PEIM that installs the PEI permanent
> > memory should depex on the PPI.
> >
>
> I see, thanks for the explanation. While this is not the main point
> of this patchset, I do think that the core should own the memory
> protections of the shadowed PEIMs, instead of relying on the platform
> to do the right thing (which it never does :). Sure, this is gated by
> PCDs that the platform sets, but that is a simpler interface than
> requiring a new depex.
>
> Perhaps this is not trivial, as I assume the permanent memory
> installation happens in a platform specific PEIM? And as such would
> require a PPI notification in the core for the memory attributes PPI,
> which would lead to additional undesirable complexity.
>
This is simply a matter of policy. If we decide that shadowed PEIMs
must always be remapped read-only, we should reorganize the code to
ensure that, and disallow shadowing otherwise. Shadowing is a
performance optimization in any case, although existing code may not
work, due to the way RegisterForShadow() is designed. But this is all
fixable.
But that doesn't change the fact that installing PEI permanent memory
is fundamentally something that is implemented in platform code. Each
platform that already implements this should be able to take advantage
of those core changes once they land.
^ permalink raw reply [flat|nested] 31+ messages in thread
* [RFC PATCH 08/10] MdeModulePkg/DxeIpl: Merge EBC, RISCV64 and LOONGARCH code
2023-05-25 14:30 [RFC PATCH 00/10] Add PPI to manage PEI phase memory attributes Ard Biesheuvel
` (6 preceding siblings ...)
2023-05-25 14:30 ` [RFC PATCH 07/10] MdeModulePkg/PeiCore: Apply restricted permissions in image loader Ard Biesheuvel
@ 2023-05-25 14:30 ` Ard Biesheuvel
2023-05-25 14:30 ` [RFC PATCH 09/10] MdeModulePkg/DxeIpl: Use memory attribute PPI to remap the stack NX Ard Biesheuvel
` (2 subsequent siblings)
10 siblings, 0 replies; 31+ messages in thread
From: Ard Biesheuvel @ 2023-05-25 14:30 UTC (permalink / raw)
To: devel
Cc: Ard Biesheuvel, Ray Ni, Jiewen Yao, Gerd Hoffmann, Taylor Beebe,
Oliver Smith-Denny, Dandan Bi, Liming Gao, Kinney, Michael D,
Leif Lindholm, Sunil V L, Andrei Warkentin
The Risc-V and LoongArch specific versions of the DXE core handoff code
in DxeIpl are essentially copies of the EBC version (modulo the
copyright in the header and some debug prints in the code).
In preparation for introducing a generic PPI based method to implement
the non-executable stack, let's merge these versions, so we only need to
add this logic once.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
MdeModulePkg/Core/DxeIplPeim/{Ebc/DxeLoadFunc.c => DxeHandoff.c} | 2 +-
MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf | 10 +--
MdeModulePkg/Core/DxeIplPeim/LoongArch64/DxeLoadFunc.c | 63 ----------------
MdeModulePkg/Core/DxeIplPeim/RiscV64/DxeLoadFunc.c | 75 --------------------
4 files changed, 3 insertions(+), 147 deletions(-)
diff --git a/MdeModulePkg/Core/DxeIplPeim/Ebc/DxeLoadFunc.c b/MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c
similarity index 92%
rename from MdeModulePkg/Core/DxeIplPeim/Ebc/DxeLoadFunc.c
rename to MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c
index c1a16b602452218e..a0f85ebea56e6cba 100644
--- a/MdeModulePkg/Core/DxeIplPeim/Ebc/DxeLoadFunc.c
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c
@@ -1,5 +1,5 @@
/** @file
- EBC-specific functionality for DxeLoad.
+ Generic version of arch-specific functionality for DxeLoad.
Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
index 052ea0ec1a6f2771..60c998be6c1bad01 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
@@ -45,17 +45,11 @@ [Sources.X64]
X64/VirtualMemory.c
X64/DxeLoadFunc.c
-[Sources.EBC]
- Ebc/DxeLoadFunc.c
-
[Sources.ARM, Sources.AARCH64]
Arm/DxeLoadFunc.c
-[Sources.RISCV64]
- RiscV64/DxeLoadFunc.c
-
-[Sources.LOONGARCH64]
- LoongArch64/DxeLoadFunc.c
+[Sources.LOONGARCH64,Sources.RISCV64,Sources.EBC]
+ DxeHandoff.c
[Packages]
MdePkg/MdePkg.dec
diff --git a/MdeModulePkg/Core/DxeIplPeim/LoongArch64/DxeLoadFunc.c b/MdeModulePkg/Core/DxeIplPeim/LoongArch64/DxeLoadFunc.c
deleted file mode 100644
index 95d3af19ea4c9f00..0000000000000000
--- a/MdeModulePkg/Core/DxeIplPeim/LoongArch64/DxeLoadFunc.c
+++ /dev/null
@@ -1,63 +0,0 @@
-/** @file
- LoongArch specifc functionality for DxeLoad.
-
- Copyright (c) 2022, Loongson Technology Corporation Limited. All rights reserved.<BR>
-
- SPDX-License-Identifier: BSD-2-Clause-Patent
-
-**/
-
-#include "DxeIpl.h"
-
-/**
- Transfers control to DxeCore.
-
- This function performs a CPU architecture specific operations to execute
- the entry point of DxeCore with the parameters of HobList.
- It also installs EFI_END_OF_PEI_PPI to signal the end of PEI phase.
-
- @param[in] DxeCoreEntryPoint The entry point of DxeCore.
- @param[in] HobList The start of HobList passed to DxeCore.
-
-**/
-VOID
-HandOffToDxeCore (
- IN EFI_PHYSICAL_ADDRESS DxeCoreEntryPoint,
- IN EFI_PEI_HOB_POINTERS HobList
- )
-{
- VOID *BaseOfStack;
- VOID *TopOfStack;
- EFI_STATUS Status;
-
- //
- // Allocate 128KB for the Stack
- //
- BaseOfStack = AllocatePages (EFI_SIZE_TO_PAGES (STACK_SIZE));
- ASSERT (BaseOfStack != NULL);
-
- //
- // Compute the top of the stack we were allocated. Pre-allocate a UINTN
- // for safety.
- //
- TopOfStack = (VOID *)((UINTN)BaseOfStack + EFI_SIZE_TO_PAGES (STACK_SIZE) * EFI_PAGE_SIZE - CPU_STACK_ALIGNMENT);
- TopOfStack = ALIGN_POINTER (TopOfStack, CPU_STACK_ALIGNMENT);
-
- //
- // End of PEI phase signal
- //
- Status = PeiServicesInstallPpi (&gEndOfPeiSignalPpi);
- ASSERT_EFI_ERROR (Status);
-
- //
- // Update the contents of BSP stack HOB to reflect the real stack info passed to DxeCore.
- //
- UpdateStackHob ((EFI_PHYSICAL_ADDRESS)(UINTN)BaseOfStack, STACK_SIZE);
-
- SwitchStack (
- (SWITCH_STACK_ENTRY_POINT)(UINTN)DxeCoreEntryPoint,
- HobList.Raw,
- NULL,
- TopOfStack
- );
-}
diff --git a/MdeModulePkg/Core/DxeIplPeim/RiscV64/DxeLoadFunc.c b/MdeModulePkg/Core/DxeIplPeim/RiscV64/DxeLoadFunc.c
deleted file mode 100644
index b3567d88f73467e7..0000000000000000
--- a/MdeModulePkg/Core/DxeIplPeim/RiscV64/DxeLoadFunc.c
+++ /dev/null
@@ -1,75 +0,0 @@
-/** @file
- RISC-V specific functionality for DxeLoad.
-
- Copyright (c) 2020, Hewlett Packard Enterprise Development LP. All rights reserved.<BR>
-
- SPDX-License-Identifier: BSD-2-Clause-Patent
-
-**/
-
-#include "DxeIpl.h"
-
-/**
- Transfers control to DxeCore.
-
- This function performs a CPU architecture specific operations to execute
- the entry point of DxeCore with the parameters of HobList.
- It also installs EFI_END_OF_PEI_PPI to signal the end of PEI phase.
-
- @param DxeCoreEntryPoint The entry point of DxeCore.
- @param HobList The start of HobList passed to DxeCore.
-
-**/
-VOID
-HandOffToDxeCore (
- IN EFI_PHYSICAL_ADDRESS DxeCoreEntryPoint,
- IN EFI_PEI_HOB_POINTERS HobList
- )
-{
- VOID *BaseOfStack;
- VOID *TopOfStack;
- EFI_STATUS Status;
-
- //
- //
- // Allocate 128KB for the Stack
- //
- BaseOfStack = AllocatePages (EFI_SIZE_TO_PAGES (STACK_SIZE));
- if (BaseOfStack == NULL) {
- DEBUG ((DEBUG_ERROR, "%a: Can't allocate memory for stack.", __func__));
- ASSERT (FALSE);
- }
-
- //
- // Compute the top of the stack we were allocated. Pre-allocate a UINTN
- // for safety.
- //
- TopOfStack = (VOID *)((UINTN)BaseOfStack + EFI_SIZE_TO_PAGES (STACK_SIZE) * EFI_PAGE_SIZE - CPU_STACK_ALIGNMENT);
- TopOfStack = ALIGN_POINTER (TopOfStack, CPU_STACK_ALIGNMENT);
-
- //
- // End of PEI phase signal
- //
- Status = PeiServicesInstallPpi (&gEndOfPeiSignalPpi);
- if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_ERROR, "%a: Fail to signal End of PEI event.", __func__));
- ASSERT (FALSE);
- }
-
- //
- // Update the contents of BSP stack HOB to reflect the real stack info passed to DxeCore.
- //
- UpdateStackHob ((EFI_PHYSICAL_ADDRESS)(UINTN)BaseOfStack, STACK_SIZE);
-
- DEBUG ((DEBUG_INFO, "DXE Core new stack at %x, stack pointer at %x\n", BaseOfStack, TopOfStack));
-
- //
- // Transfer the control to the entry point of DxeCore.
- //
- SwitchStack (
- (SWITCH_STACK_ENTRY_POINT)(UINTN)DxeCoreEntryPoint,
- HobList.Raw,
- NULL,
- TopOfStack
- );
-}
--
2.39.2
^ permalink raw reply related [flat|nested] 31+ messages in thread* [RFC PATCH 09/10] MdeModulePkg/DxeIpl: Use memory attribute PPI to remap the stack NX
2023-05-25 14:30 [RFC PATCH 00/10] Add PPI to manage PEI phase memory attributes Ard Biesheuvel
` (7 preceding siblings ...)
2023-05-25 14:30 ` [RFC PATCH 08/10] MdeModulePkg/DxeIpl: Merge EBC, RISCV64 and LOONGARCH code Ard Biesheuvel
@ 2023-05-25 14:30 ` Ard Biesheuvel
2023-05-30 7:19 ` Ni, Ray
2023-05-25 14:30 ` [RFC PATCH 10/10] MdeModulePkg/DxeIpl ARM AARCH64: Switch to generic handoff code Ard Biesheuvel
2023-05-25 17:20 ` [edk2-devel] [RFC PATCH 00/10] Add PPI to manage PEI phase memory attributes Oliver Smith-Denny
10 siblings, 1 reply; 31+ messages in thread
From: Ard Biesheuvel @ 2023-05-25 14:30 UTC (permalink / raw)
To: devel
Cc: Ard Biesheuvel, Ray Ni, Jiewen Yao, Gerd Hoffmann, Taylor Beebe,
Oliver Smith-Denny, Dandan Bi, Liming Gao, Kinney, Michael D,
Leif Lindholm, Sunil V L, Andrei Warkentin
If the associated PCD is set to TRUE, use the memory attribute PPI to
remap the stack non-executable. This provides a generic method for doing
so, which will be used by ARM and AArch64 as well once they move to the
generic DxeIpl handoff implementation.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c | 29 ++++++++++++++++++--
MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf | 5 +++-
2 files changed, 30 insertions(+), 4 deletions(-)
diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c b/MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c
index a0f85ebea56e6cba..22caabb02840ba88 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c
@@ -2,12 +2,15 @@
Generic version of arch-specific functionality for DxeLoad.
Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2023, Google, LLC. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
#include "DxeIpl.h"
+#include <Ppi/MemoryAttribute.h>
+
/**
Transfers control to DxeCore.
@@ -25,9 +28,10 @@ HandOffToDxeCore (
IN EFI_PEI_HOB_POINTERS HobList
)
{
- VOID *BaseOfStack;
- VOID *TopOfStack;
- EFI_STATUS Status;
+ VOID *BaseOfStack;
+ VOID *TopOfStack;
+ EFI_STATUS Status;
+ EDKII_MEMORY_ATTRIBUTE_PPI *MemoryPpi;
//
// Allocate 128KB for the Stack
@@ -35,6 +39,25 @@ HandOffToDxeCore (
BaseOfStack = AllocatePages (EFI_SIZE_TO_PAGES (STACK_SIZE));
ASSERT (BaseOfStack != NULL);
+ if (PcdGetBool (PcdSetNxForStack)) {
+ Status = PeiServicesLocatePpi (
+ &gEdkiiMemoryAttributePpiGuid,
+ 0,
+ NULL,
+ (VOID **)&MemoryPpi
+ );
+ ASSERT_EFI_ERROR (Status);
+
+ Status = MemoryPpi->SetPermissions (
+ MemoryPpi,
+ (UINTN)BaseOfStack,
+ STACK_SIZE,
+ EFI_MEMORY_XP,
+ 0
+ );
+ ASSERT_EFI_ERROR (Status);
+ }
+
//
// Compute the top of the stack we were allocated. Pre-allocate a UINTN
// for safety.
diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
index 60c998be6c1bad01..7126a96d8378d1f8 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
@@ -91,6 +91,7 @@ [Ppis]
gEfiPeiMemoryDiscoveredPpiGuid ## SOMETIMES_CONSUMES
gEdkiiPeiBootInCapsuleOnDiskModePpiGuid ## SOMETIMES_CONSUMES
gEdkiiPeiCapsuleOnDiskPpiGuid ## SOMETIMES_CONSUMES # Consumed on firmware update boot path
+ gEdkiiMemoryAttributePpiGuid ## SOMETIMES_CONSUMES
[Guids]
## SOMETIMES_CONSUMES ## Variable:L"MemoryTypeInformation"
@@ -117,10 +118,12 @@ [Pcd.IA32,Pcd.X64]
gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbSize ## CONSUMES
[Pcd.IA32,Pcd.X64,Pcd.ARM,Pcd.AARCH64]
- gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack ## SOMETIMES_CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy ## SOMETIMES_CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy ## SOMETIMES_CONSUMES
+[Pcd]
+ gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack ## SOMETIMES_CONSUMES
+
[Depex]
gEfiPeiLoadFilePpiGuid AND gEfiPeiMasterBootModePpiGuid
--
2.39.2
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [RFC PATCH 09/10] MdeModulePkg/DxeIpl: Use memory attribute PPI to remap the stack NX
2023-05-25 14:30 ` [RFC PATCH 09/10] MdeModulePkg/DxeIpl: Use memory attribute PPI to remap the stack NX Ard Biesheuvel
@ 2023-05-30 7:19 ` Ni, Ray
2023-05-30 10:25 ` duntan
0 siblings, 1 reply; 31+ messages in thread
From: Ni, Ray @ 2023-05-30 7:19 UTC (permalink / raw)
To: Ard Biesheuvel, devel@edk2.groups.io, Tan, Dun
Cc: Yao, Jiewen, Gerd Hoffmann, Taylor Beebe, Oliver Smith-Denny,
Bi, Dandan, Gao, Liming, Kinney, Michael D, Leif Lindholm,
Sunil V L, Warkentin, Andrei
Looks good.
@Tan, Dun, can you please evaluate if using MemoryAttribute PPI, what opens will there be for X64 DxeIpl?
> -----Original Message-----
> From: Ard Biesheuvel <ardb@kernel.org>
> Sent: Thursday, May 25, 2023 10:31 PM
> To: devel@edk2.groups.io
> Cc: Ard Biesheuvel <ardb@kernel.org>; Ni, Ray <ray.ni@intel.com>; Yao, Jiewen
> <jiewen.yao@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Taylor Beebe
> <t@taylorbeebe.com>; Oliver Smith-Denny <osd@smith-denny.com>; Bi, Dandan
> <dandan.bi@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Kinney,
> Michael D <michael.d.kinney@intel.com>; Leif Lindholm
> <quic_llindhol@quicinc.com>; Sunil V L <sunilvl@ventanamicro.com>; Warkentin,
> Andrei <andrei.warkentin@intel.com>
> Subject: [RFC PATCH 09/10] MdeModulePkg/DxeIpl: Use memory attribute PPI to
> remap the stack NX
>
> If the associated PCD is set to TRUE, use the memory attribute PPI to
> remap the stack non-executable. This provides a generic method for doing
> so, which will be used by ARM and AArch64 as well once they move to the
> generic DxeIpl handoff implementation.
>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
> MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c | 29 ++++++++++++++++++--
> MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf | 5 +++-
> 2 files changed, 30 insertions(+), 4 deletions(-)
>
> diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c
> b/MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c
> index a0f85ebea56e6cba..22caabb02840ba88 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c
> +++ b/MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c
> @@ -2,12 +2,15 @@
> Generic version of arch-specific functionality for DxeLoad.
>
>
>
> Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
>
> +Copyright (c) 2023, Google, LLC. All rights reserved.<BR>
>
> SPDX-License-Identifier: BSD-2-Clause-Patent
>
>
>
> **/
>
>
>
> #include "DxeIpl.h"
>
>
>
> +#include <Ppi/MemoryAttribute.h>
>
> +
>
> /**
>
> Transfers control to DxeCore.
>
>
>
> @@ -25,9 +28,10 @@ HandOffToDxeCore (
> IN EFI_PEI_HOB_POINTERS HobList
>
> )
>
> {
>
> - VOID *BaseOfStack;
>
> - VOID *TopOfStack;
>
> - EFI_STATUS Status;
>
> + VOID *BaseOfStack;
>
> + VOID *TopOfStack;
>
> + EFI_STATUS Status;
>
> + EDKII_MEMORY_ATTRIBUTE_PPI *MemoryPpi;
>
>
>
> //
>
> // Allocate 128KB for the Stack
>
> @@ -35,6 +39,25 @@ HandOffToDxeCore (
> BaseOfStack = AllocatePages (EFI_SIZE_TO_PAGES (STACK_SIZE));
>
> ASSERT (BaseOfStack != NULL);
>
>
>
> + if (PcdGetBool (PcdSetNxForStack)) {
>
> + Status = PeiServicesLocatePpi (
>
> + &gEdkiiMemoryAttributePpiGuid,
>
> + 0,
>
> + NULL,
>
> + (VOID **)&MemoryPpi
>
> + );
>
> + ASSERT_EFI_ERROR (Status);
>
> +
>
> + Status = MemoryPpi->SetPermissions (
>
> + MemoryPpi,
>
> + (UINTN)BaseOfStack,
>
> + STACK_SIZE,
>
> + EFI_MEMORY_XP,
>
> + 0
>
> + );
>
> + ASSERT_EFI_ERROR (Status);
>
> + }
>
> +
>
> //
>
> // Compute the top of the stack we were allocated. Pre-allocate a UINTN
>
> // for safety.
>
> diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> index 60c998be6c1bad01..7126a96d8378d1f8 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> +++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> @@ -91,6 +91,7 @@ [Ppis]
> gEfiPeiMemoryDiscoveredPpiGuid ## SOMETIMES_CONSUMES
>
> gEdkiiPeiBootInCapsuleOnDiskModePpiGuid ## SOMETIMES_CONSUMES
>
> gEdkiiPeiCapsuleOnDiskPpiGuid ## SOMETIMES_CONSUMES # Consumed
> on firmware update boot path
>
> + gEdkiiMemoryAttributePpiGuid ## SOMETIMES_CONSUMES
>
>
>
> [Guids]
>
> ## SOMETIMES_CONSUMES ## Variable:L"MemoryTypeInformation"
>
> @@ -117,10 +118,12 @@ [Pcd.IA32,Pcd.X64]
> gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbSize ## CONSUMES
>
>
>
> [Pcd.IA32,Pcd.X64,Pcd.ARM,Pcd.AARCH64]
>
> - gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack ##
> SOMETIMES_CONSUMES
>
> gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy ##
> SOMETIMES_CONSUMES
>
> gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy ##
> SOMETIMES_CONSUMES
>
>
>
> +[Pcd]
>
> + gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack ##
> SOMETIMES_CONSUMES
>
> +
>
> [Depex]
>
> gEfiPeiLoadFilePpiGuid AND gEfiPeiMasterBootModePpiGuid
>
>
>
> --
> 2.39.2
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [RFC PATCH 09/10] MdeModulePkg/DxeIpl: Use memory attribute PPI to remap the stack NX
2023-05-30 7:19 ` Ni, Ray
@ 2023-05-30 10:25 ` duntan
2023-05-30 12:51 ` Ard Biesheuvel
2023-05-31 1:29 ` Ni, Ray
0 siblings, 2 replies; 31+ messages in thread
From: duntan @ 2023-05-30 10:25 UTC (permalink / raw)
To: Ni, Ray, Ard Biesheuvel, devel@edk2.groups.io
Cc: Yao, Jiewen, Gerd Hoffmann, Taylor Beebe, Oliver Smith-Denny,
Bi, Dandan, Gao, Liming, Kinney, Michael D, Leif Lindholm,
Sunil V L, Warkentin, Andrei
Ray,
I think using MemoryAttribute PPI also looks good for X64 DxeIpl.
The only question that comes to my mind is the AMD sev feature. Since the MemoryAttribute can't handle the AMD sev feature requirements(remapping ghcb range from non-1:1 mapping to 1:1-mapping), we may need to find an appropriate place to remap the Ghcb range.
Thanks,
Dun
-----Original Message-----
From: Ni, Ray <ray.ni@intel.com>
Sent: Tuesday, May 30, 2023 3:19 PM
To: Ard Biesheuvel <ardb@kernel.org>; devel@edk2.groups.io; Tan, Dun <dun.tan@intel.com>
Cc: Yao, Jiewen <jiewen.yao@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Taylor Beebe <t@taylorbeebe.com>; Oliver Smith-Denny <osd@smith-denny.com>; Bi, Dandan <dandan.bi@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Kinney, Michael D <michael.d.kinney@intel.com>; Leif Lindholm <quic_llindhol@quicinc.com>; Sunil V L <sunilvl@ventanamicro.com>; Warkentin, Andrei <andrei.warkentin@intel.com>
Subject: RE: [RFC PATCH 09/10] MdeModulePkg/DxeIpl: Use memory attribute PPI to remap the stack NX
Looks good.
@Tan, Dun, can you please evaluate if using MemoryAttribute PPI, what opens will there be for X64 DxeIpl?
> -----Original Message-----
> From: Ard Biesheuvel <ardb@kernel.org>
> Sent: Thursday, May 25, 2023 10:31 PM
> To: devel@edk2.groups.io
> Cc: Ard Biesheuvel <ardb@kernel.org>; Ni, Ray <ray.ni@intel.com>; Yao,
> Jiewen <jiewen.yao@intel.com>; Gerd Hoffmann <kraxel@redhat.com>;
> Taylor Beebe <t@taylorbeebe.com>; Oliver Smith-Denny
> <osd@smith-denny.com>; Bi, Dandan <dandan.bi@intel.com>; Gao, Liming
> <gaoliming@byosoft.com.cn>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Leif Lindholm
> <quic_llindhol@quicinc.com>; Sunil V L <sunilvl@ventanamicro.com>;
> Warkentin, Andrei <andrei.warkentin@intel.com>
> Subject: [RFC PATCH 09/10] MdeModulePkg/DxeIpl: Use memory attribute
> PPI to remap the stack NX
>
> If the associated PCD is set to TRUE, use the memory attribute PPI to
> remap the stack non-executable. This provides a generic method for
> doing so, which will be used by ARM and AArch64 as well once they move
> to the generic DxeIpl handoff implementation.
>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
> MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c | 29 ++++++++++++++++++--
> MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf | 5 +++-
> 2 files changed, 30 insertions(+), 4 deletions(-)
>
> diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c
> b/MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c
> index a0f85ebea56e6cba..22caabb02840ba88 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c
> +++ b/MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c
> @@ -2,12 +2,15 @@
> Generic version of arch-specific functionality for DxeLoad.
>
>
>
> Copyright (c) 2006 - 2018, Intel Corporation. All rights
> reserved.<BR>
>
> +Copyright (c) 2023, Google, LLC. All rights reserved.<BR>
>
> SPDX-License-Identifier: BSD-2-Clause-Patent
>
>
>
> **/
>
>
>
> #include "DxeIpl.h"
>
>
>
> +#include <Ppi/MemoryAttribute.h>
>
> +
>
> /**
>
> Transfers control to DxeCore.
>
>
>
> @@ -25,9 +28,10 @@ HandOffToDxeCore (
> IN EFI_PEI_HOB_POINTERS HobList
>
> )
>
> {
>
> - VOID *BaseOfStack;
>
> - VOID *TopOfStack;
>
> - EFI_STATUS Status;
>
> + VOID *BaseOfStack;
>
> + VOID *TopOfStack;
>
> + EFI_STATUS Status;
>
> + EDKII_MEMORY_ATTRIBUTE_PPI *MemoryPpi;
>
>
>
> //
>
> // Allocate 128KB for the Stack
>
> @@ -35,6 +39,25 @@ HandOffToDxeCore (
> BaseOfStack = AllocatePages (EFI_SIZE_TO_PAGES (STACK_SIZE));
>
> ASSERT (BaseOfStack != NULL);
>
>
>
> + if (PcdGetBool (PcdSetNxForStack)) {
>
> + Status = PeiServicesLocatePpi (
>
> + &gEdkiiMemoryAttributePpiGuid,
>
> + 0,
>
> + NULL,
>
> + (VOID **)&MemoryPpi
>
> + );
>
> + ASSERT_EFI_ERROR (Status);
>
> +
>
> + Status = MemoryPpi->SetPermissions (
>
> + MemoryPpi,
>
> + (UINTN)BaseOfStack,
>
> + STACK_SIZE,
>
> + EFI_MEMORY_XP,
>
> + 0
>
> + );
>
> + ASSERT_EFI_ERROR (Status);
>
> + }
>
> +
>
> //
>
> // Compute the top of the stack we were allocated. Pre-allocate a
> UINTN
>
> // for safety.
>
> diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> index 60c998be6c1bad01..7126a96d8378d1f8 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> +++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> @@ -91,6 +91,7 @@ [Ppis]
> gEfiPeiMemoryDiscoveredPpiGuid ## SOMETIMES_CONSUMES
>
> gEdkiiPeiBootInCapsuleOnDiskModePpiGuid ## SOMETIMES_CONSUMES
>
> gEdkiiPeiCapsuleOnDiskPpiGuid ## SOMETIMES_CONSUMES # Consumed
> on firmware update boot path
>
> + gEdkiiMemoryAttributePpiGuid ## SOMETIMES_CONSUMES
>
>
>
> [Guids]
>
> ## SOMETIMES_CONSUMES ## Variable:L"MemoryTypeInformation"
>
> @@ -117,10 +118,12 @@ [Pcd.IA32,Pcd.X64]
> gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbSize ## CONSUMES
>
>
>
> [Pcd.IA32,Pcd.X64,Pcd.ARM,Pcd.AARCH64]
>
> - gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack ##
> SOMETIMES_CONSUMES
>
> gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy ##
> SOMETIMES_CONSUMES
>
> gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy ##
> SOMETIMES_CONSUMES
>
>
>
> +[Pcd]
>
> + gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack ##
> SOMETIMES_CONSUMES
>
> +
>
> [Depex]
>
> gEfiPeiLoadFilePpiGuid AND gEfiPeiMasterBootModePpiGuid
>
>
>
> --
> 2.39.2
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [RFC PATCH 09/10] MdeModulePkg/DxeIpl: Use memory attribute PPI to remap the stack NX
2023-05-30 10:25 ` duntan
@ 2023-05-30 12:51 ` Ard Biesheuvel
2023-05-31 7:22 ` Gerd Hoffmann
2023-05-31 1:29 ` Ni, Ray
1 sibling, 1 reply; 31+ messages in thread
From: Ard Biesheuvel @ 2023-05-30 12:51 UTC (permalink / raw)
To: Tan, Dun
Cc: Ni, Ray, devel@edk2.groups.io, Yao, Jiewen, Gerd Hoffmann,
Taylor Beebe, Oliver Smith-Denny, Bi, Dandan, Gao, Liming,
Kinney, Michael D, Leif Lindholm, Sunil V L, Warkentin, Andrei
On Tue, 30 May 2023 at 12:25, Tan, Dun <dun.tan@intel.com> wrote:
>
> Ray,
> I think using MemoryAttribute PPI also looks good for X64 DxeIpl.
> The only question that comes to my mind is the AMD sev feature. Since the MemoryAttribute can't handle the AMD sev feature requirements(remapping ghcb range from non-1:1 mapping to 1:1-mapping), we may need to find an appropriate place to remap the Ghcb range.
>
To me, the structure of that code seems entirely wrong, to be honest.
If SEV requires an additional step to be performed at end of PEI, it
should be done in a separate PEIM that registers a notification on
that signal.
However, I must admit that the X64 PEI logic is confusing to me, so I
may have missed something here. It seems to me that creating an
entirely new set of page tables in DxeIpl is both redundant (as PEI
already executes in long mode, and therefore uses page tables) and
undesirable, as it remaps the entire address space read/write/execute
without any awareness of what those regions may contain.
Would it be possible to remove this logic from DxeIpl, and set up the
page tables properly during PEI?
> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Tuesday, May 30, 2023 3:19 PM
> To: Ard Biesheuvel <ardb@kernel.org>; devel@edk2.groups.io; Tan, Dun <dun.tan@intel.com>
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Taylor Beebe <t@taylorbeebe.com>; Oliver Smith-Denny <osd@smith-denny.com>; Bi, Dandan <dandan.bi@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Kinney, Michael D <michael.d.kinney@intel.com>; Leif Lindholm <quic_llindhol@quicinc.com>; Sunil V L <sunilvl@ventanamicro.com>; Warkentin, Andrei <andrei.warkentin@intel.com>
> Subject: RE: [RFC PATCH 09/10] MdeModulePkg/DxeIpl: Use memory attribute PPI to remap the stack NX
>
> Looks good.
>
> @Tan, Dun, can you please evaluate if using MemoryAttribute PPI, what opens will there be for X64 DxeIpl?
>
> > -----Original Message-----
> > From: Ard Biesheuvel <ardb@kernel.org>
> > Sent: Thursday, May 25, 2023 10:31 PM
> > To: devel@edk2.groups.io
> > Cc: Ard Biesheuvel <ardb@kernel.org>; Ni, Ray <ray.ni@intel.com>; Yao,
> > Jiewen <jiewen.yao@intel.com>; Gerd Hoffmann <kraxel@redhat.com>;
> > Taylor Beebe <t@taylorbeebe.com>; Oliver Smith-Denny
> > <osd@smith-denny.com>; Bi, Dandan <dandan.bi@intel.com>; Gao, Liming
> > <gaoliming@byosoft.com.cn>; Kinney, Michael D
> > <michael.d.kinney@intel.com>; Leif Lindholm
> > <quic_llindhol@quicinc.com>; Sunil V L <sunilvl@ventanamicro.com>;
> > Warkentin, Andrei <andrei.warkentin@intel.com>
> > Subject: [RFC PATCH 09/10] MdeModulePkg/DxeIpl: Use memory attribute
> > PPI to remap the stack NX
> >
> > If the associated PCD is set to TRUE, use the memory attribute PPI to
> > remap the stack non-executable. This provides a generic method for
> > doing so, which will be used by ARM and AArch64 as well once they move
> > to the generic DxeIpl handoff implementation.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> > MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c | 29 ++++++++++++++++++--
> > MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf | 5 +++-
> > 2 files changed, 30 insertions(+), 4 deletions(-)
> >
> > diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c
> > b/MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c
> > index a0f85ebea56e6cba..22caabb02840ba88 100644
> > --- a/MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c
> > +++ b/MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c
> > @@ -2,12 +2,15 @@
> > Generic version of arch-specific functionality for DxeLoad.
> >
> >
> >
> > Copyright (c) 2006 - 2018, Intel Corporation. All rights
> > reserved.<BR>
> >
> > +Copyright (c) 2023, Google, LLC. All rights reserved.<BR>
> >
> > SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> >
> >
> > **/
> >
> >
> >
> > #include "DxeIpl.h"
> >
> >
> >
> > +#include <Ppi/MemoryAttribute.h>
> >
> > +
> >
> > /**
> >
> > Transfers control to DxeCore.
> >
> >
> >
> > @@ -25,9 +28,10 @@ HandOffToDxeCore (
> > IN EFI_PEI_HOB_POINTERS HobList
> >
> > )
> >
> > {
> >
> > - VOID *BaseOfStack;
> >
> > - VOID *TopOfStack;
> >
> > - EFI_STATUS Status;
> >
> > + VOID *BaseOfStack;
> >
> > + VOID *TopOfStack;
> >
> > + EFI_STATUS Status;
> >
> > + EDKII_MEMORY_ATTRIBUTE_PPI *MemoryPpi;
> >
> >
> >
> > //
> >
> > // Allocate 128KB for the Stack
> >
> > @@ -35,6 +39,25 @@ HandOffToDxeCore (
> > BaseOfStack = AllocatePages (EFI_SIZE_TO_PAGES (STACK_SIZE));
> >
> > ASSERT (BaseOfStack != NULL);
> >
> >
> >
> > + if (PcdGetBool (PcdSetNxForStack)) {
> >
> > + Status = PeiServicesLocatePpi (
> >
> > + &gEdkiiMemoryAttributePpiGuid,
> >
> > + 0,
> >
> > + NULL,
> >
> > + (VOID **)&MemoryPpi
> >
> > + );
> >
> > + ASSERT_EFI_ERROR (Status);
> >
> > +
> >
> > + Status = MemoryPpi->SetPermissions (
> >
> > + MemoryPpi,
> >
> > + (UINTN)BaseOfStack,
> >
> > + STACK_SIZE,
> >
> > + EFI_MEMORY_XP,
> >
> > + 0
> >
> > + );
> >
> > + ASSERT_EFI_ERROR (Status);
> >
> > + }
> >
> > +
> >
> > //
> >
> > // Compute the top of the stack we were allocated. Pre-allocate a
> > UINTN
> >
> > // for safety.
> >
> > diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> > b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> > index 60c998be6c1bad01..7126a96d8378d1f8 100644
> > --- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> > +++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> > @@ -91,6 +91,7 @@ [Ppis]
> > gEfiPeiMemoryDiscoveredPpiGuid ## SOMETIMES_CONSUMES
> >
> > gEdkiiPeiBootInCapsuleOnDiskModePpiGuid ## SOMETIMES_CONSUMES
> >
> > gEdkiiPeiCapsuleOnDiskPpiGuid ## SOMETIMES_CONSUMES # Consumed
> > on firmware update boot path
> >
> > + gEdkiiMemoryAttributePpiGuid ## SOMETIMES_CONSUMES
> >
> >
> >
> > [Guids]
> >
> > ## SOMETIMES_CONSUMES ## Variable:L"MemoryTypeInformation"
> >
> > @@ -117,10 +118,12 @@ [Pcd.IA32,Pcd.X64]
> > gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbSize ## CONSUMES
> >
> >
> >
> > [Pcd.IA32,Pcd.X64,Pcd.ARM,Pcd.AARCH64]
> >
> > - gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack ##
> > SOMETIMES_CONSUMES
> >
> > gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy ##
> > SOMETIMES_CONSUMES
> >
> > gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy ##
> > SOMETIMES_CONSUMES
> >
> >
> >
> > +[Pcd]
> >
> > + gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack ##
> > SOMETIMES_CONSUMES
> >
> > +
> >
> > [Depex]
> >
> > gEfiPeiLoadFilePpiGuid AND gEfiPeiMasterBootModePpiGuid
> >
> >
> >
> > --
> > 2.39.2
>
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [RFC PATCH 09/10] MdeModulePkg/DxeIpl: Use memory attribute PPI to remap the stack NX
2023-05-30 12:51 ` Ard Biesheuvel
@ 2023-05-31 7:22 ` Gerd Hoffmann
0 siblings, 0 replies; 31+ messages in thread
From: Gerd Hoffmann @ 2023-05-31 7:22 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Tan, Dun, Ni, Ray, devel@edk2.groups.io, Yao, Jiewen,
Taylor Beebe, Oliver Smith-Denny, Bi, Dandan, Gao, Liming,
Kinney, Michael D, Leif Lindholm, Sunil V L, Warkentin, Andrei
Hi,
> However, I must admit that the X64 PEI logic is confusing to me, so I
> may have missed something here. It seems to me that creating an
> entirely new set of page tables in DxeIpl is both redundant (as PEI
> already executes in long mode, and therefore uses page tables)
Well, there is the 32bit PEI / 64bit DXE case. Which might be the
reason why a new set of page tables is created (unconditionally) even
though it should not be needed in the 64bit PEI / 64bit DXE case.
take care,
Gerd
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH 09/10] MdeModulePkg/DxeIpl: Use memory attribute PPI to remap the stack NX
2023-05-30 10:25 ` duntan
2023-05-30 12:51 ` Ard Biesheuvel
@ 2023-05-31 1:29 ` Ni, Ray
2023-05-31 19:03 ` [edk2-devel] " Lendacky, Thomas
1 sibling, 1 reply; 31+ messages in thread
From: Ni, Ray @ 2023-05-31 1:29 UTC (permalink / raw)
To: Tan, Dun, Ard Biesheuvel, devel@edk2.groups.io, Abner Chang,
Tom Lendacky
Cc: Yao, Jiewen, Gerd Hoffmann, Taylor Beebe, Oliver Smith-Denny,
Bi, Dandan, Gao, Liming, Kinney, Michael D, Leif Lindholm,
Sunil V L, Warkentin, Andrei
+@Abner Chang and @Tom Lendacky
> -----Original Message-----
> From: Tan, Dun <dun.tan@intel.com>
> Sent: Tuesday, May 30, 2023 6:25 PM
> To: Ni, Ray <ray.ni@intel.com>; Ard Biesheuvel <ardb@kernel.org>;
> devel@edk2.groups.io
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Gerd Hoffmann
> <kraxel@redhat.com>; Taylor Beebe <t@taylorbeebe.com>; Oliver Smith-
> Denny <osd@smith-denny.com>; Bi, Dandan <dandan.bi@intel.com>; Gao,
> Liming <gaoliming@byosoft.com.cn>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Leif Lindholm <quic_llindhol@quicinc.com>;
> Sunil V L <sunilvl@ventanamicro.com>; Warkentin, Andrei
> <andrei.warkentin@intel.com>
> Subject: RE: [RFC PATCH 09/10] MdeModulePkg/DxeIpl: Use memory
> attribute PPI to remap the stack NX
>
> Ray,
> I think using MemoryAttribute PPI also looks good for X64 DxeIpl.
> The only question that comes to my mind is the AMD sev feature. Since the
> MemoryAttribute can't handle the AMD sev feature requirements(remapping
> ghcb range from non-1:1 mapping to 1:1-mapping), we may need to find an
> appropriate place to remap the Ghcb range.
>
> Thanks,
> Dun
>
> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Tuesday, May 30, 2023 3:19 PM
> To: Ard Biesheuvel <ardb@kernel.org>; devel@edk2.groups.io; Tan, Dun
> <dun.tan@intel.com>
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Gerd Hoffmann
> <kraxel@redhat.com>; Taylor Beebe <t@taylorbeebe.com>; Oliver Smith-
> Denny <osd@smith-denny.com>; Bi, Dandan <dandan.bi@intel.com>; Gao,
> Liming <gaoliming@byosoft.com.cn>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Leif Lindholm <quic_llindhol@quicinc.com>;
> Sunil V L <sunilvl@ventanamicro.com>; Warkentin, Andrei
> <andrei.warkentin@intel.com>
> Subject: RE: [RFC PATCH 09/10] MdeModulePkg/DxeIpl: Use memory
> attribute PPI to remap the stack NX
>
> Looks good.
>
> @Tan, Dun, can you please evaluate if using MemoryAttribute PPI, what
> opens will there be for X64 DxeIpl?
>
> > -----Original Message-----
> > From: Ard Biesheuvel <ardb@kernel.org>
> > Sent: Thursday, May 25, 2023 10:31 PM
> > To: devel@edk2.groups.io
> > Cc: Ard Biesheuvel <ardb@kernel.org>; Ni, Ray <ray.ni@intel.com>; Yao,
> > Jiewen <jiewen.yao@intel.com>; Gerd Hoffmann <kraxel@redhat.com>;
> > Taylor Beebe <t@taylorbeebe.com>; Oliver Smith-Denny
> > <osd@smith-denny.com>; Bi, Dandan <dandan.bi@intel.com>; Gao, Liming
> > <gaoliming@byosoft.com.cn>; Kinney, Michael D
> > <michael.d.kinney@intel.com>; Leif Lindholm
> > <quic_llindhol@quicinc.com>; Sunil V L <sunilvl@ventanamicro.com>;
> > Warkentin, Andrei <andrei.warkentin@intel.com>
> > Subject: [RFC PATCH 09/10] MdeModulePkg/DxeIpl: Use memory attribute
> > PPI to remap the stack NX
> >
> > If the associated PCD is set to TRUE, use the memory attribute PPI to
> > remap the stack non-executable. This provides a generic method for
> > doing so, which will be used by ARM and AArch64 as well once they move
> > to the generic DxeIpl handoff implementation.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> > MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c | 29
> ++++++++++++++++++--
> > MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf | 5 +++-
> > 2 files changed, 30 insertions(+), 4 deletions(-)
> >
> > diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c
> > b/MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c
> > index a0f85ebea56e6cba..22caabb02840ba88 100644
> > --- a/MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c
> > +++ b/MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c
> > @@ -2,12 +2,15 @@
> > Generic version of arch-specific functionality for DxeLoad.
> >
> >
> >
> > Copyright (c) 2006 - 2018, Intel Corporation. All rights
> > reserved.<BR>
> >
> > +Copyright (c) 2023, Google, LLC. All rights reserved.<BR>
> >
> > SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> >
> >
> > **/
> >
> >
> >
> > #include "DxeIpl.h"
> >
> >
> >
> > +#include <Ppi/MemoryAttribute.h>
> >
> > +
> >
> > /**
> >
> > Transfers control to DxeCore.
> >
> >
> >
> > @@ -25,9 +28,10 @@ HandOffToDxeCore (
> > IN EFI_PEI_HOB_POINTERS HobList
> >
> > )
> >
> > {
> >
> > - VOID *BaseOfStack;
> >
> > - VOID *TopOfStack;
> >
> > - EFI_STATUS Status;
> >
> > + VOID *BaseOfStack;
> >
> > + VOID *TopOfStack;
> >
> > + EFI_STATUS Status;
> >
> > + EDKII_MEMORY_ATTRIBUTE_PPI *MemoryPpi;
> >
> >
> >
> > //
> >
> > // Allocate 128KB for the Stack
> >
> > @@ -35,6 +39,25 @@ HandOffToDxeCore (
> > BaseOfStack = AllocatePages (EFI_SIZE_TO_PAGES (STACK_SIZE));
> >
> > ASSERT (BaseOfStack != NULL);
> >
> >
> >
> > + if (PcdGetBool (PcdSetNxForStack)) {
> >
> > + Status = PeiServicesLocatePpi (
> >
> > + &gEdkiiMemoryAttributePpiGuid,
> >
> > + 0,
> >
> > + NULL,
> >
> > + (VOID **)&MemoryPpi
> >
> > + );
> >
> > + ASSERT_EFI_ERROR (Status);
> >
> > +
> >
> > + Status = MemoryPpi->SetPermissions (
> >
> > + MemoryPpi,
> >
> > + (UINTN)BaseOfStack,
> >
> > + STACK_SIZE,
> >
> > + EFI_MEMORY_XP,
> >
> > + 0
> >
> > + );
> >
> > + ASSERT_EFI_ERROR (Status);
> >
> > + }
> >
> > +
> >
> > //
> >
> > // Compute the top of the stack we were allocated. Pre-allocate a
> > UINTN
> >
> > // for safety.
> >
> > diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> > b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> > index 60c998be6c1bad01..7126a96d8378d1f8 100644
> > --- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> > +++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> > @@ -91,6 +91,7 @@ [Ppis]
> > gEfiPeiMemoryDiscoveredPpiGuid ## SOMETIMES_CONSUMES
> >
> > gEdkiiPeiBootInCapsuleOnDiskModePpiGuid ## SOMETIMES_CONSUMES
> >
> > gEdkiiPeiCapsuleOnDiskPpiGuid ## SOMETIMES_CONSUMES #
> Consumed
> > on firmware update boot path
> >
> > + gEdkiiMemoryAttributePpiGuid ## SOMETIMES_CONSUMES
> >
> >
> >
> > [Guids]
> >
> > ## SOMETIMES_CONSUMES ## Variable:L"MemoryTypeInformation"
> >
> > @@ -117,10 +118,12 @@ [Pcd.IA32,Pcd.X64]
> > gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbSize ##
> CONSUMES
> >
> >
> >
> > [Pcd.IA32,Pcd.X64,Pcd.ARM,Pcd.AARCH64]
> >
> > - gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack ##
> > SOMETIMES_CONSUMES
> >
> > gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy ##
> > SOMETIMES_CONSUMES
> >
> > gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy ##
> > SOMETIMES_CONSUMES
> >
> >
> >
> > +[Pcd]
> >
> > + gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack ##
> > SOMETIMES_CONSUMES
> >
> > +
> >
> > [Depex]
> >
> > gEfiPeiLoadFilePpiGuid AND gEfiPeiMasterBootModePpiGuid
> >
> >
> >
> > --
> > 2.39.2
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [edk2-devel] [RFC PATCH 09/10] MdeModulePkg/DxeIpl: Use memory attribute PPI to remap the stack NX
2023-05-31 1:29 ` Ni, Ray
@ 2023-05-31 19:03 ` Lendacky, Thomas
2023-05-31 21:01 ` Ard Biesheuvel
0 siblings, 1 reply; 31+ messages in thread
From: Lendacky, Thomas @ 2023-05-31 19:03 UTC (permalink / raw)
To: devel, ray.ni, Tan, Dun, Ard Biesheuvel, Abner Chang
Cc: Yao, Jiewen, Gerd Hoffmann, Taylor Beebe, Oliver Smith-Denny,
Bi, Dandan, Gao, Liming, Kinney, Michael D, Leif Lindholm,
Sunil V L, Warkentin, Andrei
On 5/30/23 20:29, Ni, Ray via groups.io wrote:
> +@Abner Chang and @Tom Lendacky
>
>> -----Original Message-----
>> From: Tan, Dun <dun.tan@intel.com>
>> Sent: Tuesday, May 30, 2023 6:25 PM
>> To: Ni, Ray <ray.ni@intel.com>; Ard Biesheuvel <ardb@kernel.org>;
>> devel@edk2.groups.io
>> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Gerd Hoffmann
>> <kraxel@redhat.com>; Taylor Beebe <t@taylorbeebe.com>; Oliver Smith-
>> Denny <osd@smith-denny.com>; Bi, Dandan <dandan.bi@intel.com>; Gao,
>> Liming <gaoliming@byosoft.com.cn>; Kinney, Michael D
>> <michael.d.kinney@intel.com>; Leif Lindholm <quic_llindhol@quicinc.com>;
>> Sunil V L <sunilvl@ventanamicro.com>; Warkentin, Andrei
>> <andrei.warkentin@intel.com>
>> Subject: RE: [RFC PATCH 09/10] MdeModulePkg/DxeIpl: Use memory
>> attribute PPI to remap the stack NX
>>
>> Ray,
>> I think using MemoryAttribute PPI also looks good for X64 DxeIpl.
>> The only question that comes to my mind is the AMD sev feature. Since the
>> MemoryAttribute can't handle the AMD sev feature requirements(remapping
>> ghcb range from non-1:1 mapping to 1:1-mapping), we may need to find an
>> appropriate place to remap the Ghcb range.
I'm not sure I follow. How and where would the PPI be used? And what is
meant by "remapping the ghcb range from non-1:1 mapping to 1:1 mapping?
Thanks,
Tom
>>
>> Thanks,
>> Dun
>>
>> -----Original Message-----
>> From: Ni, Ray <ray.ni@intel.com>
>> Sent: Tuesday, May 30, 2023 3:19 PM
>> To: Ard Biesheuvel <ardb@kernel.org>; devel@edk2.groups.io; Tan, Dun
>> <dun.tan@intel.com>
>> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Gerd Hoffmann
>> <kraxel@redhat.com>; Taylor Beebe <t@taylorbeebe.com>; Oliver Smith-
>> Denny <osd@smith-denny.com>; Bi, Dandan <dandan.bi@intel.com>; Gao,
>> Liming <gaoliming@byosoft.com.cn>; Kinney, Michael D
>> <michael.d.kinney@intel.com>; Leif Lindholm <quic_llindhol@quicinc.com>;
>> Sunil V L <sunilvl@ventanamicro.com>; Warkentin, Andrei
>> <andrei.warkentin@intel.com>
>> Subject: RE: [RFC PATCH 09/10] MdeModulePkg/DxeIpl: Use memory
>> attribute PPI to remap the stack NX
>>
>> Looks good.
>>
>> @Tan, Dun, can you please evaluate if using MemoryAttribute PPI, what
>> opens will there be for X64 DxeIpl?
>>
>>> -----Original Message-----
>>> From: Ard Biesheuvel <ardb@kernel.org>
>>> Sent: Thursday, May 25, 2023 10:31 PM
>>> To: devel@edk2.groups.io
>>> Cc: Ard Biesheuvel <ardb@kernel.org>; Ni, Ray <ray.ni@intel.com>; Yao,
>>> Jiewen <jiewen.yao@intel.com>; Gerd Hoffmann <kraxel@redhat.com>;
>>> Taylor Beebe <t@taylorbeebe.com>; Oliver Smith-Denny
>>> <osd@smith-denny.com>; Bi, Dandan <dandan.bi@intel.com>; Gao, Liming
>>> <gaoliming@byosoft.com.cn>; Kinney, Michael D
>>> <michael.d.kinney@intel.com>; Leif Lindholm
>>> <quic_llindhol@quicinc.com>; Sunil V L <sunilvl@ventanamicro.com>;
>>> Warkentin, Andrei <andrei.warkentin@intel.com>
>>> Subject: [RFC PATCH 09/10] MdeModulePkg/DxeIpl: Use memory attribute
>>> PPI to remap the stack NX
>>>
>>> If the associated PCD is set to TRUE, use the memory attribute PPI to
>>> remap the stack non-executable. This provides a generic method for
>>> doing so, which will be used by ARM and AArch64 as well once they move
>>> to the generic DxeIpl handoff implementation.
>>>
>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>>> ---
>>> MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c | 29
>> ++++++++++++++++++--
>>> MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf | 5 +++-
>>> 2 files changed, 30 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c
>>> b/MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c
>>> index a0f85ebea56e6cba..22caabb02840ba88 100644
>>> --- a/MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c
>>> +++ b/MdeModulePkg/Core/DxeIplPeim/DxeHandoff.c
>>> @@ -2,12 +2,15 @@
>>> Generic version of arch-specific functionality for DxeLoad.
>>>
>>>
>>>
>>> Copyright (c) 2006 - 2018, Intel Corporation. All rights
>>> reserved.<BR>
>>>
>>> +Copyright (c) 2023, Google, LLC. All rights reserved.<BR>
>>>
>>> SPDX-License-Identifier: BSD-2-Clause-Patent
>>>
>>>
>>>
>>> **/
>>>
>>>
>>>
>>> #include "DxeIpl.h"
>>>
>>>
>>>
>>> +#include <Ppi/MemoryAttribute.h>
>>>
>>> +
>>>
>>> /**
>>>
>>> Transfers control to DxeCore.
>>>
>>>
>>>
>>> @@ -25,9 +28,10 @@ HandOffToDxeCore (
>>> IN EFI_PEI_HOB_POINTERS HobList
>>>
>>> )
>>>
>>> {
>>>
>>> - VOID *BaseOfStack;
>>>
>>> - VOID *TopOfStack;
>>>
>>> - EFI_STATUS Status;
>>>
>>> + VOID *BaseOfStack;
>>>
>>> + VOID *TopOfStack;
>>>
>>> + EFI_STATUS Status;
>>>
>>> + EDKII_MEMORY_ATTRIBUTE_PPI *MemoryPpi;
>>>
>>>
>>>
>>> //
>>>
>>> // Allocate 128KB for the Stack
>>>
>>> @@ -35,6 +39,25 @@ HandOffToDxeCore (
>>> BaseOfStack = AllocatePages (EFI_SIZE_TO_PAGES (STACK_SIZE));
>>>
>>> ASSERT (BaseOfStack != NULL);
>>>
>>>
>>>
>>> + if (PcdGetBool (PcdSetNxForStack)) {
>>>
>>> + Status = PeiServicesLocatePpi (
>>>
>>> + &gEdkiiMemoryAttributePpiGuid,
>>>
>>> + 0,
>>>
>>> + NULL,
>>>
>>> + (VOID **)&MemoryPpi
>>>
>>> + );
>>>
>>> + ASSERT_EFI_ERROR (Status);
>>>
>>> +
>>>
>>> + Status = MemoryPpi->SetPermissions (
>>>
>>> + MemoryPpi,
>>>
>>> + (UINTN)BaseOfStack,
>>>
>>> + STACK_SIZE,
>>>
>>> + EFI_MEMORY_XP,
>>>
>>> + 0
>>>
>>> + );
>>>
>>> + ASSERT_EFI_ERROR (Status);
>>>
>>> + }
>>>
>>> +
>>>
>>> //
>>>
>>> // Compute the top of the stack we were allocated. Pre-allocate a
>>> UINTN
>>>
>>> // for safety.
>>>
>>> diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
>>> b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
>>> index 60c998be6c1bad01..7126a96d8378d1f8 100644
>>> --- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
>>> +++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
>>> @@ -91,6 +91,7 @@ [Ppis]
>>> gEfiPeiMemoryDiscoveredPpiGuid ## SOMETIMES_CONSUMES
>>>
>>> gEdkiiPeiBootInCapsuleOnDiskModePpiGuid ## SOMETIMES_CONSUMES
>>>
>>> gEdkiiPeiCapsuleOnDiskPpiGuid ## SOMETIMES_CONSUMES #
>> Consumed
>>> on firmware update boot path
>>>
>>> + gEdkiiMemoryAttributePpiGuid ## SOMETIMES_CONSUMES
>>>
>>>
>>>
>>> [Guids]
>>>
>>> ## SOMETIMES_CONSUMES ## Variable:L"MemoryTypeInformation"
>>>
>>> @@ -117,10 +118,12 @@ [Pcd.IA32,Pcd.X64]
>>> gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbSize ##
>> CONSUMES
>>>
>>>
>>>
>>> [Pcd.IA32,Pcd.X64,Pcd.ARM,Pcd.AARCH64]
>>>
>>> - gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack ##
>>> SOMETIMES_CONSUMES
>>>
>>> gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy ##
>>> SOMETIMES_CONSUMES
>>>
>>> gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy ##
>>> SOMETIMES_CONSUMES
>>>
>>>
>>>
>>> +[Pcd]
>>>
>>> + gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack ##
>>> SOMETIMES_CONSUMES
>>>
>>> +
>>>
>>> [Depex]
>>>
>>> gEfiPeiLoadFilePpiGuid AND gEfiPeiMasterBootModePpiGuid
>>>
>>>
>>>
>>> --
>>> 2.39.2
>
>
>
>
>
>
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [edk2-devel] [RFC PATCH 09/10] MdeModulePkg/DxeIpl: Use memory attribute PPI to remap the stack NX
2023-05-31 19:03 ` [edk2-devel] " Lendacky, Thomas
@ 2023-05-31 21:01 ` Ard Biesheuvel
0 siblings, 0 replies; 31+ messages in thread
From: Ard Biesheuvel @ 2023-05-31 21:01 UTC (permalink / raw)
To: Tom Lendacky
Cc: devel, ray.ni, Tan, Dun, Abner Chang, Yao, Jiewen, Gerd Hoffmann,
Taylor Beebe, Oliver Smith-Denny, Bi, Dandan, Gao, Liming,
Kinney, Michael D, Leif Lindholm, Sunil V L, Warkentin, Andrei
On Wed, 31 May 2023 at 21:04, Tom Lendacky <thomas.lendacky@amd.com> wrote:
>
> On 5/30/23 20:29, Ni, Ray via groups.io wrote:
> > +@Abner Chang and @Tom Lendacky
> >
> >> -----Original Message-----
> >> From: Tan, Dun <dun.tan@intel.com>
> >> Sent: Tuesday, May 30, 2023 6:25 PM
> >> To: Ni, Ray <ray.ni@intel.com>; Ard Biesheuvel <ardb@kernel.org>;
> >> devel@edk2.groups.io
> >> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Gerd Hoffmann
> >> <kraxel@redhat.com>; Taylor Beebe <t@taylorbeebe.com>; Oliver Smith-
> >> Denny <osd@smith-denny.com>; Bi, Dandan <dandan.bi@intel.com>; Gao,
> >> Liming <gaoliming@byosoft.com.cn>; Kinney, Michael D
> >> <michael.d.kinney@intel.com>; Leif Lindholm <quic_llindhol@quicinc.com>;
> >> Sunil V L <sunilvl@ventanamicro.com>; Warkentin, Andrei
> >> <andrei.warkentin@intel.com>
> >> Subject: RE: [RFC PATCH 09/10] MdeModulePkg/DxeIpl: Use memory
> >> attribute PPI to remap the stack NX
> >>
> >> Ray,
> >> I think using MemoryAttribute PPI also looks good for X64 DxeIpl.
> >> The only question that comes to my mind is the AMD sev feature. Since the
> >> MemoryAttribute can't handle the AMD sev feature requirements(remapping
> >> ghcb range from non-1:1 mapping to 1:1-mapping), we may need to find an
> >> appropriate place to remap the Ghcb range.
>
> I'm not sure I follow. How and where would the PPI be used? And what is
> meant by "remapping the ghcb range from non-1:1 mapping to 1:1 mapping?
>
The problem is that, for some reason, the x86 code that recreates the
page tables in permanent PEI memory is part of the DxeIpl, and
executes just before handing over to DXE core (as opposed to when
permanent PEI memory first becomes available.)
So we ended up with a highly bespoke API that creates a new set of
page tablles from scratch, with special handling of the DXE stack and
GHCB region, as they need special permissions in the page tables.
IMHO it would make more sense to
- create the new page tables as soon as PEI permanent memory becomes available
- map the GHCB region shared from a SEV specific PEIM
- map shadowed PEIMs RO as they are being dispatched
- map the PEI stack and DXE stack NX as they are allocated (or even
better, map all memory NX by default and convert to R-X as needed)
Most of these cases could make use of the new generic MemoryAttributes
PPI that I am proposing, but this requires some refactoring first to
move the pieces out of DxeIpl that are better done elsewhere.
The generic DxeIpl code that I am proposing only manages the
permissions of the DXE stack, which it allocates, and uses the PPI.
X64 should be able to reuse the same code once the above changes are
implemented.
^ permalink raw reply [flat|nested] 31+ messages in thread
* [RFC PATCH 10/10] MdeModulePkg/DxeIpl ARM AARCH64: Switch to generic handoff code
2023-05-25 14:30 [RFC PATCH 00/10] Add PPI to manage PEI phase memory attributes Ard Biesheuvel
` (8 preceding siblings ...)
2023-05-25 14:30 ` [RFC PATCH 09/10] MdeModulePkg/DxeIpl: Use memory attribute PPI to remap the stack NX Ard Biesheuvel
@ 2023-05-25 14:30 ` Ard Biesheuvel
2023-05-25 17:20 ` [edk2-devel] [RFC PATCH 00/10] Add PPI to manage PEI phase memory attributes Oliver Smith-Denny
10 siblings, 0 replies; 31+ messages in thread
From: Ard Biesheuvel @ 2023-05-25 14:30 UTC (permalink / raw)
To: devel
Cc: Ard Biesheuvel, Ray Ni, Jiewen Yao, Gerd Hoffmann, Taylor Beebe,
Oliver Smith-Denny, Dandan Bi, Liming Gao, Kinney, Michael D,
Leif Lindholm, Sunil V L, Andrei Warkentin
Now that we have a generic method to manage memory permissions using a
PPI, we can switch to the generic version of the DXE handoff code in
DxeIpl, and drop the ARM specific version.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c | 71 --------------------
MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf | 11 +--
2 files changed, 1 insertion(+), 81 deletions(-)
diff --git a/MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c b/MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c
deleted file mode 100644
index f62b6dcb38a702d7..0000000000000000
--- a/MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c
+++ /dev/null
@@ -1,71 +0,0 @@
-/** @file
- ARM specifc functionality for DxeLoad.
-
-Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
-Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR>
-
-SPDX-License-Identifier: BSD-2-Clause-Patent
-
-**/
-
-#include "DxeIpl.h"
-
-#include <Library/ArmMmuLib.h>
-
-/**
- Transfers control to DxeCore.
-
- This function performs a CPU architecture specific operations to execute
- the entry point of DxeCore with the parameters of HobList.
- It also installs EFI_END_OF_PEI_PPI to signal the end of PEI phase.
-
- @param DxeCoreEntryPoint The entry point of DxeCore.
- @param HobList The start of HobList passed to DxeCore.
-
-**/
-VOID
-HandOffToDxeCore (
- IN EFI_PHYSICAL_ADDRESS DxeCoreEntryPoint,
- IN EFI_PEI_HOB_POINTERS HobList
- )
-{
- VOID *BaseOfStack;
- VOID *TopOfStack;
- EFI_STATUS Status;
-
- //
- // Allocate 128KB for the Stack
- //
- BaseOfStack = AllocatePages (EFI_SIZE_TO_PAGES (STACK_SIZE));
- ASSERT (BaseOfStack != NULL);
-
- if (PcdGetBool (PcdSetNxForStack)) {
- Status = ArmSetMemoryRegionNoExec ((UINTN)BaseOfStack, STACK_SIZE);
- ASSERT_EFI_ERROR (Status);
- }
-
- //
- // Compute the top of the stack we were allocated. Pre-allocate a UINTN
- // for safety.
- //
- TopOfStack = (VOID *)((UINTN)BaseOfStack + EFI_SIZE_TO_PAGES (STACK_SIZE) * EFI_PAGE_SIZE - CPU_STACK_ALIGNMENT);
- TopOfStack = ALIGN_POINTER (TopOfStack, CPU_STACK_ALIGNMENT);
-
- //
- // End of PEI phase singal
- //
- Status = PeiServicesInstallPpi (&gEndOfPeiSignalPpi);
- ASSERT_EFI_ERROR (Status);
-
- //
- // Update the contents of BSP stack HOB to reflect the real stack info passed to DxeCore.
- //
- UpdateStackHob ((EFI_PHYSICAL_ADDRESS)(UINTN)BaseOfStack, STACK_SIZE);
-
- SwitchStack (
- (SWITCH_STACK_ENTRY_POINT)(UINTN)DxeCoreEntryPoint,
- HobList.Raw,
- NULL,
- TopOfStack
- );
-}
diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
index 7126a96d8378d1f8..f1990eac77607854 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
@@ -45,19 +45,13 @@ [Sources.X64]
X64/VirtualMemory.c
X64/DxeLoadFunc.c
-[Sources.ARM, Sources.AARCH64]
- Arm/DxeLoadFunc.c
-
-[Sources.LOONGARCH64,Sources.RISCV64,Sources.EBC]
+[Sources.LOONGARCH64,Sources.RISCV64,Sources.EBC,Sources.ARM,Sources.AARCH64]
DxeHandoff.c
[Packages]
MdePkg/MdePkg.dec
MdeModulePkg/MdeModulePkg.dec
-[Packages.ARM, Packages.AARCH64]
- ArmPkg/ArmPkg.dec
-
[LibraryClasses]
PcdLib
MemoryAllocationLib
@@ -74,9 +68,6 @@ [LibraryClasses]
PeiServicesTablePointerLib
PerformanceLib
-[LibraryClasses.ARM, LibraryClasses.AARCH64]
- ArmMmuLib
-
[Ppis]
gEfiDxeIplPpiGuid ## PRODUCES
gEfiPeiDecompressPpiGuid ## PRODUCES
--
2.39.2
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [edk2-devel] [RFC PATCH 00/10] Add PPI to manage PEI phase memory attributes
2023-05-25 14:30 [RFC PATCH 00/10] Add PPI to manage PEI phase memory attributes Ard Biesheuvel
` (9 preceding siblings ...)
2023-05-25 14:30 ` [RFC PATCH 10/10] MdeModulePkg/DxeIpl ARM AARCH64: Switch to generic handoff code Ard Biesheuvel
@ 2023-05-25 17:20 ` Oliver Smith-Denny
2023-05-25 21:43 ` Ard Biesheuvel
10 siblings, 1 reply; 31+ messages in thread
From: Oliver Smith-Denny @ 2023-05-25 17:20 UTC (permalink / raw)
To: devel, ardb
Cc: Ray Ni, Jiewen Yao, Gerd Hoffmann, Taylor Beebe,
Oliver Smith-Denny, Dandan Bi, Liming Gao, Kinney, Michael D,
Leif Lindholm, Sunil V L, Andrei Warkentin
On 5/25/2023 7:30 AM, Ard Biesheuvel wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4468
>
>
>
> This is a proof-of-concept RFC that implements a PEI phase PPI to manage
>
> memory permission attributes, and wires it up to the PEI image loader so
>
> that shadowed PEIMs as well as the DXE core are remapped with the
>
> appropriate, restricted memory permission attributes before execution.
>
>
>
> This means that neither shadowed PEIMs nor the DXE core will ever
>
> execute with writable code regions. It also removes the need on the part
>
> of PEI for memory to be mapped with both writable and executable
>
> permissions by default out of reset. Similar work still needs to be done
>
> to address the early DXE phase (before the CPU arch protocol becomes
>
> available), but once that is out of the way as well, platforms should be
>
> able to map all memory non-executable from the beginning.
>
>
>
> This by itself is a major improvement in terms of robustness. It is also
>
> a prerequisite for enabling the WXN MMU control on AArch64, which makes
>
> all writable memory mappings non-executable regardless of the non-exec
>
> page table attribute.
>
>
>
> Patches #1 to #4 are prepatory work.
>
> Patch #5 proposes the memory attribute PPI protocol interface.
>
> Patch #6 implements it for ARM and AARCH64.
>
> Patch #7 wires it up into the PEI image loader.
>
> Patches #8 to #10 update the DxeIpl to use this PPI on ARM/AARCH64 for
>
> mapping the stack NX.
>
> instead of an explicit reference to ArmMmuLib. Other architectures
>
> (except IA32/X64) will seamlessly inherit this once they implement the
>
> PPI as well.
>
Hi Ard,
Thanks for the RFC. This same issue exists for X64, I saw your mailing
list question about IA32 PEI, do you have plans for extending this to
X64 PEI (I'm not sure its state of readiness)?
If so, do you think it is feasible to merge the X64 DxeLoadFunc with
the generic one you have created?
Overall, this seems a reasonable approach to me towards making DXE more
protected with the additional benefit of protecting shadowed PEIMs.
I did wonder if the same discussion we are having on the DXE side about
moving these MMU manipulations to the core instead of the CPU driver
make sense in PEI, too, but with the greatly reduced complexity of the
environment (no GCD, etc.), I don't think it makes sense now and that
focusing on the DXE reorganization and expansion is going to deliver
a much greater impact.
Thanks,
Oliver
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [edk2-devel] [RFC PATCH 00/10] Add PPI to manage PEI phase memory attributes
2023-05-25 17:20 ` [edk2-devel] [RFC PATCH 00/10] Add PPI to manage PEI phase memory attributes Oliver Smith-Denny
@ 2023-05-25 21:43 ` Ard Biesheuvel
0 siblings, 0 replies; 31+ messages in thread
From: Ard Biesheuvel @ 2023-05-25 21:43 UTC (permalink / raw)
To: Oliver Smith-Denny
Cc: devel, Ray Ni, Jiewen Yao, Gerd Hoffmann, Taylor Beebe,
Oliver Smith-Denny, Dandan Bi, Liming Gao, Kinney, Michael D,
Leif Lindholm, Sunil V L, Andrei Warkentin
On Thu, 25 May 2023 at 19:21, Oliver Smith-Denny
<osde@linux.microsoft.com> wrote:
>
> On 5/25/2023 7:30 AM, Ard Biesheuvel wrote:
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4468
> >
> >
> >
> > This is a proof-of-concept RFC that implements a PEI phase PPI to manage
> >
> > memory permission attributes, and wires it up to the PEI image loader so
> >
> > that shadowed PEIMs as well as the DXE core are remapped with the
> >
> > appropriate, restricted memory permission attributes before execution.
> >
> >
> >
> > This means that neither shadowed PEIMs nor the DXE core will ever
> >
> > execute with writable code regions. It also removes the need on the part
> >
> > of PEI for memory to be mapped with both writable and executable
> >
> > permissions by default out of reset. Similar work still needs to be done
> >
> > to address the early DXE phase (before the CPU arch protocol becomes
> >
> > available), but once that is out of the way as well, platforms should be
> >
> > able to map all memory non-executable from the beginning.
> >
> >
> >
> > This by itself is a major improvement in terms of robustness. It is also
> >
> > a prerequisite for enabling the WXN MMU control on AArch64, which makes
> >
> > all writable memory mappings non-executable regardless of the non-exec
> >
> > page table attribute.
> >
> >
> >
> > Patches #1 to #4 are prepatory work.
> >
> > Patch #5 proposes the memory attribute PPI protocol interface.
> >
> > Patch #6 implements it for ARM and AARCH64.
> >
> > Patch #7 wires it up into the PEI image loader.
> >
> > Patches #8 to #10 update the DxeIpl to use this PPI on ARM/AARCH64 for
> >
> > mapping the stack NX.
> >
> > instead of an explicit reference to ArmMmuLib. Other architectures
> >
> > (except IA32/X64) will seamlessly inherit this once they implement the
> >
> > PPI as well.
> >
>
> Hi Ard,
>
Hi,
Thanks for taking a look.
> Thanks for the RFC. This same issue exists for X64, I saw your mailing
> list question about IA32 PEI, do you have plans for extending this to
> X64 PEI (I'm not sure its state of readiness)?
>
Yes, but I need to wrap my head around that code first (I'm not as
familiar with low-level X64 as I am with ARM). I though I'd send this
RFC out in the meantime to get some alignment on the general shape of
things.
> If so, do you think it is feasible to merge the X64 DxeLoadFunc with
> the generic one you have created?
>
Hopefully. There are some pieces there that are highly X64 specific,
though, but at least the permissions management code could be shared,
in which case it can be moved out of the arch-specific source file and
into the generic one (this is one of the reasons I merged that code
between ARM, AARCH64, RISC-V and LoongArch64). Pure IA32 will simply
not implement the PPI, and the long mode switch stuff can remain IA32
specific afaict (although we'll be retaining that only for diagnostic
purposes)
> Overall, this seems a reasonable approach to me towards making DXE more
> protected with the additional benefit of protecting shadowed PEIMs.
>
> I did wonder if the same discussion we are having on the DXE side about
> moving these MMU manipulations to the core instead of the CPU driver
> make sense in PEI, too, but with the greatly reduced complexity of the
> environment (no GCD, etc.), I don't think it makes sense now and that
> focusing on the DXE reorganization and expansion is going to deliver
> a much greater impact.
>
IMHO the modularity has its advantages in some cases, and this
particular use case does not force us to deviate from that principle,
so I'd rather keep that as a separate discussion. On ARM (and now on
X64 for TDX) we already have PEI-less platforms where SEC dispatches
the DXE core directly, so I'm not convinced we really need something
in between.
--
Ard.
^ permalink raw reply [flat|nested] 31+ messages in thread