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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 12 September 2018 at 02:55, Ni, Ruiyu <ruiyu.ni@intel.com> wrote:
>
>
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard
>> Biesheuvel
>> Sent: Wednesday, September 12, 2018 5:03 AM
>> To: Ni, Ruiyu <ruiyu.ni@intel.com>
>> 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 <ruiyu.ni@intel.com> wrote:
>> > On 9/11/2018 4:57 PM, Ard Biesheuvel wrote:
>> >>
>> >> On 11 September 2018 at 07:16, Jian J Wang <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.
>> >
>> > 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.