Re: [PATCH 0/5] expire the use of PcdSetNxForStack

["Wang, Jian J" <jian.j.wang@intel.com>]

Hi Ard,

Sorry for the title which misleads you (I used a wrong word too. Shame!).
The real problem this patch series try to address is the confusion in these
two PCDs, PcdSetNxForStack and PcdDxeNxMemoryProtectionPolicy.
One of them will enable NX feature in cpu but another won’t. And there’s
also functionality overlap between them because stack memory is actually
type of EfiBootServiceData, which is also controlled by BIT4 of
PcdDxeNxMemoryProtectionPolicy.

Regards,
Jian

From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
Sent: Tuesday, September 11, 2018 4:58 PM
To: Wang, Jian J <jian.j.wang@intel.com>; Laszlo Ersek <lersek@redhat.com>; Charles Garcia-Tobin <Charles.Garcia-Tobin@arm.com>; Leif Lindholm <leif.lindholm@linaro.org>
Cc: edk2-devel@lists.01.org
Subject: Re: [edk2] [PATCH 0/5] expire the use of PcdSetNxForStack

On 11 September 2018 at 07:16, Jian J Wang <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> wrote:
> BZ#: https://bugzilla.tianocore.org/show_bug.cgi?id=1116
>
> Since the stack memory is allocated as EfiBootServicesData, its NX protection
> can be covered by BIT4 of PcdDxeNxMemoryProtectionPolicy. To avoid confusing
> in setting related PCDs, PcdSetNxForStack will be expired. Instead, If BIT4
> of PcdDxeNxMemoryProtectionPolicy is set, the DxeIpl will set NX bit in page
> table entries mapping the stack memory.
>

I disagree. This removes the possibility to map EfiBootServicesData as
executable while still mapping the stack NX. As we all know, an
executable stack is in a class of its own when it comes to
exploitability, and should *never* be mapped executable unless in
highly exceptional cases. Mapping all EfiBootServicesData as
non-executable may cause backward compatibility problems.

In particular, this makes it impossible for AArch64 to populate the
1:1 mapping using 64k pages (which is necessary for 52-bit address
support) and still have a non-executable stack, since
PcdDxeNxMemoryProtectionPolicy is disabled in that scenario.

So please disregard these patches.


> Jian J Wang (5):
>   MdeModulePkg/DxeIplPeim: expire the use of PcdSetNxForStack
>   OvmfPkg/PlatformPei: expire the use of PcdSetNxForStack
>   OvmfPkg: expire the use of PcdSetNxForStack
>   ArmVirtPkg/ArmVirt.dsc.inc: expire the use of PcdSetNxForStack
>   MdeModulePkg: expire PcdSetNxForStack
>
>  ArmVirtPkg/ArmVirt.dsc.inc                       |  5 -----
>  MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c   |  6 +++++-
>  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf          |  2 +-
>  MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  |  3 ++-
>  MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c   |  2 +-
>  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 14 +++++++++++---
>  MdeModulePkg/MdeModulePkg.dec                    | 10 +---------
>  MdeModulePkg/MdeModulePkg.uni                    | 10 +---------
>  OvmfPkg/OvmfPkgIa32.dsc                          |  1 -
>  OvmfPkg/OvmfPkgIa32X64.dsc                       |  1 -
>  OvmfPkg/OvmfPkgX64.dsc                           |  1 -
>  OvmfPkg/PlatformPei/Platform.c                   |  1 -
>  OvmfPkg/PlatformPei/PlatformPei.inf              |  1 -
>  13 files changed, 22 insertions(+), 35 deletions(-)
>
> --
> 2.16.2.windows.1
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> https://lists.01.org/mailman/listinfo/edk2-devel