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

["Ard Biesheuvel" <ardb@kernel.org>]

On Tue, 30 May 2023 at 12:25, Tan, Dun <dun.tan@intel.com> wrote:
>
> 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.
>

To me, the structure of that code seems entirely wrong, to be honest.
If SEV requires an additional step to be performed at end of PEI, it
should be done in a separate PEIM that registers a notification on
that signal.

However, I must admit that the X64 PEI logic is confusing to me, so I
may have missed something here. It seems to me that creating an
entirely new set of page tables in DxeIpl is both redundant (as PEI
already executes in long mode, and therefore uses page tables) and
undesirable, as it remaps the entire address space read/write/execute
without any awareness of what those regions may contain.

Would it be possible to remove this logic from DxeIpl, and set up the
page tables properly during PEI?





> -----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
>