Re: [edk2] [PATCH 2/5] OvmfPkg/PlatformPei: expire the use of PcdSetNxForStack
Laszlo,
> My understanding has been that it's not about enabling/disabling a CPU
> feature, but about marking specific pages as non-executable. Under that
> interpretation, it should be possible to mark pages used for stack
> purposes as non-executable, and leave everything else executable, even
> on x86.
I mentioned that because, for x86 cpu, IA32_EFER.NXE must be set before
marking any page as non-executable. But it's not true for current implementation
relating to PcdDxeNxMemoryProtectionPolicy. Anyway, it's not the key
point of what we've been talking about.
Thank you very much for the comments. I'll send out new patch soon.
Regards,
Jian
From: Laszlo Ersek [mailto:ler...@redhat.com]
Sent: Wednesday, September 12, 2018 6:42 PM
To: Wang, Jian J ; edk2-devel@lists.01.org
Cc: Ni, Ruiyu ; Justen, Jordan L
; Yao, Jiewen ; Zeng, Star
; Ard Biesheuvel
Subject: Re: [edk2] [PATCH 2/5] OvmfPkg/PlatformPei: expire the use of
PcdSetNxForStack
On 09/12/18 04:11, Wang, Jian J wrote:
> Hi Laszlo and Ard,
>
> Retiring the PcdSetNxForStack is not the intention of this patch series. It's
>just
> a side effect of solving problem stated in BZ#1116. Sorry again for the
>misleading
> title. I'm not insisting that we have to remove this PCD anyway (My personal
> opinion. Others might have different ones).
>
> I think I understand the importance of PcdSetNxForStack to arm/aarch64 now.
>And I
> agree that it'd be better to enable NX for stack independent of enabling NX
>for
> EfiBootServcesData memory. But since the stack is type of EfiBootServicesData,
> I think we should avoid any confusion in enabling/disabling NX for them.
>That's what
> BZ#1116 tries to complain about. But I'm still not clear any concrete
>suggestion on
> how to solve the BZ#1116 from all comment so far, if my patch series cannot
>satisfy
> all kinds of platforms. Simply keep PcdSetNxForStack doesn't help, I think.
>(It doesn't
> imply we must remove it.)
>
> As I commented in the BZ#1116, maybe we can just simply assert if there's one
> trying to disable NX for stack but enable NX for EfiBootServicesData at the
>same time,
> because it doesn't make sense.
Yes, that's what I thought as well. Catch the one case with an assert
and/or CpuDeadLoop() that is an invalid configuration.
> I think all other setting combinations won't have
> such confusion and don't need to take care specifically.
>
> And for x86 CPU, we'll always enable CPU NX feature if any NX related PCDs
>have bits
> set. Current DxeIpl will just enable NX for PcdSetNxForStack only.
My understanding has been that it's not about enabling/disabling a CPU
feature, but about marking specific pages as non-executable. Under that
interpretation, it should be possible to mark pages used for stack
purposes as non-executable, and leave everything else executable, even
on x86.
Laszlo
>
> Regards,
> Jian
>
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Tuesday, September 11, 2018 11:53 PM
> To: Wang, Jian J ; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu ; Justen, Jordan L
>; Yao, Jiewen ; Zeng, Star
>; Ard Biesheuvel
> Subject: Re: [edk2] [PATCH 2/5] OvmfPkg/PlatformPei: expire the use of
>PcdSetNxForStack
>
> On 09/11/18 07:16, Jian J Wang wrote:
>> BZ#: https://bugzilla.tianocore.org/show_bug.cgi?id=1116
>>
>> Since PcdSetNxForStack is expired, remove related config code.
>> PcdDxeNxMemoryProtectionPolicy cannot be used as dynamic PCD.
>> There's no need to add it in code to replace PcdSetNxForStack.
>>
>> Cc: Laszlo Ersek
>> Cc: Star Zeng
>> Cc: Jordan Justen
>> Cc: Ard Biesheuvel
>> Cc: Ruiyu Ni
>> Cc: Jiewen Yao
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Jian J Wang
>> ---
>> OvmfPkg/PlatformPei/Platform.c | 1 -
>> OvmfPkg/PlatformPei/PlatformPei.inf | 1 -
>> 2 files changed, 2 deletions(-)
>>
>> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
>> index 5a78668126..6627d236e0 100644
>> --- a/OvmfPkg/PlatformPei/Platform.c
>> +++ b/OvmfPkg/PlatformPei/Platform.c
>> @@ -340,7 +340,6 @@ NoexecDxeInitialization (
>> )
>> {
>> UPDATE_BOOLEAN_PCD_FROM_FW_CFG (PcdPropertiesTableEnable);
>> - UPDATE_BOOLEAN_PCD_FROM_FW_CFG (PcdSetNxForStack);
>> }
>>
>> VOID
>> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf
>>b/OvmfPkg/PlatformPei/PlatformPei.inf
>> index 9c5ad9961c..ef5dcb7693 100644
>> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
>> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
>> @@ -96,7 +96,6 @@
>> gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumerati
Re: [edk2] [PATCH 2/5] OvmfPkg/PlatformPei: expire the use of PcdSetNxForStack
On 09/12/18 04:11, Wang, Jian J wrote:
> Hi Laszlo and Ard,
>
> Retiring the PcdSetNxForStack is not the intention of this patch series. It's
> just
> a side effect of solving problem stated in BZ#1116. Sorry again for the
> misleading
> title. I'm not insisting that we have to remove this PCD anyway (My personal
> opinion. Others might have different ones).
>
> I think I understand the importance of PcdSetNxForStack to arm/aarch64 now.
> And I
> agree that it'd be better to enable NX for stack independent of enabling NX
> for
> EfiBootServcesData memory. But since the stack is type of EfiBootServicesData,
> I think we should avoid any confusion in enabling/disabling NX for them.
> That's what
> BZ#1116 tries to complain about. But I'm still not clear any concrete
> suggestion on
> how to solve the BZ#1116 from all comment so far, if my patch series cannot
> satisfy
> all kinds of platforms. Simply keep PcdSetNxForStack doesn't help, I think.
> (It doesn't
> imply we must remove it.)
>
> As I commented in the BZ#1116, maybe we can just simply assert if there's one
> trying to disable NX for stack but enable NX for EfiBootServicesData at the
> same time,
> because it doesn't make sense.
Yes, that's what I thought as well. Catch the one case with an assert
and/or CpuDeadLoop() that is an invalid configuration.
> I think all other setting combinations won't have
> such confusion and don't need to take care specifically.
>
> And for x86 CPU, we'll always enable CPU NX feature if any NX related PCDs
> have bits
> set. Current DxeIpl will just enable NX for PcdSetNxForStack only.
My understanding has been that it's not about enabling/disabling a CPU
feature, but about marking specific pages as non-executable. Under that
interpretation, it should be possible to mark pages used for stack
purposes as non-executable, and leave everything else executable, even
on x86.
Laszlo
>
> Regards,
> Jian
>
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Tuesday, September 11, 2018 11:53 PM
> To: Wang, Jian J ; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu ; Justen, Jordan L
> ; Yao, Jiewen ; Zeng, Star
> ; Ard Biesheuvel
> Subject: Re: [edk2] [PATCH 2/5] OvmfPkg/PlatformPei: expire the use of
> PcdSetNxForStack
>
> On 09/11/18 07:16, Jian J Wang wrote:
>> BZ#: https://bugzilla.tianocore.org/show_bug.cgi?id=1116
>>
>> Since PcdSetNxForStack is expired, remove related config code.
>> PcdDxeNxMemoryProtectionPolicy cannot be used as dynamic PCD.
>> There's no need to add it in code to replace PcdSetNxForStack.
>>
>> Cc: Laszlo Ersek
>> Cc: Star Zeng
>> Cc: Jordan Justen
>> Cc: Ard Biesheuvel
>> Cc: Ruiyu Ni
>> Cc: Jiewen Yao
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Jian J Wang
>> ---
>> OvmfPkg/PlatformPei/Platform.c | 1 -
>> OvmfPkg/PlatformPei/PlatformPei.inf | 1 -
>> 2 files changed, 2 deletions(-)
>>
>> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
>> index 5a78668126..6627d236e0 100644
>> --- a/OvmfPkg/PlatformPei/Platform.c
>> +++ b/OvmfPkg/PlatformPei/Platform.c
>> @@ -340,7 +340,6 @@ NoexecDxeInitialization (
>> )
>> {
>> UPDATE_BOOLEAN_PCD_FROM_FW_CFG (PcdPropertiesTableEnable);
>> - UPDATE_BOOLEAN_PCD_FROM_FW_CFG (PcdSetNxForStack);
>> }
>>
>> VOID
>> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf
>> b/OvmfPkg/PlatformPei/PlatformPei.inf
>> index 9c5ad9961c..ef5dcb7693 100644
>> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
>> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
>> @@ -96,7 +96,6 @@
>> gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration
>> gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode
>> gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable
>> - gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack
>> gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable
>> gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable
>> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask
>>
>
> I disagree with this change. I explained my reasons in
> <https://bugzilla.tianocore.org/show_bug.cgi?id=1116#c6>, but for the
> sake of discussion, I'll state the same here:
>
>> Ard's got a point here. Just because one PCD implies another, the
>> reverse is not necessarily true.
>>
>> For example, in OVMF, we set PcdSetNxForStack to TRUE originally, but
>> then we had to make it conditional, due to old GRUB variants breaking
>> with a non-executable stack. (Please refer to commit d2
Re: [edk2] [PATCH 2/5] OvmfPkg/PlatformPei: expire the use of PcdSetNxForStack
Hi Laszlo and Ard,
Retiring the PcdSetNxForStack is not the intention of this patch series. It's
just
a side effect of solving problem stated in BZ#1116. Sorry again for the
misleading
title. I'm not insisting that we have to remove this PCD anyway (My personal
opinion. Others might have different ones).
I think I understand the importance of PcdSetNxForStack to arm/aarch64 now. And
I
agree that it'd be better to enable NX for stack independent of enabling NX for
EfiBootServcesData memory. But since the stack is type of EfiBootServicesData,
I think we should avoid any confusion in enabling/disabling NX for them. That's
what
BZ#1116 tries to complain about. But I'm still not clear any concrete
suggestion on
how to solve the BZ#1116 from all comment so far, if my patch series cannot
satisfy
all kinds of platforms. Simply keep PcdSetNxForStack doesn't help, I think. (It
doesn't
imply we must remove it.)
As I commented in the BZ#1116, maybe we can just simply assert if there's one
trying to disable NX for stack but enable NX for EfiBootServicesData at the
same time,
because it doesn't make sense. I think all other setting combinations won't have
such confusion and don't need to take care specifically.
And for x86 CPU, we'll always enable CPU NX feature if any NX related PCDs have
bits
set. Current DxeIpl will just enable NX for PcdSetNxForStack only.
Regards,
Jian
From: Laszlo Ersek [mailto:ler...@redhat.com]
Sent: Tuesday, September 11, 2018 11:53 PM
To: Wang, Jian J ; edk2-devel@lists.01.org
Cc: Ni, Ruiyu ; Justen, Jordan L
; Yao, Jiewen ; Zeng, Star
; Ard Biesheuvel
Subject: Re: [edk2] [PATCH 2/5] OvmfPkg/PlatformPei: expire the use of
PcdSetNxForStack
On 09/11/18 07:16, Jian J Wang wrote:
> BZ#: https://bugzilla.tianocore.org/show_bug.cgi?id=1116
>
> Since PcdSetNxForStack is expired, remove related config code.
> PcdDxeNxMemoryProtectionPolicy cannot be used as dynamic PCD.
> There's no need to add it in code to replace PcdSetNxForStack.
>
> Cc: Laszlo Ersek
> Cc: Star Zeng
> Cc: Jordan Justen
> Cc: Ard Biesheuvel
> Cc: Ruiyu Ni
> Cc: Jiewen Yao
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang
> ---
> OvmfPkg/PlatformPei/Platform.c | 1 -
> OvmfPkg/PlatformPei/PlatformPei.inf | 1 -
> 2 files changed, 2 deletions(-)
>
> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
> index 5a78668126..6627d236e0 100644
> --- a/OvmfPkg/PlatformPei/Platform.c
> +++ b/OvmfPkg/PlatformPei/Platform.c
> @@ -340,7 +340,6 @@ NoexecDxeInitialization (
>)
> {
>UPDATE_BOOLEAN_PCD_FROM_FW_CFG (PcdPropertiesTableEnable);
> - UPDATE_BOOLEAN_PCD_FROM_FW_CFG (PcdSetNxForStack);
> }
>
> VOID
> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf
>b/OvmfPkg/PlatformPei/PlatformPei.inf
> index 9c5ad9961c..ef5dcb7693 100644
> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
> @@ -96,7 +96,6 @@
>gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration
>gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode
>gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable
> - gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack
>gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable
>gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable
>gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask
>
I disagree with this change. I explained my reasons in
<https://bugzilla.tianocore.org/show_bug.cgi?id=1116#c6>, but for the
sake of discussion, I'll state the same here:
> Ard's got a point here. Just because one PCD implies another, the
> reverse is not necessarily true.
>
> For example, in OVMF, we set PcdSetNxForStack to TRUE originally, but
> then we had to make it conditional, due to old GRUB variants breaking
> with a non-executable stack. (Please refer to commit d20b06a3afdf,
> "OvmfPkg: disable no-exec DXE stack by default", 2015-09-15).
> Therefore we now default to FALSE, and let the user set it to TRUE on
> the QEMU command line.
>
> In addition, we intentionally inherit a zero
> PcdDxeNxMemoryProtectionPolicy from "MdeModulePkg.dec".
>
> Both of the above configurations satisfy the requirement
>
> ((PcdGet64 (PcdDxeNxMemoryProtectionPolicy) &
> (1 << EfiBootServicesData)) == 0 ||
>PcdGetBool (PcdSetNxForStack))
>
> i.e. the requirement that "NX for BS data" imply "NX for stack".
>
> If you remove the standalone knob for PcdSetNxForStack, then the
> default behavior of OVMF will continue to work, however the command
> line option, for setting PcdSetNxForStack *only*, will break.
I'd like to add another comment just here (not mentioned in the BZ):
To my understanding, the Heap Guard is a debug feature [1
Re: [edk2] [PATCH 2/5] OvmfPkg/PlatformPei: expire the use of PcdSetNxForStack
On 09/11/18 07:16, Jian J Wang wrote:
> BZ#: https://bugzilla.tianocore.org/show_bug.cgi?id=1116
>
> Since PcdSetNxForStack is expired, remove related config code.
> PcdDxeNxMemoryProtectionPolicy cannot be used as dynamic PCD.
> There's no need to add it in code to replace PcdSetNxForStack.
>
> Cc: Laszlo Ersek
> Cc: Star Zeng
> Cc: Jordan Justen
> Cc: Ard Biesheuvel
> Cc: Ruiyu Ni
> Cc: Jiewen Yao
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang
> ---
> OvmfPkg/PlatformPei/Platform.c | 1 -
> OvmfPkg/PlatformPei/PlatformPei.inf | 1 -
> 2 files changed, 2 deletions(-)
>
> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
> index 5a78668126..6627d236e0 100644
> --- a/OvmfPkg/PlatformPei/Platform.c
> +++ b/OvmfPkg/PlatformPei/Platform.c
> @@ -340,7 +340,6 @@ NoexecDxeInitialization (
>)
> {
>UPDATE_BOOLEAN_PCD_FROM_FW_CFG (PcdPropertiesTableEnable);
> - UPDATE_BOOLEAN_PCD_FROM_FW_CFG (PcdSetNxForStack);
> }
>
> VOID
> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf
> b/OvmfPkg/PlatformPei/PlatformPei.inf
> index 9c5ad9961c..ef5dcb7693 100644
> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
> @@ -96,7 +96,6 @@
>gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration
>gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode
>gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable
> - gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack
>gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable
>gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable
>gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask
>
I disagree with this change. I explained my reasons in
, but for the
sake of discussion, I'll state the same here:
> Ard's got a point here. Just because one PCD implies another, the
> reverse is not necessarily true.
>
> For example, in OVMF, we set PcdSetNxForStack to TRUE originally, but
> then we had to make it conditional, due to old GRUB variants breaking
> with a non-executable stack. (Please refer to commit d20b06a3afdf,
> "OvmfPkg: disable no-exec DXE stack by default", 2015-09-15).
> Therefore we now default to FALSE, and let the user set it to TRUE on
> the QEMU command line.
>
> In addition, we intentionally inherit a zero
> PcdDxeNxMemoryProtectionPolicy from "MdeModulePkg.dec".
>
> Both of the above configurations satisfy the requirement
>
> ((PcdGet64 (PcdDxeNxMemoryProtectionPolicy) &
> (1 << EfiBootServicesData)) == 0 ||
>PcdGetBool (PcdSetNxForStack))
>
> i.e. the requirement that "NX for BS data" imply "NX for stack".
>
> If you remove the standalone knob for PcdSetNxForStack, then the
> default behavior of OVMF will continue to work, however the command
> line option, for setting PcdSetNxForStack *only*, will break.
I'd like to add another comment just here (not mentioned in the BZ):
To my understanding, the Heap Guard is a debug feature [1]. Over time,
I've reviewed and regression-tested all the Heap Guard patches that have
crossed my path with the understanding that "this is not enabled by
default in OVMF". With those points in mind, I certainly don't intend to
enable the Heap Guard as a FixedPCD -- which is the only way it can be
enabled -- in the OVMF DSC files anytime soon.
On the other hand, the user should set the stack NX preferably at all
times -- as I wrote above, that was our original idea for the OVMF
default as well, until we were forced to revert it for compatibility's
sake, and to expose the knob to the end-user instead.
With this patch series, the configuration that's currently deemed the
best compromise for OVMF (Heap Guard off, stack NX) would be lost.
[1]
http://mid.mail-archive.com/D827630B58408649ACB04F44C510003624DDC8B3@SHSMSX103.ccr.corp.intel.com
Thanks
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH 2/5] OvmfPkg/PlatformPei: expire the use of PcdSetNxForStack
BZ#: https://bugzilla.tianocore.org/show_bug.cgi?id=1116
Since PcdSetNxForStack is expired, remove related config code.
PcdDxeNxMemoryProtectionPolicy cannot be used as dynamic PCD.
There's no need to add it in code to replace PcdSetNxForStack.
Cc: Laszlo Ersek
Cc: Star Zeng
Cc: Jordan Justen
Cc: Ard Biesheuvel
Cc: Ruiyu Ni
Cc: Jiewen Yao
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang
---
OvmfPkg/PlatformPei/Platform.c | 1 -
OvmfPkg/PlatformPei/PlatformPei.inf | 1 -
2 files changed, 2 deletions(-)
diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
index 5a78668126..6627d236e0 100644
--- a/OvmfPkg/PlatformPei/Platform.c
+++ b/OvmfPkg/PlatformPei/Platform.c
@@ -340,7 +340,6 @@ NoexecDxeInitialization (
)
{
UPDATE_BOOLEAN_PCD_FROM_FW_CFG (PcdPropertiesTableEnable);
- UPDATE_BOOLEAN_PCD_FROM_FW_CFG (PcdSetNxForStack);
}
VOID
diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf
b/OvmfPkg/PlatformPei/PlatformPei.inf
index 9c5ad9961c..ef5dcb7693 100644
--- a/OvmfPkg/PlatformPei/PlatformPei.inf
+++ b/OvmfPkg/PlatformPei/PlatformPei.inf
@@ -96,7 +96,6 @@
gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration
gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode
gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable
- gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack
gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable
gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable
gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask
--
2.16.2.windows.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
