This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH] x86-64: Restore PIC check for PCREL reloc against protected symbol
- From: "H.J. Lu" <hjl dot tools at gmail dot com>
- To: Binutils <binutils at sourceware dot org>, Nick Clifton <nickc at redhat dot com>
- Cc: robinlee dot sysu at gmail dot com
- Date: Wed, 6 Feb 2019 06:30:21 -0800
- Subject: Re: [PATCH] x86-64: Restore PIC check for PCREL reloc against protected symbol
- References: <20190202181305.26328-1-hjl.tools@gmail.com>
On Sat, Feb 2, 2019 at 10:13 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> commit bd7ab16b4537788ad53521c45469a1bdae84ad4a
> Author: H.J. Lu <hjl.tools@gmail.com>
> Date: Tue Feb 13 07:34:22 2018 -0800
>
> x86-64: Generate branch with PLT32 relocation
>
> removed check R_X86_64_PC32 relocation against protected symbols in
> shared objects. Since elf_x86_64_check_relocs is called after we
> have seen all input files, we can check for PC-relative relocations in
> elf_x86_64_check_relocs. We should not allow PC-relative relocations
> against protected symbols since address of protected function and
> location of protected data may not be in the shared object.
>
> bfd/
>
> PR ld/24151
> * elf64-x86-64.c (elf_x86_64_need_pic): Check
> SYMBOL_DEFINED_NON_SHARED_P instead of def_regular.
> (elf_x86_64_relocate_section): Move PIC check for PC-relative
> relocations to ...
> (elf_x86_64_check_relocs): Here.
> (elf_x86_64_finish_dynamic_symbol): Use SYMBOL_DEFINED_NON_SHARED_P
> to check if a symbol is defined in a non-shared object.
> * elfxx-x86.h (SYMBOL_DEFINED_NON_SHARED_P): New.
>
> ld/
>
> PR ld/24151
> * testsuite/ld-x86-64/pr24151a-x32.d: New file.
> * testsuite/ld-x86-64/pr24151a.d: Likewise.
> * testsuite/ld-x86-64/pr24151a.s: Likewise.
> * testsuite/ld-x86-64/x86-64.exp: Run pr24151a and pr24151a-x32.
> ---
> bfd/elf64-x86-64.c | 133 +++++++++++++++-----------
> bfd/elfxx-x86.h | 7 ++
> ld/testsuite/ld-x86-64/pr24151a-x32.d | 4 +
> ld/testsuite/ld-x86-64/pr24151a.d | 3 +
> ld/testsuite/ld-x86-64/pr24151a.s | 9 ++
> ld/testsuite/ld-x86-64/x86-64.exp | 2 +
> 6 files changed, 104 insertions(+), 54 deletions(-)
> create mode 100644 ld/testsuite/ld-x86-64/pr24151a-x32.d
> create mode 100644 ld/testsuite/ld-x86-64/pr24151a.d
> create mode 100644 ld/testsuite/ld-x86-64/pr24151a.s
>
> diff --git a/bfd/elf64-x86-64.c b/bfd/elf64-x86-64.c
> index c7d8bca710..6a4b366fdd 100644
> --- a/bfd/elf64-x86-64.c
> +++ b/bfd/elf64-x86-64.c
> @@ -1426,7 +1426,7 @@ elf_x86_64_need_pic (struct bfd_link_info *info,
> break;
> }
>
> - if (!h->def_regular && !h->def_dynamic)
> + if (!SYMBOL_DEFINED_NON_SHARED_P (h) && !h->def_dynamic)
> und = _("undefined ");
> }
> else
> @@ -1855,6 +1855,7 @@ elf_x86_64_check_relocs (bfd *abfd, struct bfd_link_info *info,
> const char *name;
> bfd_boolean size_reloc;
> bfd_boolean converted_reloc;
> + bfd_boolean do_check_pic;
>
> r_symndx = htab->r_sym (rel->r_info);
> r_type = ELF32_R_TYPE (rel->r_info);
> @@ -2130,6 +2131,13 @@ elf_x86_64_check_relocs (bfd *abfd, struct bfd_link_info *info,
> size_reloc = TRUE;
> goto do_size;
>
> + case R_X86_64_PC8:
> + case R_X86_64_PC16:
> + case R_X86_64_PC32:
> + case R_X86_64_PC32_BND:
> + do_check_pic = TRUE;
> + goto check_pic;
> +
> case R_X86_64_32:
> if (!ABI_64_P (abfd))
> goto pointer;
> @@ -2153,13 +2161,11 @@ elf_x86_64_check_relocs (bfd *abfd, struct bfd_link_info *info,
> &x86_64_elf_howto_table[r_type]);
> /* Fall through. */
>
> - case R_X86_64_PC8:
> - case R_X86_64_PC16:
> - case R_X86_64_PC32:
> - case R_X86_64_PC32_BND:
> case R_X86_64_PC64:
> case R_X86_64_64:
> pointer:
> + do_check_pic = FALSE;
> +check_pic:
> if (eh != NULL && (sec->flags & SEC_CODE) != 0)
> eh->zero_undefweak |= 0x2;
> /* We are called after all symbols have been resolved. Only
> @@ -2223,6 +2229,67 @@ pointer:
> }
> }
>
> + if (do_check_pic)
> + {
> + /* Don't complain about -fPIC if the symbol is undefined
> + when building executable unless it is unresolved weak
> + symbol, references a dynamic definition in PIE or
> + -z nocopyreloc is used. */
> + bfd_boolean no_copyreloc_p
> + = (info->nocopyreloc
> + || (h != NULL
> + && eh->def_protected
> + && elf_has_no_copy_on_protected (h->root.u.def.section->owner)));
> + if ((sec->flags & SEC_ALLOC) != 0
> + && (sec->flags & SEC_READONLY) != 0
> + && h != NULL
> + && ((bfd_link_executable (info)
> + && ((h->root.type == bfd_link_hash_undefweak
> + && (eh == NULL
> + || !UNDEFINED_WEAK_RESOLVED_TO_ZERO (info,
> + eh)))
> + || (bfd_link_pie (info)
> + && !SYMBOL_DEFINED_NON_SHARED_P (h)
> + && h->def_dynamic)
> + || (no_copyreloc_p
> + && h->def_dynamic
> + && !(h->root.u.def.section->flags & SEC_CODE))))
> + || bfd_link_dll (info)))
> + {
> + bfd_boolean fail = FALSE;
> + if (SYMBOL_REFERENCES_LOCAL_P (info, h))
> + {
> + /* Symbol is referenced locally. Make sure it is
> + defined locally. */
> + fail = !SYMBOL_DEFINED_NON_SHARED_P (h);
> + }
> + else if (bfd_link_pie (info))
> + {
> + /* We can only use PC-relative relocations in PIE
> + from non-code sections. */
> + if (h->type == STT_FUNC
> + && (sec->flags & SEC_CODE) != 0)
> + fail = TRUE;
> + }
> + else if (no_copyreloc_p || bfd_link_dll (info))
> + {
> + /* Symbol doesn't need copy reloc and isn't
> + referenced locally. Don't allow PC-relative
> + relocations against default and protected
> + symbols since address of protected function
> + and location of protected data may not be in
> + the shared object. */
> + fail = (ELF_ST_VISIBILITY (h->other) == STV_DEFAULT
> + || ELF_ST_VISIBILITY (h->other) == STV_PROTECTED);
> + }
> +
> + if (fail)
> + return elf_x86_64_need_pic (info, abfd, sec, h,
> + symtab_hdr, isym,
> + &x86_64_elf_howto_table[r_type]);
> + }
> + }
> +
> size_reloc = FALSE;
> do_size:
> if (NEED_DYNAMIC_RELOCATION_P (info, TRUE, h, sec, r_type,
> @@ -3065,56 +3132,14 @@ use_plt:
> case R_X86_64_PC16:
> case R_X86_64_PC32:
> case R_X86_64_PC32_BND:
> - /* Don't complain about -fPIC if the symbol is undefined when
> - building executable unless it is unresolved weak symbol,
> - references a dynamic definition in PIE or -z nocopyreloc
> - is used. */
> - if ((input_section->flags & SEC_ALLOC) != 0
> - && (input_section->flags & SEC_READONLY) != 0
> - && h != NULL
> - && ((bfd_link_executable (info)
> - && ((h->root.type == bfd_link_hash_undefweak
> - && !resolved_to_zero)
> - || (bfd_link_pie (info)
> - && !h->def_regular
> - && h->def_dynamic)
> - || ((info->nocopyreloc
> - || (eh->def_protected
> - && elf_has_no_copy_on_protected (h->root.u.def.section->owner)))
> - && h->def_dynamic
> - && !(h->root.u.def.section->flags & SEC_CODE))))
> - || bfd_link_dll (info)))
> - {
> - bfd_boolean fail = FALSE;
> - if (SYMBOL_REFERENCES_LOCAL_P (info, h))
> - {
> - /* Symbol is referenced locally. Make sure it is
> - defined locally. */
> - fail = !(h->def_regular || ELF_COMMON_DEF_P (h));
> - }
> - else if (!(bfd_link_pie (info)
> - && (h->needs_copy || eh->needs_copy)))
> - {
> - /* Symbol doesn't need copy reloc and isn't referenced
> - locally. Address of protected function may not be
> - reachable at run-time. */
> - fail = (ELF_ST_VISIBILITY (h->other) == STV_DEFAULT
> - || (ELF_ST_VISIBILITY (h->other) == STV_PROTECTED
> - && h->type == STT_FUNC));
> - }
> -
> - if (fail)
> - return elf_x86_64_need_pic (info, input_bfd, input_section,
> - h, NULL, NULL, howto);
> - }
> /* Since x86-64 has PC-relative PLT, we can use PLT in PIE
> as function address. */
> - else if (h != NULL
> - && (input_section->flags & SEC_CODE) == 0
> - && bfd_link_pie (info)
> - && h->type == STT_FUNC
> - && !h->def_regular
> - && h->def_dynamic)
> + if (h != NULL
> + && (input_section->flags & SEC_CODE) == 0
> + && bfd_link_pie (info)
> + && h->type == STT_FUNC
> + && !h->def_regular
> + && h->def_dynamic)
> goto use_plt;
> /* Fall through. */
>
> @@ -4271,7 +4296,7 @@ elf_x86_64_finish_dynamic_symbol (bfd *output_bfd,
> else if (bfd_link_pic (info)
> && SYMBOL_REFERENCES_LOCAL_P (info, h))
> {
> - if (!(h->def_regular || ELF_COMMON_DEF_P (h)))
> + if (!SYMBOL_DEFINED_NON_SHARED_P (h))
> return FALSE;
> BFD_ASSERT((h->got.offset & 1) != 0);
> rela.r_info = htab->r_info (0, R_X86_64_RELATIVE);
> diff --git a/bfd/elfxx-x86.h b/bfd/elfxx-x86.h
> index dce24dc106..28d540b83b 100644
> --- a/bfd/elfxx-x86.h
> +++ b/bfd/elfxx-x86.h
> @@ -161,6 +161,13 @@
> || (ELF_ST_VISIBILITY ((H)->other) \
> && (H)->root.type == bfd_link_hash_undefweak))
>
> +/* TRUE if this symbol isn't defined by a shared object. */
> +#define SYMBOL_DEFINED_NON_SHARED_P(H) \
> + ((H)->def_regular \
> + || (H)->root.linker_def \
> + || (H)->root.ldscript_def \
> + || ELF_COMMON_DEF_P (H))
> +
> /* TRUE if relative relocation should be generated. GOT reference to
> global symbol in PIC will lead to dynamic symbol. It becomes a
> problem when "time" or "times" is defined as a variable in an
> diff --git a/ld/testsuite/ld-x86-64/pr24151a-x32.d b/ld/testsuite/ld-x86-64/pr24151a-x32.d
> new file mode 100644
> index 0000000000..130611ddf4
> --- /dev/null
> +++ b/ld/testsuite/ld-x86-64/pr24151a-x32.d
> @@ -0,0 +1,4 @@
> +#source: pr24151a.s
> +#as: --x32
> +#ld: -shared -melf32_x86_64
> +#error: .*relocation R_X86_64_PC32 against protected symbol `foo' can not be used when making a shared object
> diff --git a/ld/testsuite/ld-x86-64/pr24151a.d b/ld/testsuite/ld-x86-64/pr24151a.d
> new file mode 100644
> index 0000000000..783b85a1a6
> --- /dev/null
> +++ b/ld/testsuite/ld-x86-64/pr24151a.d
> @@ -0,0 +1,3 @@
> +#as: --64
> +#ld: -shared -melf_x86_64
> +#error: .*relocation R_X86_64_PC32 against protected symbol `foo' can not be used when making a shared object
> diff --git a/ld/testsuite/ld-x86-64/pr24151a.s b/ld/testsuite/ld-x86-64/pr24151a.s
> new file mode 100644
> index 0000000000..e4ec7c8ce4
> --- /dev/null
> +++ b/ld/testsuite/ld-x86-64/pr24151a.s
> @@ -0,0 +1,9 @@
> + .text
> + .globl bar
> + .type bar,@function
> +bar:
> + movl $30, foo(%rip)
> + .size bar, .-bar
> + .protected foo
> + .type foo,@object
> + .comm foo,4,4
> diff --git a/ld/testsuite/ld-x86-64/x86-64.exp b/ld/testsuite/ld-x86-64/x86-64.exp
> index 86b163fc7a..5eb556515f 100644
> --- a/ld/testsuite/ld-x86-64/x86-64.exp
> +++ b/ld/testsuite/ld-x86-64/x86-64.exp
> @@ -424,6 +424,8 @@ run_dump_test "pr23486d-x32"
> run_dump_test "pr23854"
> run_dump_test "pr23930"
> run_dump_test "pr23930-x32"
> +run_dump_test "pr24151a"
> +run_dump_test "pr24151a-x32"
>
> if { ![istarget "x86_64-*-linux*"] && ![istarget "x86_64-*-nacl*"]} {
> return
> --
> 2.20.1
>
I will backport it to 2.31 and 2.32 branches.
--
H.J.