From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by mx.groups.io with SMTP id smtpd.web11.29136.1676325611681231132 for ; Mon, 13 Feb 2023 14:00:11 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=K1td+iAZ; spf=pass (domain: kernel.org, ip: 139.178.84.217, mailfrom: ardb@kernel.org) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id E319461307 for ; Mon, 13 Feb 2023 22:00:10 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 51983C433EF for ; Mon, 13 Feb 2023 22:00:10 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1676325610; bh=J5kNKYY136tr1lFw9p/xaeIktW8UggCrN5a4uiZAdl8=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=K1td+iAZzfFslpJx0+t1NYREV2MzXk4W2aEvjwag7dz3f8KtVWL1yK/kepkaxOcFa 9Iy81YQa3ujby7VBAdxGJScgNSbUse6ivHGy/OdGsjZWEEefyG1JeNKBOssLXLoP27 185Hvug93OZRGSpNqSAPcjhr1Pi4wL2JNUK03GJTt7RjED9H5fOnqwED3+bhxaMTF1 E0yUdL7NWBt27foTiUM0hoBivFa3soaJk+JtGa+nrJBjV/I0UJ2GXDrURFUsTvQAbN YYNhaPAOrJMoFr/ZJxqnvBA1dTw26Fkc8FCH8yWNYEP7CgDwYMoY+N6euMlp8YiPWO +lMGpEJMy+FtQ== Received: by mail-lj1-f172.google.com with SMTP id a9so16307791ljr.13 for ; Mon, 13 Feb 2023 14:00:10 -0800 (PST) X-Gm-Message-State: AO0yUKUXBOfkF281qo8TdEIsc45qXHgXzRQ9iqYX6X1GPzKCnpbf0P4b H+hABXkUat3lFZKahqhifIllGkPmYs9Nv34MOJ4= X-Google-Smtp-Source: AK7set9v4zCm03Pbsnb0mpKd//mqA4VIg6acjy2YjHObSVHkxnrjl0Lt0NVfBFqzZ5FiPAN1JkrasyvWFJSwJEtXGZk= X-Received: by 2002:a2e:88d6:0:b0:28e:1ca4:b62 with SMTP id a22-20020a2e88d6000000b0028e1ca40b62mr560ljk.97.1676325608092; Mon, 13 Feb 2023 14:00:08 -0800 (PST) MIME-Version: 1.0 References: <20230213151810.2301480-14-ardb@kernel.org> <18425.1676323010561670513@groups.io> In-Reply-To: <18425.1676323010561670513@groups.io> From: "Ard Biesheuvel" Date: Mon, 13 Feb 2023 22:59:56 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [RFC 13/13] ArmVirtPkg/ArmVirtQemu: Enable hardware enforced W^X memory permissions To: =?UTF-8?Q?Marvin_H=C3=A4user?= Cc: devel@edk2.groups.io Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Mon, 13 Feb 2023 at 22:16, Marvin H=C3=A4user 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 > --- > 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=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 previou= s patch that .plt was added to the linker script, was there a particular re= ason -fno-plt wasn't used here? I just read it may have some unexpected sid= e-effects, but I thought it would be safe for our statically-linked UEFI en= vironment. > 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 unbearabl= e to us downstream. I went through a bunch of old commits which deal with P= IE and saw it was usually disabled but for X64. The funny thing with X64 (e= ven currently) is, that -fpie is combined with -q (a.k.a. --emit-relocs), y= ielding both object file relocs (.rela.sectname) and PIE-related relative r= elocs (.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 ha= ve 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 o= ver a dance with the object file relocs. This change will cause the same be= haviour 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 e= fficient pcrel addressing) should be using PIE, as most (often all with X64= ) GOT references can be relaxed, as we strictly deal with local symbols. Th= ough I have to wonder how unideal the world really is. :) >