Re: [PATCH] BaseTools/GenFw ARM: don't permit R_ARM_GOT_PREL relocations

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On Wed, 12 Dec 2018 at 01:24, Gao, Liming <liming.gao@intel.com> wrote:
>
> That's good. Reviewed-by: Liming Gao <liming.gao@intel.com>
>

Thanks all

Pushed as e07092edca84..0d68ce514b92

> >-----Original Message-----
> >From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> >Sent: Tuesday, December 11, 2018 9:46 PM
> >To: Gao, Liming <liming.gao@intel.com>
> >Cc: Leif Lindholm <leif.lindholm@linaro.org>; edk2-devel@lists.01.org; Laszlo
> >Ersek <lersek@redhat.com>; Feng, Bob C <bob.c.feng@intel.com>
> >Subject: Re: [PATCH] BaseTools/GenFw ARM: don't permit
> >R_ARM_GOT_PREL relocations
> >
> >On Tue, 11 Dec 2018 at 14:40, Gao, Liming <liming.gao@intel.com> wrote:
> >>
> >> Ard:
> >>   With this change, GenFw will report what error message if ELF image has
> >R_ARM_GOT_PREL relocations.
> >>
> >
> >Numerous occurrences of
> >
> >GenFw: ERROR 3000: Invalid
> >  WriteSections ():
> >/home/ard/build/edk2-workspace/Build/ArmVirtQemuKernel-
> >ARM/RELEASE_CLANG38/ARM/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreReloc
> >atable/DEBUG/ArmVirtPrePiUniCoreRelocatable.dll
> >unsupported ELF EM_ARM relocation 0x60.
> >
> >
> >> > -----Original Message-----
> >> > From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> >> > Sent: Tuesday, December 11, 2018 7:21 PM
> >> > To: Leif Lindholm <leif.lindholm@linaro.org>
> >> > Cc: edk2-devel@lists.01.org; Laszlo Ersek <lersek@redhat.com>; Feng,
> >Bob C <bob.c.feng@intel.com>; Gao, Liming
> >> > <liming.gao@intel.com>
> >> > Subject: Re: [PATCH] BaseTools/GenFw ARM: don't permit
> >R_ARM_GOT_PREL relocations
> >> >
> >> > On Tue, 11 Dec 2018 at 12:19, Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >wrote:
> >> > >
> >> > > On Tue, 11 Dec 2018 at 10:53, Leif Lindholm <leif.lindholm@linaro.org>
> >wrote:
> >> > > >
> >> > > > On Tue, Dec 11, 2018 at 10:37:15AM +0100, Ard Biesheuvel wrote:
> >> > > > > We currently permit R_ARM_GOT_PREL relocations in the ELF32
> >conversion
> >> > > > > routines, under the assumption that relative relocations are fine as
> >> > > > > long as the section layout is the same between ELF and PE/COFF.
> >> > > > >
> >> > > > > However, as is the case with any proxy generating relocation, it is
> >> > > > > up to the linker to emit an entry in the GOT table and populate it
> >> > > > > with the correct absolute address, which should also be fixed up at
> >> > > > > PE/COFF load time. Unfortunately, the relocations covering the GOT
> >> > > > > section are not emitted into the static relocation sections processed
> >> > > > > by GenFw, but only in the dynamic relocation section as a
> >R_ARM_RELATIVE
> >> > > > > relocation, and so GenFw fails to emit the correct PE/COFF relocation
> >> > > > > data for GOT entries.
> >> > > > >
> >> > > > > Since GOT indirection is pointless anyway for PE/COFF modules
> >running
> >> > > > > in UEFI context, let's just drop the references to R_ARM_GOT_PREL
> >from
> >> > > > > GenFw, resulting in a build time failure rather than a runtime failure
> >> > > > > if such relocations do occur.
> >> > > > >
> >> > > > > Cc: Bob Feng <bob.c.feng@intel.com>
> >> > > > > Cc: Liming Gao <liming.gao@intel.com>
> >> > > > > Cc: Leif Lindholm <leif.lindholm@linaro.org>
> >> > > > > Contributed-under: TianoCore Contribution Agreement 1.1
> >> > > > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> > > >
> >> > > > Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> >> > > >
> >> > > > Ouch. This sounds like the best move for now. But how do we deal
> >with
> >> > > > builds that actually break?
> >> > > >
> >> > >
> >> > > So the only builds that are breaking due to this are ones where we run
> >> > > the linker in PIE mode (which only happens in
> >> > > ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf), and using the
> >> > > GNU gold linker. The reason we need the -pie option is to force the
> >> > > linker to emit dynamic relocations into the binary so it can relocate
> >> > > itself. This is necessary because the firmware image may execute from
> >> > > a a priori unknown memory offset.
> >> > >
> >> > > I am playing around with hidden visibility and other tweaks to coerce
> >> > > the linker into emitting direct relative references instead of GOT
> >> > > based ones, and it is very tedious. The GOLD linker really doesn't
> >> > > appear to be set up for bare metal binaries.
> >> >
> >> > Oh, and on AARCH64 it is even more annoying, given that the relative
> >> > GOT references are emitted as ADRP/ADD pairs, which means we have
> >the
> >> > 4 KB alignment issue as well.