[PATCH v2] x86: Properly handle relocation against local ABS symbol

[H.J. Lu]

On Thu, May 15, 2025 at 2:22 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 15.05.2025 04:00, H.J. Lu wrote:
> > With PIC on x86-64,
> >
> > movq symbol@GOTPCREL(%rip), %rax
> >
> > is used to get the symbol address via GOT.  If the symbol turns out to
> > be a hidden symbol, linker normally converts it into
> >
> > leaq symbol(%rip), %rax
> >
> > But if the symbol is an ABS symbol, RAX won't have the correct value at
> > run-time and it will have the symbol value + load address.  If the symbol
> > is declared as hidden, compiler will generate
> >
> > leaq symbol(%rip), %rax
> >
> > to get the symbol address.  If the hidden symbol is an ABS symbol, RAX
> > will have the same incorrect value at run-time.  On i386,
> >
> > movl symbol@GOT(%reg1), %reg2
> >
> > is used to get the symbol address via GOT.  If the symbol turns out to
> > be a hidden symbol, linker normally converts it into
> >
> > leal symbol@GOTOFF(%reg1), %reg2
> >
> > If the symbol is declared as hidden, compiler will generate
> >
> > leal symbol@GOTOFF(%reg1), %reg2
> >
> > In both cases, if the hidden symbol is an ABS symbol, REG2 will have the
> > same incorrect value at run-time.  This patch changes the x86-64 linker
> > to convert
> >
> > movq symbol@GOTPCREL(%rip), %rax
> > and
> > leaq symbol(%rip), %rax
> > to
> > movq $symbol, %rax
> >
> > and changes the i386 linker to convert
> >
> > movl symbol@GOT(%reg1), %reg2
> > and
> > leal symbol@GOTOFF(%reg1), %reg2
> > to
> > movl $symbol, %reg2
> >
> > for hidden ABS symbols.
> >
> > bfd/
> >
> > PR ld/32443
> > * elf32-i386.c (elf_i386_convert_load_reloc): Skip relocation
> > against linker-script hidden absolute symbol in PIC.
> > (elf_i386_finish_dynamic_symbol): Don't generate dynamic
> > relocation against inker-script hidden absolute symbol.
>
> You're again checking opcode bytes without considering prefixes, in
> particular (but not limited to) VEX/XOP/EVEX ones. I understand this is
> a pre-existing issue, but I'd prefer if the problem wasn't widened.

I think my patch is acceptable as it fixes the wrong run-time result.

> Also why is it that this is done only for STV_HIDDEN, but not also for
> STV_PROTECTED?

I changed HIDDEN_ABS_SYMBOL_P to LOCAL_ABS_SYMBOL_P.

> Further, why is the conversion to MOV only done for PIC? The MOV
> (immediate) is more efficient even outside of PIC/PIE, isn't it?

Fixed:

/* Return TRUE if the symbol described by a linker hash entry H is a
   local symbol which is going to be absolute.  */
#define LOCAL_ABS_SYMBOL_P(INFO, H) \
  (bfd_is_abs_symbol (&(H)->root) \
   && (bfd_link_executable (INFO) \
       || ELF_ST_VISIBILITY ((H)->other) == STV_HIDDEN \
       || ELF_ST_VISIBILITY ((H)->other) == STV_PROTECTED))

with tests.

> > * elf64-x86-64.c (elf_x86_64_convert_load_reloc): Skip
> > relocation against linker-script hidden absolute symbol in PIC.
> > (elf_x86_64_finish_dynamic_symbol): Don't generate dynamic
> > relocation against inker-script hidden absolute symbol.
>
> Here afaics you only check the opcode byte, but not ModR/M (to limit
> to the %rip-relative form only).

R_X86_64_PC32 is generated only for foo(%rip).

> You convert "if (opcode != 0x8b)" to "else if (opcode != 0x8b)" in
> one of the rearrangements - is that, strictly speaking, correct? The
> original code included the 0xff case, when that's now excluded. If it's
> correct, the earlier

R_X86_64_PC32 is new and only

 leaq symbol(%rip), %rax

is allowed.

> +  else
> +    {
> +      if (opcode == 0xff)
>
> would want to become just
>
> +  else if (opcode == 0xff)
>
> limiting code churn quite a bit.
>
> Like you have done in the 32-bit code, can you indent the lea_to_mov
> label some, please?

Fixed.

> > * elfxx-x86.c (elf_x86_allocate_dynrelocs): Don't allocate
> > dynamic relocation against inker-script hidden absolute symbol.
> > * elfxx-x86.h (HIDDEN_ABS_SYMBOL_P): New.
>
> Why is it that HIDDEN_ABS_SYMBOL_P() doesn't use ABS_SYMBOL_P()? IOW why
> is ->root.ldscript_def relevant to check for the latter, but not for the
> new predicate?

ABS_SYMBOL_P excludes linker defined symbols because of

https://sourceware.org/bugzilla/show_bug.cgi?id=25754
https://sourceware.org/bugzilla/show_bug.cgi?id=25749

Here is the v2 patch.  I also added the overflow check

         if (r == bfd_reloc_overflow)
            {
              if ((r_type == (rex_w && ABI_64_P (link_info->output_bfd)
                              ? R_X86_64_32S : R_X86_64_32))
                  && reloc_pc32
                  && LOCAL_ABS_SYMBOL_P (link_info, h))
                {
                  /* Since R_X86_64_PC32S/R_X86_64_32 relocation, which
                     is converted from R_X86_64_PC32, against a local
                     ABS symbol in PIC overflows, the original relocation
                     will also overflow.  */
                  howto = elf_x86_64_rtype_to_howto (abfd,
                                                     R_X86_64_PC32);
                  link_info->callbacks->reloc_overflow
                    (link_info, &h->root, h->root.root.string,
                     howto->name, (bfd_vma) 0, abfd, input_section,
                   irel->r_offset);
                  return false;
                }
              return true;
            }

with tests.


--
H.J.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: v2-0001-x86-Properly-handle-relocation-against-local-ABS-.patch
Type: text/x-patch
Size: 41814 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/binutils/attachments/20250516/b29cd861/attachment-0001.bin>