On 09/17/18 07:57, Zeng, Star wrote: > How about we see the problem in another way? > > If my understanding is correct, current discussion and patches think FALSE/0 > means disable/clear NX, but that is not the fact. > According to the code implementation, FALSE/0 seems mean *AS IS* to do thing > (no code to disable/clear NX). > > PcdSetNxForStack > TRUE: Set NX for stack. > FALSE: No code to clear NX for stack. > > PcdDxeNxMemoryProtectionPolicy > BITX 1: Set NX for that memory type. > BITX 0: No code to clear NX for that memory type. > > PcdImageProtectionPolicy > BITX 1: Set NX for the image data section. > BITX 0: Not code to clear NX for the image data section. > > So, how about we think one PCD just works for itself and it does not impact > other PCDs to protect? > That means TRUE/1 is to protect and FALSE/0 is *AS IS* to do nothing. > The description of these PCDs could be enhancement if we think it is a good > way to see the problem.
Sure, that too could work for me, but then the documentation in the DEC / UNI files has to be really clear. The initial worry for the current discussion was that some platform might - protect e.g. BootServicesData type memory, - not set PcdSetNxForStack, - expect the stack to remain executable. The actual results might surprise the platform owner. If the documentation dispelled any possible misconceptions, I think your idea could work too (and it would be a lot easier to code). Thanks Laszlo > > > Thanks, > Star > -----Original Message----- > From: Wang, Jian J > Sent: Monday, September 17, 2018 10:11 AM > To: Laszlo Ersek <ler...@redhat.com>; edk2-devel@lists.01.org > Cc: Zeng, Star <star.z...@intel.com>; Ard Biesheuvel > <ard.biesheu...@linaro.org>; Ni, Ruiyu <ruiyu...@intel.com>; Yao, Jiewen > <jiewen....@intel.com> > Subject: RE: [PATCH] MdeModulePkg/DxeIpl: support more NX related PCDs > > Laszlo, > > Thanks for the comments. > > Regards, > Jian > >> -----Original Message----- >> From: Laszlo Ersek [mailto:ler...@redhat.com] >> Sent: Friday, September 14, 2018 5:51 PM >> To: Wang, Jian J <jian.j.w...@intel.com>; edk2-devel@lists.01.org >> Cc: Zeng, Star <star.z...@intel.com>; Ard Biesheuvel >> <ard.biesheu...@linaro.org>; Ni, Ruiyu <ruiyu...@intel.com>; Yao, >> Jiewen <jiewen....@intel.com> >> Subject: Re: [PATCH] MdeModulePkg/DxeIpl: support more NX related PCDs >> >> I've got some comments on the code as well: >> >> On 09/14/18 07:13, Jian J Wang wrote: >>> 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. >>> >>> Due to the fact that NX memory type of stack (enabled by >>> PcdSetNxForStack) >>> and image data section (enabled by PcdImageProtectionPolicy) are >>> also >>> part of PcdDxeNxMemoryProtectionPolicy, this patch also add more >>> checks >>> to warn (ASSERT) users any unreasonable setting combinations. For >>> example, >>> >>> PcdSetNxForStack == FALSE && >>> (PcdDxeNxMemoryProtectionPolicy & (1 <<EfiBootServicesData)) >>> != 0 >>> >>> PcdImageProtectionPolicy == 0 && >>> (PcdDxeNxMemoryProtectionPolicy & (1 << >>> EfiRuntimeServicesData)) != 0 >>> >>> PcdImageProtectionPolicy == 0 && >>> (PcdDxeNxMemoryProtectionPolicy & (1 <<EfiBootServicesData)) >>> != 0 >>> >>> PcdImageProtectionPolicy == 0 && >>> (PcdDxeNxMemoryProtectionPolicy & (1 <<EfiLoaderData)) != 0 >>> >>> In other words, PcdSetNxForStack and PcdImageProtectionPolicy have >>> priority over PcdDxeNxMemoryProtectionPolicy. >>> >>> Cc: Star Zeng <star.z...@intel.com> >>> Cc: Laszlo Ersek <ler...@redhat.com> >>> Cc: Ard Biesheuvel <ard.biesheu...@linaro.org> >>> Cc: Ruiyu Ni <ruiyu...@intel.com> >>> Cc: Jiewen Yao <jiewen....@intel.com> >>> Contributed-under: TianoCore Contribution Agreement 1.1 >>> Signed-off-by: Jian J Wang <jian.j.w...@intel.com> >>> --- >>> MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf | 2 + >>> MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c | 4 +- >>> MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 55 +++++++++++++ >> ++++++++++- >>> MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h | 33 +++++++++++++ >> + >>> 4 files changed, 91 insertions(+), 3 deletions(-) >>> >>> diff -- >> git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf >> b/MdeModulePkg/Core/DxeIp lPeim/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 ## >>> SOMETIM >> ES_CONSUMES >>> + gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy ## >> SOMETIMES_CONSUMES >>> + gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy ## >>> SOME >> TIMES_CONSUMES >>> >>> [Depex] >>> gEfiPeiLoadFilePpiGuid AND gEfiPeiMasterBootModePpiGuid >>> diff -- >> git a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c b/MdeModulePkg/ >> Core/DxeIplPeim/Ia32/DxeLoadFunc.c >>> index d28baa3615..9a97205ef8 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 (ToEnableExecuteDisableFeature ()) { >>> return TRUE; >>> } >>> >>> @@ -436,7 +436,7 @@ HandOffToDxeCore ( >>> BuildPageTablesIa32Pae = ToBuildPageTable (); >>> if (BuildPageTablesIa32Pae) { >>> PageTables = Create4GPageTablesIa32Pae (BaseOfStack, >>> STACK_SIZE); >>> - if (IsExecuteDisableBitAvailable ()) { >>> + if (ToEnableExecuteDisableFeature ()) { >>> EnableExecuteDisableBit(); >>> } >>> } >>> diff -- >> git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c b/MdeModulePkg >> /Core/DxeIplPeim/X64/VirtualMemory.c >>> index 496e219913..253fe84223 100644 >>> --- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c >>> +++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c >>> @@ -106,6 +106,56 @@ 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 >>> +ToEnableExecuteDisableFeature ( >>> + VOID >>> + ) >> >> I think we're over-complicating the name of this function. First, "To" >> looks unnecessary. Second, "Enable Execute Disable" is just an >> engineer's way to say "Disable Execution". Can we say right that: >> DisableExec()? >> >> Or at least, if we consider "NX" a word in its own right, "EnableNX()"? > > I prefer more general one. Let's use DisableExec(). > >> >>> +{ >>> + if (!IsExecuteDisableBitAvailable ()) { >>> + return FALSE; >>> + } >>> + >>> + // >>> + // Normally stack is type of EfiBootServicesData. Disabling NX >>> for stack >>> + // but enabling NX for EfiBootServicesData doesn't make any sense. >>> + // >> >> This comment is good. >> >>> + if (PcdGetBool (PcdSetNxForStack) == FALSE && >> >> Please don't compare PcdGetBool() against TRUE or FALSE, just say >> PcdGetBool(), or !PcdGetBool(). >> >>> + (PcdGet64 (PcdDxeNxMemoryProtectionPolicy) & >>> STACK_MEMORY_TYPE) >> != 0) { >>> + DEBUG ((DEBUG_ERROR, >>> + "ERROR: NX for stack is disabled but NX for its memory >>> type is enabled >> !\r\n")); >>> + ASSERT(!(PcdGetBool (PcdSetNxForStack) == FALSE && >>> + (PcdGet64 (PcdDxeNxMemoryProtectionPolicy) & >>> STACK_MEMORY_T >> YPE) != 0)); >>> + } >> >> Please drop both the explicit "if", and the DEBUG message. Just keep >> the comment (which is already fine) and the ASSERT(). The ASSERT() >> will tell people where to look, and the comment will explain the >> assertion. Also, in a RELEASE build, the check should be eliminated >> entirely, but that might not work for the explicit "if" (dependent on >> compilers and/or fixed vs. dynamic PCDs). >> >> Furthermore, keeping the logical negation operator as the outermost >> operator makes the code a lot harder to read. It's much better to just >> assert what we actually require, which is: >> >> (DxeNxMemoryProtectionPolicy covers BSD) --> SetNxForStack >> >> put differently, >> >> NOT(DxeNxMemoryProtectionPolicy covers BSD) OR SetNxForStack >> >> in C: >> >> ASSERT ( >> (PcdGet64 (PcdDxeNxMemoryProtectionPolicy) & STACK_MEMORY_TYPE) == >> 0 || >> PcdGetBool (PcdSetNxForStack) >> ); >> > > I don't have strong opinions on these. So let's do it your way. > >>> + >>> + // >>> + // Image data section could be type of EfiLoaderData, >>> EfiBootServicesData >>> + // or EfiRuntimeServicesData. Disabling NX for image data but >>> enabling NX >>> + // for any those memory types doesn't make any sense. >>> + // >> >> The comment is good, I just suggest extending it with the origin of >> the >> image: "Disabling NX for image data (regardless of image origin) for >> any those memory types ...". >> > > Sure. I'll add it. > >>> + if (PcdGet32 (PcdImageProtectionPolicy) == 0 && >>> + (PcdGet64 (PcdDxeNxMemoryProtectionPolicy) & IMAGE_DATA_MEMOR >> Y_TYPE) != 0) { >>> + DEBUG ((DEBUG_ERROR, >>> + "ERROR: NX for image data is disabled but NX for its >>> memory type(s) is >> enabled!\r\n")); >>> + ASSERT (!(PcdGet32 (PcdImageProtectionPolicy) == 0 && >>> + (PcdGet64 (PcdDxeNxMemoryProtectionPolicy) & >>> IMAGE_DATA_ME >> MORY_TYPE) != 0)); >>> + } >> >> Summarizing my points from before, here we should have: >> >> ASSERT ( >> (PcdGet64 (PcdDxeNxMemoryProtectionPolicy) & IMAGE_DATA_MEMORY_TY >> PE) == 0 || >> PcdGet32 (PcdImageProtectionPolicy) == 3 >> ); >> >> That is, >> >> - If we disable DxeNxMemoryProtectionPolicy for all of EfiLoaderData, >> EfiBootServicesData, and EfiRuntimeServicesData, then any >> ImageProtectionPolicy is fine. >> >> - If we enable DxeNxMemoryProtectionPolicy for any of EfiLoaderData, >> EfiBootServicesData, and EfiRuntimeServicesData, then we require the >> platform to set ImageProtectionPolicy regardless of image origin. >> >> Thanks >> Laszlo >> > > Good catch. I missed that part. Thanks. > >>> + >>> + // >>> + // 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); >>> +} >>> + >>> /** >>> Enable Execute Disable Bit. >>> >>> @@ -755,7 +805,10 @@ CreateIdentityMappingPageTables ( >>> // >>> EnablePageTableProtection ((UINTN)PageMap, TRUE); >>> >>> - if (PcdGetBool (PcdSetNxForStack)) { >>> + // >>> + // Set IA32_EFER.NXE if necessary. >>> + // >>> + if (ToEnableExecuteDisableFeature ()) { >>> EnableExecuteDisableBit (); >>> } >>> >>> diff -- >> git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h b/MdeModulePkg >> /Core/DxeIplPeim/X64/VirtualMemory.h >>> index 85457ff937..9f152e6531 100644 >>> --- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h >>> +++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h >>> @@ -179,6 +179,39 @@ typedef struct { >>> UINTN FreePages; >>> } PAGE_TABLE_POOL; >>> >>> +// >>> +// Bit field repsentations of some EFI_MEMORY_TYPE, for page table >>> initializ >> ation. >>> +// >>> +#define STACK_MEMORY_TYPE (1 << EfiBootServicesData) >>> /* 0x10 */ >>> +#define IMAGE_DATA_MEMORY_TYPE ((1 << EfiLoaderData) | >>> /* 0x04 >> */\ >>> + (1 << EfiBootServicesData) | >>> /* 0x10 */\ >>> + (1 << >>> EfiRuntimeServicesData)/* 0x40 */\ >>> + ) >>> /* 0x54 */ >>> + >>> +/** >>> + 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 >>> +ToEnableExecuteDisableFeature ( >>> + 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 edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
