[edk2-devel] [RFC PATCH 09/10] MdeModulePkg/DxeIpl: Use memory attribute PPI to remap the stack NX
Ard Biesheuvel
ardb at kernel.org
Tue May 30 12:51:21 UTC 2023
On Tue, 30 May 2023 at 12:25, Tan, Dun <dun.tan at 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 at intel.com>
> Sent: Tuesday, May 30, 2023 3:19 PM
> To: Ard Biesheuvel <ardb at kernel.org>; devel at edk2.groups.io; Tan, Dun <dun.tan at intel.com>
> Cc: Yao, Jiewen <jiewen.yao at intel.com>; Gerd Hoffmann <kraxel at redhat.com>; Taylor Beebe <t at taylorbeebe.com>; Oliver Smith-Denny <osd at smith-denny.com>; Bi, Dandan <dandan.bi at intel.com>; Gao, Liming <gaoliming at byosoft.com.cn>; Kinney, Michael D <michael.d.kinney at intel.com>; Leif Lindholm <quic_llindhol at quicinc.com>; Sunil V L <sunilvl at ventanamicro.com>; Warkentin, Andrei <andrei.warkentin at 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 at kernel.org>
> > Sent: Thursday, May 25, 2023 10:31 PM
> > To: devel at edk2.groups.io
> > Cc: Ard Biesheuvel <ardb at kernel.org>; Ni, Ray <ray.ni at intel.com>; Yao,
> > Jiewen <jiewen.yao at intel.com>; Gerd Hoffmann <kraxel at redhat.com>;
> > Taylor Beebe <t at taylorbeebe.com>; Oliver Smith-Denny
> > <osd at smith-denny.com>; Bi, Dandan <dandan.bi at intel.com>; Gao, Liming
> > <gaoliming at byosoft.com.cn>; Kinney, Michael D
> > <michael.d.kinney at intel.com>; Leif Lindholm
> > <quic_llindhol at quicinc.com>; Sunil V L <sunilvl at ventanamicro.com>;
> > Warkentin, Andrei <andrei.warkentin at 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 at 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
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105449): https://edk2.groups.io/g/devel/message/105449
Mute This Topic: https://groups.io/mt/99131196/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list