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

[Laszlo Ersek <lersek@redhat.com>]

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