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

["Marvin Häuser" <mhaeuser@posteo.de>]

[-- Attachment #1: Type: text/plain, Size: 4294 bytes --]

Hey Ard,

*Praise* to you for this series. Comments inline.

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.

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?

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. :)

Best regards,
Marvin

> 
> GCC:*_*_AARCH64_DLINK_FLAGS =3D -z common-page-size=3D0x10000=0D
> =0D
> [LibraryClasses.common]=0D
> diff --git
> a/ArmVirtPkg/Library/ArmPlatformLibQemu/AArch64/ArmPlatformHelpe=
> r.S b/ArmVirtPkg/Library/ArmPlatformLibQemu/AArch64/ArmPlatformHelper.S
> index 5ac7c732f6ec..51c089a45ffc 100644
> --- a/ArmVirtPkg/Library/ArmPlatformLibQemu/AArch64/ArmPlatformHelper.S
> +++ b/ArmVirtPkg/Library/ArmPlatformLibQemu/AArch64/ArmPlatformHelper.S
> @@ -38,7 +38,7 @@
> .set SCTLR_EL1_ITD, 0x1 << 7=0D
> .set SCTLR_EL1_RES1, (0x1 << 11) | (0x1 << 20) | (0x1 << 22) | (0=
> x1 << 28) | (0x1 << 29)=0D
> .set sctlrval, SCTLR_ELx_M | SCTLR_ELx_C | SCTLR_ELx_SA | SCTLR_EL1_IT=
> D | SCTLR_EL1_SED=0D
> - .set sctlrval, sctlrval | SCTLR_ELx_I | SCTLR_EL1_SPAN | SCTLR_EL1_RES=
> 1=0D
> + .set sctlrval, sctlrval | SCTLR_ELx_I | SCTLR_EL1_SPAN | SCTLR_EL1_RES=
> 1 | SCTLR_EL1_WXN=0D
> =0D
> =0D
> ASM_FUNC(ArmPlatformPeiBootAction)=0D
> --=20
> 2.39.1

[-- Attachment #2: Type: text/html, Size: 4549 bytes --]