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 <[email protected]> >>> Cc: Laszlo Ersek <[email protected]> >>> Cc: Ard Biesheuvel <[email protected]> >>> Cc: Ruiyu Ni <[email protected]> >>> Cc: Jiewen Yao <[email protected]> >>> Contributed-under: TianoCore Contribution Agreement 1.1 >>> Signed-off-by: Jian J Wang <[email protected]> >>> --- >>> 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 >> <[email protected]>. >> >> >> 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. >>> > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
