On 10/9/23 02:07, Taylor Beebe wrote:
> Because the platform memory protection settings will be stored
> in the HOB, the HOB list should be marked read-only and non-executable
> as soon as possible in boot.
>
> This patch page-aligns the allocated HOB list in DXE and marks
> it RO/NX during memory protection initialization.
>
> Signed-off-by: Taylor Beebe <taylor.d.be...@gmail.com>
> Cc: Jian J Wang <jian.j.w...@intel.com>
> Cc: Liming Gao <gaolim...@byosoft.com.cn>
> Cc: Dandan Bi <dandan...@intel.com>
> ---
> MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 18 ++++++------
> MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 29 ++++++++++++++++++++
> 2 files changed, 38 insertions(+), 9 deletions(-)
>
> diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> index 792cd2e0af23..72bd036eab1e 100644
> --- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> +++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> @@ -2764,21 +2764,21 @@ CoreInitializeGcdServices (
> }
>
> //
> - // Relocate HOB List to an allocated pool buffer.
> + // Relocate HOB List to allocated pages.
> // The relocation should be at after all the tested memory resources added
> // (except the memory space that covers HOB List) to the memory services,
> // because the memory resource found in CoreInitializeMemoryServices()
> // may have not enough remaining resource for HOB List.
> //
> - NewHobList = AllocateCopyPool (
> - (UINTN)PhitHob->EfiFreeMemoryBottom - (UINTN)(*HobStart),
> - *HobStart
> - );
> - ASSERT (NewHobList != NULL);
> -
> - *HobStart = NewHobList;
> - gHobList = NewHobList;
> + NewHobList = AllocatePages (EFI_SIZE_TO_PAGES
> ((UINTN)PhitHob->EfiFreeMemoryBottom - (UINTN)(*HobStart)));
> + if (NewHobList != NULL) {
> + CopyMem (NewHobList, *HobStart, (UINTN)PhitHob->EfiFreeMemoryBottom -
> (UINTN)(*HobStart));
> + *HobStart = NewHobList;
> + } else {
> + ASSERT (NewHobList != NULL);
> + }
>
> + gHobList = *HobStart;
> if (MemorySpaceMapHobList != NULL) {
> //
> // Add and allocate the memory space that covers HOB List to the memory
> services
I suggest using this as an opportunity to eliminate one instance of
ASSERT-for-error-checking.
> diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> index 7cc829b17402..94ed3111688b 100644
> --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> @@ -967,6 +967,32 @@ InitializeDxeNxMemoryProtectionPolicy (
> }
> }
>
> +/**
> + Mark the HOB list as read-only and non-executable.
> +**/
> +STATIC
> +VOID
> +ProtectHobList (
> + VOID
> + )
> +{
> + EFI_PEI_HOB_POINTERS Hob;
> +
> + Hob.Raw = GetHobList ();
> +
> + // Find the end of the HOB list.
> + while (!END_OF_HOB_LIST (Hob)) {
> + Hob.Raw = GET_NEXT_HOB (Hob);
> + }
> +
> + // Protect the HOB list.
> + SetUefiImageMemoryAttributes (
> + (UINTN)gHobList,
> + ALIGN_VALUE (((UINTN)Hob.Raw + GET_HOB_LENGTH (Hob)) - (UINTN)GetHobList
> (), EFI_PAGE_SIZE),
> + EFI_MEMORY_XP | EFI_MEMORY_RO
> + );
> +}
> +
> /**
> A notification for CPU_ARCH protocol.
>
> @@ -1007,6 +1033,9 @@ MemoryProtectionCpuArchProtocolNotify (
> //
> HeapGuardCpuArchProtocolNotify ();
>
> + // Mark the HOB list XP and RO.
> + ProtectHobList ();
> +
> if (mImageProtectionPolicy == 0) {
> goto Done;
> }
I was curious if this step was consistent with the HOB Design from the
PI spec, and it seems to be. In PI v1.7 volume 3, section "4.4 HOB
List", we have:
Only HOB producer phase components are allowed to make additions or
changes to HOBs. Once the HOB list is passed into the HOB consumer
phase, it is effectively read only. The ramification of a read-only
HOB list is that handoff information, such as boot mode, must be
handled in a distinguished fashion. For example, if the HOB consumer
phase were to engender a recovery condition, it would not update the
boot mode but instead would implement the action using a special
type of reset call.
I'm not checking your implementation, but from that r/o perspective at
least:
Acked-by: Laszlo Ersek <ler...@redhat.com>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109436): https://edk2.groups.io/g/devel/message/109436
Mute This Topic: https://groups.io/mt/101843347/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
