Protect mips_hi16_list from fuzzed debug info

  This is another fix for the testcase mentioned in
https://sourceware.org/pipermail/binutils/2023-February/125915.html
either of which will stop the addr2line segfault.  This one also fixes
a potential problem when linking corrupted debug info.

OK to apply?

	* elfxx-mips.c (struct mips_elf_obj_tdata): Add freeze_mips_hi16_list.
	(_bfd_mips_elf_hi16_reloc): Heed freeze_mips_hi16_list.
	(_bfd_mips_elf_lo16_reloc): Likewise.
	(find_nearest_line): Rename from _bfd_mips_elf_find_nearest_line
	and make static.
	(_bfd_mips_elf_find_nearest_line): New wrapper function setting
	freeze_mips_hi16_list around find_nearest_line.
  

On Mon, 6 Feb 2023, Alan Modra wrote:

> This is another fix for the testcase mentioned in
> https://sourceware.org/pipermail/binutils/2023-February/125915.html
> either of which will stop the addr2line segfault.  This one also fixes
> a potential problem when linking corrupted debug info.

 Hmm, from the other message I gather DWARF info is not going to be 
processed twice anymore, so why is this change to the MIPS backend also 
required?

 Also would it be possible to have a MIPS test case for your change?  
Orchestrating HI16/LO16 relocations in a debug section should be pretty 
straightforward with the use of the `.reloc' pseudo-op.  This might help 
me understand what is really going on here.

 Also one concern about code proposed itself, see below.

> @@ -2596,38 +2600,41 @@ _bfd_mips_elf_lo16_reloc (bfd *abfd, arelent *reloc_entry, asymbol *symbol,
[...]
> +  if (!tdata->freeze_mips_hi16_list)

 This conditional ought to wrap all the preceding code in the function as 
well (including the declaration block), because it's sole purpose is to 
retrieve `vallo', which is only used within the `while' loop now placed 
under the conditional...

> +    while (tdata->mips_hi16_list != NULL)
> +      {
> +	bfd_reloc_status_type ret;
> +	struct mips_hi16 *hi;
> +
> +	hi = tdata->mips_hi16_list;
> +
> +	/* R_MIPS*_GOT16 relocations are something of a special case.
> +	   We want to install the addend in the same way as for a
> +	   R_MIPS*_HI16 relocation (with a rightshift of 16).
> +	   However, since GOT16 relocations can also be used with
> +	   global symbols, their howto has a rightshift of 0.  */
> +	if (hi->rel.howto->type == R_MIPS_GOT16)
> +	  hi->rel.howto = MIPS_ELF_RTYPE_TO_HOWTO (abfd, R_MIPS_HI16, false);
> +	else if (hi->rel.howto->type == R_MIPS16_GOT16)
> +	  hi->rel.howto = MIPS_ELF_RTYPE_TO_HOWTO (abfd, R_MIPS16_HI16, false);
> +	else if (hi->rel.howto->type == R_MICROMIPS_GOT16)
> +	  hi->rel.howto = MIPS_ELF_RTYPE_TO_HOWTO (abfd, R_MICROMIPS_HI16,
> +						   false);
> +
> +	/* VALLO is a signed 16-bit number.  Bias it by 0x8000 so that
> +	   any carry or borrow will induce a change of +1 or -1 in the
> +	   high part.  */
> +	hi->rel.addend += (vallo + 0x8000) & 0xffff;

 ... here.

> +  /* Debug info should not contain hi16 or lo16 relocs.  If it does
> +     then someone is playing fuzzing games.  Altering the hi16 list
> +     during linking when printing an error message is bad.  */

 And I really cannot extract the meaning of the second sentence here.  I 
mean I know what it literally means, but that does not really tell me 
anything.  Why would altering the list be a problem given that we're 
bailing out anyway?  I'm confused.

  Maciej
  

On Tue, Feb 07, 2023 at 02:11:23PM +0000, Maciej W. Rozycki wrote:
> On Mon, 6 Feb 2023, Alan Modra wrote:
> 
> > This is another fix for the testcase mentioned in
> > https://sourceware.org/pipermail/binutils/2023-February/125915.html
> > either of which will stop the addr2line segfault.  This one also fixes
> > a potential problem when linking corrupted debug info.
> 
>  Hmm, from the other message I gather DWARF info is not going to be 
> processed twice anymore, so why is this change to the MIPS backend also 
> required?

See below.

>  Also would it be possible to have a MIPS test case for your change?  
> Orchestrating HI16/LO16 relocations in a debug section should be pretty 
> straightforward with the use of the `.reloc' pseudo-op.  This might help 
> me understand what is really going on here.

I could do that, but my time is limited for mips problems.  I'll
understand if you say the patch is not worth committing just to cover
a potential fuzzed object file segfault.

>  Also one concern about code proposed itself, see below.
> 
> > @@ -2596,38 +2600,41 @@ _bfd_mips_elf_lo16_reloc (bfd *abfd, arelent *reloc_entry, asymbol *symbol,
> [...]
> > +  if (!tdata->freeze_mips_hi16_list)
> 
>  This conditional ought to wrap all the preceding code in the function as 
> well (including the declaration block), because it's sole purpose is to 
> retrieve `vallo', which is only used within the `while' loop now placed 
> under the conditional...

OK, done.  I'm presuming I don't need to repost the patch.

> > +  /* Debug info should not contain hi16 or lo16 relocs.  If it does
> > +     then someone is playing fuzzing games.  Altering the hi16 list
> > +     during linking when printing an error message is bad.  */

s/an error/a warning/

>  And I really cannot extract the meaning of the second sentence here.  I 
> mean I know what it literally means, but that does not really tell me 
> anything.  Why would altering the list be a problem given that we're 
> bailing out anyway?  I'm confused.

A number of the error/warning handlers in ldmain.c use %C.  This can
cause debug info to be parsed for the first time in order to print
file/function/line.  If one of those warnings is triggered after some
hi16 relocs have been processed but before the matching lo16 reloc is
handled, *and* the debug info is corrupted with a lo16 reloc, then the
mips_hi16_list will be flushed with the result that printing a warning
changes linker output.  It is also possible that corrupted debug info
adds to the hi16 list, with the result that when the linker handles a
later lo16 reloc in a text section, ld will segfault accessing
mips_hi16.data after the debug buffers have be freed.  Is this likely
to happen in the real world?  No, of course not, but fuzzers keep
finding this sort of thing, and the occasional real problem found by
the fuzzers is enough that I haven't yet decided to ignore all fuzzing
reports.
  

On Wed, 8 Feb 2023, Alan Modra wrote:

> >  Also would it be possible to have a MIPS test case for your change?  
> > Orchestrating HI16/LO16 relocations in a debug section should be pretty 
> > straightforward with the use of the `.reloc' pseudo-op.  This might help 
> > me understand what is really going on here.
> 
> I could do that, but my time is limited for mips problems.

 Completely understood (and mine BTW too).

>  I'll
> understand if you say the patch is not worth committing just to cover
> a potential fuzzed object file segfault.

 No, not at all, I agree we do need to maintain at least basic quality,
and preventing crashes from happening even for garbage input is about the 
minimum required.

> >  Also one concern about code proposed itself, see below.
> > 
> > > @@ -2596,38 +2600,41 @@ _bfd_mips_elf_lo16_reloc (bfd *abfd, arelent *reloc_entry, asymbol *symbol,
> > [...]
> > > +  if (!tdata->freeze_mips_hi16_list)
> > 
> >  This conditional ought to wrap all the preceding code in the function as 
> > well (including the declaration block), because it's sole purpose is to 
> > retrieve `vallo', which is only used within the `while' loop now placed 
> > under the conditional...
> 
> OK, done.  I'm presuming I don't need to repost the patch.

 Sure, no need to repost.

> > > +  /* Debug info should not contain hi16 or lo16 relocs.  If it does
> > > +     then someone is playing fuzzing games.  Altering the hi16 list
> > > +     during linking when printing an error message is bad.  */
> 
> s/an error/a warning/

 Thanks, that changes things a bit.

> >  And I really cannot extract the meaning of the second sentence here.  I 
> > mean I know what it literally means, but that does not really tell me 
> > anything.  Why would altering the list be a problem given that we're 
> > bailing out anyway?  I'm confused.
> 
> A number of the error/warning handlers in ldmain.c use %C.  This can
> cause debug info to be parsed for the first time in order to print
> file/function/line.

 Hmm, I find it an interesting general phenomenon.  What it means the 
order sections are processed in can change depending on whether a warning 
has been issued in the course or not.  Is it not a problem in the first 
place?  Shouldn't we give priority to debugs sections and parse them first 
then before moving on to the other sections?

>  If one of those warnings is triggered after some
> hi16 relocs have been processed but before the matching lo16 reloc is
> handled, *and* the debug info is corrupted with a lo16 reloc, then the
> mips_hi16_list will be flushed with the result that printing a warning
> changes linker output.

 OK, but wouldn't these relocs be resolved in the same problematic way 
anyway when the turn came to processing debug sections?

>  It is also possible that corrupted debug info
> adds to the hi16 list, with the result that when the linker handles a
> later lo16 reloc in a text section, ld will segfault accessing
> mips_hi16.data after the debug buffers have be freed.

 This smells a HI16/LO16 pair processing bug to me by itself.  Such pairs 
must come from the same relocation section, so any HI16/LO16 relocations 
in a relocation section associated with a debug section are not supposed 
to influence any such relocations referring to the text section.  I think 
I need to look into it (though see above as to my availability).

>  Is this likely
> to happen in the real world?  No, of course not, but fuzzers keep
> finding this sort of thing, and the occasional real problem found by
> the fuzzers is enough that I haven't yet decided to ignore all fuzzing
> reports.

 Well, we must not crash, period!

 I hope there's no hurry with this change, so please let me chew it over 
yet for a couple days.

 Long-term I think the MIPS target would benefit from proper day-to-day 
maintenance (and I can't just clone myself no matter how much I might 
desire).

  Maciej
  

On Wed, Feb 08, 2023 at 11:28:08PM +0000, Maciej W. Rozycki wrote:
> On Wed, 8 Feb 2023, Alan Modra wrote:
> > A number of the error/warning handlers in ldmain.c use %C.  This can
> > cause debug info to be parsed for the first time in order to print
> > file/function/line.
> 
>  Hmm, I find it an interesting general phenomenon.  What it means the 
> order sections are processed in can change depending on whether a warning 
> has been issued in the course or not.  Is it not a problem in the first 
> place?  Shouldn't we give priority to debugs sections and parse them first 
> then before moving on to the other sections?

The normal linker processing of sections occurs as usual.  Parsing of
the debug info is separate to this, relocations being applied to
.debug_info by bfd_simple_get_relocated_section_contents for the
error/warning message.  That relocated copy of .debug_info is not used
by the linker to produce the output file .debug_info.

> >  If one of those warnings is triggered after some
> > hi16 relocs have been processed but before the matching lo16 reloc is
> > handled, *and* the debug info is corrupted with a lo16 reloc, then the
> > mips_hi16_list will be flushed with the result that printing a warning
> > changes linker output.
> 
>  OK, but wouldn't these relocs be resolved in the same problematic way 
> anyway when the turn came to processing debug sections?
> 
> >  It is also possible that corrupted debug info
> > adds to the hi16 list, with the result that when the linker handles a
> > later lo16 reloc in a text section, ld will segfault accessing
> > mips_hi16.data after the debug buffers have be freed.
> 
>  This smells a HI16/LO16 pair processing bug to me by itself.  Such pairs 
> must come from the same relocation section, so any HI16/LO16 relocations 
> in a relocation section associated with a debug section are not supposed 
> to influence any such relocations referring to the text section.  I think 
> I need to look into it (though see above as to my availability).

If it is really true that hi16/lo16 pairs are always in the same
section, then we wouldn't need freeze_mips_hi16_list.  Instead it
would be much better to attach the list to mips_elf_section_data.  I
wasn't sure enough to do that, given things like gcc's hot/cold
section partitioning, when I moved the mip_hi16_list to be per-bfd.
  

On Thu, 9 Feb 2023, Alan Modra wrote:

> >  Hmm, I find it an interesting general phenomenon.  What it means the 
> > order sections are processed in can change depending on whether a warning 
> > has been issued in the course or not.  Is it not a problem in the first 
> > place?  Shouldn't we give priority to debugs sections and parse them first 
> > then before moving on to the other sections?
> 
> The normal linker processing of sections occurs as usual.  Parsing of
> the debug info is separate to this, relocations being applied to
> .debug_info by bfd_simple_get_relocated_section_contents for the
> error/warning message.  That relocated copy of .debug_info is not used
> by the linker to produce the output file .debug_info.

 Ack, makes sense to me.

> >  This smells a HI16/LO16 pair processing bug to me by itself.  Such pairs 
> > must come from the same relocation section, so any HI16/LO16 relocations 
> > in a relocation section associated with a debug section are not supposed 
> > to influence any such relocations referring to the text section.  I think 
> > I need to look into it (though see above as to my availability).
> 
> If it is really true that hi16/lo16 pairs are always in the same
> section, then we wouldn't need freeze_mips_hi16_list.

 It is, by definition[1]:

"The AHL addend is a composite computed from the addends of two 
consecutive relocation entries.  Each relocation type of R_MIPS_HI16 must 
have an associated R_MIPS_LO16 entry immediately following it in the list 
of relocations.

"These relocation entries are always processed as a pair and both addend 
fields contribute to the AHL addend.  If AHI and ALO are the addends from 
the paired R_MIPS_HI16 and R_MIPS_LO16 entries, then the addend AHL is com 
puted as (AHI << 16) + (short)ALO.  R_MIPS_LO16 entries without an 
R_MIPS_HI16 entry immediately preceding are orphaned and the previously 
defined R_MIPS_HI16 is used for computing the addend."

Two consecutive entries must come from a single .rel.* section or they 
wouldn't be consecutive (there's no relationship between different .rel.* 
sections).

 While not explicitly stated I don't think anyone considered reusing an 
R_MIPS_HI16 reloc defined in another relocation section for an orphaned 
R_MIPS_LO16 reloc.  That would IMO make no semantic sense (for instance 
you can shuffle sections via a linker script in a relocatable link) and I 
think we ought not strive for doing so (i.e. there is not supposed to be 
any "previously defined R_MIPS_HI16" at the beginning of any given reloc 
section).

 Of course this consideration applies to the REL format only (i.e. o32); 
there's no need to track HI16/LO16 pairing with RELA objects as the addend 
is readily available.  AFAICT we don't get this right either: for 
`!partial_inplace' howtos we need not use `_bfd_mips_elf_hi16_reloc' or 
`_bfd_mips_elf_lo16_reloc' and just `_bfd_mips_elf_generic_reloc' will do.

>  Instead it
> would be much better to attach the list to mips_elf_section_data.  I
> wasn't sure enough to do that, given things like gcc's hot/cold
> section partitioning, when I moved the mip_hi16_list to be per-bfd.

 That sounds like a plan to me, but it's unrealistic for me to commit to 
it in the next two days (and then I head out to California for a week).

References:

[1] "SYSTEM V APPLICATION BINARY INTERFACE, MIPS RISC Processor
    Supplement, 3rd Edition", Section "Relocation", pp. 4-17, 4-18

  Maciej
  

On Thu, Feb 09, 2023 at 01:26:11AM +0000, Maciej W. Rozycki wrote:
> On Thu, 9 Feb 2023, Alan Modra wrote:
> > If it is really true that hi16/lo16 pairs are always in the same
> > section, then we wouldn't need freeze_mips_hi16_list.
> 
>  It is, by definition[1]:
> 
> "The AHL addend is a composite computed from the addends of two 
> consecutive relocation entries.  Each relocation type of R_MIPS_HI16 must 
> have an associated R_MIPS_LO16 entry immediately following it in the list 
> of relocations.
> 
> "These relocation entries are always processed as a pair and both addend 
> fields contribute to the AHL addend.  If AHI and ALO are the addends from 
> the paired R_MIPS_HI16 and R_MIPS_LO16 entries, then the addend AHL is com 
> puted as (AHI << 16) + (short)ALO.  R_MIPS_LO16 entries without an 
> R_MIPS_HI16 entry immediately preceding are orphaned and the previously 
> defined R_MIPS_HI16 is used for computing the addend."
> 
> Two consecutive entries must come from a single .rel.* section or they 
> wouldn't be consecutive (there's no relationship between different .rel.* 
> sections).

Yes, but see the comment before _bfd_mips_elf_hi16_reloc

   The ABI requires that the *LO16 immediately follow the *HI16.
   However, as a GNU extension, we permit an arbitrary number of
   *HI16s to be associated with a single *LO16.  This significantly
   simplies the relocation handling in gcc.

>  While not explicitly stated I don't think anyone considered reusing an 
> R_MIPS_HI16 reloc defined in another relocation section for an orphaned 
> R_MIPS_LO16 reloc.  That would IMO make no semantic sense (for instance 
> you can shuffle sections via a linker script in a relocatable link) and I 
> think we ought not strive for doing so (i.e. there is not supposed to be 
> any "previously defined R_MIPS_HI16" at the beginning of any given reloc 
> section).
> 
>  Of course this consideration applies to the REL format only (i.e. o32); 
> there's no need to track HI16/LO16 pairing with RELA objects as the addend 
> is readily available.  AFAICT we don't get this right either: for 
> `!partial_inplace' howtos we need not use `_bfd_mips_elf_hi16_reloc' or 
> `_bfd_mips_elf_lo16_reloc' and just `_bfd_mips_elf_generic_reloc' will do.
> 
> >  Instead it
> > would be much better to attach the list to mips_elf_section_data.  I
> > wasn't sure enough to do that, given things like gcc's hot/cold
> > section partitioning, when I moved the mip_hi16_list to be per-bfd.
> 
>  That sounds like a plan to me, but it's unrealistic for me to commit to 
> it in the next two days (and then I head out to California for a week).

Patch attached.  Note the FIXME.

> References:
> 
> [1] "SYSTEM V APPLICATION BINARY INTERFACE, MIPS RISC Processor
>     Supplement, 3rd Edition", Section "Relocation", pp. 4-17, 4-18
  

On Thu, 9 Feb 2023, Alan Modra wrote:

> > Two consecutive entries must come from a single .rel.* section or they 
> > wouldn't be consecutive (there's no relationship between different .rel.* 
> > sections).
> 
> Yes, but see the comment before _bfd_mips_elf_hi16_reloc
> 
>    The ABI requires that the *LO16 immediately follow the *HI16.
>    However, as a GNU extension, we permit an arbitrary number of
>    *HI16s to be associated with a single *LO16.  This significantly
>    simplies the relocation handling in gcc.

 Yes, I am aware of this GNU extension, but it does not change anything 
for this consideration.  There's just meant not to be any predefined HI16 
value at the beginning of any relocation section (IOW any initial orphan 
LO16 reloc has to resolve with the high part of the addend implied zero).

> >  That sounds like a plan to me, but it's unrealistic for me to commit to 
> > it in the next two days (and then I head out to California for a week).
> 
> Patch attached.  Note the FIXME.

 I think there's no need to bail out on unmatched HI16 relocs.  These 
relocs are used with the LUI instruction as the first part of an address 
load sequence.  If the low part is never referred, then the address will 
not have been fully loaded anyway, so such code won't do anything harmful.  

 I think such orphan HI16 relocs used to appear in harmless leftovers from 
optimised code with certain versions of GCC, so adding such an error would 
cause a regression as such code wouldn't link anymore.  I wouldn't add any 
extra error cases here beyond what we might already have.

 I'll have a look into this issue and dive into code to better understand 
it once I'm back from California.  I'll try to address the RELA side too.

 Thank you for your patches and your involvement with this issue, really 
appreciated.  And as I say, more proper support is required with the MIPS 
target given it's still commercially active.  It's not like say VAX, which 
only has a bunch of enthusiasts to fiddle with.

  Maciej
  

This is a slightly modified version of the patch posted at
https://sourceware.org/pipermail/binutils/2023-February/125916.html
with the logic for detecting orphan hi16 relocs in free_mips_hi16_list
improved so that the warning can be enabled.

OK to apply?

===
This patch is in response to fuzzing testcases that manage to cause
segfaults due to stale references to freed memory via mips_hi16.data.

A number of the error/warning handlers in ldmain.c use %C.  This can
cause debug info to be parsed for the first time in order to print
file/function/line.  If one of those warnings is triggered after some
hi16 relocs have been processed but before the matching lo16 reloc is
handled, *and* the debug info is corrupted with a lo16 reloc, then the
mips_hi16_list will be flushed with the result that printing a warning
changes linker output.  It is also possible that corrupted debug info
adds to the hi16 list, with the result that when the linker handles a
later lo16 reloc in a text section, ld will segfault accessing
mips_hi16.data after the debug buffers have be freed.  Both of these
problems are fixed by keeping a per-section mips_hi16_list rather than
a per-file list.

	* elfxx-mips.c (struct mips_hi16): Move earlier, deleting
	input_section and data fields.
	(struct _mips_elf_section_data): Add mips_hi16_list.
	(struct mips_elf_obj_tdata): Delete mips_hi16_list.
	(free_mips_hi16_list): New function.
	(_bfd_mips_elf_close_and_cleanup): Adjust to suit new location
	of mips_hi16_list.
	(_bfd_mips_elf_hi16_reloc, _bfd_mips_elf_lo16_reloc): Likewise.
	(_bfd_elf_mips_get_relocated_section_contents): Likewise.

diff --git a/bfd/elfxx-mips.c b/bfd/elfxx-mips.c
index 49355a42f7d..18b04a1abb5 100644
--- a/bfd/elfxx-mips.c
+++ b/bfd/elfxx-mips.c
@@ -222,6 +222,15 @@ struct mips_elf_traverse_got_arg
   int value;
 };
 
+/* Used to store a REL high-part relocation such as R_MIPS_HI16 or
+   R_MIPS_GOT16.  */
+
+struct mips_hi16
+{
+  struct mips_hi16 *next;
+  arelent rel;
+};
+
 struct _mips_elf_section_data
 {
   struct bfd_elf_section_data elf;
@@ -229,6 +238,8 @@ struct _mips_elf_section_data
   {
     bfd_byte *tdata;
   } u;
+
+  struct mips_hi16 *mips_hi16_list;
 };
 
 #define mips_elf_section_data(sec) \
@@ -549,19 +560,6 @@ struct mips_htab_traverse_info
   bool error;
 };
 
-/* Used to store a REL high-part relocation such as R_MIPS_HI16 or
-   R_MIPS_GOT16.  REL is the relocation, INPUT_SECTION is the section
-   that contains the relocation field and DATA points to the start of
-   INPUT_SECTION.  */
-
-struct mips_hi16
-{
-  struct mips_hi16 *next;
-  bfd_byte *data;
-  asection *input_section;
-  arelent rel;
-};
-
 /* MIPS ELF private object data.  */
 
 struct mips_elf_obj_tdata
@@ -597,8 +595,6 @@ struct mips_elf_obj_tdata
   asymbol *elf_text_symbol;
   asection *elf_data_section;
   asection *elf_text_section;
-
-  struct mips_hi16 *mips_hi16_list;
 };
 
 /* Get MIPS ELF private object data from BFD's tdata.  */
@@ -1418,6 +1414,30 @@ free_ecoff_debug (struct ecoff_debug_info *debug)
   debug->external_ext = NULL;
 }
 
+/* Free the mips_hi16_list attached to S.  Return true if there were
+   unmatched hi16 relocs.  */
+
+static bool
+free_mips_hi16_list (asection *s)
+{
+  struct mips_hi16 *hi;
+  struct mips_hi16 **hip = &mips_elf_section_data (s)->mips_hi16_list;
+  bool ret = false;
+
+  while ((hi = *hip) != NULL)
+    {
+      *hip = hi->next;
+      /* See gas/config/tc-mips.c reloc_needs_lo_p.  Not all hi16
+	 relocs need lo16 relocs.  */
+      if (hi->rel.howto->type == R_MIPS_HI16
+	  || hi->rel.howto->type == R_MIPS16_HI16
+	  || hi->rel.howto->type == R_MICROMIPS_HI16)
+	ret = true;
+      free (hi);
+    }
+  return ret;
+}
+
 bool
 _bfd_mips_elf_close_and_cleanup (bfd *abfd)
 {
@@ -1427,15 +1447,13 @@ _bfd_mips_elf_close_and_cleanup (bfd *abfd)
       if (tdata != NULL)
 	{
 	  BFD_ASSERT (tdata->root.object_id == MIPS_ELF_DATA);
-	  while (tdata->mips_hi16_list != NULL)
-	    {
-	      struct mips_hi16 *hi = tdata->mips_hi16_list;
-	      tdata->mips_hi16_list = hi->next;
-	      free (hi);
-	    }
 	  if (tdata->find_line_info != NULL)
 	    free_ecoff_debug (&tdata->find_line_info->d);
 	}
+      for (asection *s = abfd->sections; s; s = s->next)
+	if (free_mips_hi16_list (s))
+	  _bfd_error_handler
+	    (_("%pB(%pA): unmatched hi16 reloc"), abfd, s);
     }
   return _bfd_elf_close_and_cleanup (abfd);
 }
@@ -2557,26 +2575,22 @@ _bfd_mips_elf_gprel16_with_gp (bfd *abfd, asymbol *symbol,
 
 bfd_reloc_status_type
 _bfd_mips_elf_hi16_reloc (bfd *abfd, arelent *reloc_entry,
-			  asymbol *symbol ATTRIBUTE_UNUSED, void *data,
+			  asymbol *symbol ATTRIBUTE_UNUSED,
+			  void *data ATTRIBUTE_UNUSED,
 			  asection *input_section, bfd *output_bfd,
 			  char **error_message ATTRIBUTE_UNUSED)
 {
-  struct mips_hi16 *n;
-  struct mips_elf_obj_tdata *tdata;
-
   if (reloc_entry->address > bfd_get_section_limit (abfd, input_section))
     return bfd_reloc_outofrange;
 
-  n = bfd_malloc (sizeof *n);
+  struct mips_hi16 *n = bfd_malloc (sizeof *n);
   if (n == NULL)
     return bfd_reloc_outofrange;
 
-  tdata = mips_elf_tdata (abfd);
-  n->next = tdata->mips_hi16_list;
-  n->data = data;
-  n->input_section = input_section;
+  struct _mips_elf_section_data *sdata = mips_elf_section_data (input_section);
+  n->next = sdata->mips_hi16_list;
   n->rel = *reloc_entry;
-  tdata->mips_hi16_list = n;
+  sdata->mips_hi16_list = n;
 
   if (output_bfd != NULL)
     reloc_entry->address += input_section->output_offset;
@@ -2615,40 +2629,40 @@ _bfd_mips_elf_lo16_reloc (bfd *abfd, arelent *reloc_entry, asymbol *symbol,
 			  bfd *output_bfd, char **error_message)
 {
   bfd_vma vallo;
-  bfd_byte *location = (bfd_byte *) data + reloc_entry->address;
-  struct mips_elf_obj_tdata *tdata;
+  struct _mips_elf_section_data *sdata = mips_elf_section_data (input_section);
 
-  if (!bfd_reloc_offset_in_range (reloc_entry->howto, abfd, input_section,
-				  reloc_entry->address))
-    return bfd_reloc_outofrange;
+  if (sdata->mips_hi16_list != NULL)
+    {
+      if (!bfd_reloc_offset_in_range (reloc_entry->howto, abfd, input_section,
+				      reloc_entry->address))
+	return bfd_reloc_outofrange;
 
-  _bfd_mips_elf_reloc_unshuffle (abfd, reloc_entry->howto->type, false,
-				 location);
-  /* The high 16 bits of the addend are stored in the high insn, the
-     low 16 bits in the low insn, but there is a catch:  You can't
-     just concatenate the high and low parts.  The high part of the
-     addend is adjusted for the fact that the low part is sign
-     extended.  For example, an addend of 0x38000 would have 0x0004 in
-     the high part and 0x8000 (=0xff..f8000) in the low part.
-     To extract the actual addend, calculate (a)
-     ((hi & 0xffff) << 16) + ((lo & 0xffff) ^ 0x8000) - 0x8000.
-     We will be applying (symbol + addend) & 0xffff to the low insn,
-     and we want to apply (b) (symbol + addend + 0x8000) >> 16 to the
-     high insn (the +0x8000 adjusting for when the applied low part is
-     negative).  Substituting (a) into (b) and recognising that
-     (hi & 0xffff) is already in the high insn gives a high part
-     addend adjustment of (lo & 0xffff) ^ 0x8000.  */
-  vallo = (bfd_get_32 (abfd, location) & 0xffff) ^ 0x8000;
-  _bfd_mips_elf_reloc_shuffle (abfd, reloc_entry->howto->type, false,
-			       location);
+      bfd_byte *location = (bfd_byte *) data + reloc_entry->address;
+      _bfd_mips_elf_reloc_unshuffle (abfd, reloc_entry->howto->type, false,
+				     location);
+      /* The high 16 bits of the addend are stored in the high insn, the
+	 low 16 bits in the low insn, but there is a catch:  You can't
+	 just concatenate the high and low parts.  The high part of the
+	 addend is adjusted for the fact that the low part is sign
+	 extended.  For example, an addend of 0x38000 would have 0x0004 in
+	 the high part and 0x8000 (=0xff..f8000) in the low part.
+	 To extract the actual addend, calculate (a)
+	 ((hi & 0xffff) << 16) + ((lo & 0xffff) ^ 0x8000) - 0x8000.
+	 We will be applying (symbol + addend) & 0xffff to the low insn,
+	 and we want to apply (b) (symbol + addend + 0x8000) >> 16 to the
+	 high insn (the +0x8000 adjusting for when the applied low part is
+	 negative).  Substituting (a) into (b) and recognising that
+	 (hi & 0xffff) is already in the high insn gives a high part
+	 addend adjustment of (lo & 0xffff) ^ 0x8000.  */
+      vallo = (bfd_get_32 (abfd, location) & 0xffff) ^ 0x8000;
+      _bfd_mips_elf_reloc_shuffle (abfd, reloc_entry->howto->type, false,
+				   location);
+    }
 
-  tdata = mips_elf_tdata (abfd);
-  while (tdata->mips_hi16_list != NULL)
+  while (sdata->mips_hi16_list != NULL)
     {
       bfd_reloc_status_type ret;
-      struct mips_hi16 *hi;
-
-      hi = tdata->mips_hi16_list;
+      struct mips_hi16 *hi = sdata->mips_hi16_list;
 
       /* R_MIPS*_GOT16 relocations are something of a special case.  We
 	 want to install the addend in the same way as for a R_MIPS*_HI16
@@ -2664,13 +2678,13 @@ _bfd_mips_elf_lo16_reloc (bfd *abfd, arelent *reloc_entry, asymbol *symbol,
 
       hi->rel.addend += vallo;
 
-      ret = _bfd_mips_elf_generic_reloc (abfd, &hi->rel, symbol, hi->data,
-					 hi->input_section, output_bfd,
+      ret = _bfd_mips_elf_generic_reloc (abfd, &hi->rel, symbol, data,
+					 input_section, output_bfd,
 					 error_message);
       if (ret != bfd_reloc_ok)
 	return ret;
 
-      tdata->mips_hi16_list = hi->next;
+      sdata->mips_hi16_list = hi->next;
       free (hi);
     }
 
@@ -13344,24 +13358,8 @@ _bfd_elf_mips_get_relocated_section_contents
   reloc_vector = (arelent **) bfd_malloc (reloc_size);
   if (reloc_vector == NULL)
     {
-      struct mips_elf_obj_tdata *tdata;
-      struct mips_hi16 **hip, *hi;
     error_return:
-      /* If we are going to return an error, remove entries on
-	 mips_hi16_list that point into this section's data.  Data
-	 will typically be freed on return from this function.  */
-      tdata = mips_elf_tdata (abfd);
-      hip = &tdata->mips_hi16_list;
-      while ((hi = *hip) != NULL)
-	{
-	  if (hi->input_section == input_section)
-	    {
-	      *hip = hi->next;
-	      free (hi);
-	    }
-	  else
-	    hip = &hi->next;
-	}
+      free_mips_hi16_list (input_section);
       if (orig_data == NULL)
 	free (data);
       data = NULL;
  

On Sat, 20 May 2023, Alan Modra wrote:

> This is a slightly modified version of the patch posted at
> https://sourceware.org/pipermail/binutils/2023-February/125916.html
> with the logic for detecting orphan hi16 relocs in free_mips_hi16_list
> improved so that the warning can be enabled.

 I haven't lost your orignal fix from my radar and meant to come back to 
it around this time.  I still need a couple days to get through all this 
again.  I'll appreciate a bit of patience.

  Maciej