[edk2-devel] [PATCH v5 06/28] MdeModulePkg: Apply Protections to the HOB List

[Laszlo Ersek]

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.beebe at gmail.com>
> Cc: Jian J Wang <jian.j.wang at intel.com>
> Cc: Liming Gao <gaoliming at byosoft.com.cn>
> Cc: Dandan Bi <dandan.bi at 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 <lersek at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3943202/1813853/130120423/xyzzy [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-