Re: [edk2-devel] [RFT PATCH v2 1/6] BaseTools/tools_def XCODE: Link X64 with -read_only_relocs suppress

["Ard Biesheuvel" <ardb@kernel.org>]

On Fri, 31 Mar 2023 at 10:29, Marvin Häuser <mhaeuser@posteo.de> wrote:
>
>
> > On 31. Mar 2023, at 09:39, Ard Biesheuvel <ardb@kernel.org> wrote:
> > Hi Marvin,
> >
> > Thanks for the context.
> >
> >
> > On Thu, 30 Mar 2023 at 23:54, Marvin Häuser <mhaeuser@posteo.de> wrote:
> >>
> >> Hi Ard,
> >>
> >> 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?
> >>
> >> 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 it up tomorrow), but for X64 (but not IA32, which is why this is enabled there), relocs are relative to the first *writable* segment. In other words, any 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 additional text relocs beyond those you introduced. As they target the exception handler, I guess you’d 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.

> > 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 that we always generate a writable dummy segment at the beginning of the address 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 mtoc, we explicitly require this segment to be present and verify its virtual address 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 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 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.
>
> Well, that assumption is more than fair to make for the status quo platforms, but this is just another rock in the way of doing things the right way (even if it’s just VMs).
>

How so? SEC and PEI could be mapped read-only today, it's just that we
never bother.

> Cc Gerd for an OVMF security perspective. Is PEI-time memory protection something you’d 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.

> >
> > - 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’s an actual plan on doing that. We depend on XCODE5 downstream, but I think it would literally be easier for us if the upstream version was dropped than rebasing against hacks that our slightly modded variant does not require.
>
> Cc Andrew and Rebecca. I don’t know anyone else who might still 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?