Re: [edk2-devel] [RFC 13/13] ArmVirtPkg/ArmVirtQemu: Enable hardware enforced W^X memory permissions

["Ard Biesheuvel" <ardb@kernel.org>]

On Mon, 13 Feb 2023 at 22:16, Marvin Häuser <mhaeuser@posteo.de> wrote:
>
> Hey Ard,
>
> *Praise* to you for this series. Comments inline.
>

Thanks :-)

> On Mon, Feb 13, 2023 at 07:19 AM, Ard Biesheuvel wrote:
>
> Enable the WXN system control bit straight out of reset when running in
> EL1 with the initial ID map from flash. This setting will be inherited
> by the page table code after it sets up the permanent boot time page
> tables, resulting in all memory mappings that are not explicitly mapped
> as read-only to be non-executable.
>
> Note that this requires runtime drivers to be built with position
> independent codegen, to ensure that all absolute symbol references are
> moved into a separate section in the binary. Otherwise, unmapping the
> pages that are subject to relocation fixups at runtime (during the
> invocation of SetVirtualAddressMap()) could result in code mappings
> losing their executable permissions.
>
> I never actually thought about this. SetVirtualAddressMap() will have to relocate its own parent binary, causing issues for software W^X when .text relocs are present (like with MSVC builds). :(
>
>
> Signed-off-by: Ard Biesheuvel <ardb@...>
> ---
> ArmVirtPkg/ArmVirt.dsc.inc | 1 +
> ArmVirtPkg/Library/ArmPlatformLibQemu/AArch64/ArmPlatformHelper.S | 2 +-
> 2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
> index 5b18184be263..928dd6330edb 100644
> --- a/ArmVirtPkg/ArmVirt.dsc.inc
> +++ b/ArmVirtPkg/ArmVirt.dsc.inc
> @@ -31,6 +31,7 @@ [BuildOptions.common.EDKII.DXE_CORE,BuildOptions.common.E=
> DKII.DXE_DRIVER,BuildOp
> =0D
> [BuildOptions.common.EDKII.DXE_RUNTIME_DRIVER]=0D
> GCC:*_*_ARM_DLINK_FLAGS =3D -z common-page-size=3D0x1000=0D
> + GCC:*_*_AARCH64_CC_FLAGS =3D -fpie=0D
>
> Doesn't this mean -pie must be passed to the linker? I saw in the previous patch that .plt was added to the linker script, was there a particular reason -fno-plt wasn't used here? I just read it may have some unexpected side-effects, but I thought it would be safe for our statically-linked UEFI environment.
>

No, the only reason for adding -fpie here is to ensure that statically
initialized CONST pointers are emitted into .data.rel.ro and not into
.rodata, as this is under the control of the compiler. Although,
thinking about this, I wonder if we need to pass this to the linker
for codegen under LTO as well. But the PIE link itself should be
unnecessary here.

> On another (related) matter, I've been spending my last two days looking into the whole ELF-to-PE process, because GenFw has been becoming unbearable to us downstream. I went through a bunch of old commits which deal with PIE and saw it was usually disabled but for X64. The funny thing with X64 (even currently) is, that -fpie is combined with -q (a.k.a. --emit-relocs), yielding both object file relocs (.rela.sectname) and PIE-related relative relocs (.rela) in the same binary (as documented in GenFw, they may overlap!). It's my understanding that GenFw currently processes exclusively the -q relocs and not the -fpie relocs (which should be safe as done for X64, I have no experience with ARM whatsoever). However, when PIE is involved anyway, it makes most sense to me to use its related relocs for the translation over a dance with the object file relocs. This change will cause the same behaviour for AARCH64 RT drivers now, right?
>

It will if you pass -pie to the linker, which is why I would prefer to
avoid that. The main issue IIRC is that the emit-relocs section does
not cover the entries in the GOT table that also require relocation,
and are only covered by the PIE .rela section. For AArch64, I added
relaxation logic to GenFw to actually patch the instructions instead,
which is always possible given the absence of dynamic linking.
(d2687f23c909475d80cef32cdf9a5d121f0a9ae6,
7b8f69d7e10628d473dd225224d8c2122d25a38d)

This means that we don't have to care about compiler generated symbol
references, and so the relocs emitted by emit-relocs are sufficient,
and the additional ones emitted into .rela are unused anyway. The only
remaining absolute references are the ones resulting from statically
initialized globals, and those will either be in .data or in
.data.rel.ro (if -fpie is being used)

But I agree that not using --emit-relocs and only relying on the .rela
section to populate the PE/COFF reloc section would be far cleaner.

> In an ideal world, I suppose all architectures but IA32 (due to lacking efficient pcrel addressing) should be using PIE, as most (often all with X64) GOT references can be relaxed, as we strictly deal with local symbols. Though I have to wonder how unideal the world really is. :)
>