xcoff reading dynamic relocs

  This adds a sanity check to relocation symbol indices, and tidies code
a little.

The patch does result in a couple of testsuite failures
rs6000-aix7.2  +FAIL: TLS relocations (32-bit)
rs6000-aix7.2  +FAIL: TLS relocations (64-bit)

That seems reasonable to me, because prior to this patch l_symndx was
being set to -1 and -2 for .tdata and .tbss symbols resulting in a
buffer overflow when accessing the syms array.  (objdump -R on the
testcase .so segfaults.)

bfd/
	* xcofflink.c (_bfd_xcoff_canonicalize_dynamic_reloc): Prevent
	symbol array overflow on invalid relocation symbol index.
	Tidy code for relocs against standard sections.
	(xcoff_create_ldrel): Remove cast.
include/
	* coff/xcoff.h (struct internal_ldrel): Make l_symndx uint32_t.
	Make l_rtype and l_rsecnm int16_t.
  

On Fri, Dec 13, 2024 at 03:24:49PM +1030, Alan Modra wrote:
> That seems reasonable to me, because prior to this patch l_symndx was
> being set to -1 and -2 for .tdata and .tbss symbols resulting in a
> buffer overflow when accessing the syms array.  (objdump -R on the
> testcase .so segfaults.)

Perhaps this?  Using -1 for .tdata l_symndx was clearly wrong since
that value is already used..  However, I'm unsure what to do here.
ldrel is a public interface.

	* xcofflink.c (_bfd_xcoff_canonicalize_dynamic_reloc): Use
	.tdata and .tbss section symbols.
	(xcoff_create_ldrel): Set l_symndx to -3 for .tdata.

diff --git a/bfd/xcofflink.c b/bfd/xcofflink.c
index b75fb42eed5..057dfbd72e2 100644
--- a/bfd/xcofflink.c
+++ b/bfd/xcofflink.c
@@ -441,10 +441,11 @@ _bfd_xcoff_canonicalize_dynamic_reloc (bfd *abfd,
 
       if (ldrel.l_symndx == -1u)
 	relbuf->sym_ptr_ptr = bfd_abs_section_ptr->symbol_ptr_ptr;
-      else if (ldrel.l_symndx < 3)
+      else if (ldrel.l_symndx + 3 < 6)
 	{
-	  static const char stdsec[3][8] = { ".text", ".data", ".bss" };
-	  const char *name = stdsec[ldrel.l_symndx];
+	  static const char stdsec[6][8]
+	    = { ".tdata", ".tbss", "", ".text", ".data", ".bss" };
+	  const char *name = stdsec[ldrel.l_symndx + 3];
 	  asection *sec = bfd_get_section_by_name (abfd, name);
 	  if (sec == NULL)
 	    {
@@ -5063,7 +5064,7 @@ xcoff_create_ldrel (bfd *output_bfd, struct xcoff_final_link_info *flinfo,
       else if (strcmp (secname, ".bss") == 0)
 	ldrel.l_symndx = 2;
       else if (strcmp (secname, ".tdata") == 0)
-	ldrel.l_symndx = -1;
+	ldrel.l_symndx = -3;
       else if (strcmp (secname, ".tbss") == 0)
 	ldrel.l_symndx = -2;
       else
  

Hi Alan,

First, I'm no longer actively developing on AIX, it's been ages since
those patches and not sure who can be considered as a maintainer of it
now... But I still try to answer !


On Fri, Dec 13, 2024 at 6:39 AM Alan Modra <amodra@gmail.com> wrote:
>
> On Fri, Dec 13, 2024 at 03:24:49PM +1030, Alan Modra wrote:
> > That seems reasonable to me, because prior to this patch l_symndx was
> > being set to -1 and -2 for .tdata and .tbss symbols resulting in a
> > buffer overflow when accessing the syms array.  (objdump -R on the
> > testcase .so segfaults.)
>
> Perhaps this?  Using -1 for .tdata l_symndx was clearly wrong since
> that value is already used..  However, I'm unsure what to do here.
> ldrel is a public interface.

The XCOFF documentation doesn't state it, but AFAIK, .tdata and .tbss
have indeed fixed negative values.
The idea was to introduce those sections without breaking the
compatibility with previously made binaries.
Out of my memory, without it TLS is not possible and thus we must preserve them.

I didn't notice that default -1 at that time. I don't think we ever
fallback to it and it's probably dead code. A loader relocation should
target a section (and have those magic numbers) or a symbol (and thus
be > 3).

Apart from that I'm totally fine with this idea if it can restore the
above regression.
Might give it a try at some point using the GCC buildfarm VM for AIX.
Though no promise as I don't have much time recently...

>
>         * xcofflink.c (_bfd_xcoff_canonicalize_dynamic_reloc): Use
>         .tdata and .tbss section symbols.
>         (xcoff_create_ldrel): Set l_symndx to -3 for .tdata.
>
> diff --git a/bfd/xcofflink.c b/bfd/xcofflink.c
> index b75fb42eed5..057dfbd72e2 100644
> --- a/bfd/xcofflink.c
> +++ b/bfd/xcofflink.c
> @@ -441,10 +441,11 @@ _bfd_xcoff_canonicalize_dynamic_reloc (bfd *abfd,
>
>        if (ldrel.l_symndx == -1u)
>         relbuf->sym_ptr_ptr = bfd_abs_section_ptr->symbol_ptr_ptr;
> -      else if (ldrel.l_symndx < 3)
> +      else if (ldrel.l_symndx + 3 < 6)
>         {
> -         static const char stdsec[3][8] = { ".text", ".data", ".bss" };
> -         const char *name = stdsec[ldrel.l_symndx];
> +         static const char stdsec[6][8]
> +           = { ".tdata", ".tbss", "", ".text", ".data", ".bss" };
> +         const char *name = stdsec[ldrel.l_symndx + 3];
>           asection *sec = bfd_get_section_by_name (abfd, name);
>           if (sec == NULL)
>             {
> @@ -5063,7 +5064,7 @@ xcoff_create_ldrel (bfd *output_bfd, struct xcoff_final_link_info *flinfo,
>        else if (strcmp (secname, ".bss") == 0)
>         ldrel.l_symndx = 2;
>        else if (strcmp (secname, ".tdata") == 0)
> -       ldrel.l_symndx = -1;
> +       ldrel.l_symndx = -3;
>        else if (strcmp (secname, ".tbss") == 0)
>         ldrel.l_symndx = -2;
>        else
>
> --
> Alan Modra
  

On Fri, Dec 13, 2024 at 03:07:08PM +0100, Clément Chigot wrote:
> Hi Alan,
> 
> First, I'm no longer actively developing on AIX, it's been ages since
> those patches and not sure who can be considered as a maintainer of it
> now... But I still try to answer !

Thanks for replying, I understand.

> On Fri, Dec 13, 2024 at 6:39 AM Alan Modra <amodra@gmail.com> wrote:
> >
> > On Fri, Dec 13, 2024 at 03:24:49PM +1030, Alan Modra wrote:
> > > That seems reasonable to me, because prior to this patch l_symndx was
> > > being set to -1 and -2 for .tdata and .tbss symbols resulting in a
> > > buffer overflow when accessing the syms array.  (objdump -R on the
> > > testcase .so segfaults.)
> >
> > Perhaps this?  Using -1 for .tdata l_symndx was clearly wrong since
> > that value is already used..  However, I'm unsure what to do here.
> > ldrel is a public interface.
> 
> The XCOFF documentation doesn't state it, but AFAIK, .tdata and .tbss
> have indeed fixed negative values.

I guess I should assume you chose the correct values when you last
changed xcoff_create_ldrel, and instead abort on what I've now
verified as dead code by inspecting all the calls to
xcoff_create_ldrel.  I hadn't done that analysis and just assumed -1
was used for "no symbol".

OK, so the followup patch which I'm about to commit is this one:

	* xcofflink.c (_bfd_xcoff_canonicalize_dynamic_reloc): Use
	.tdata and .tbss section symbols.
	(xcoff_create_ldrel): Abort on h and hsec both NULL.

diff --git a/bfd/xcofflink.c b/bfd/xcofflink.c
index b75fb42eed5..6eb4529b85a 100644
--- a/bfd/xcofflink.c
+++ b/bfd/xcofflink.c
@@ -439,12 +439,11 @@ _bfd_xcoff_canonicalize_dynamic_reloc (bfd *abfd,
 
       bfd_xcoff_swap_ldrel_in (abfd, elrel, &ldrel);
 
-      if (ldrel.l_symndx == -1u)
-	relbuf->sym_ptr_ptr = bfd_abs_section_ptr->symbol_ptr_ptr;
-      else if (ldrel.l_symndx < 3)
+      if (ldrel.l_symndx + 2 < 5)
 	{
-	  static const char stdsec[3][8] = { ".text", ".data", ".bss" };
-	  const char *name = stdsec[ldrel.l_symndx];
+	  static const char stdsec[5][8]
+	    = { ".tbss", ".tdata", ".text", ".data", ".bss" };
+	  const char *name = stdsec[ldrel.l_symndx + 2];
 	  asection *sec = bfd_get_section_by_name (abfd, name);
 	  if (sec == NULL)
 	    {
@@ -5090,7 +5089,7 @@ xcoff_create_ldrel (bfd *output_bfd, struct xcoff_final_link_info *flinfo,
       ldrel.l_symndx = h->ldindx;
     }
   else
-    ldrel.l_symndx = -1;
+    abort ();
 
   ldrel.l_rtype = (irel->r_size << 8) | irel->r_type;
   ldrel.l_rsecnm = output_section->target_index;


> The idea was to introduce those sections without breaking the
> compatibility with previously made binaries.
> Out of my memory, without it TLS is not possible and thus we must preserve them.
> 
> I didn't notice that default -1 at that time. I don't think we ever
> fallback to it and it's probably dead code. A loader relocation should
> target a section (and have those magic numbers) or a symbol (and thus
> be > 3).
> 
> Apart from that I'm totally fine with this idea if it can restore the
> above regression.
> Might give it a try at some point using the GCC buildfarm VM for AIX.
> Though no promise as I don't have much time recently...
> 
> >
> >         * xcofflink.c (_bfd_xcoff_canonicalize_dynamic_reloc): Use
> >         .tdata and .tbss section symbols.
> >         (xcoff_create_ldrel): Set l_symndx to -3 for .tdata.
> >
> > diff --git a/bfd/xcofflink.c b/bfd/xcofflink.c
> > index b75fb42eed5..057dfbd72e2 100644
> > --- a/bfd/xcofflink.c
> > +++ b/bfd/xcofflink.c
> > @@ -441,10 +441,11 @@ _bfd_xcoff_canonicalize_dynamic_reloc (bfd *abfd,
> >
> >        if (ldrel.l_symndx == -1u)
> >         relbuf->sym_ptr_ptr = bfd_abs_section_ptr->symbol_ptr_ptr;
> > -      else if (ldrel.l_symndx < 3)
> > +      else if (ldrel.l_symndx + 3 < 6)
> >         {
> > -         static const char stdsec[3][8] = { ".text", ".data", ".bss" };
> > -         const char *name = stdsec[ldrel.l_symndx];
> > +         static const char stdsec[6][8]
> > +           = { ".tdata", ".tbss", "", ".text", ".data", ".bss" };
> > +         const char *name = stdsec[ldrel.l_symndx + 3];
> >           asection *sec = bfd_get_section_by_name (abfd, name);
> >           if (sec == NULL)
> >             {
> > @@ -5063,7 +5064,7 @@ xcoff_create_ldrel (bfd *output_bfd, struct xcoff_final_link_info *flinfo,
> >        else if (strcmp (secname, ".bss") == 0)
> >         ldrel.l_symndx = 2;
> >        else if (strcmp (secname, ".tdata") == 0)
> > -       ldrel.l_symndx = -1;
> > +       ldrel.l_symndx = -3;
> >        else if (strcmp (secname, ".tbss") == 0)
> >         ldrel.l_symndx = -2;
> >        else
> >
> > --
> > Alan Modra