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

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

[Ard Biesheuvel]

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>
---

Without this patch, CLANG38 builds of ArmVirtQemuKernel-ARM (in LTO mode)
succeed, but the resulting binaries are broken. This is due to the fact that
the PIE linker running in LTO mode insists on emitting GOT based relocations,
while we don't have the code to fix up the contents of the GOT. This change
puts it in line with the AARCH64 build of the same platform/toolchains,
which chokes on GOT based relocations as well. Since the use of the PIE
linker is a peculiarity of ArmVirtQemuKernel/ArmVirtXen, and the fact that
it is impossible to prevent the linker from emitting GOT based relocations,
let's not go out of our way to fix it in the tools, but just drop CLANG38
support from those platforms.

 BaseTools/Source/C/GenFw/Elf32Convert.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/BaseTools/Source/C/GenFw/Elf32Convert.c b/BaseTools/Source/C/GenFw/Elf32Convert.c
index 3d7de6d5c123..23e8065756e6 100644
--- a/BaseTools/Source/C/GenFw/Elf32Convert.c
+++ b/BaseTools/Source/C/GenFw/Elf32Convert.c
@@ -837,7 +837,6 @@ WriteSections32 (
           case R_ARM_LDC_PC_G0:
           case R_ARM_LDC_PC_G1:
           case R_ARM_LDC_PC_G2:
-          case R_ARM_GOT_PREL:
           case R_ARM_THM_JUMP11:
           case R_ARM_THM_JUMP8:
           case R_ARM_TLS_GD32:
@@ -964,7 +963,6 @@ WriteRelocations32 (
             case R_ARM_LDC_PC_G0:
             case R_ARM_LDC_PC_G1:
             case R_ARM_LDC_PC_G2:
-            case R_ARM_GOT_PREL:
             case R_ARM_THM_JUMP11:
             case R_ARM_THM_JUMP8:
             case R_ARM_TLS_GD32:
-- 
2.19.2

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

[Leif Lindholm]

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?

> ---
> 
> Without this patch, CLANG38 builds of ArmVirtQemuKernel-ARM (in LTO mode)
> succeed, but the resulting binaries are broken. This is due to the fact that
> the PIE linker running in LTO mode insists on emitting GOT based relocations,
> while we don't have the code to fix up the contents of the GOT. This change
> puts it in line with the AARCH64 build of the same platform/toolchains,
> which chokes on GOT based relocations as well. Since the use of the PIE
> linker is a peculiarity of ArmVirtQemuKernel/ArmVirtXen, and the fact that
> it is impossible to prevent the linker from emitting GOT based relocations,
> let's not go out of our way to fix it in the tools, but just drop CLANG38
> support from those platforms.
> 
>  BaseTools/Source/C/GenFw/Elf32Convert.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/BaseTools/Source/C/GenFw/Elf32Convert.c b/BaseTools/Source/C/GenFw/Elf32Convert.c
> index 3d7de6d5c123..23e8065756e6 100644
> --- a/BaseTools/Source/C/GenFw/Elf32Convert.c
> +++ b/BaseTools/Source/C/GenFw/Elf32Convert.c
> @@ -837,7 +837,6 @@ WriteSections32 (
>            case R_ARM_LDC_PC_G0:
>            case R_ARM_LDC_PC_G1:
>            case R_ARM_LDC_PC_G2:
> -          case R_ARM_GOT_PREL:
>            case R_ARM_THM_JUMP11:
>            case R_ARM_THM_JUMP8:
>            case R_ARM_TLS_GD32:
> @@ -964,7 +963,6 @@ WriteRelocations32 (
>              case R_ARM_LDC_PC_G0:
>              case R_ARM_LDC_PC_G1:
>              case R_ARM_LDC_PC_G2:
> -            case R_ARM_GOT_PREL:
>              case R_ARM_THM_JUMP11:
>              case R_ARM_THM_JUMP8:
>              case R_ARM_TLS_GD32:
> -- 
> 2.19.2
> 

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

[Ard Biesheuvel]

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.

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

[Ard Biesheuvel]

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.

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

[Gao, Liming]

Ard:
  With this change, GenFw will report what error message if ELF image has R_ARM_GOT_PREL relocations.

Thanks
Liming
> -----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.

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

[Ard Biesheuvel]

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/ArmVirtPrePiUniCoreRelocatable/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.

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

[Gao, Liming]

That's good. Reviewed-by: Liming Gao <liming.gao@intel.com>

>-----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.

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

[Ard Biesheuvel]

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.

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

[Laszlo Ersek]

On 12/11/18 10:37, 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>
> ---
> 
> Without this patch, CLANG38 builds of ArmVirtQemuKernel-ARM (in LTO mode)
> succeed, but the resulting binaries are broken. This is due to the fact that
> the PIE linker running in LTO mode insists on emitting GOT based relocations,
> while we don't have the code to fix up the contents of the GOT. This change
> puts it in line with the AARCH64 build of the same platform/toolchains,
> which chokes on GOT based relocations as well. Since the use of the PIE
> linker is a peculiarity of ArmVirtQemuKernel/ArmVirtXen, and the fact that
> it is impossible to prevent the linker from emitting GOT based relocations,
> let's not go out of our way to fix it in the tools, but just drop CLANG38
> support from those platforms.
> 
>  BaseTools/Source/C/GenFw/Elf32Convert.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/BaseTools/Source/C/GenFw/Elf32Convert.c b/BaseTools/Source/C/GenFw/Elf32Convert.c
> index 3d7de6d5c123..23e8065756e6 100644
> --- a/BaseTools/Source/C/GenFw/Elf32Convert.c
> +++ b/BaseTools/Source/C/GenFw/Elf32Convert.c
> @@ -837,7 +837,6 @@ WriteSections32 (
>            case R_ARM_LDC_PC_G0:
>            case R_ARM_LDC_PC_G1:
>            case R_ARM_LDC_PC_G2:
> -          case R_ARM_GOT_PREL:
>            case R_ARM_THM_JUMP11:
>            case R_ARM_THM_JUMP8:
>            case R_ARM_TLS_GD32:
> @@ -964,7 +963,6 @@ WriteRelocations32 (
>              case R_ARM_LDC_PC_G0:
>              case R_ARM_LDC_PC_G1:
>              case R_ARM_LDC_PC_G2:
> -            case R_ARM_GOT_PREL:
>              case R_ARM_THM_JUMP11:
>              case R_ARM_THM_JUMP8:
>              case R_ARM_TLS_GD32:
> 

Acked-by: Laszlo Ersek <lersek@redhat.com>