* [PATCH v2 0/2] clarify NXE enabling logic
@ 2018-09-20 6:02 Jian J Wang
2018-09-20 6:02 ` [PATCH v2 1/2] MdeModulePkg/MdeModulePkg.dec/.uni: clarify PCDs usage Jian J Wang
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Jian J Wang @ 2018-09-20 6:02 UTC (permalink / raw)
To: edk2-devel; +Cc: Star Zeng, Laszlo Ersek, Ard Biesheuvel, Ruiyu Ni, Jiewen Yao
> v2 changes:
> Incorporates review comments from Laszlo and Star.
BZ#1116: https://bugzilla.tianocore.org/show_bug.cgi?id=1116
Test:
a. try all related PCDs combinations and check the page table attributes
b. boot to shell on real intel platform with valid PCD setting combinations
(IA32/X64)
c. boot to fedora26, ubuntu18.04, windows 7 and windows 10 on OVMF emulated
platform (X64)
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>
Jian J Wang (2):
MdeModulePkg/MdeModulePkg.dec/.uni: clarify PCDs usage
MdeModulePkg/DxeIpl: support more NX related PCDs
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 +++++++++++++++++++
MdeModulePkg/MdeModulePkg.dec | 20 ++++++++++++----
MdeModulePkg/MdeModulePkg.uni | 13 ++++++----
6 files changed, 81 insertions(+), 12 deletions(-)
--
2.16.2.windows.1
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH v2 1/2] MdeModulePkg/MdeModulePkg.dec/.uni: clarify PCDs usage 2018-09-20 6:02 [PATCH v2 0/2] clarify NXE enabling logic Jian J Wang @ 2018-09-20 6:02 ` Jian J Wang 2018-09-21 5:49 ` Zeng, Star 2018-09-20 6:02 ` [PATCH v2 2/2] MdeModulePkg/DxeIpl: support more NX related PCDs Jian J Wang 2018-09-20 11:31 ` [PATCH v2 0/2] clarify NXE enabling logic Laszlo Ersek 2 siblings, 1 reply; 9+ messages in thread From: Jian J Wang @ 2018-09-20 6:02 UTC (permalink / raw) To: edk2-devel; +Cc: Star Zeng, Laszlo Ersek, Ard Biesheuvel, Ruiyu Ni, Jiewen Yao > v2 changes: > Newly added patch to clarify PCDs usage. BZ#1116: https://bugzilla.tianocore.org/show_bug.cgi?id=1116 The usage of following PCDs described in MdeModulePkg.dec don't match the implementation exactly. This patch updates related description in both .dec and .uni files to avoid confusion in platform configuration. PcdSetNxForStack PcdImageProtectionPolicy PcdDxeNxMemoryProtectionPolicy The main change is at the statement on how to handle the FALSE or 0 setting value in those PCDs. Current statement says the implementation should unset or disable related features but in fact the related code just do nothing (leave it to AS-IS). That means the result might be disabled, or might be not. It depends on other features or platform policy. For example, if one don't want to enforce NX onto stack memory, he/she needs to set PcdSetNxForStack to FALSE as well as to clear BIT4 of PcdDxeNxMemoryProtectionPolicy. 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/MdeModulePkg.dec | 20 +++++++++++++++----- MdeModulePkg/MdeModulePkg.uni | 13 +++++++++---- 2 files changed, 24 insertions(+), 9 deletions(-) diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec index 74a699cbb7..6566b57fd4 100644 --- a/MdeModulePkg/MdeModulePkg.dec +++ b/MdeModulePkg/MdeModulePkg.dec @@ -1288,17 +1288,23 @@ ## Set image protection policy. The policy is bitwise. # If a bit is set, the image will be protected by DxeCore if it is aligned. # The code section becomes read-only, and the data section becomes non-executable. - # If a bit is clear, the image will not be protected.<BR><BR> + # If a bit is clear, nothing will be done to image code/data sections.<BR><BR> # BIT0 - Image from unknown device. <BR> # BIT1 - Image from firmware volume.<BR> + # <BR> + # Note: If a bit is cleared, the data section could be still non-executable if + # PcdDxeNxMemoryProtectionPolicy is enabled for EfiLoaderData, EfiBootServicesData + # and/or EfiRuntimeServicesData.<BR> + # <BR> # @Prompt Set image protection policy. # @ValidRange 0x80000002 | 0x00000000 - 0x0000001F gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy|0x00000002|UINT32|0x00001047 ## Set DXE memory protection policy. The policy is bitwise. # If a bit is set, memory regions of the associated type will be mapped - # non-executable.<BR><BR> - # + # non-executable.<BR> + # If a bit is cleared, nothing will be done to associated type of memory.<BR> + # <BR> # Below is bit mask for this PCD: (Order is same as UEFI spec)<BR> # EfiReservedMemoryType 0x0001<BR> # EfiLoaderCode 0x0002<BR> @@ -1890,8 +1896,12 @@ # For the DxeIpl and the DxeCore are both X64, set NX for stack feature also require PcdDxeIplBuildPageTables be TRUE.<BR> # For the DxeIpl and the DxeCore are both IA32 (PcdDxeIplSwitchToLongMode is FALSE), set NX for stack feature also require # IA32 PAE is supported and Execute Disable Bit is available.<BR> - # TRUE - to set NX for stack.<BR> - # FALSE - Not to set NX for stack.<BR> + # <BR> + # Note: If this PCD is set to FALSE, NX could be still applied to stack due to PcdDxeNxMemoryProtectionPolicy enabled for + # EfiBootServicesData.<BR> + # <BR> + # TRUE - Set NX for stack.<BR> + # FALSE - Do nothing for stack.<BR> # @Prompt Set NX for stack. gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|FALSE|BOOLEAN|0x0001006f diff --git a/MdeModulePkg/MdeModulePkg.uni b/MdeModulePkg/MdeModulePkg.uni index 080b8a62c0..61befdc1e4 100644 --- a/MdeModulePkg/MdeModulePkg.uni +++ b/MdeModulePkg/MdeModulePkg.uni @@ -345,8 +345,9 @@ "For the DxeIpl and the DxeCore are both X64, set NX for stack feature also require PcdDxeIplBuildPageTables be TRUE.<BR>" "For the DxeIpl and the DxeCore are both IA32 (PcdDxeIplSwitchToLongMode is FALSE), set NX for stack feature also require" "IA32 PAE is supported and Execute Disable Bit is available.<BR>" - "TRUE - to set NX for stack.<BR>" - "FALSE - Not to set NX for stack.<BR>" + "Note: If this PCD is set to FALSE, NX could be still applied to stack due to PcdDxeNxMemoryProtectionPolicy enabled for EfiBootServicesData.<BR>" + "TRUE - Set NX for stack.<BR>" + "FALSE - Do nothing for stack.<BR>" #string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdAcpiS3Enable_PROMPT #language en-US "ACPI S3 Enable" @@ -1098,15 +1099,19 @@ #string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdImageProtectionPolicy_HELP #language en-US "Set image protection policy. The policy is bitwise.\n" "If a bit is set, the image will be protected by DxeCore if it is aligned.\n" "The code section becomes read-only, and the data section becomes non-executable.\n" - "If a bit is clear, the image will not be protected.<BR><BR>\n" + "If a bit is clear, nothing will be done to image code/data sections.<BR><BR>\n" "BIT0 - Image from unknown device. <BR>\n" "BIT1 - Image from firmware volume.<BR>" + "Note: If a bit is cleared, the data section could be still non-executable if\n" + "PcdDxeNxMemoryProtectionPolicy is enabled for EfiLoaderData, EfiBootServicesData\n" + "and/or EfiRuntimeServicesData.<BR>" #string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdDxeNxMemoryProtectionPolicy_PROMPT #language en-US "Set DXE memory protection policy." #string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdDxeNxMemoryProtectionPolicy_HELP #language en-US "Set DXE memory protection policy. The policy is bitwise.\n" "If a bit is set, memory regions of the associated type will be mapped\n" - "non-executable.<BR><BR>\n" + "non-executable.<BR>\n" + "If a bit is cleared, nothing will be done to associated type of memory.<BR><BR>\n" "\n" "Below is bit mask for this PCD: (Order is same as UEFI spec)<BR>\n" "EfiReservedMemoryType 0x0001<BR>\n" -- 2.16.2.windows.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] MdeModulePkg/MdeModulePkg.dec/.uni: clarify PCDs usage 2018-09-20 6:02 ` [PATCH v2 1/2] MdeModulePkg/MdeModulePkg.dec/.uni: clarify PCDs usage Jian J Wang @ 2018-09-21 5:49 ` Zeng, Star 0 siblings, 0 replies; 9+ messages in thread From: Zeng, Star @ 2018-09-21 5:49 UTC (permalink / raw) To: Jian J Wang, edk2-devel; +Cc: Ruiyu Ni, Jiewen Yao, Laszlo Ersek, star.zeng Jian, The clarifications are very good. There is a very superficial comment at below. On 2018/9/20 14:02, Jian J Wang wrote: >> v2 changes: >> Newly added patch to clarify PCDs usage. > > BZ#1116: https://bugzilla.tianocore.org/show_bug.cgi?id=1116 > > The usage of following PCDs described in MdeModulePkg.dec don't match > the implementation exactly. This patch updates related description in > both .dec and .uni files to avoid confusion in platform configuration. > > PcdSetNxForStack > PcdImageProtectionPolicy > PcdDxeNxMemoryProtectionPolicy > > The main change is at the statement on how to handle the FALSE or 0 > setting value in those PCDs. Current statement says the implementation > should unset or disable related features but in fact the related code > just do nothing (leave it to AS-IS). That means the result might be > disabled, or might be not. It depends on other features or platform > policy. > > For example, if one don't want to enforce NX onto stack memory, he/she > needs to set PcdSetNxForStack to FALSE as well as to clear BIT4 of > PcdDxeNxMemoryProtectionPolicy. > > 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/MdeModulePkg.dec | 20 +++++++++++++++----- > MdeModulePkg/MdeModulePkg.uni | 13 +++++++++---- > 2 files changed, 24 insertions(+), 9 deletions(-) > > diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec > index 74a699cbb7..6566b57fd4 100644 > --- a/MdeModulePkg/MdeModulePkg.dec > +++ b/MdeModulePkg/MdeModulePkg.dec > @@ -1288,17 +1288,23 @@ > ## Set image protection policy. The policy is bitwise. > # If a bit is set, the image will be protected by DxeCore if it is aligned. > # The code section becomes read-only, and the data section becomes non-executable. > - # If a bit is clear, the image will not be protected.<BR><BR> > + # If a bit is clear, nothing will be done to image code/data sections.<BR><BR> > # BIT0 - Image from unknown device. <BR> > # BIT1 - Image from firmware volume.<BR> > + # <BR> > + # Note: If a bit is cleared, the data section could be still non-executable if > + # PcdDxeNxMemoryProtectionPolicy is enabled for EfiLoaderData, EfiBootServicesData > + # and/or EfiRuntimeServicesData.<BR> > + # <BR> > # @Prompt Set image protection policy. > # @ValidRange 0x80000002 | 0x00000000 - 0x0000001F > gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy|0x00000002|UINT32|0x00001047 > > ## Set DXE memory protection policy. The policy is bitwise. > # If a bit is set, memory regions of the associated type will be mapped > - # non-executable.<BR><BR> > - # > + # non-executable.<BR> > + # If a bit is cleared, nothing will be done to associated type of memory.<BR> > + # <BR> > # Below is bit mask for this PCD: (Order is same as UEFI spec)<BR> > # EfiReservedMemoryType 0x0001<BR> > # EfiLoaderCode 0x0002<BR> > @@ -1890,8 +1896,12 @@ > # For the DxeIpl and the DxeCore are both X64, set NX for stack feature also require PcdDxeIplBuildPageTables be TRUE.<BR> > # For the DxeIpl and the DxeCore are both IA32 (PcdDxeIplSwitchToLongMode is FALSE), set NX for stack feature also require > # IA32 PAE is supported and Execute Disable Bit is available.<BR> > - # TRUE - to set NX for stack.<BR> > - # FALSE - Not to set NX for stack.<BR> > + # <BR> > + # Note: If this PCD is set to FALSE, NX could be still applied to stack due to PcdDxeNxMemoryProtectionPolicy enabled for > + # EfiBootServicesData.<BR> How about adding this sentence to be after TRUE/FALSE statements below? Anyway, Reviewed-by: Star Zeng <star.zeng@intel.com>. Thanks, Star > + # <BR> > + # TRUE - Set NX for stack.<BR> > + # FALSE - Do nothing for stack.<BR> > # @Prompt Set NX for stack. > gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|FALSE|BOOLEAN|0x0001006f > > diff --git a/MdeModulePkg/MdeModulePkg.uni b/MdeModulePkg/MdeModulePkg.uni > index 080b8a62c0..61befdc1e4 100644 > --- a/MdeModulePkg/MdeModulePkg.uni > +++ b/MdeModulePkg/MdeModulePkg.uni > @@ -345,8 +345,9 @@ > "For the DxeIpl and the DxeCore are both X64, set NX for stack feature also require PcdDxeIplBuildPageTables be TRUE.<BR>" > "For the DxeIpl and the DxeCore are both IA32 (PcdDxeIplSwitchToLongMode is FALSE), set NX for stack feature also require" > "IA32 PAE is supported and Execute Disable Bit is available.<BR>" > - "TRUE - to set NX for stack.<BR>" > - "FALSE - Not to set NX for stack.<BR>" > + "Note: If this PCD is set to FALSE, NX could be still applied to stack due to PcdDxeNxMemoryProtectionPolicy enabled for EfiBootServicesData.<BR>" > + "TRUE - Set NX for stack.<BR>" > + "FALSE - Do nothing for stack.<BR>" > > #string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdAcpiS3Enable_PROMPT #language en-US "ACPI S3 Enable" > > @@ -1098,15 +1099,19 @@ > #string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdImageProtectionPolicy_HELP #language en-US "Set image protection policy. The policy is bitwise.\n" > "If a bit is set, the image will be protected by DxeCore if it is aligned.\n" > "The code section becomes read-only, and the data section becomes non-executable.\n" > - "If a bit is clear, the image will not be protected.<BR><BR>\n" > + "If a bit is clear, nothing will be done to image code/data sections.<BR><BR>\n" > "BIT0 - Image from unknown device. <BR>\n" > "BIT1 - Image from firmware volume.<BR>" > + "Note: If a bit is cleared, the data section could be still non-executable if\n" > + "PcdDxeNxMemoryProtectionPolicy is enabled for EfiLoaderData, EfiBootServicesData\n" > + "and/or EfiRuntimeServicesData.<BR>" > > #string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdDxeNxMemoryProtectionPolicy_PROMPT #language en-US "Set DXE memory protection policy." > > #string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdDxeNxMemoryProtectionPolicy_HELP #language en-US "Set DXE memory protection policy. The policy is bitwise.\n" > "If a bit is set, memory regions of the associated type will be mapped\n" > - "non-executable.<BR><BR>\n" > + "non-executable.<BR>\n" > + "If a bit is cleared, nothing will be done to associated type of memory.<BR><BR>\n" > "\n" > "Below is bit mask for this PCD: (Order is same as UEFI spec)<BR>\n" > "EfiReservedMemoryType 0x0001<BR>\n" > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 2/2] MdeModulePkg/DxeIpl: support more NX related PCDs 2018-09-20 6:02 [PATCH v2 0/2] clarify NXE enabling logic Jian J Wang 2018-09-20 6:02 ` [PATCH v2 1/2] MdeModulePkg/MdeModulePkg.dec/.uni: clarify PCDs usage Jian J Wang @ 2018-09-20 6:02 ` Jian J Wang 2018-09-21 6:00 ` Zeng, Star 2018-09-20 11:31 ` [PATCH v2 0/2] clarify NXE enabling logic Laszlo Ersek 2 siblings, 1 reply; 9+ messages in thread From: Jian J Wang @ 2018-09-20 6:02 UTC (permalink / raw) To: edk2-devel; +Cc: Star Zeng, Laszlo Ersek, Ard Biesheuvel, Ruiyu Ni, Jiewen Yao > 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); +} + /** 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. -- 2.16.2.windows.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] MdeModulePkg/DxeIpl: support more NX related PCDs 2018-09-20 6:02 ` [PATCH v2 2/2] MdeModulePkg/DxeIpl: support more NX related PCDs Jian J Wang @ 2018-09-21 6:00 ` Zeng, Star 2018-09-21 8:42 ` Zeng, Star 0 siblings, 1 reply; 9+ messages in thread From: Zeng, Star @ 2018-09-21 6:00 UTC (permalink / raw) To: Jian J Wang, edk2-devel; +Cc: Ruiyu Ni, Jiewen Yao, Laszlo Ersek, star.zeng 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. > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] MdeModulePkg/DxeIpl: support more NX related PCDs 2018-09-21 6:00 ` Zeng, Star @ 2018-09-21 8:42 ` Zeng, Star 2018-09-21 10:14 ` Laszlo Ersek 0 siblings, 1 reply; 9+ messages in thread From: Zeng, Star @ 2018-09-21 8:42 UTC (permalink / raw) To: Jian J Wang, edk2-devel; +Cc: Ruiyu Ni, Laszlo Ersek, Jiewen Yao, star.zeng 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. >> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] MdeModulePkg/DxeIpl: support more NX related PCDs 2018-09-21 8:42 ` Zeng, Star @ 2018-09-21 10:14 ` Laszlo Ersek 2018-09-25 3:23 ` Wang, Jian J 0 siblings, 1 reply; 9+ messages in thread From: Laszlo Ersek @ 2018-09-21 10:14 UTC (permalink / raw) To: Zeng, Star, Jian J Wang, edk2-devel; +Cc: Ruiyu Ni, Jiewen Yao 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. >>> > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] MdeModulePkg/DxeIpl: support more NX related PCDs 2018-09-21 10:14 ` Laszlo Ersek @ 2018-09-25 3:23 ` Wang, Jian J 0 siblings, 0 replies; 9+ messages in thread From: Wang, Jian J @ 2018-09-25 3:23 UTC (permalink / raw) To: Laszlo Ersek, Zeng, Star, edk2-devel@lists.01.org; +Cc: Ni, Ruiyu, Yao, Jiewen 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. > >>> > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 0/2] clarify NXE enabling logic 2018-09-20 6:02 [PATCH v2 0/2] clarify NXE enabling logic Jian J Wang 2018-09-20 6:02 ` [PATCH v2 1/2] MdeModulePkg/MdeModulePkg.dec/.uni: clarify PCDs usage Jian J Wang 2018-09-20 6:02 ` [PATCH v2 2/2] MdeModulePkg/DxeIpl: support more NX related PCDs Jian J Wang @ 2018-09-20 11:31 ` Laszlo Ersek 2 siblings, 0 replies; 9+ messages in thread From: Laszlo Ersek @ 2018-09-20 11:31 UTC (permalink / raw) To: Jian J Wang, edk2-devel; +Cc: Star Zeng, Ard Biesheuvel, Ruiyu Ni, Jiewen Yao On 09/20/18 08:02, Jian J Wang wrote: >> v2 changes: >> Incorporates review comments from Laszlo and Star. > > BZ#1116: https://bugzilla.tianocore.org/show_bug.cgi?id=1116 > > Test: > a. try all related PCDs combinations and check the page table attributes > b. boot to shell on real intel platform with valid PCD setting combinations > (IA32/X64) > c. boot to fedora26, ubuntu18.04, windows 7 and windows 10 on OVMF emulated > platform (X64) > > 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> > > Jian J Wang (2): > MdeModulePkg/MdeModulePkg.dec/.uni: clarify PCDs usage > MdeModulePkg/DxeIpl: support more NX related PCDs > > 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 +++++++++++++++++++ > MdeModulePkg/MdeModulePkg.dec | 20 ++++++++++++---- > MdeModulePkg/MdeModulePkg.uni | 13 ++++++---- > 6 files changed, 81 insertions(+), 12 deletions(-) > Reviewed-by: Laszlo Ersek <lersek@redhat.com> ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-09-25 3:23 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-09-20 6:02 [PATCH v2 0/2] clarify NXE enabling logic Jian J Wang 2018-09-20 6:02 ` [PATCH v2 1/2] MdeModulePkg/MdeModulePkg.dec/.uni: clarify PCDs usage Jian J Wang 2018-09-21 5:49 ` Zeng, Star 2018-09-20 6:02 ` [PATCH v2 2/2] MdeModulePkg/DxeIpl: support more NX related PCDs Jian J Wang 2018-09-21 6:00 ` Zeng, Star 2018-09-21 8:42 ` Zeng, Star 2018-09-21 10:14 ` Laszlo Ersek 2018-09-25 3:23 ` Wang, Jian J 2018-09-20 11:31 ` [PATCH v2 0/2] clarify NXE enabling logic Laszlo Ersek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox