Re: [edk2] [PATCH 0/5] expire the use of PcdSetNxForStack
On 12 September 2018 at 02:55, Ni, Ruiyu wrote: > > >> -Original Message- >> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Ard >> Biesheuvel >> Sent: Wednesday, September 12, 2018 5:03 AM >> To: Ni, Ruiyu >> Cc: edk2-devel@lists.01.org >> Subject: Re: [edk2] [PATCH 0/5] expire the use of PcdSetNxForStack >> >> On 11 September 2018 at 11:13, Ni, Ruiyu wrote: >> > On 9/11/2018 4:57 PM, Ard Biesheuvel wrote: >> >> >> >> On 11 September 2018 at 07:16, Jian J Wang 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. >> > >> > Ard, >> > Are you saying you want the capability of setting certain range of BS >> > data as executable? Why does ARM need such capability? >> > >> >> No, I am saying that mapping all BS data executable should be a separate >> decision from mapping the stack executable: if your platform cannot support >> the >> former (for historical reasons) you will likely still want the latter. > > Let me try to understand the specific problem in ARM64: > ARM64 uses 64KB page size to support 2^52 memory space. With 4KB page size, > it can only support 2^48 memory space. > But due to the DXE core AllocatePages() implementation, the hard-code 4KB > granularity > (defined by UEFI spec) causes the page table protection for BS_DATA/BS_CODE > is impossible. > So ARM64 chooses to disable the BS_DATA/BS_CODE protection, but only enable > the stack protection. > Correct? > If so, is changing spec to allow page-size platform configurable a better > option? > I don't think so. We need to retain compatibility with PE/COFF and third party UEFI images, so changing the page size is likely to result in much more pain than it cures. ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 0/5] expire the use of PcdSetNxForStack
> -Original Message- > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Ard > Biesheuvel > Sent: Wednesday, September 12, 2018 5:03 AM > To: Ni, Ruiyu > Cc: edk2-devel@lists.01.org > Subject: Re: [edk2] [PATCH 0/5] expire the use of PcdSetNxForStack > > On 11 September 2018 at 11:13, Ni, Ruiyu wrote: > > On 9/11/2018 4:57 PM, Ard Biesheuvel wrote: > >> > >> On 11 September 2018 at 07:16, Jian J Wang 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. > > > > Ard, > > Are you saying you want the capability of setting certain range of BS > > data as executable? Why does ARM need such capability? > > > > No, I am saying that mapping all BS data executable should be a separate > decision from mapping the stack executable: if your platform cannot support > the > former (for historical reasons) you will likely still want the latter. Let me try to understand the specific problem in ARM64: ARM64 uses 64KB page size to support 2^52 memory space. With 4KB page size, it can only support 2^48 memory space. But due to the DXE core AllocatePages() implementation, the hard-code 4KB granularity (defined by UEFI spec) causes the page table protection for BS_DATA/BS_CODE is impossible. So ARM64 chooses to disable the BS_DATA/BS_CODE protection, but only enable the stack protection. Correct? If so, is changing spec to allow page-size platform configurable a better option? > ___ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 0/5] expire the use of PcdSetNxForStack
On 11 September 2018 at 11:13, Ni, Ruiyu wrote: > On 9/11/2018 4:57 PM, Ard Biesheuvel wrote: >> >> On 11 September 2018 at 07:16, Jian J Wang 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. > > Ard, > Are you saying you want the capability of setting certain range of BS data > as executable? Why does ARM need such capability? > No, I am saying that mapping all BS data executable should be a separate decision from mapping the stack executable: if your platform cannot support the former (for historical reasons) you will likely still want the latter. ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 0/5] expire the use of PcdSetNxForStack
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.biesheu...@linaro.org] Sent: Tuesday, September 11, 2018 4:58 PM To: Wang, Jian J ; Laszlo Ersek ; Charles Garcia-Tobin ; Leif Lindholm 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 mailto:jian.j.w...@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 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 0/5] expire the use of PcdSetNxForStack
On 9/11/2018 4:57 PM, Ard Biesheuvel wrote: On 11 September 2018 at 07:16, Jian J Wang 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. Ard, Are you saying you want the capability of setting certain range of BS data as executable? Why does ARM need such capability? 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 https://lists.01.org/mailman/listinfo/edk2-devel ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel -- Thanks, Ray ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 0/5] expire the use of PcdSetNxForStack
On 11 September 2018 at 07:16, Jian J Wang 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 > https://lists.01.org/mailman/listinfo/edk2-devel ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 0/5] expire the use of PcdSetNxForStack
Thanks to simply the PCD usage. Reviewed-by: jiewen@intel.com > -Original Message- > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] > Sent: Tuesday, September 11, 2018 1:17 PM > To: edk2-devel@lists.01.org > Subject: [edk2] [PATCH 0/5] expire the use of PcdSetNxForStack > > 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. > > 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 > https://lists.01.org/mailman/listinfo/edk2-devel ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH 0/5] expire the use of PcdSetNxForStack
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. 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 https://lists.01.org/mailman/listinfo/edk2-devel
