From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mout02.posteo.de (mout02.posteo.de [185.67.36.66]) by mx.groups.io with SMTP id smtpd.web10.50477.1680254859405521268 for ; Fri, 31 Mar 2023 02:27:40 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@posteo.de header.s=2017 header.b=dJMaYKtU; spf=pass (domain: posteo.de, ip: 185.67.36.66, mailfrom: mhaeuser@posteo.de) Received: from submission (posteo.de [185.67.36.169]) by mout02.posteo.de (Postfix) with ESMTPS id 871E824027B for ; Fri, 31 Mar 2023 11:27:36 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.de; s=2017; t=1680254856; bh=laYnTECN9sINFCIxvG28TIBjLyvClGX/9/dof1EeHyo=; h=From:Subject:Date:Cc:To:From; b=dJMaYKtUEUHrdBCs41QhVd4r28eXEi2f95z9nUtTW7a3Zo2+dvyq6+EPpX/GYEGwQ uLql26vuk/jxZCSAaYBnuVNQnAsZQzLVafuRsLFHCiybacymmROBCjq2GzLnmBJUgL xvT63aJ0qMFAJTSAMKKyUtPAcRLpT97jJ4EStV01OIzReXQpLxzSkqFC0m5kcUU1g3 kv0s0N3F3OPF9gLgBenAoB+wpcGlOwroHUeFecAz5riaCKODrGIJlLEZnCxrisI1G0 vsdz8u2yr8Pv8PqYTnkeyq4HynCp2ZlT91m60hgxTZ0JdGTGwlkfGBTDpdHIWHCS0U 8ZnRSoyY/bD7Q== Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4Pnw1M35g4z6tvJ; Fri, 31 Mar 2023 11:27:35 +0200 (CEST) From: =?UTF-8?B?TWFydmluIEjDpHVzZXI=?= Mime-Version: 1.0 (1.0) Subject: Re: [edk2-devel] [RFT PATCH v2 1/6] BaseTools/tools_def XCODE: Link X64 with -read_only_relocs suppress Date: Fri, 31 Mar 2023 09:27:34 +0000 Message-Id: References: Cc: devel@edk2.groups.io, Gerd Hoffmann , Rebecca Cran , Andrew Fish In-Reply-To: To: Ard Biesheuvel Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable > On 31. Mar 2023, at 10:59, Ard Biesheuvel wrote: >=20 > =EF=BB=BFOn Fri, 31 Mar 2023 at 10:29, Marvin H=C3=A4user wrote: >>=20 >>=20 >>>> On 31. Mar 2023, at 09:39, Ard Biesheuvel wrote: >>> =EF=BB=BFHi Marvin, >>>=20 >>> Thanks for the context. >>>=20 >>>=20 >>> On Thu, 30 Mar 2023 at 23:54, Marvin H=C3=A4user wr= ote: >>>>=20 >>>> Hi Ard, >>>>=20 >>>> Sorry, I cannot preserve the CC list as the groups.io interface doesn't= seem to allow it. Can you please CC me on future revisions? >>>>=20 >>>> This patch will badly corrupt binaries. I cannot cite a source right no= w (if you want me to, please remind me in your response, so I can look it up= tomorrow), but for X64 (but not IA32, which is why this is enabled there), r= elocs are relative to the first *writable* segment. In other words, any relo= cation to __TEXT will badly corrupt binaries this way. >>>=20 >>> OMG. >>>=20 >>> I can't believe how buggy all this stuff is. But I can confirm that >>> the resulting binaries don't look right, even though they appear to >>> boot fine. >>=20 >> Codegen does not change from the suppress flag, so there will be no addit= ional text relocs beyond those you introduced. As they target the exception h= andler, I guess you=E2=80=99d need to actively provoke the broken code paths= (and may end up with a nice recursion :) ). >>=20 >=20 > I understand that the codegen is the same. I was specifically talking > about the PE relocations, which seem to be lacking entirely. Sure, I was just elaborating on the =E2=80=9Cappear to boot fine=E2=80=9D pa= rt, which does make sense. When I last tried, the relocs were not absent but= underflown. Might be mtoc drops them somehow, I think I inspected the Mach-= O directly. Whatever, you reproduce the issue. :) >=20 >>> In particular, when I dump the PE relocations using >>> llvm-readobj --coff-basereloc, I don't see any relocations referring >>> to the .text section. >>>=20 >>>> In AUDK, we support this with two essential changes. The first is that w= e always generate a writable dummy segment at the beginning of the address s= pace [1], making the relocs relative to the image base. The second is that i= n ocmtoc, our fork of the abandoned (and pretty badly-bugged) Apple mtoc, we= explicitly require this segment to be present and verify its virtual addres= s is the minimum virtual address [2]. It is then omitted from the conversion= process [3]. I suggest you replicate these changes and fully switch to ocmt= oc for XCODE5 builds. >>>=20 >>> I'm not going to do any of that. Instead, I am going to drop this >>> change, and do the following: >>>=20 >>> - modify the SecPei version of CpuExceptionHandlerLib to put the >>> vector templates in .data, as I proposed before. This works around the >>> issue, and given that SEC/PEI is assumed to be read-only anyway (as it >>> may execute in place from flash) and does not use page alignment for >>> the sections due to size constraints, it is reasonable to assume that >>> .text and .data will be mapped executable anyway. >>=20 >> Well, that assumption is more than fair to make for the status quo platfo= rms, but this is just another rock in the way of doing things the right way (= even if it=E2=80=99s just VMs). >>=20 >=20 > How so? SEC and PEI could be mapped read-only today, it's just that we > never bother. I=E2=80=99m not concerned about read-only but NX. >=20 >> Cc Gerd for an OVMF security perspective. Is PEI-time memory protection s= omething you=E2=80=99d be interested in in the future? >>=20 >=20 > My WXN series for ARM maps all PEIMs read-only, and turns off > shadowing entirely (which makes no sense for VMs). So we have most of > what we need to do that, and this change has no bearing on that. Well yes, if everything is read-only, you guarantee W^X implicitly, but down= stream we have plans for the full deal including NX data. It=E2=80=99s howev= er shelved for the distant future, so as long as this is changed with the in= tention of reverting it once XCODE5 is fixed or dropped, that sounds fine to= me. I just don=E2=80=99t like the notion of intentionally breaking the memo= ry permission model as a hack. I rather hope we=E2=80=99ll make some swift p= rogress on removing XCODE5 as a source of frustration. :) >=20 >>>=20 >>> - update the version that performs the runtime fixups to only do so >>> when using the XCODE toolchain - we can phase that out once we drop >>> XCODE support. >>=20 >> I agree if there=E2=80=99s an actual plan on doing that. We depend on XCO= DE5 downstream, but I think it would literally be easier for us if the upstr= eam version was dropped than rebasing against hacks that our slightly modded= variant does not require. >>=20 >> Cc Andrew and Rebecca. I don=E2=80=99t know anyone else who might still b= e using XCODE5. Any objections to dropping it? If so, any plans to pick up m= y proposed changes instead? >>=20 >=20 > I wouldn't mind dropping it. In fact, I'm wondering - given that you > need to install nasm and iasl anyway - if it wouldn't make more sense > to use the CLANGPDB toolchain on macOS, and avoid the mtoc mess > entirely? I=E2=80=99d say using XCODE5 is a historical thing for us. Years ago, Vitaly= evaluated both CLANG38 and CLANGPDB and found various things including debu= gging to be badly broken. In fact, CLANG38 turned out to have issues like mi= saligning UINT64s *for years*. However, those issues might have been fixed a= nd it=E2=80=99s not impossible Vitaly will give it another try eventually. I= n any case, I think our downstream variant of XCODE5 doesn=E2=80=99t require= any level of special care, so it doesn=E2=80=99t really matter to us. (Another thing to consider is despite the bugs are fixed, mtoc has a much hi= gher overall code quality and more safety checks than GenFw, which is used f= or CLANGDWARF.) The upstream toolchain has no future in my opinion, as mtoc has been depreca= ted and already failed to compile certain things (like it lacked Standalone M= M types). The reason it still =E2=80=9Cworked=E2=80=9D was because homebrew s= ilently shipped a variant with a subset of our ocmtoc patches. So as I see i= t, taking our changes or dropping it entirely are the only sane options, eve= n regardless of this particular issue you=E2=80=99re trying to fix. Personal= ly, I have no preference. Best regards, Marvin=