Re: [edk2-devel] [PATCH] BaseTools/GenFw AARCH64: Convert more types of explicit GOT references

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

On Wed, 7 Sept 2022 at 14:04, Feng, Bob C <bob.c.feng@intel.com> wrote:
>
> Acked-by: Bob Feng <bob.c.feng@intel.com>
>

Merged as #3322

Thanks all,

> -----Original Message-----
> From: Leif Lindholm <quic_llindhol@quicinc.com>
> Sent: Tuesday, September 6, 2022 7:54 PM
> To: Ard Biesheuvel <ardb@kernel.org>; Rebecca Cran <rebecca@bsdio.com>
> Cc: devel@edk2.groups.io; Feng, Bob C <bob.c.feng@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: Re: [edk2-devel] [PATCH] BaseTools/GenFw AARCH64: Convert more types of explicit GOT references
>
> On 2022-09-06 11:30, Ard Biesheuvel wrote:
> > Bob, Liming, Leif: any thoughts?
>
> I'm happy with this.
> (Spotted one typo - "ofset".)
>
> Acked-by: Leif Lindholm <quic_llindhol@quicinc.com>
>
> > On Sun, 21 Aug 2022 at 16:33, Rebecca Cran <rebecca@bsdio.com> wrote:
> >>
> >> Tested-by: Rebecca Cran <rebecca@bsdio.com>
> >>
> >>
> >> On 8/21/22 08:16, Ard Biesheuvel wrote:
> >>> Rebecca reports that builds of AArch64 DSCs that involve PIE linking
> >>> when using ELF based toolchains are failing in some cases, resulting
> >>> in an error message like
> >>>
> >>>     bad definition for symbol '_GLOBAL_OFFSET_TABLE_'@0x72d8 or
> >>>     unsupported symbol type.  For example, absolute and undefined symbols
> >>>     are not supported.
> >>>
> >>> The reason turns out to be that, while GenFw does carry some logic
> >>> to convert GOT based symbol references into direct ones (which is
> >>> always possible given that our ELF to PE/COFF conversion only
> >>> supports fully linked executables), it does not support all possible
> >>> combinations of relocations that the linker may emit to load symbol
> >>> addresses from the GOT.
> >>>
> >>> In particular, when performing a non-LTO link on object code built
> >>> with GCC using -fpie, we may end up with GOT based references such
> >>> as the one below, where the address of the GOT itself is taken, and
> >>> the ofset of the symbol in the GOT is reflected in the immediate
> >>> offset of the subsequent LDR instruction.
> >>>
> >>>     838:   adrp    x0, 16000
> >>>     838: R_AARCH64_ADR_PREL_PG_HI21 _GLOBAL_OFFSET_TABLE_
> >>>     83c:   ldr     x0, [x0, #2536]
> >>>     83c: R_AARCH64_LD64_GOTPAGE_LO15        _gPcd_BinaryPatch_PcdFdBaseAddress
> >>>
> >>> The reason that we omit GOT based symbol references when performing
> >>> ELF to PE/COFF conversion is that the GOT is not described by static
> >>> ELF relocations, which means that the ELF file lacks the metadata to
> >>> generate the PE/COFF relocations covering the GOT table in the
> >>> PE/COFF executable. Given that none of the usual motivations for
> >>> using a GOT (copy on write footprint, shared libraries) apply to EFI
> >>> executables in the first place, the easiest way around this is to
> >>> convert all GOT based symbol address loads to PC relative ADR/ADRP instructions.
> >>>
> >>> So implement this handling for R_AARCH64_LD64_GOTPAGE_LO15 and
> >>> R_AARCH64_LD64_GOTOFF_LO15 relocations as well, and turn the LDR
> >>> instructions in question into ADR instructions that generate the
> >>> address immediately.
> >>>
> >>> This leaves the reference to _GLOBAL_OFFSET_TABLE_ itself, which is
> >>> what generated the error to begin with. Considering that this symbol
> >>> is never referenced (i.e., it doesn't appear anywhere in the code)
> >>> and is only meaningful in combination with R_*_GOT_* based
> >>> relocations that follow it, we can just disregard any references to
> >>> it entirely, given that we convert all of those followup relocations into direct references.
> >>>
> >>> Cc: Rebecca Cran <rebecca@bsdio.com>
> >>> Cc: Bob Feng <bob.c.feng@intel.com>
> >>> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> >>> Cc: "Kinney, Michael D" <michael.d.kinney@intel.com>
> >>> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> >>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> >>> ---
> >>>
> >>> This patch can be tested using the following method:
> >>>
> >>> - add the following lines to
> >>> ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf
> >>>
> >>> [BuildOptions]
> >>>     GCC:*_*_AARCH64_CC_FLAGS = -fpie
> >>>     GCC:*_*_AARCH64_DLINK_FLAGS = -Wl,-Bsymbolic,-pie
> >>>
> >>> - build ArmVirtPkg/ArmVirtQemuKernel.dsc in DEBUG mode using GCC49
> >>>     (other combos might work as well) and observe the build failure
> >>>
> >>> - apply this patch and observe that the build failure is gone
> >>>
> >>> - boot the resulting image in QEMU using the -kernel ../QEMU_EFI.fd
> >>>     command line option and observe that the image boots as usual
> >>>
> >>>    BaseTools/Source/C/GenFw/Elf64Convert.c | 35 ++++++++++++++++++++
> >>>    1 file changed, 35 insertions(+)
> >>>
> >>> diff --git a/BaseTools/Source/C/GenFw/Elf64Convert.c
> >>> b/BaseTools/Source/C/GenFw/Elf64Convert.c
> >>> index 35e96dd05bc2..3173ca9280f4 100644
> >>> --- a/BaseTools/Source/C/GenFw/Elf64Convert.c
> >>> +++ b/BaseTools/Source/C/GenFw/Elf64Convert.c
> >>> @@ -1305,6 +1305,22 @@ WriteSections64 (
> >>>            Elf_Shdr *SymShdr;
> >>>            UINT8    *Targ;
> >>>
> >>> +        //
> >>> +        // The _GLOBAL_OFFSET_TABLE_ symbol is not actually an absolute symbol,
> >>> +        // but carries the SHN_ABS section index for historical reasons.
> >>> +        // It must be accompanied by a R_*_GOT_* type relocation on a
> >>> +        // subsequent instruction, which we handle below, specifically to avoid
> >>> +        // the GOT indirection, and to refer to the symbol directly. This means
> >>> +        // we can simply disregard direct references to the GOT symbol itself,
> >>> +        // as the resulting value will never be used.
> >>> +        //
> >>> +        if (Sym->st_shndx == SHN_ABS) {
> >>> +          const UINT8 *SymName = GetSymName (Sym);
> >>> +          if (strcmp ((CHAR8 *)SymName, "_GLOBAL_OFFSET_TABLE_") == 0) {
> >>> +            continue;
> >>> +          }
> >>> +        }
> >>> +
> >>>            //
> >>>            // Check section header index found in symbol table and get the section
> >>>            // header location.
> >>> @@ -1448,6 +1464,23 @@ WriteSections64 (
> >>>              switch (ELF_R_TYPE(Rel->r_info)) {
> >>>                INT64 Offset;
> >>>
> >>> +          case R_AARCH64_LD64_GOTOFF_LO15:
> >>> +          case R_AARCH64_LD64_GOTPAGE_LO15:
> >>> +            //
> >>> +            // Convert into an ADR instruction that refers to the symbol directly.
> >>> +            //
> >>> +            Offset = Sym->st_value - Rel->r_offset;
> >>> +
> >>> +            *(UINT32 *)Targ &= 0x1000001f;
> >>> +            *(UINT32 *)Targ |= ((Offset & 0x1ffffc) << (5 - 2)) |
> >>> + ((Offset & 0x3) << 29);
> >>> +
> >>> +            if (Offset < -0x100000 || Offset > 0xfffff) {
> >>> +              Error (NULL, 0, 3000, "Invalid", "WriteSections64(): %s failed to relax GOT based symbol reference - image is too big (>1 MiB).",
> >>> +                mInImageName);
> >>> +              break;
> >>> +            }
> >>> +            break;
> >>> +
> >>>              case R_AARCH64_LD64_GOT_LO12_NC:
> >>>                //
> >>>                // Convert into an ADD instruction - see R_AARCH64_ADR_GOT_PAGE below.
> >>> @@ -1686,6 +1719,8 @@ WriteRelocations64 (
> >>>                case R_AARCH64_LDST128_ABS_LO12_NC:
> >>>                case R_AARCH64_ADR_GOT_PAGE:
> >>>                case R_AARCH64_LD64_GOT_LO12_NC:
> >>> +            case R_AARCH64_LD64_GOTOFF_LO15:
> >>> +            case R_AARCH64_LD64_GOTPAGE_LO15:
> >>>                  //
> >>>                  // No fixups are required for relative relocations, provided that
> >>>                  // the relative offsets between sections have been
> >>> preserved in
>