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

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

+@Abner Chang and @Tom Lendacky

> -----Original Message-----
> From: Tan, Dun <dun.tan@intel.com>
> Sent: Tuesday, May 30, 2023 6:25 PM
> To: Ni, Ray <ray.ni@intel.com>; Ard Biesheuvel <ardb@kernel.org>;
> devel@edk2.groups.io
> 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
> 
> 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