* [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
* [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 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
* 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
* 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
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