From mboxrd@z Thu Jan 1 00:00:00 1970 Subject: Re: [edk2-devel] [RFC 13/13] ArmVirtPkg/ArmVirtQemu: Enable hardware enforced W^X memory permissions To: Ard Biesheuvel ,devel@edk2.groups.io From: =?UTF-8?B?TWFydmluIEjDpHVzZXI=?= X-Originating-Location: Berlin, Land Berlin, DE (104.28.114.20) X-Originating-Platform: Mac Safari 16.3 User-Agent: GROUPS.IO Web Poster MIME-Version: 1.0 Date: Mon, 13 Feb 2023 13:16:50 -0800 References: <20230213151810.2301480-14-ardb@kernel.org> In-Reply-To: <20230213151810.2301480-14-ardb@kernel.org> Message-ID: <18425.1676323010561670513@groups.io> Content-Type: multipart/alternative; boundary="wTPUbolsXg3lq3svzkwi" --wTPUbolsXg3lq3svzkwi Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hey Ard, *Praise* to you for this series. Comments inline. On Mon, Feb 13, 2023 at 07:19 AM, Ard Biesheuvel wrote: >=20 > 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. >=20 > 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 re= locate its own parent binary, causing issues for software W^X when .text re= locs are present (like with MSVC builds). :( >=20 >=20 > Signed-off-by: Ard Biesheuvel > --- > ArmVirtPkg/ArmVirt.dsc.inc | 1 + > ArmVirtPkg/Library/ArmPlatformLibQemu/AArch64/ArmPlatformHelper.S | 2 +- > 2 files changed, 2 insertions(+), 1 deletion(-) >=20 > 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=3D > DKII.DXE_DRIVER,BuildOp > =3D0D > [BuildOptions.common.EDKII.DXE_RUNTIME_DRIVER]=3D0D > GCC:*_*_ARM_DLINK_FLAGS =3D3D -z common-page-size=3D3D0x1000=3D0D > + GCC:*_*_AARCH64_CC_FLAGS =3D3D -fpie=3D0D 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 reas= on -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 envi= ronment. On another (related) matter, I've been spending my last two days looking in= to 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 (eve= n currently) is, that -fpie is combined with -q (a.k.a. --emit-relocs), yie= lding both object file relocs (.rela.sectname) and PIE-related relative rel= ocs (.rela) in the same binary (as documented in GenFw, they may overlap!).= It's my understanding that GenFw currently processes exclusively the -q re= locs 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 ove= r a dance with the object file relocs. This change will cause the same beha= viour for AARCH64 RT drivers now, right? In an ideal world, I suppose all architectures but IA32 (due to lacking eff= icient pcrel addressing) should be using PIE, as most (often all with X64) = GOT references can be relaxed, as we strictly deal with local symbols. Thou= gh I have to wonder how unideal the world really is. :) Best regards, Marvin >=20 > GCC:*_*_AARCH64_DLINK_FLAGS =3D3D -z common-page-size=3D3D0x10000=3D0D > =3D0D > [LibraryClasses.common]=3D0D > diff --git > a/ArmVirtPkg/Library/ArmPlatformLibQemu/AArch64/ArmPlatformHelpe=3D > 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=3D0D > .set SCTLR_EL1_RES1, (0x1 << 11) | (0x1 << 20) | (0x1 << 22) | (0=3D > x1 << 28) | (0x1 << 29)=3D0D > .set sctlrval, SCTLR_ELx_M | SCTLR_ELx_C | SCTLR_ELx_SA | SCTLR_EL1_IT=3D > D | SCTLR_EL1_SED=3D0D > - .set sctlrval, sctlrval | SCTLR_ELx_I | SCTLR_EL1_SPAN | SCTLR_EL1_RES= =3D > 1=3D0D > + .set sctlrval, sctlrval | SCTLR_ELx_I | SCTLR_EL1_SPAN | SCTLR_EL1_RES= =3D > 1 | SCTLR_EL1_WXN=3D0D > =3D0D > =3D0D > ASM_FUNC(ArmPlatformPeiBootAction)=3D0D > --=3D20 > 2.39.1 --wTPUbolsXg3lq3svzkwi Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: quoted-printable 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 ru= nning 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 t= ime page
tables, resulting in all memory mappings that are not explici= tly mapped
as read-only to be non-executable.

Note that thi= s requires runtime drivers to be built with position
independent codeg= en, to ensure that all absolute symbol references are
moved into a sep= arate section in the binary. Otherwise, unmapping the
pages that are s= ubject to relocation fixups at runtime (during the
invocation of SetVi= rtualAddressMap()) could result in code mappings
losing their executab= le permissions.
I never actually thought about this. SetVirtualAddressMap() will have to re= locate its own parent binary, causing issues for software W^X when .text re= locs are present (like with MSVC builds). :(


Signed-off-by: Ard Biesheuvel <ardb@...>
---ArmVirtPkg/ArmVirt.dsc.inc | 1 +
ArmVirtPkg/Library/ArmPlatformLib= Qemu/AArch64/ArmPlatformHelper.S | 2 +-
2 files changed, 2 insertions(= +), 1 deletion(-)

diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmV= irtPkg/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= =3D
DKII.DXE_DRIVER,BuildOp
=3D0D
[BuildOptions.common.EDKII= .DXE_RUNTIME_DRIVER]=3D0D
GCC:*_*_ARM_DLINK_FLAGS =3D3D -z common-page= -size=3D3D0x1000=3D0D
+ GCC:*_*_AARCH64_CC_FLAGS =3D3D -fpie=3D0D 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 reas= on -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 envi= ronment.

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 e= xclusively 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 effici= ent 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 regard= s,
Marvin

GCC:*_*_AARCH64_DLINK_FLAGS =3D3D -z common-page-size=3D3D0x100= 00=3D0D
=3D0D
[LibraryClasses.common]=3D0D
diff --git a/ArmV= irtPkg/Library/ArmPlatformLibQemu/AArch64/ArmPlatformHelpe=3D
r.S b/Ar= mVirtPkg/Library/ArmPlatformLibQemu/AArch64/ArmPlatformHelper.S
index = 5ac7c732f6ec..51c089a45ffc 100644
--- a/ArmVirtPkg/Library/ArmPlatform= LibQemu/AArch64/ArmPlatformHelper.S
+++ b/ArmVirtPkg/Library/ArmPlatfo= rmLibQemu/AArch64/ArmPlatformHelper.S
@@ -38,7 +38,7 @@
.set SCTL= R_EL1_ITD, 0x1 << 7=3D0D
.set SCTLR_EL1_RES1, (0x1 << 11) = | (0x1 << 20) | (0x1 << 22) | (0=3D
x1 << 28) | (0x1= << 29)=3D0D
.set sctlrval, SCTLR_ELx_M | SCTLR_ELx_C | SCTLR_EL= x_SA | SCTLR_EL1_IT=3D
D | SCTLR_EL1_SED=3D0D
- .set sctlrval, sc= tlrval | SCTLR_ELx_I | SCTLR_EL1_SPAN | SCTLR_EL1_RES=3D
1=3D0D
+= .set sctlrval, sctlrval | SCTLR_ELx_I | SCTLR_EL1_SPAN | SCTLR_EL1_RES=3D<= br />1 | SCTLR_EL1_WXN=3D0D
=3D0D
=3D0D
ASM_FUNC(ArmPlatform= PeiBootAction)=3D0D
--=3D20
2.39.1
--wTPUbolsXg3lq3svzkwi--