RISC-V: PR32499, Fix PR18841 segfault caused by ifunc relocation ordering

Message ID 20250114071935.2477-1-nelson@rivosinc.com
State New
Headers
Series RISC-V: PR32499, Fix PR18841 segfault caused by ifunc relocation ordering |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_binutils_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 success Test passed
linaro-tcwg-bot/tcwg_binutils_check--master-arm success Test passed

Commit Message

Nelson Chu Jan. 14, 2025, 7:19 a.m. UTC
  Even though the relocation isn't IRELATIVE, it still should be come last if
refering to ifunc symbol.  In order to get the ifunc relocs properly sorted
the correct class needs to be returned.  The code mimics what has been done
for x86, sparc, aarch64 and arm32.

bfd/
	PR 18841
	PR 32499
	* elfnn-riscv.c (riscv_reloc_type_class): Handle ifunc relocation
	ordering, even though it's not IRELATIVE, it still should be come
	last if refering ifunc symbol.
---
 bfd/elfnn-riscv.c | 36 +++++++++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)
  

Comments

Nelson Chu Jan. 17, 2025, 4:47 a.m. UTC | #1
Hi Guys,

Would it be too rushed to do this at 2.44, or would it be more appropriate
to do it at 2.45?  Both are fine to me.

Thanks
Nelson

On Tue, Jan 14, 2025 at 3:19 PM Nelson Chu <nelson@rivosinc.com> wrote:

> Even though the relocation isn't IRELATIVE, it still should be come last if
> refering to ifunc symbol.  In order to get the ifunc relocs properly sorted
> the correct class needs to be returned.  The code mimics what has been done
> for x86, sparc, aarch64 and arm32.
>
> bfd/
>         PR 18841
>         PR 32499
>         * elfnn-riscv.c (riscv_reloc_type_class): Handle ifunc relocation
>         ordering, even though it's not IRELATIVE, it still should be come
>         last if refering ifunc symbol.
> ---
>  bfd/elfnn-riscv.c | 36 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 35 insertions(+), 1 deletion(-)
>
> diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
> index 2122aa36c7c..c3881bb9edd 100644
> --- a/bfd/elfnn-riscv.c
> +++ b/bfd/elfnn-riscv.c
> @@ -3658,13 +3658,47 @@ riscv_elf_plt_sym_val (bfd_vma i, const asection
> *plt,
>    return plt->vma + PLT_HEADER_SIZE + i * PLT_ENTRY_SIZE;
>  }
>
> +/* Used to decide how to sort relocs in an optimal manner for the
> +   dynamic linker, before writing them out.  */
> +
>  static enum elf_reloc_type_class
> -riscv_reloc_type_class (const struct bfd_link_info *info ATTRIBUTE_UNUSED,
> +riscv_reloc_type_class (const struct bfd_link_info *info,
>                         const asection *rel_sec ATTRIBUTE_UNUSED,
>                         const Elf_Internal_Rela *rela)
>  {
> +  struct riscv_elf_link_hash_table *htab = riscv_elf_hash_table (info);
> +
> +  if (htab->elf.dynsym != NULL
> +      && htab->elf.dynsym->contents != NULL)
> +    {
> +      /* Check relocation against STT_GNU_IFUNC symbol if there are
> +        dynamic symbols.  */
> +      bfd *abfd = info->output_bfd;
> +      const struct elf_backend_data *bed = get_elf_backend_data (abfd);
> +      unsigned long r_symndx = ELFNN_R_SYM (rela->r_info);
> +      if (r_symndx != STN_UNDEF)
> +       {
> +         Elf_Internal_Sym sym;
> +         if (!bed->s->swap_symbol_in (abfd,
> +                                      (htab->elf.dynsym->contents
> +                                       + r_symndx * bed->s->sizeof_sym),
> +                                      0, &sym))
> +           {
> +             /* xgettext:c-format */
> +             _bfd_error_handler (_("%pB symbol number %lu references"
> +                                   " nonexistent SHT_SYMTAB_SHNDX
> section"),
> +                                 abfd, r_symndx);
> +             /* Ideally an error class should be returned here.  */
> +           }
> +         else if (ELF_ST_TYPE (sym.st_info) == STT_GNU_IFUNC)
> +           return reloc_class_ifunc;
> +       }
> +    }
> +
>    switch (ELFNN_R_TYPE (rela->r_info))
>      {
> +    case R_RISCV_IRELATIVE:
> +      return reloc_class_ifunc;
>      case R_RISCV_RELATIVE:
>        return reloc_class_relative;
>      case R_RISCV_JUMP_SLOT:
> --
> 2.39.3 (Apple Git-146)
>
>
  
Alan Modra Jan. 17, 2025, 6:03 a.m. UTC | #2
On Fri, Jan 17, 2025 at 12:47:16PM +0800, Nelson Chu wrote:
> Hi Guys,
> 
> Would it be too rushed to do this at 2.44, or would it be more appropriate
> to do it at 2.45?  Both are fine to me.

Looks reasonable for 2.44 to me.

> Thanks
> Nelson
> 
> On Tue, Jan 14, 2025 at 3:19 PM Nelson Chu <nelson@rivosinc.com> wrote:
> 
> > Even though the relocation isn't IRELATIVE, it still should be come last if
> > refering to ifunc symbol.  In order to get the ifunc relocs properly sorted
> > the correct class needs to be returned.  The code mimics what has been done
> > for x86, sparc, aarch64 and arm32.
> >
> > bfd/
> >         PR 18841
> >         PR 32499
> >         * elfnn-riscv.c (riscv_reloc_type_class): Handle ifunc relocation
> >         ordering, even though it's not IRELATIVE, it still should be come
> >         last if refering ifunc symbol.
> > ---
> >  bfd/elfnn-riscv.c | 36 +++++++++++++++++++++++++++++++++++-
> >  1 file changed, 35 insertions(+), 1 deletion(-)
> >
> > diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
> > index 2122aa36c7c..c3881bb9edd 100644
> > --- a/bfd/elfnn-riscv.c
> > +++ b/bfd/elfnn-riscv.c
> > @@ -3658,13 +3658,47 @@ riscv_elf_plt_sym_val (bfd_vma i, const asection
> > *plt,
> >    return plt->vma + PLT_HEADER_SIZE + i * PLT_ENTRY_SIZE;
> >  }
> >
> > +/* Used to decide how to sort relocs in an optimal manner for the
> > +   dynamic linker, before writing them out.  */
> > +
> >  static enum elf_reloc_type_class
> > -riscv_reloc_type_class (const struct bfd_link_info *info ATTRIBUTE_UNUSED,
> > +riscv_reloc_type_class (const struct bfd_link_info *info,
> >                         const asection *rel_sec ATTRIBUTE_UNUSED,
> >                         const Elf_Internal_Rela *rela)
> >  {
> > +  struct riscv_elf_link_hash_table *htab = riscv_elf_hash_table (info);
> > +
> > +  if (htab->elf.dynsym != NULL
> > +      && htab->elf.dynsym->contents != NULL)
> > +    {
> > +      /* Check relocation against STT_GNU_IFUNC symbol if there are
> > +        dynamic symbols.  */
> > +      bfd *abfd = info->output_bfd;
> > +      const struct elf_backend_data *bed = get_elf_backend_data (abfd);
> > +      unsigned long r_symndx = ELFNN_R_SYM (rela->r_info);
> > +      if (r_symndx != STN_UNDEF)
> > +       {
> > +         Elf_Internal_Sym sym;
> > +         if (!bed->s->swap_symbol_in (abfd,
> > +                                      (htab->elf.dynsym->contents
> > +                                       + r_symndx * bed->s->sizeof_sym),
> > +                                      0, &sym))
> > +           {
> > +             /* xgettext:c-format */
> > +             _bfd_error_handler (_("%pB symbol number %lu references"
> > +                                   " nonexistent SHT_SYMTAB_SHNDX
> > section"),
> > +                                 abfd, r_symndx);
> > +             /* Ideally an error class should be returned here.  */
> > +           }
> > +         else if (ELF_ST_TYPE (sym.st_info) == STT_GNU_IFUNC)
> > +           return reloc_class_ifunc;
> > +       }
> > +    }
> > +
> >    switch (ELFNN_R_TYPE (rela->r_info))
> >      {
> > +    case R_RISCV_IRELATIVE:
> > +      return reloc_class_ifunc;
> >      case R_RISCV_RELATIVE:
> >        return reloc_class_relative;
> >      case R_RISCV_JUMP_SLOT:
> > --
> > 2.39.3 (Apple Git-146)
> >
> >
  
Nelson Chu Jan. 17, 2025, 8:58 a.m. UTC | #3
Thanks, committed after passing the riscv-gnu-toolchain gcc/binutils
regressions at least.

Nelson

On Fri, Jan 17, 2025 at 2:03 PM Alan Modra <amodra@gmail.com> wrote:

> On Fri, Jan 17, 2025 at 12:47:16PM +0800, Nelson Chu wrote:
> > Hi Guys,
> >
> > Would it be too rushed to do this at 2.44, or would it be more
> appropriate
> > to do it at 2.45?  Both are fine to me.
>
> Looks reasonable for 2.44 to me.
>
> > Thanks
> > Nelson
> >
> > On Tue, Jan 14, 2025 at 3:19 PM Nelson Chu <nelson@rivosinc.com> wrote:
> >
> > > Even though the relocation isn't IRELATIVE, it still should be come
> last if
> > > refering to ifunc symbol.  In order to get the ifunc relocs properly
> sorted
> > > the correct class needs to be returned.  The code mimics what has been
> done
> > > for x86, sparc, aarch64 and arm32.
> > >
> > > bfd/
> > >         PR 18841
> > >         PR 32499
> > >         * elfnn-riscv.c (riscv_reloc_type_class): Handle ifunc
> relocation
> > >         ordering, even though it's not IRELATIVE, it still should be
> come
> > >         last if refering ifunc symbol.
> > > ---
> > >  bfd/elfnn-riscv.c | 36 +++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 35 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
> > > index 2122aa36c7c..c3881bb9edd 100644
> > > --- a/bfd/elfnn-riscv.c
> > > +++ b/bfd/elfnn-riscv.c
> > > @@ -3658,13 +3658,47 @@ riscv_elf_plt_sym_val (bfd_vma i, const
> asection
> > > *plt,
> > >    return plt->vma + PLT_HEADER_SIZE + i * PLT_ENTRY_SIZE;
> > >  }
> > >
> > > +/* Used to decide how to sort relocs in an optimal manner for the
> > > +   dynamic linker, before writing them out.  */
> > > +
> > >  static enum elf_reloc_type_class
> > > -riscv_reloc_type_class (const struct bfd_link_info *info
> ATTRIBUTE_UNUSED,
> > > +riscv_reloc_type_class (const struct bfd_link_info *info,
> > >                         const asection *rel_sec ATTRIBUTE_UNUSED,
> > >                         const Elf_Internal_Rela *rela)
> > >  {
> > > +  struct riscv_elf_link_hash_table *htab = riscv_elf_hash_table
> (info);
> > > +
> > > +  if (htab->elf.dynsym != NULL
> > > +      && htab->elf.dynsym->contents != NULL)
> > > +    {
> > > +      /* Check relocation against STT_GNU_IFUNC symbol if there are
> > > +        dynamic symbols.  */
> > > +      bfd *abfd = info->output_bfd;
> > > +      const struct elf_backend_data *bed = get_elf_backend_data
> (abfd);
> > > +      unsigned long r_symndx = ELFNN_R_SYM (rela->r_info);
> > > +      if (r_symndx != STN_UNDEF)
> > > +       {
> > > +         Elf_Internal_Sym sym;
> > > +         if (!bed->s->swap_symbol_in (abfd,
> > > +                                      (htab->elf.dynsym->contents
> > > +                                       + r_symndx *
> bed->s->sizeof_sym),
> > > +                                      0, &sym))
> > > +           {
> > > +             /* xgettext:c-format */
> > > +             _bfd_error_handler (_("%pB symbol number %lu references"
> > > +                                   " nonexistent SHT_SYMTAB_SHNDX
> > > section"),
> > > +                                 abfd, r_symndx);
> > > +             /* Ideally an error class should be returned here.  */
> > > +           }
> > > +         else if (ELF_ST_TYPE (sym.st_info) == STT_GNU_IFUNC)
> > > +           return reloc_class_ifunc;
> > > +       }
> > > +    }
> > > +
> > >    switch (ELFNN_R_TYPE (rela->r_info))
> > >      {
> > > +    case R_RISCV_IRELATIVE:
> > > +      return reloc_class_ifunc;
> > >      case R_RISCV_RELATIVE:
> > >        return reloc_class_relative;
> > >      case R_RISCV_JUMP_SLOT:
> > > --
> > > 2.39.3 (Apple Git-146)
> > >
> > >
>
> --
> Alan Modra
>
  

Patch

diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
index 2122aa36c7c..c3881bb9edd 100644
--- a/bfd/elfnn-riscv.c
+++ b/bfd/elfnn-riscv.c
@@ -3658,13 +3658,47 @@  riscv_elf_plt_sym_val (bfd_vma i, const asection *plt,
   return plt->vma + PLT_HEADER_SIZE + i * PLT_ENTRY_SIZE;
 }
 
+/* Used to decide how to sort relocs in an optimal manner for the
+   dynamic linker, before writing them out.  */
+
 static enum elf_reloc_type_class
-riscv_reloc_type_class (const struct bfd_link_info *info ATTRIBUTE_UNUSED,
+riscv_reloc_type_class (const struct bfd_link_info *info,
 			const asection *rel_sec ATTRIBUTE_UNUSED,
 			const Elf_Internal_Rela *rela)
 {
+  struct riscv_elf_link_hash_table *htab = riscv_elf_hash_table (info);
+
+  if (htab->elf.dynsym != NULL
+      && htab->elf.dynsym->contents != NULL)
+    {
+      /* Check relocation against STT_GNU_IFUNC symbol if there are
+	 dynamic symbols.  */
+      bfd *abfd = info->output_bfd;
+      const struct elf_backend_data *bed = get_elf_backend_data (abfd);
+      unsigned long r_symndx = ELFNN_R_SYM (rela->r_info);
+      if (r_symndx != STN_UNDEF)
+	{
+	  Elf_Internal_Sym sym;
+	  if (!bed->s->swap_symbol_in (abfd,
+				       (htab->elf.dynsym->contents
+					+ r_symndx * bed->s->sizeof_sym),
+				       0, &sym))
+	    {
+	      /* xgettext:c-format */
+	      _bfd_error_handler (_("%pB symbol number %lu references"
+				    " nonexistent SHT_SYMTAB_SHNDX section"),
+				  abfd, r_symndx);
+	      /* Ideally an error class should be returned here.  */
+	    }
+	  else if (ELF_ST_TYPE (sym.st_info) == STT_GNU_IFUNC)
+	    return reloc_class_ifunc;
+	}
+    }
+
   switch (ELFNN_R_TYPE (rela->r_info))
     {
+    case R_RISCV_IRELATIVE:
+      return reloc_class_ifunc;
     case R_RISCV_RELATIVE:
       return reloc_class_relative;
     case R_RISCV_JUMP_SLOT: