Re: Protect mips_hi16_list from fuzzed debug info

[Alan Modra <amodra@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2875 bytes --]

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

-- 
Alan Modra
Australia Development Lab, IBM

[-- Attachment #2: 0002-Move-mips_hi16_list-to-mips_elf_section_data.patch --]
[-- Type: text/x-diff, Size: 6946 bytes --]

From 4febdc8426f3b834bc5bb5a9e6cc67f2315018c4 Mon Sep 17 00:00:00 2001
From: Alan Modra <amodra@gmail.com>
Date: Thu, 9 Feb 2023 14:24:49 +1030
Subject: Move mips_hi16_list to mips_elf_section_data

	* 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 e9fb61ff9e7..8dcfba3c27e 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.  */
@@ -1380,19 +1376,37 @@ _bfd_mips_elf_mkobject (bfd *abfd)
 				  MIPS_ELF_DATA);
 }
 
+/* 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 = *hip != NULL;
+
+  while ((hi = *hip) != NULL)
+    {
+      *hip = hi->next;
+      free (hi);
+    }
+  return ret;
+}
+
 bool
 _bfd_mips_elf_close_and_cleanup (bfd *abfd)
 {
-  struct mips_elf_obj_tdata *tdata = mips_elf_tdata (abfd);
-  if (tdata != NULL && bfd_get_format (abfd) == bfd_object)
+  if (bfd_get_format (abfd) == bfd_object)
     {
-      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);
-	}
+      for (asection *s = abfd->sections; s; s = s->next)
+	if (free_mips_hi16_list (s) && 0)
+	   /* FIXME: Disabled until _bfd_mips_elf_got16_reloc is
+	      taught that vxworks does not pair the got16 relocs with
+	      lo16 relocs, and thus should not put them on this list.
+	      See gas/config/tc-mips.c reloc_needs_lo_p.  */
+	  _bfd_error_handler
+	    (_("%pB(%pA): unmatched hi16 reloc"), abfd, s);
     }
   return _bfd_elf_close_and_cleanup (abfd);
 }
@@ -2524,26 +2538,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;
@@ -2583,7 +2593,6 @@ _bfd_mips_elf_lo16_reloc (bfd *abfd, arelent *reloc_entry, asymbol *symbol,
 {
   bfd_vma vallo;
   bfd_byte *location = (bfd_byte *) data + reloc_entry->address;
-  struct mips_elf_obj_tdata *tdata;
 
   if (!bfd_reloc_offset_in_range (reloc_entry->howto, abfd, input_section,
 				  reloc_entry->address))
@@ -2595,13 +2604,11 @@ _bfd_mips_elf_lo16_reloc (bfd *abfd, arelent *reloc_entry, asymbol *symbol,
   _bfd_mips_elf_reloc_shuffle (abfd, reloc_entry->howto->type, false,
 			       location);
 
-  tdata = mips_elf_tdata (abfd);
-  while (tdata->mips_hi16_list != NULL)
+  struct _mips_elf_section_data *sdata = mips_elf_section_data (input_section);
+  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
@@ -2619,13 +2626,13 @@ _bfd_mips_elf_lo16_reloc (bfd *abfd, arelent *reloc_entry, asymbol *symbol,
 	 carry or borrow will induce a change of +1 or -1 in the high part.  */
       hi->rel.addend += (vallo + 0x8000) & 0xffff;
 
-      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);
     }
 
@@ -13314,24 +13321,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;