Re: [PATCH v2 2/2] MdeModulePkg/DxeIpl: support more NX related PCDs

["Wang, Jian J" <jian.j.wang@intel.com>]

I'm fine with them too. So we have two minor changes:

1. Change EnableNonExec to IsEnableNonExecNeeded
2. Move IsExecuteDisableBitAvailable() to VirtualMemory.c

I think v3 is not necessary. I'll push the patch after enough test.

Regards,
Jian

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Friday, September 21, 2018 6:14 PM
> To: Zeng, Star <star.zeng@intel.com>; Wang, Jian J <jian.j.wang@intel.com>;
> edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
> Subject: Re: [edk2] [PATCH v2 2/2] MdeModulePkg/DxeIpl: support more NX
> related PCDs
> 
> On 09/21/18 10:42, Zeng, Star wrote:
> > Another minor suggestion is to move IsExecuteDisableBitAvailable() to
> > VirtualMemory.c, then there will be no need to declare it in
> > VirtualMemeory.h.
> 
> I'm fine with both ideas (name change as you see fit, and code movement).
> 
> Thanks
> Laszlo
> 
> >
> >
> > Thanks,
> > Star
> > On 2018/9/21 14:00, Zeng, Star wrote:
> >> Jian and Laszlo,
> >>
> >> There is also a superficial comment at below.
> >>
> >> On 2018/9/20 14:02, Jian J Wang wrote:
> >>>> v2 changes:
> >>>>     a. remove macros no longer needed
> >>>>     b. remove DEBUG and ASSERT in ToEnableExecuteDisableFeature()
> >>>>     c. change ToEnableExecuteDisableFeature to EnableNonExec
> >>>
> >>> BZ#1116: https://bugzilla.tianocore.org/show_bug.cgi?id=1116
> >>>
> >>> Currently IA32_EFER.NXE is only set against PcdSetNxForStack. This
> >>> confuses developers because following two other PCDs also need NXE
> >>> to be set, but actually not.
> >>>
> >>>      PcdDxeNxMemoryProtectionPolicy
> >>>      PcdImageProtectionPolicy
> >>>
> >>> This patch solves this issue by adding logic to enable IA32_EFER.NXE
> >>> if any of those PCDs have anything enabled.
> >>>
> >>> Cc: Star Zeng <star.zeng@intel.com>
> >>> Cc: Laszlo Ersek <lersek@redhat.com>
> >>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> >>> Cc: Jiewen Yao <jiewen.yao@intel.com>
> >>> Contributed-under: TianoCore Contribution Agreement 1.1
> >>> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> >>> ---
> >>>   MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf          |  2 ++
> >>>   MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  |  4 ++--
> >>>   MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 30
> >>> +++++++++++++++++++++++-
> >>>   MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h | 24
> >>> +++++++++++++++++++
> >>>   4 files changed, 57 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> >>> b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> >>> index fd82657404..068e700074 100644
> >>> --- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> >>> +++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> >>> @@ -117,6 +117,8 @@
> >>>   [Pcd.IA32,Pcd.X64,Pcd.ARM,Pcd.AARCH64]
> >>>     gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack               ##
> >>> SOMETIMES_CONSUMES
> >>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy
> ##
> >>> SOMETIMES_CONSUMES
> >>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy       ##
> >>> SOMETIMES_CONSUMES
> >>>   [Depex]
> >>>     gEfiPeiLoadFilePpiGuid AND gEfiPeiMasterBootModePpiGuid
> >>> diff --git a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
> >>> b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
> >>> index d28baa3615..ccd30f964b 100644
> >>> --- a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
> >>> +++ b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
> >>> @@ -245,7 +245,7 @@ ToBuildPageTable (
> >>>       return TRUE;
> >>>     }
> >>> -  if (PcdGetBool (PcdSetNxForStack) && IsExecuteDisableBitAvailable
> >>> ()) {
> >>> +  if (EnableNonExec ()) {
> >>>       return TRUE;
> >>>     }
> >>> @@ -436,7 +436,7 @@ HandOffToDxeCore (
> >>>       BuildPageTablesIa32Pae = ToBuildPageTable ();
> >>>       if (BuildPageTablesIa32Pae) {
> >>>         PageTables = Create4GPageTablesIa32Pae (BaseOfStack,
> >>> STACK_SIZE);
> >>> -      if (IsExecuteDisableBitAvailable ()) {
> >>> +      if (EnableNonExec ()) {
> >>>           EnableExecuteDisableBit();
> >>>         }
> >>>       }
> >>> diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> >>> b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> >>> index 496e219913..73b0f67c6b 100644
> >>> --- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> >>> +++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> >>> @@ -106,6 +106,31 @@ IsNullDetectionEnabled (
> >>>     return ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT0) !=
> >>> 0);
> >>>   }
> >>> +/**
> >>> +  Check if Execute Disable Bit (IA32_EFER.NXE) should be enabled or
> >>> not.
> >>> +
> >>> +  @retval TRUE    IA32_EFER.NXE should be enabled.
> >>> +  @retval FALSE   IA32_EFER.NXE should not be enabled.
> >>> +
> >>> +**/
> >>> +BOOLEAN
> >>> +EnableNonExec (
> >>> +  VOID
> >>> +  )
> >>> +{
> >>> +  if (!IsExecuteDisableBitAvailable ()) {
> >>> +    return FALSE;
> >>> +  }
> >>> +
> >>> +  //
> >>> +  // XD flag (BIT63) in page table entry is only valid if
> >>> IA32_EFER.NXE is set.
> >>> +  // Features controlled by Following PCDs need this feature to be
> >>> enabled.
> >>> +  //
> >>> +  return (PcdGetBool (PcdSetNxForStack) ||
> >>> +          PcdGet64 (PcdDxeNxMemoryProtectionPolicy) != 0 ||
> >>> +          PcdGet32 (PcdImageProtectionPolicy) != 0);
> >>> +}
> >>
> >> I am a little confused by this function name compared with
> >> EnableExecuteDisableBit(). This function is not really to enable NX,
> >> but just to check whether enable NX is needed or not. How about using
> >> name IsEnableNonExecNeeded or IsEnableNxNeeded or
> IsDisableExecuteNeeded?
> >>
> >>
> >> Sorry I did not raise this comment in V1 patch thread.
> >> If you agree with the name changing, Reviewed-by: Star Zeng
> >> <star.zeng@intel.com>.
> >>
> >>
> >> Thanks,
> >> Star
> >>> +
> >>>   /**
> >>>     Enable Execute Disable Bit.
> >>> @@ -755,7 +780,10 @@ CreateIdentityMappingPageTables (
> >>>     //
> >>>     EnablePageTableProtection ((UINTN)PageMap, TRUE);
> >>> -  if (PcdGetBool (PcdSetNxForStack)) {
> >>> +  //
> >>> +  // Set IA32_EFER.NXE if necessary.
> >>> +  //
> >>> +  if (EnableNonExec ()) {
> >>>       EnableExecuteDisableBit ();
> >>>     }
> >>> diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h
> >>> b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h
> >>> index 85457ff937..09085312aa 100644
> >>> --- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h
> >>> +++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h
> >>> @@ -179,6 +179,30 @@ typedef struct {
> >>>     UINTN           FreePages;
> >>>   } PAGE_TABLE_POOL;
> >>> +/**
> >>> +  Check if Execute Disable Bit (IA32_EFER.NXE) should be enabled or
> >>> not.
> >>> +
> >>> +  @retval TRUE    IA32_EFER.NXE should be enabled.
> >>> +  @retval FALSE   IA32_EFER.NXE should not be enabled.
> >>> +
> >>> +**/
> >>> +BOOLEAN
> >>> +EnableNonExec (
> >>> +  VOID
> >>> +  );
> >>> +
> >>> +/**
> >>> +  The function will check if Execute Disable Bit is available.
> >>> +
> >>> +  @retval TRUE      Execute Disable Bit is available.
> >>> +  @retval FALSE     Execute Disable Bit is not available.
> >>> +
> >>> +**/
> >>> +BOOLEAN
> >>> +IsExecuteDisableBitAvailable (
> >>> +  VOID
> >>> +  );
> >>> +
> >>>   /**
> >>>     Enable Execute Disable Bit.
> >>>
> >