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

["Lendacky, Thomas" <thomas.lendacky@amd.com>]

On 5/30/23 20:29, Ni, Ray via groups.io wrote:
> +@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.

I'm not sure I follow. How and where would the PPI be used? And what is 
meant by "remapping the ghcb range from non-1:1 mapping to 1:1 mapping?

Thanks,
Tom

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