[edk2-devel][PATCH v1 1/1] MdeModulePkg: DxeCore: Don't Guard Large Runtime Granularity Allocations

[edk2-devel][PATCH v1 1/1] MdeModulePkg: DxeCore: Don't Guard Large Runtime Granularity Allocations

[Oliver Smith-Denny]

Currently, there are multiple issues when page or pool guards are
allocated for runtime and ACPI memory regions that are aligned to
non-EFI_PAGE_SIZE alignments. Multiple other issues have been fixed
for these same systems (notably ARM64 which has a 64k runtime page
allocation granularity) recently. The heap guard system is only
built to support 4k guard pages and 4k alignment.

Today, the address returned to a caller of AllocatePages will not
be aligned correctly to the runtime page allocation granularity,
because the heap guard system does not take non-4k alignment
requirements into consideration.

However, even with this bug fixed, the Memory Allocation Table
cannot be produced and an OS with a larger than 4k page granularity
will not have aligned memory regions because the guard pages are
reported as part of the same memory allocation. So what would have
been, on an ARM64 system, a 64k runtime memory allocation is actually
a 72k memory allocation as tracked by the Page.c code because the
guard pages are tracked as part of the same allocation. This is a
core function of the current heap guard architecture.

This could also be fixed with rearchitecting the heap guard system to
respect alignment requirements and shift the guard pages inside of the
outer rounded allocation or by having guard pages be the runtime
granularity. Both of these approaches have issues, in the former, the
allocator of runtime memory would get an address that was not aligned
with the runtime granularity (the head guard would be, but not the
usuable address), which seems fraught with peril. In the latter case,
an immense amount of memory is wasted to support such large guard pages,
and with pool guard many systems could not support an additional 128k
allocation for all runtime memory.

The simpler and safer solution is to disallow page and pool guards for
runtime memory allocations for systems that have a runtime granularity
greater than the EFI_PAGE_SIZE (4k). The usefulness of such guards is
limited, as OSes do not map guard pages today, so there is only boot
time protection of these ranges, which runtime memory is typically
used minimally in boot time. This also prevents other bugs from being
exposed by using guards for regions that have a non-4k alignment
requirement, as again, multiple have cropped up because the heap guard
system was not built to support it.

This patch adds both a static assert to ensure that either the runtime
granularity is the EFI_PAGE_SIZE or that the PCD bits are not set to
enable heap guard for runtime memory regions. It also adds a check in
the page and pool allocation system to ensure that at runtime we are not
allocating a runtime region and attempt to guard it (the PCDs are close to
being removed in favor of dynamic heap guard configurations).

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4674
Github PR: https://github.com/tianocore/edk2/pull/5378

Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>

Signed-off-by: Oliver Smith-Denny <osde@linux.microsoft.com>
---
 MdeModulePkg/MdeModulePkg.dec         | 10 ++++++++++
 MdeModulePkg/Core/Dxe/Mem/HeapGuard.h | 14 ++++++++++++++
 MdeModulePkg/Core/Dxe/Mem/Page.c      | 11 +++++++++++
 MdeModulePkg/Core/Dxe/Mem/Pool.c      | 11 +++++++++++
 4 files changed, 46 insertions(+)

diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index a2cd83345f5b..884734aff592 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -1027,6 +1027,11 @@ [PcdsFixedAtBuild]
   # free pages for all of them. The page allocation for the type related to
   # cleared bits keeps the same as ususal.
   #
+  # The heap guard system only supports guarding EfiRuntimeServicesCode, EfiRuntimeServicesData,
+  # EfiACPIReclaimMemory, and EfiACPIMemoryNVS memory types for systems that have
+  # RUNTIME_PAGE_ALLOCATION_GRANULARITY == EFI_PAGE_SIZE. This is to preserve alignment requirements
+  # without extending the page guard size to very large granularities.
+  #
   # This PCD is only valid if BIT0 and/or BIT2 are set in PcdHeapGuardPropertyMask.
   #
   # Below is bit mask for this PCD: (Order is same as UEFI spec)<BR>
@@ -1058,6 +1063,11 @@ [PcdsFixedAtBuild]
   # if there's enough free memory for all of them. The pool allocation for the
   # type related to cleared bits keeps the same as ususal.
   #
+  # The heap guard system only supports guarding EfiRuntimeServicesCode, EfiRuntimeServicesData,
+  # EfiACPIReclaimMemory, and EfiACPIMemoryNVS memory types for systems that have
+  # RUNTIME_PAGE_ALLOCATION_GRANULARITY == EFI_PAGE_SIZE. This is to preserve alignment requirements
+  # without extending the page guard size to very large granularities.
+  #
   # This PCD is only valid if BIT1 and/or BIT3 are set in PcdHeapGuardPropertyMask.
   #
   # Below is bit mask for this PCD: (Order is same as UEFI spec)<BR>
diff --git a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.h b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.h
index 24b4206c0e02..d4f283977b04 100644
--- a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.h
+++ b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.h
@@ -469,4 +469,18 @@ PromoteGuardedFreePages (
 
 extern BOOLEAN  mOnGuarding;
 
+//
+// the heap guard system does not support non-EFI_PAGE_SIZE alignments
+// architectures that require larger RUNTIME_PAGE_ALLOCATION_GRANULARITY
+// cannot have EfiRuntimeServicesCode, EfiRuntimeServicesData, EfiACPIReclaimMemory,
+// and EfiACPIMemoryNVS guarded. OSes do not map guard pages anyway, so this is a
+// minimal loss. Not guarding prevents alignment mismatches
+//
+STATIC_ASSERT (
+  RUNTIME_PAGE_ALLOCATION_GRANULARITY == EFI_PAGE_SIZE ||
+  (((FixedPcdGet64 (PcdHeapGuardPageType) & 0x660) == 0) &&
+   ((FixedPcdGet64 (PcdHeapGuardPoolType) & 0x660) == 0)),
+  "Unsupported Heap Guard configuration on system with greater than EFI_PAGE_SIZE RUNTIME_PAGE_ALLOCATION_GRANULARITY"
+  );
+
 #endif
diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
index 3205732ede17..ed3908b9768e 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Page.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
@@ -1411,6 +1411,17 @@ CoreInternalAllocatePages (
     Alignment = RUNTIME_PAGE_ALLOCATION_GRANULARITY;
   }
 
+  //
+  // the heap guard system does not support non-EFI_PAGE_SIZE alignments
+  // architectures that require larger RUNTIME_PAGE_ALLOCATION_GRANULARITY
+  // will have the runtime and ACPI memory regions unguarded. OSes do not
+  // map guard pages anyway, so this is a minimal loss. Not guarding prevents
+  // alignment mismatches
+  //
+  if (Alignment != EFI_PAGE_SIZE) {
+    NeedGuard = FALSE;
+  }
+
   if (Type == AllocateAddress) {
     if ((*Memory & (Alignment - 1)) != 0) {
       return EFI_NOT_FOUND;
diff --git a/MdeModulePkg/Core/Dxe/Mem/Pool.c b/MdeModulePkg/Core/Dxe/Mem/Pool.c
index 716dd045f9fd..beed5f814510 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Pool.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Pool.c
@@ -380,6 +380,17 @@ CoreAllocatePoolI (
     Granularity = DEFAULT_PAGE_ALLOCATION_GRANULARITY;
   }
 
+  //
+  // the heap guard system does not support non-EFI_PAGE_SIZE alignments
+  // architectures that require larger RUNTIME_PAGE_ALLOCATION_GRANULARITY
+  // will have the runtime and ACPI memory regions unguarded. OSes do not
+  // map guard pages anyway, so this is a minimal loss. Not guarding prevents
+  // alignment mismatches
+  //
+  if (Granularity != EFI_PAGE_SIZE) {
+    NeedGuard = FALSE;
+  }
+
   //
   // Adjust the size by the pool header & tail overhead
   //
-- 
2.40.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115474): https://edk2.groups.io/g/devel/message/115474
Mute This Topic: https://groups.io/mt/104364784/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel][PATCH v1 1/1] MdeModulePkg: DxeCore: Don't Guard Large Runtime Granularity Allocations

[Ard Biesheuvel]

Hey Oliver,

On Thu, 15 Feb 2024 at 01:34, Oliver Smith-Denny
<osde@linux.microsoft.com> wrote:
>
> Currently, there are multiple issues when page or pool guards are
> allocated for runtime and ACPI memory regions that are aligned to
> non-EFI_PAGE_SIZE alignments. Multiple other issues have been fixed
> for these same systems (notably ARM64 which has a 64k runtime page
> allocation granularity) recently. The heap guard system is only
> built to support 4k guard pages and 4k alignment.
>
> Today, the address returned to a caller of AllocatePages will not
> be aligned correctly to the runtime page allocation granularity,
> because the heap guard system does not take non-4k alignment
> requirements into consideration.
>
> However, even with this bug fixed, the Memory Allocation Table
> cannot be produced and an OS with a larger than 4k page granularity
> will not have aligned memory regions because the guard pages are
> reported as part of the same memory allocation. So what would have
> been, on an ARM64 system, a 64k runtime memory allocation is actually
> a 72k memory allocation as tracked by the Page.c code because the
> guard pages are tracked as part of the same allocation. This is a
> core function of the current heap guard architecture.
>
> This could also be fixed with rearchitecting the heap guard system to
> respect alignment requirements and shift the guard pages inside of the
> outer rounded allocation or by having guard pages be the runtime
> granularity. Both of these approaches have issues, in the former, the
> allocator of runtime memory would get an address that was not aligned
> with the runtime granularity (the head guard would be, but not the
> usuable address), which seems fraught with peril.

This would be my preference, and I wouldn't expect huge problems with
code expecting a certain alignment for such allocations. The 64k
requirement is entirely to ensure that the OS does not have to guess
how it should map 64k pages that have conflicting memory attributes
due to being covered by two different misaligned entries in the UEFI
memory map.

This is also why this is important for the MAT and runtime services
code/data regions: without 64k alignment, there will be a piece in the
middle of each runtime DXE that requires both write and execute
permissions.


> In the latter case,
> an immense amount of memory is wasted to support such large guard pages,
> and with pool guard many systems could not support an additional 128k
> allocation for all runtime memory.
>

Agreed.

> The simpler and safer solution is to disallow page and pool guards for
> runtime memory allocations for systems that have a runtime granularity
> greater than the EFI_PAGE_SIZE (4k). The usefulness of such guards is
> limited, as OSes do not map guard pages today, so there is only boot
> time protection of these ranges, which runtime memory is typically
> used minimally in boot time. This also prevents other bugs from being
> exposed by using guards for regions that have a non-4k alignment
> requirement, as again, multiple have cropped up because the heap guard
> system was not built to support it.
>
> This patch adds both a static assert to ensure that either the runtime
> granularity is the EFI_PAGE_SIZE or that the PCD bits are not set to
> enable heap guard for runtime memory regions. It also adds a check in
> the page and pool allocation system to ensure that at runtime we are not
> allocating a runtime region and attempt to guard it (the PCDs are close to
> being removed in favor of dynamic heap guard configurations).
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4674
> Github PR: https://github.com/tianocore/edk2/pull/5378
>
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
>
> Signed-off-by: Oliver Smith-Denny <osde@linux.microsoft.com>
> ---
>  MdeModulePkg/MdeModulePkg.dec         | 10 ++++++++++
>  MdeModulePkg/Core/Dxe/Mem/HeapGuard.h | 14 ++++++++++++++
>  MdeModulePkg/Core/Dxe/Mem/Page.c      | 11 +++++++++++
>  MdeModulePkg/Core/Dxe/Mem/Pool.c      | 11 +++++++++++
>  4 files changed, 46 insertions(+)
>
> diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
> index a2cd83345f5b..884734aff592 100644
> --- a/MdeModulePkg/MdeModulePkg.dec
> +++ b/MdeModulePkg/MdeModulePkg.dec
> @@ -1027,6 +1027,11 @@ [PcdsFixedAtBuild]
>    # free pages for all of them. The page allocation for the type related to
>    # cleared bits keeps the same as ususal.
>    #
> +  # The heap guard system only supports guarding EfiRuntimeServicesCode, EfiRuntimeServicesData,
> +  # EfiACPIReclaimMemory, and EfiACPIMemoryNVS memory types for systems that have

I looked at the EFI spec again, and EfiACPIReclaimMemory is not
actually listed as a memory type that has this 64k alignment
requirement. This makes sense, given that this memory type has no
significance to the firmware itself, only to the OS. OTOH, reserved
memory does appear there.

So I suggest we fix that first, and then drop any mention of
EfiACPIReclaimMemory from this patch. At least we'll have heap guard
coverage for ACPI table allocations on arm64 going forward.

The logic in question was added in 2007 in commit 28a00297189c, so
this was probably the rule on Itanium, but that support is long gone.


diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
index 6497af573353..755b36527d38 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Page.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
@@ -1301,7 +1301,7 @@ CoreInternalAllocatePages (

   Alignment = DEFAULT_PAGE_ALLOCATION_GRANULARITY;

-  if ((MemoryType == EfiACPIReclaimMemory) ||
+  if ((MemoryType == EfiReservedMemoryType) ||
       (MemoryType == EfiACPIMemoryNVS) ||
       (MemoryType == EfiRuntimeServicesCode) ||
       (MemoryType == EfiRuntimeServicesData))


(there is another occurrence in MdeModulePkg/Core/Pei/Memory/MemoryServices.c)

> +  # RUNTIME_PAGE_ALLOCATION_GRANULARITY == EFI_PAGE_SIZE. This is to preserve alignment requirements
> +  # without extending the page guard size to very large granularities.
> +  #
>    # This PCD is only valid if BIT0 and/or BIT2 are set in PcdHeapGuardPropertyMask.
>    #
>    # Below is bit mask for this PCD: (Order is same as UEFI spec)<BR>
> @@ -1058,6 +1063,11 @@ [PcdsFixedAtBuild]
>    # if there's enough free memory for all of them. The pool allocation for the
>    # type related to cleared bits keeps the same as ususal.
>    #
> +  # The heap guard system only supports guarding EfiRuntimeServicesCode, EfiRuntimeServicesData,
> +  # EfiACPIReclaimMemory, and EfiACPIMemoryNVS memory types for systems that have
> +  # RUNTIME_PAGE_ALLOCATION_GRANULARITY == EFI_PAGE_SIZE. This is to preserve alignment requirements
> +  # without extending the page guard size to very large granularities.
> +  #
>    # This PCD is only valid if BIT1 and/or BIT3 are set in PcdHeapGuardPropertyMask.
>    #
>    # Below is bit mask for this PCD: (Order is same as UEFI spec)<BR>
> diff --git a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.h b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.h
> index 24b4206c0e02..d4f283977b04 100644
> --- a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.h
> +++ b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.h
> @@ -469,4 +469,18 @@ PromoteGuardedFreePages (
>
>  extern BOOLEAN  mOnGuarding;
>
> +//
> +// the heap guard system does not support non-EFI_PAGE_SIZE alignments
> +// architectures that require larger RUNTIME_PAGE_ALLOCATION_GRANULARITY
> +// cannot have EfiRuntimeServicesCode, EfiRuntimeServicesData, EfiACPIReclaimMemory,
> +// and EfiACPIMemoryNVS guarded. OSes do not map guard pages anyway, so this is a
> +// minimal loss. Not guarding prevents alignment mismatches
> +//
> +STATIC_ASSERT (
> +  RUNTIME_PAGE_ALLOCATION_GRANULARITY == EFI_PAGE_SIZE ||
> +  (((FixedPcdGet64 (PcdHeapGuardPageType) & 0x660) == 0) &&
> +   ((FixedPcdGet64 (PcdHeapGuardPoolType) & 0x660) == 0)),
> +  "Unsupported Heap Guard configuration on system with greater than EFI_PAGE_SIZE RUNTIME_PAGE_ALLOCATION_GRANULARITY"
> +  );
> +
>  #endif
> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
> index 3205732ede17..ed3908b9768e 100644
> --- a/MdeModulePkg/Core/Dxe/Mem/Page.c
> +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
> @@ -1411,6 +1411,17 @@ CoreInternalAllocatePages (
>      Alignment = RUNTIME_PAGE_ALLOCATION_GRANULARITY;
>    }
>
> +  //
> +  // the heap guard system does not support non-EFI_PAGE_SIZE alignments
> +  // architectures that require larger RUNTIME_PAGE_ALLOCATION_GRANULARITY
> +  // will have the runtime and ACPI memory regions unguarded. OSes do not
> +  // map guard pages anyway, so this is a minimal loss. Not guarding prevents
> +  // alignment mismatches
> +  //
> +  if (Alignment != EFI_PAGE_SIZE) {
> +    NeedGuard = FALSE;
> +  }
> +
>    if (Type == AllocateAddress) {
>      if ((*Memory & (Alignment - 1)) != 0) {
>        return EFI_NOT_FOUND;
> diff --git a/MdeModulePkg/Core/Dxe/Mem/Pool.c b/MdeModulePkg/Core/Dxe/Mem/Pool.c
> index 716dd045f9fd..beed5f814510 100644
> --- a/MdeModulePkg/Core/Dxe/Mem/Pool.c
> +++ b/MdeModulePkg/Core/Dxe/Mem/Pool.c
> @@ -380,6 +380,17 @@ CoreAllocatePoolI (
>      Granularity = DEFAULT_PAGE_ALLOCATION_GRANULARITY;
>    }
>
> +  //
> +  // the heap guard system does not support non-EFI_PAGE_SIZE alignments
> +  // architectures that require larger RUNTIME_PAGE_ALLOCATION_GRANULARITY
> +  // will have the runtime and ACPI memory regions unguarded. OSes do not
> +  // map guard pages anyway, so this is a minimal loss. Not guarding prevents
> +  // alignment mismatches
> +  //
> +  if (Granularity != EFI_PAGE_SIZE) {
> +    NeedGuard = FALSE;
> +  }
> +
>    //
>    // Adjust the size by the pool header & tail overhead
>    //
> --
> 2.40.1
>
>
>
> ------------
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#115474): https://edk2.groups.io/g/devel/message/115474
> Mute This Topic: https://groups.io/mt/104364784/5717338
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [ardb+tianocore@kernel.org]
> ------------
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115490): https://edk2.groups.io/g/devel/message/115490
Mute This Topic: https://groups.io/mt/104364784/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel][PATCH v1 1/1] MdeModulePkg: DxeCore: Don't Guard Large Runtime Granularity Allocations

[Oliver Smith-Denny]

On 2/14/2024 11:50 PM, Ard Biesheuvel wrote> On Thu, 15 Feb 2024 at 
01:34, Oliver Smith-Denny
> <osde@linux.microsoft.com> wrote >> This could also be fixed with rearchitecting the heap guard system to
>> respect alignment requirements and shift the guard pages inside of the
>> outer rounded allocation or by having guard pages be the runtime
>> granularity. Both of these approaches have issues, in the former, the
>> allocator of runtime memory would get an address that was not aligned
>> with the runtime granularity (the head guard would be, but not the
>> usuable address), which seems fraught with peril.
> 
> This would be my preference, and I wouldn't expect huge problems with
> code expecting a certain alignment for such allocations. The 64k
> requirement is entirely to ensure that the OS does not have to guess
> how it should map 64k pages that have conflicting memory attributes
> due to being covered by two different misaligned entries in the UEFI
> memory map.
> 
> This is also why this is important for the MAT and runtime services
> code/data regions: without 64k alignment, there will be a piece in the
> middle of each runtime DXE that requires both write and execute
> permissions.
> 
> 

I do have a PR up to fix the misalignment bug (I was doing a CI check on
it before sending the patch when I did some further testing to discover
the guard pages pushing out the allocation size). Would you prefer that
I update that to do the guard page allocation inside the 64k allocation?

I can certainly do that, again my concern is that the code starts
getting more complex, with more room for errors. The heap guard code now
needs to know the actual size requested by the caller, not the rounded
size, so we can see, oh, the allocation requested is 64k, so I need
another 64k region to fit the guards into, unless we already have shared
guard pages, in which case we may not need to, or one guard may be
shared and the other isn't, etc. It is doable, but I worry about the
added complexity for only a small window of protection for these runtime
memory regions. We could also say no shared guard pages for runtime
regions if you don't have runtime allocation granularity equal to the
EFI_PAGE_SIZE.

Based on our offline conversation, I thought you were ok with the simple
approach of disable the guards for these regions, the value of
protecting these regions at boot time is not worth the additional
complexity. But, I can update my PR to put the guards inside the
allocation and we can compare the relative complexity.

>> diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
>> index a2cd83345f5b..884734aff592 100644
>> --- a/MdeModulePkg/MdeModulePkg.dec
>> +++ b/MdeModulePkg/MdeModulePkg.dec
>> @@ -1027,6 +1027,11 @@ [PcdsFixedAtBuild]
>>     # free pages for all of them. The page allocation for the type related to
>>     # cleared bits keeps the same as ususal.
>>     #
>> +  # The heap guard system only supports guarding EfiRuntimeServicesCode, EfiRuntimeServicesData,
>> +  # EfiACPIReclaimMemory, and EfiACPIMemoryNVS memory types for systems that have
> 
> I looked at the EFI spec again, and EfiACPIReclaimMemory is not
> actually listed as a memory type that has this 64k alignment
> requirement. This makes sense, given that this memory type has no
> significance to the firmware itself, only to the OS. OTOH, reserved
> memory does appear there.
> 
> So I suggest we fix that first, and then drop any mention of
> EfiACPIReclaimMemory from this patch. At least we'll have heap guard
> coverage for ACPI table allocations on arm64 going forward.
> 
> The logic in question was added in 2007 in commit 28a00297189c, so
> this was probably the rule on Itanium, but that support is long gone.
> 

Thanks for looking this up. I'll update either this patch or the unsent
patch I have depending on the direction we go.

Thanks,
Oliver


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115528): https://edk2.groups.io/g/devel/message/115528
Mute This Topic: https://groups.io/mt/104364784/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel][PATCH v1 1/1] MdeModulePkg: DxeCore: Don't Guard Large Runtime Granularity Allocations

[Ard Biesheuvel]

On Thu, 15 Feb 2024 at 18:08, Oliver Smith-Denny
<osde@linux.microsoft.com> wrote:
>
> On 2/14/2024 11:50 PM, Ard Biesheuvel wrote> On Thu, 15 Feb 2024 at
> 01:34, Oliver Smith-Denny
> > <osde@linux.microsoft.com> wrote >> This could also be fixed with rearchitecting the heap guard system to
> >> respect alignment requirements and shift the guard pages inside of the
> >> outer rounded allocation or by having guard pages be the runtime
> >> granularity. Both of these approaches have issues, in the former, the
> >> allocator of runtime memory would get an address that was not aligned
> >> with the runtime granularity (the head guard would be, but not the
> >> usuable address), which seems fraught with peril.
> >
> > This would be my preference, and I wouldn't expect huge problems with
> > code expecting a certain alignment for such allocations. The 64k
> > requirement is entirely to ensure that the OS does not have to guess
> > how it should map 64k pages that have conflicting memory attributes
> > due to being covered by two different misaligned entries in the UEFI
> > memory map.
> >
> > This is also why this is important for the MAT and runtime services
> > code/data regions: without 64k alignment, there will be a piece in the
> > middle of each runtime DXE that requires both write and execute
> > permissions.
> >
> >
>
> I do have a PR up to fix the misalignment bug (I was doing a CI check on
> it before sending the patch when I did some further testing to discover
> the guard pages pushing out the allocation size). Would you prefer that
> I update that to do the guard page allocation inside the 64k allocation?
>
> I can certainly do that, again my concern is that the code starts
> getting more complex, with more room for errors. The heap guard code now
> needs to know the actual size requested by the caller, not the rounded
> size, so we can see, oh, the allocation requested is 64k, so I need
> another 64k region to fit the guards into, unless we already have shared
> guard pages, in which case we may not need to, or one guard may be
> shared and the other isn't, etc. It is doable, but I worry about the
> added complexity for only a small window of protection for these runtime
> memory regions. We could also say no shared guard pages for runtime
> regions if you don't have runtime allocation granularity equal to the
> EFI_PAGE_SIZE.
>
> Based on our offline conversation, I thought you were ok with the simple
> approach of disable the guards for these regions, the value of
> protecting these regions at boot time is not worth the additional
> complexity. But, I can update my PR to put the guards inside the
> allocation and we can compare the relative complexity.
>

Of the two options you presented in this paragraph, I prefer the one
where the allocation presented to the caller may not be aligned, but
the region plus guards is.

But disabling it entirely for these regions is still perfectly fine
with me, especially if the remove ACPI reclaim memory from the set.
Heap guard is a hardening feature, and if the implementation is too
complex to reason about comfortably, I don't think we can confidently
rely on it.

And as far as the OS is concerned: with the MAT, the runtime DXEs are
mapped in a way where the read-only regions are interleaved with the
read-write regions, and the holes in between are not mapped at all (at
least on Linux). IOW, there is some implicit guarding going on
already.


> >> diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
> >> index a2cd83345f5b..884734aff592 100644
> >> --- a/MdeModulePkg/MdeModulePkg.dec
> >> +++ b/MdeModulePkg/MdeModulePkg.dec
> >> @@ -1027,6 +1027,11 @@ [PcdsFixedAtBuild]
> >>     # free pages for all of them. The page allocation for the type related to
> >>     # cleared bits keeps the same as ususal.
> >>     #
> >> +  # The heap guard system only supports guarding EfiRuntimeServicesCode, EfiRuntimeServicesData,
> >> +  # EfiACPIReclaimMemory, and EfiACPIMemoryNVS memory types for systems that have
> >
> > I looked at the EFI spec again, and EfiACPIReclaimMemory is not
> > actually listed as a memory type that has this 64k alignment
> > requirement. This makes sense, given that this memory type has no
> > significance to the firmware itself, only to the OS. OTOH, reserved
> > memory does appear there.
> >
> > So I suggest we fix that first, and then drop any mention of
> > EfiACPIReclaimMemory from this patch. At least we'll have heap guard
> > coverage for ACPI table allocations on arm64 going forward.
> >
> > The logic in question was added in 2007 in commit 28a00297189c, so
> > this was probably the rule on Itanium, but that support is long gone.
> >
>
> Thanks for looking this up. I'll update either this patch or the unsent
> patch I have depending on the direction we go.
>

Let's go with this approach.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115529): https://edk2.groups.io/g/devel/message/115529
Mute This Topic: https://groups.io/mt/104364784/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel][PATCH v1 1/1] MdeModulePkg: DxeCore: Don't Guard Large Runtime Granularity Allocations

[Oliver Smith-Denny]

On 2/15/2024 9:21 AM, Ard Biesheuvel wrote:
> Of the two options you presented in this paragraph, I prefer the one
> where the allocation presented to the caller may not be aligned, but
> the region plus guards is.
> 
> But disabling it entirely for these regions is still perfectly fine
> with me, especially if the remove ACPI reclaim memory from the set.
> Heap guard is a hardening feature, and if the implementation is too
> complex to reason about comfortably, I don't think we can confidently
> rely on it.
> 
> And as far as the OS is concerned: with the MAT, the runtime DXEs are
> mapped in a way where the read-only regions are interleaved with the
> read-write regions, and the holes in between are not mapped at all (at
> least on Linux). IOW, there is some implicit guarding going on
> already.
> 

Looking back at the UEFI spec (section 2.3.6), I see this:

"If a 64KiB physical page contains any 4KiB page with any of the
following types listed below, then all 4KiB pages in the 64KiB page
must use identical ARM Memory Page Attributes" where the following
types are what you listed in the last email.

Then there is a further statement:

"Mixed attribute mappings within a larger page are not allowed."

So this would seem to indicate that pushing the guard pages inside
of the 64KiB would break the spec. Now, I think it could be hidden
and still meet the intent of the spec, which would be that the OS
gets consistent memory attributes reported, but still, the way the
spec is written this would seem to be a violation.

I'll send out a v2 with the type change.

Thanks,
Oliver


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115532): https://edk2.groups.io/g/devel/message/115532
Mute This Topic: https://groups.io/mt/104364784/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel][PATCH v1 1/1] MdeModulePkg: DxeCore: Don't Guard Large Runtime Granularity Allocations

[Oliver Smith-Denny]

On 2/14/2024 11:50 PM, Ard Biesheuvel wrote:
> I looked at the EFI spec again, and EfiACPIReclaimMemory is not
> actually listed as a memory type that has this 64k alignment
> requirement. This makes sense, given that this memory type has no
> significance to the firmware itself, only to the OS. OTOH, reserved
> memory does appear there.
> 
> So I suggest we fix that first, and then drop any mention of
> EfiACPIReclaimMemory from this patch. At least we'll have heap guard
> coverage for ACPI table allocations on arm64 going forward.
> 
> The logic in question was added in 2007 in commit 28a00297189c, so
> this was probably the rule on Itanium, but that support is long gone.
> 

I wanted to understand this a little bit further. I do see that the spec
does not call out EfiACPIReclaimMemory as needing 64k alignment.
However, your statement that the memory type does not have have
significance to the FW, only to the OS, so we don't need the 64k
alignment is confusing to me. Isn't the 64k alignment needed only for
regions that the OS does care about? In order to read this information
at their 64k page granularity, doesn't this need to be aligned to 64k?

Or are you saying the OS can just read the 64k page(s) containing these
4k aligned EfiACPIReclaimMemory pages and then start reading at the
calculated offset from within the larger page, since these pages don't
have any pointers to outside memory, unlike image sections?

Thanks,
Oliver


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115533): https://edk2.groups.io/g/devel/message/115533
Mute This Topic: https://groups.io/mt/104364784/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel][PATCH v1 1/1] MdeModulePkg: DxeCore: Don't Guard Large Runtime Granularity Allocations

[Ard Biesheuvel]

On Thu, 15 Feb 2024 at 20:55, Oliver Smith-Denny
<osde@linux.microsoft.com> wrote:
>
> On 2/14/2024 11:50 PM, Ard Biesheuvel wrote:
> > I looked at the EFI spec again, and EfiACPIReclaimMemory is not
> > actually listed as a memory type that has this 64k alignment
> > requirement. This makes sense, given that this memory type has no
> > significance to the firmware itself, only to the OS. OTOH, reserved
> > memory does appear there.
> >
> > So I suggest we fix that first, and then drop any mention of
> > EfiACPIReclaimMemory from this patch. At least we'll have heap guard
> > coverage for ACPI table allocations on arm64 going forward.
> >
> > The logic in question was added in 2007 in commit 28a00297189c, so
> > this was probably the rule on Itanium, but that support is long gone.
> >
>
> I wanted to understand this a little bit further. I do see that the spec
> does not call out EfiACPIReclaimMemory as needing 64k alignment.
> However, your statement that the memory type does not have have
> significance to the FW, only to the OS, so we don't need the 64k
> alignment is confusing to me. Isn't the 64k alignment needed only for
> regions that the OS does care about? In order to read this information
> at their 64k page granularity, doesn't this need to be aligned to 64k?
>

It is not about accessing the information in the pages. Most consumers
don't care about this at all, and even code that does will rarely be
materially different between 4k and 64k OSes.

When running under the firmware, we run with 4k pages and programming
the memory attributes is entirely under the control of the firmware
itself. So in this phase, the alignment truly does not matter,
especially because all of RAM is 1:1 mapped anyway.

When running under the OS, all memory that is part of the OS's memory
pool is entirely under its control, and this includes ACPI reclaim
memory, given that the firmware itself should not be accessing this:
the purpose of this memory type is specifically to expose information
to the OS in memory that it can use as it sees fit after it is done
processing the information.

The alignment issues we need to avoid are:
- runtime DXEs where we want to map code RO and data NX, which implies
that the boundary between the two must be 64k aligned;
- other EFI_MEMORY_RUNTIME regions that may be described with
EFI_MEMORY_WC rather than EFI_MEMORY_WB; this could be related to
implementations of the varstore or other runtime services, and the OS
has no idea what the region is for and how the runtime code is using
it;
- reserved regions that are mapped, e.g., by AML code, as a
SystemMemory OpRegion; there are examples in RAS code where the memory
needs to be mapped uncached so that stores are immediately visible to
the BMC or other agents that sit on the interconnect. The existence of
a cacheable alias of the same mapping will interfere with this, i.e.,
in many cases it will simply make both mappings cacheable.


> Or are you saying the OS can just read the 64k page(s) containing these
> 4k aligned EfiACPIReclaimMemory pages and then start reading at the
> calculated offset from within the larger page, since these pages don't
> have any pointers to outside memory, unlike image sections?
>

In all these examples, the alignment *itself* is not the goal. The 64k
alignment is way to ensure that the OS is never in a situation where a
64k page frame is shared between a cacheable and an uncacheable
mapping, or between a ROX and a W+NX mapping. Note that, in the
runtime DXE case, the only way to map the 64k page that has both code
and data is to map it RWX, given that the contents need to appear
contiguous in virtual memory (we had great fun with that when we
designed the MAT)

For the other cases, there may be mappings that are completely
disjoint in the virtual address space. But if two mappings exist of
the same physical page, one cacheable via one alias, and one
non-cacheable via another, the result is unpredictable and things like
RAS or BMC comms from runtime firmware may be completely broken.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115534): https://edk2.groups.io/g/devel/message/115534
Mute This Topic: https://groups.io/mt/104364784/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel][PATCH v1 1/1] MdeModulePkg: DxeCore: Don't Guard Large Runtime Granularity Allocations

[Oliver Smith-Denny]

On 2/15/2024 2:17 PM, Ard Biesheuvel wrote:
> On Thu, 15 Feb 2024 at 20:55, Oliver Smith-Denny
> <osde@linux.microsoft.com> wrote:
>>
>> On 2/14/2024 11:50 PM, Ard Biesheuvel wrote:
>>> I looked at the EFI spec again, and EfiACPIReclaimMemory is not
>>> actually listed as a memory type that has this 64k alignment
>>> requirement. This makes sense, given that this memory type has no
>>> significance to the firmware itself, only to the OS. OTOH, reserved
>>> memory does appear there.
>>>
>>> So I suggest we fix that first, and then drop any mention of
>>> EfiACPIReclaimMemory from this patch. At least we'll have heap guard
>>> coverage for ACPI table allocations on arm64 going forward.
>>>
>>> The logic in question was added in 2007 in commit 28a00297189c, so
>>> this was probably the rule on Itanium, but that support is long gone.
>>>
>>
>> I wanted to understand this a little bit further. I do see that the spec
>> does not call out EfiACPIReclaimMemory as needing 64k alignment.
>> However, your statement that the memory type does not have have
>> significance to the FW, only to the OS, so we don't need the 64k
>> alignment is confusing to me. Isn't the 64k alignment needed only for
>> regions that the OS does care about? In order to read this information
>> at their 64k page granularity, doesn't this need to be aligned to 64k?
>>
> 
> It is not about accessing the information in the pages. Most consumers
> don't care about this at all, and even code that does will rarely be
> materially different between 4k and 64k OSes.
> 
> When running under the firmware, we run with 4k pages and programming
> the memory attributes is entirely under the control of the firmware
> itself. So in this phase, the alignment truly does not matter,
> especially because all of RAM is 1:1 mapped anyway.
> 
> When running under the OS, all memory that is part of the OS's memory
> pool is entirely under its control, and this includes ACPI reclaim
> memory, given that the firmware itself should not be accessing this:
> the purpose of this memory type is specifically to expose information
> to the OS in memory that it can use as it sees fit after it is done
> processing the information.
> 
> The alignment issues we need to avoid are:
> - runtime DXEs where we want to map code RO and data NX, which implies
> that the boundary between the two must be 64k aligned;
> - other EFI_MEMORY_RUNTIME regions that may be described with
> EFI_MEMORY_WC rather than EFI_MEMORY_WB; this could be related to
> implementations of the varstore or other runtime services, and the OS
> has no idea what the region is for and how the runtime code is using
> it;
> - reserved regions that are mapped, e.g., by AML code, as a
> SystemMemory OpRegion; there are examples in RAS code where the memory
> needs to be mapped uncached so that stores are immediately visible to
> the BMC or other agents that sit on the interconnect. The existence of
> a cacheable alias of the same mapping will interfere with this, i.e.,
> in many cases it will simply make both mappings cacheable.
> 
> 
>> Or are you saying the OS can just read the 64k page(s) containing these
>> 4k aligned EfiACPIReclaimMemory pages and then start reading at the
>> calculated offset from within the larger page, since these pages don't
>> have any pointers to outside memory, unlike image sections?
>>
> 
> In all these examples, the alignment *itself* is not the goal. The 64k
> alignment is way to ensure that the OS is never in a situation where a
> 64k page frame is shared between a cacheable and an uncacheable
> mapping, or between a ROX and a W+NX mapping. Note that, in the
> runtime DXE case, the only way to map the 64k page that has both code
> and data is to map it RWX, given that the contents need to appear
> contiguous in virtual memory (we had great fun with that when we
> designed the MAT)
> 
> For the other cases, there may be mappings that are completely
> disjoint in the virtual address space. But if two mappings exist of
> the same physical page, one cacheable via one alias, and one
> non-cacheable via another, the result is unpredictable and things like
> RAS or BMC comms from runtime firmware may be completely broken.
> 
> 

I see, thanks this was very helpful. I was focused on the alignment to
meet access (which as you say isn't really needed) and for image
sections, but the cacheable/uncacheable piece makes sense, too. I'll
send out the v2 for this patch.

Thanks,
Oliver


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115535): https://edk2.groups.io/g/devel/message/115535
Mute This Topic: https://groups.io/mt/104364784/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-