Re: [RFC PATCH 09/10] MdeModulePkg/DxeIpl: Use memory attribute PPI to remap the stack NX

["Ni, Ray" <ray.ni@intel.com>]

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