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.50529.1680255382539772738 for ; Fri, 31 Mar 2023 02:36:22 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=XjhHD1ob; 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 A824462697 for ; Fri, 31 Mar 2023 09:36:21 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 19C16C4339C for ; Fri, 31 Mar 2023 09:36:21 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1680255381; bh=B3zDmZu3HkVa2fQ8d7ki+F8nnNO6xY5MEQWx57LrC3M=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=XjhHD1obvuG3j32TPcZkJC/B6sJYtaPxZqvgH3r/mK7zwCvEtrmpJeCBC31MYrVOx 8WkraSfiDAWXie2MKc+kYQDqK7nBpo/UUytYVxveksL15VaZx/A4yZGTHZdLfh3Pc0 HitFcWtrCBjmlljgm/oLsA7IUCcBO0J6TknmzQZMdy9Ggvd2hwm5lFzfPhczB95+1/ nvu6pcT++uzngLesUxs6BNeaUzZrvulkGzRaSi46m73HRBveA/993Y/w+2+EBirCwe U2D6AC8m2nRr3qPeDVuKqK+7Br6M/byxUgfdbOuRZma9XPJDLnUhhcfdkMZFjyN/Bw NbMszPkixWegQ== Received: by mail-lj1-f181.google.com with SMTP id x20so22374380ljq.9 for ; Fri, 31 Mar 2023 02:36:20 -0700 (PDT) X-Gm-Message-State: AAQBX9cvJRi+fiu4+2cKb62JVLGqL0T4Oae1s7Oepn9aLpGQRU6RikEz gmQXvokMGwT+jfRORyPVFbdqMmrH6ppsHvdICqE= X-Google-Smtp-Source: AKy350b0stVEuH0GgGfokQ7ukN0LveZ2YMgCS+S7gfqls+qs5FgFxWhnWquRC3b/DILu4ioCTmflRRIZ39XX1zxjx2Y= X-Received: by 2002:a2e:9c0f:0:b0:299:ac5e:376e with SMTP id s15-20020a2e9c0f000000b00299ac5e376emr8172043lji.2.1680255379036; Fri, 31 Mar 2023 02:36:19 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: "Ard Biesheuvel" Date: Fri, 31 Mar 2023 11:36:07 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [RFT PATCH v2 1/6] BaseTools/tools_def XCODE: Link X64 with -read_only_relocs suppress To: =?UTF-8?Q?Marvin_H=C3=A4user?= Cc: devel@edk2.groups.io, Gerd Hoffmann , Rebecca Cran , Andrew Fish Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Fri, 31 Mar 2023 at 11:27, Marvin H=C3=A4user wrote= : > > > > On 31. Mar 2023, at 10:59, Ard Biesheuvel wrote: > > > > =EF=BB=BFOn Fri, 31 Mar 2023 at 10:29, Marvin H=C3=A4user wrote: > >> > >> > >>>> On 31. Mar 2023, at 09:39, Ard Biesheuvel wrote: > >>> =EF=BB=BFHi Marvin, > >>> > >>> Thanks for the context. > >>> > >>> > >>> On Thu, 30 Mar 2023 at 23:54, Marvin H=C3=A4user = wrote: > >>>> > >>>> Hi Ard, > >>>> > >>>> Sorry, I cannot preserve the CC list as the groups.io interface does= n't seem to allow it. Can you please CC me on future revisions? > >>>> > >>>> This patch will badly corrupt binaries. I cannot cite a source right= now (if you want me to, please remind me in your response, so I can look i= t up tomorrow), but for X64 (but not IA32, which is why this is enabled the= re), relocs are relative to the first *writable* segment. In other words, a= ny relocation to __TEXT will badly corrupt binaries this way. > >>> > >>> OMG. > >>> > >>> 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. > >> > >> Codegen does not change from the suppress flag, so there will be no ad= ditional text relocs beyond those you introduced. As they target the except= ion handler, I guess you=E2=80=99d need to actively provoke the broken code= paths (and may end up with a nice recursion :) ). > >> > > > > 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= part, 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. :) > Fair enough. > > > >>> In particular, when I dump the PE relocations using > >>> llvm-readobj --coff-basereloc, I don't see any relocations referring > >>> to the .text section. > >>> > >>>> In AUDK, we support this with two essential changes. The first is th= at we always generate a writable dummy segment at the beginning of the addr= ess space [1], making the relocs relative to the image base. The second is = that in ocmtoc, our fork of the abandoned (and pretty badly-bugged) Apple m= toc, we explicitly require this segment to be present and verify its virtua= l address is the minimum virtual address [2]. It is then omitted from the c= onversion process [3]. I suggest you replicate these changes and fully swit= ch to ocmtoc for XCODE5 builds. > >>> > >>> I'm not going to do any of that. Instead, I am going to drop this > >>> change, and do the following: > >>> > >>> - modify the SecPei version of CpuExceptionHandlerLib to put the > >>> vector templates in .data, as I proposed before. This works around th= e > >>> issue, and given that SEC/PEI is assumed to be read-only anyway (as i= t > >>> 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. > >> > >> Well, that assumption is more than fair to make for the status quo pla= tforms, but this is just another rock in the way of doing things the right = way (even if it=E2=80=99s just VMs). > >> > > > > 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. > We don't have writable data in SEC or PEI, so this would require SEC, PEI_CORE and every PEIM to have split .text and .rodata, and round them up to page size. Not sure this is worth it, especially given the fact that CoCo targets seems to be skipping the PEI phase entirely. > > > >> Cc Gerd for an OVMF security perspective. Is PEI-time memory protectio= n something you=E2=80=99d be interested in in the future? > >> > > > > 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 d= ownstream we have plans for the full deal including NX data. It=E2=80=99s h= owever shelved for the distant future, so as long as this is changed with t= he intention of reverting it once XCODE5 is fixed or dropped, that sounds f= ine to me. I just don=E2=80=99t like the notion of intentionally breaking t= he memory permission model as a hack. I rather hope we=E2=80=99ll make some= swift progress on removing XCODE5 as a source of frustration. :) > Pardon my bluntness, but why should I care about the shelved future plans of some downstream project? > > > >>> > >>> - 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. > >> > >> I agree if there=E2=80=99s an actual plan on doing that. We depend on = XCODE5 downstream, but I think it would literally be easier for us if the u= pstream version was dropped than rebasing against hacks that our slightly m= odded variant does not require. > >> > >> Cc Andrew and Rebecca. I don=E2=80=99t know anyone else who might stil= l be using XCODE5. Any objections to dropping it? If so, any plans to pick = up my proposed changes instead? > >> > > > > 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, Vit= aly evaluated both CLANG38 and CLANGPDB and found various things including = debugging to be badly broken. In fact, CLANG38 turned out to have issues li= ke misaligning UINT64s *for years*. Wow, that is very bad. Was that reported to the mailing list? > However, those issues might have been fixed and it=E2=80=99s not impossib= le Vitaly will give it another try eventually. In any case, I think our dow= nstream 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= higher overall code quality and more safety checks than GenFw, which is us= ed for CLANGDWARF.) > > The upstream toolchain has no future in my opinion, as mtoc has been depr= ecated and already failed to compile certain things (like it lacked Standal= one MM types). The reason it still =E2=80=9Cworked=E2=80=9D was because hom= ebrew silently shipped a variant with a subset of our ocmtoc patches. So as= I see it, taking our changes or dropping it entirely are the only sane opt= ions, even regardless of this particular issue you=E2=80=99re trying to fix= . Personally, I have no preference. > I think both GenFw and mtoc are horrible hacks that should be phased out once we can - with good cross-architecture Clang support for native PE binaries, I'd hope macOS could move to CLANGPDB for all targets.