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

["Zeng, Star" <star.zeng@intel.com>]

Another minor suggestion is to move IsExecuteDisableBitAvailable() to 
VirtualMemory.c, then there will be no need to declare it in 
VirtualMemeory.h.


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