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

["duntan" <dun.tan@intel.com>]

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