PR28306, segfault in _bfd_mips_elf_reloc_unshuffle

Message ID YTiXxtuLP+Cwcbkw@squeak.grove.modra.org
State New
Headers show
Series
  • PR28306, segfault in _bfd_mips_elf_reloc_unshuffle
Related show

Commit Message

CHIGOT, CLEMENT via Binutils Sept. 8, 2021, 11 a.m.
Protect the _bfd_mips_elf_reloc_unshuffle call in mips16_gprel_reloc
by checking the reloc offset.  The other changes catch potential
buffer overflows when processing relocations near the end of a
section.

OK to apply?

	PR 28306
	* elf64-mips.c (mips16_gprel_reloc): Sanity check reloc offset.
	(mips_elf64_gprel32_reloc): Use bfd_reloc_offset_in_range.
	* elfxx-mips.c (_bfd_mips_elf_gprel16_with_gp): Likewise.
	(_bfd_mips_elf_hi16_reloc, _bfd_mips_elf_lo16_reloc): Likewise.
	(_bfd_mips_elf_generic_reloc): Likewise.  Comment.
	* elf32-mips.c (gprel32_with_gp): Use bfd_reloc_offset_in_range.
	* elfn32-mips.c (gprel32_with_gp): Likewise.
	(elf_mips_howto_table_rela <R_MIPS_NONE>): Correct size.


-- 
Alan Modra
Australia Development Lab, IBM

Comments

Maciej W. Rozycki Sept. 9, 2021, 9:51 a.m. | #1
On Wed, 8 Sep 2021, Alan Modra wrote:

> Protect the _bfd_mips_elf_reloc_unshuffle call in mips16_gprel_reloc

> by checking the reloc offset.  The other changes catch potential

> buffer overflows when processing relocations near the end of a

> section.

> 

> OK to apply?


 Thank you for working on this issue.  Overall it looks good to me, but 
see one question below.

> diff --git a/bfd/elfn32-mips.c b/bfd/elfn32-mips.c

> index dc607e776d1..2ab0bae976a 100644

> --- a/bfd/elfn32-mips.c

> +++ b/bfd/elfn32-mips.c

> @@ -877,7 +877,7 @@ static reloc_howto_type elf_mips_howto_table_rela[] =

>    /* No relocation.  */

>    HOWTO (R_MIPS_NONE,		/* type */

>  	 0,			/* rightshift */

> -	 0,			/* size (0 = byte, 1 = short, 2 = long) */

> +	 3,			/* size (0 = byte, 1 = short, 2 = long) */

>  	 0,			/* bitsize */

>  	 false,			/* pc_relative */

>  	 0,			/* bitpos */


 This might well be a separate change, applied right away as obvious.

> diff --git a/bfd/elfxx-mips.c b/bfd/elfxx-mips.c

> index e4827fd17de..aef5ede3ef0 100644

> --- a/bfd/elfxx-mips.c

> +++ b/bfd/elfxx-mips.c

[...]
> @@ -2595,9 +2598,19 @@ _bfd_mips_elf_generic_reloc (bfd *abfd ATTRIBUTE_UNUSED, arelent *reloc_entry,

>    bfd_reloc_status_type status;

>    bool relocatable;

>  

> +  /* ld -r or gas.  */

>    relocatable = (output_bfd != NULL);

>  

> -  if (reloc_entry->address > bfd_get_section_limit (abfd, input_section))

> +  /* We only use bfd_reloc_offset_in_range for final linking because

> +     mips object files may use relocations that seem to access beyond

> +     section limits.  gas/testsuite/gas/mips/dla-reloc.s is an example

> +     that puts R_MIPS_SUB, a 64-bit relocation, on the last

> +     instruction in the section.  If final linking that object file

> +     the R_MIPS_SUB won't be processed here since it applies to the

> +     addend for the next reloc rather than the section contents.  */

> +  if (!relocatable

> +      && !bfd_reloc_offset_in_range (reloc_entry->howto, abfd,

> +				     input_section, reloc_entry->address))

>      return bfd_reloc_outofrange;


 Would a correct check be feasible here?  For a composed relocation only 
the final entry is applied to output, so could we instead check if there 
is a follow-up relocation?

  Maciej
CHIGOT, CLEMENT via Binutils Sept. 9, 2021, 2:14 p.m. | #2
On Thu, Sep 09, 2021 at 11:51:48AM +0200, Maciej W. Rozycki wrote:
> On Wed, 8 Sep 2021, Alan Modra wrote:

> > +  /* ld -r or gas.  */

> >    relocatable = (output_bfd != NULL);

> >  

> > -  if (reloc_entry->address > bfd_get_section_limit (abfd, input_section))

> > +  /* We only use bfd_reloc_offset_in_range for final linking because

> > +     mips object files may use relocations that seem to access beyond

> > +     section limits.  gas/testsuite/gas/mips/dla-reloc.s is an example

> > +     that puts R_MIPS_SUB, a 64-bit relocation, on the last

> > +     instruction in the section.  If final linking that object file

> > +     the R_MIPS_SUB won't be processed here since it applies to the

> > +     addend for the next reloc rather than the section contents.  */

> > +  if (!relocatable

> > +      && !bfd_reloc_offset_in_range (reloc_entry->howto, abfd,

> > +				     input_section, reloc_entry->address))

> >      return bfd_reloc_outofrange;

> 

>  Would a correct check be feasible here?  For a composed relocation only 

> the final entry is applied to output, so could we instead check if there 

> is a follow-up relocation?


I don't think there is any easy and safe way of doing that.  Even
though there is a nice tidy array of NULL terminated arelent pointers,
the special_function doesn't see an arelent** but rather an arelent*.

Hmm, how about replacing !relocatable above with
!(relocatable && !reloc_entry->howto->partial_inplace) ie. the
condition under which _bfd_mips_elf_generic_reloc writes section
contents?

-- 
Alan Modra
Australia Development Lab, IBM
CHIGOT, CLEMENT via Binutils Sept. 10, 2021, 8:27 a.m. | #3
On Thu, Sep 09, 2021 at 11:44:38PM +0930, Alan Modra wrote:
> On Thu, Sep 09, 2021 at 11:51:48AM +0200, Maciej W. Rozycki wrote:

> > On Wed, 8 Sep 2021, Alan Modra wrote:

> > > +  /* ld -r or gas.  */

> > >    relocatable = (output_bfd != NULL);

> > >  

> > > -  if (reloc_entry->address > bfd_get_section_limit (abfd, input_section))

> > > +  /* We only use bfd_reloc_offset_in_range for final linking because

> > > +     mips object files may use relocations that seem to access beyond

> > > +     section limits.  gas/testsuite/gas/mips/dla-reloc.s is an example

> > > +     that puts R_MIPS_SUB, a 64-bit relocation, on the last

> > > +     instruction in the section.  If final linking that object file

> > > +     the R_MIPS_SUB won't be processed here since it applies to the

> > > +     addend for the next reloc rather than the section contents.  */

> > > +  if (!relocatable

> > > +      && !bfd_reloc_offset_in_range (reloc_entry->howto, abfd,

> > > +				     input_section, reloc_entry->address))

> > >      return bfd_reloc_outofrange;

> > 

> >  Would a correct check be feasible here?  For a composed relocation only 

> > the final entry is applied to output, so could we instead check if there 

> > is a follow-up relocation?

> 

> I don't think there is any easy and safe way of doing that.  Even

> though there is a nice tidy array of NULL terminated arelent pointers,

> the special_function doesn't see an arelent** but rather an arelent*.

> 

> Hmm, how about replacing !relocatable above with

> !(relocatable && !reloc_entry->howto->partial_inplace) ie. the

> condition under which _bfd_mips_elf_generic_reloc writes section

> contents?


Testing revealed some fails
mipsisa32r2el-elf  +FAIL: MIPS reloc against local symbol overflow
mipstx39-elf  +FAIL: MIPS reloc against local symbol overflow

The test in question puts a ".half" at the end of a section, with
resultant R_MIPS_16, a 4 byte relocation, 2 bytes before the end of
the section.  I think the test should fail on these targets.  With a
very carefully crafted testcase it should be possible to cause a gas
buffer overflow.

-- 
Alan Modra
Australia Development Lab, IBM
Maciej W. Rozycki Sept. 10, 2021, 9:50 a.m. | #4
On Fri, 10 Sep 2021, Alan Modra wrote:

> > I don't think there is any easy and safe way of doing that.  Even

> > though there is a nice tidy array of NULL terminated arelent pointers,

> > the special_function doesn't see an arelent** but rather an arelent*.

> > 

> > Hmm, how about replacing !relocatable above with

> > !(relocatable && !reloc_entry->howto->partial_inplace) ie. the

> > condition under which _bfd_mips_elf_generic_reloc writes section

> > contents?

> 

> Testing revealed some fails

> mipsisa32r2el-elf  +FAIL: MIPS reloc against local symbol overflow

> mipstx39-elf  +FAIL: MIPS reloc against local symbol overflow

> 

> The test in question puts a ".half" at the end of a section, with

> resultant R_MIPS_16, a 4 byte relocation, 2 bytes before the end of

> the section.  I think the test should fail on these targets.  With a

> very carefully crafted testcase it should be possible to cause a gas

> buffer overflow.


 Hmm, it looks to me like a bug in the implementation of the `.half' 
pseudo-op (that it emits a 16-bit rather than a 32-bit data quantity with 
R_MIPS_16 attached to the least significant halfword), but I'm not sure if 
at this time of MIPS target's history it is safe to fix it.  I'll have to 
chew it over a bit and I'll be travelling over the next couple of days 
anyway, so I'll get back to this discussion after the weekend (including 
the issue of `arelent*' vs `arelent**').

  Maciej
CHIGOT, CLEMENT via Binutils Sept. 10, 2021, 11:01 a.m. | #5
On Fri, Sep 10, 2021 at 11:50:04AM +0200, Maciej W. Rozycki wrote:
> On Fri, 10 Sep 2021, Alan Modra wrote:

> 

> > > I don't think there is any easy and safe way of doing that.  Even

> > > though there is a nice tidy array of NULL terminated arelent pointers,

> > > the special_function doesn't see an arelent** but rather an arelent*.

> > > 

> > > Hmm, how about replacing !relocatable above with

> > > !(relocatable && !reloc_entry->howto->partial_inplace) ie. the

> > > condition under which _bfd_mips_elf_generic_reloc writes section

> > > contents?

> > 

> > Testing revealed some fails

> > mipsisa32r2el-elf  +FAIL: MIPS reloc against local symbol overflow

> > mipstx39-elf  +FAIL: MIPS reloc against local symbol overflow

> > 

> > The test in question puts a ".half" at the end of a section, with

> > resultant R_MIPS_16, a 4 byte relocation, 2 bytes before the end of

> > the section.  I think the test should fail on these targets.  With a

> > very carefully crafted testcase it should be possible to cause a gas

> > buffer overflow.

> 

>  Hmm, it looks to me like a bug in the implementation of the `.half' 

> pseudo-op (that it emits a 16-bit rather than a 32-bit data quantity with 

> R_MIPS_16 attached to the least significant halfword), but I'm not sure if 

> at this time of MIPS target's history it is safe to fix it.


R_MIPS_16 is specified by the ABI to be the low 16-bits within a
32-bit word.  That's a problem with .half as it is currently, since
gas emits the R_MIPS_16 at the .half address if a reloc is needed.
The result is that ld applies the relocation to the *next* halfword on
big-endian mips targets.  Effectively that means .half can only be
used with constants.

>  I'll have to 

> chew it over a bit and I'll be travelling over the next couple of days 

> anyway, so I'll get back to this discussion after the weekend (including 

> the issue of `arelent*' vs `arelent**').

> 

>   Maciej


-- 
Alan Modra
Australia Development Lab, IBM

Patch

diff --git a/bfd/elf32-mips.c b/bfd/elf32-mips.c
index f8467de478b..100e6e57143 100644
--- a/bfd/elf32-mips.c
+++ b/bfd/elf32-mips.c
@@ -1858,7 +1858,8 @@  gprel32_with_gp (bfd *abfd, asymbol *symbol, arelent *reloc_entry,
   relocation += symbol->section->output_section->vma;
   relocation += symbol->section->output_offset;
 
-  if (reloc_entry->address > bfd_get_section_limit (abfd, input_section))
+  if (!bfd_reloc_offset_in_range (reloc_entry->howto, abfd,
+				  input_section, reloc_entry->address))
     return bfd_reloc_outofrange;
 
   /* Set val to the offset into the section or symbol.  */
diff --git a/bfd/elf64-mips.c b/bfd/elf64-mips.c
index 9ad884fafb6..764c686c0e8 100644
--- a/bfd/elf64-mips.c
+++ b/bfd/elf64-mips.c
@@ -3581,7 +3581,8 @@  mips_elf64_gprel32_reloc (bfd *abfd, arelent *reloc_entry, asymbol *symbol,
   relocation += symbol->section->output_section->vma;
   relocation += symbol->section->output_offset;
 
-  if (reloc_entry->address > bfd_get_section_limit (abfd, input_section))
+  if (!bfd_reloc_offset_in_range (reloc_entry->howto, abfd,
+				  input_section, reloc_entry->address))
     return bfd_reloc_outofrange;
 
   /* Set val to the offset into the section or symbol.  */
@@ -3662,6 +3663,10 @@  mips16_gprel_reloc (bfd *abfd, arelent *reloc_entry, asymbol *symbol,
   if (ret != bfd_reloc_ok)
     return ret;
 
+  if (!bfd_reloc_offset_in_range (reloc_entry->howto, abfd,
+				  input_section, reloc_entry->address))
+    return bfd_reloc_outofrange;
+
   location = (bfd_byte *) data + reloc_entry->address;
   _bfd_mips_elf_reloc_unshuffle (abfd, reloc_entry->howto->type, false,
 				 location);
diff --git a/bfd/elfn32-mips.c b/bfd/elfn32-mips.c
index dc607e776d1..2ab0bae976a 100644
--- a/bfd/elfn32-mips.c
+++ b/bfd/elfn32-mips.c
@@ -877,7 +877,7 @@  static reloc_howto_type elf_mips_howto_table_rela[] =
   /* No relocation.  */
   HOWTO (R_MIPS_NONE,		/* type */
 	 0,			/* rightshift */
-	 0,			/* size (0 = byte, 1 = short, 2 = long) */
+	 3,			/* size (0 = byte, 1 = short, 2 = long) */
 	 0,			/* bitsize */
 	 false,			/* pc_relative */
 	 0,			/* bitpos */
@@ -3412,7 +3412,8 @@  gprel32_with_gp (bfd *abfd, asymbol *symbol, arelent *reloc_entry,
   relocation += symbol->section->output_section->vma;
   relocation += symbol->section->output_offset;
 
-  if (reloc_entry->address > bfd_get_section_limit (abfd, input_section))
+  if (!bfd_reloc_offset_in_range (reloc_entry->howto, abfd,
+				  input_section, reloc_entry->address))
     return bfd_reloc_outofrange;
 
   if (reloc_entry->howto->src_mask == 0)
diff --git a/bfd/elfxx-mips.c b/bfd/elfxx-mips.c
index e4827fd17de..aef5ede3ef0 100644
--- a/bfd/elfxx-mips.c
+++ b/bfd/elfxx-mips.c
@@ -2416,7 +2416,8 @@  _bfd_mips_elf_gprel16_with_gp (bfd *abfd, asymbol *symbol,
   relocation += symbol->section->output_section->vma;
   relocation += symbol->section->output_offset;
 
-  if (reloc_entry->address > bfd_get_section_limit (abfd, input_section))
+  if (!bfd_reloc_offset_in_range (reloc_entry->howto, abfd,
+				  input_section, reloc_entry->address))
     return bfd_reloc_outofrange;
 
   /* Set val to the offset into the section or symbol.  */
@@ -2475,14 +2476,15 @@  static struct mips_hi16 *mips_hi16_list;
    simplies the relocation handling in gcc.  */
 
 bfd_reloc_status_type
-_bfd_mips_elf_hi16_reloc (bfd *abfd ATTRIBUTE_UNUSED, arelent *reloc_entry,
+_bfd_mips_elf_hi16_reloc (bfd *abfd, arelent *reloc_entry,
 			  asymbol *symbol ATTRIBUTE_UNUSED, void *data,
 			  asection *input_section, bfd *output_bfd,
 			  char **error_message ATTRIBUTE_UNUSED)
 {
   struct mips_hi16 *n;
 
-  if (reloc_entry->address > bfd_get_section_limit (abfd, input_section))
+  if (!bfd_reloc_offset_in_range (reloc_entry->howto, abfd,
+				  input_section, reloc_entry->address))
     return bfd_reloc_outofrange;
 
   n = bfd_malloc (sizeof *n);
@@ -2534,7 +2536,8 @@  _bfd_mips_elf_lo16_reloc (bfd *abfd, arelent *reloc_entry, asymbol *symbol,
   bfd_vma vallo;
   bfd_byte *location = (bfd_byte *) data + reloc_entry->address;
 
-  if (reloc_entry->address > bfd_get_section_limit (abfd, input_section))
+  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,
@@ -2586,7 +2589,7 @@  _bfd_mips_elf_lo16_reloc (bfd *abfd, arelent *reloc_entry, asymbol *symbol,
    bfd_perform_relocation and bfd_install_relocation.  */
 
 bfd_reloc_status_type
-_bfd_mips_elf_generic_reloc (bfd *abfd ATTRIBUTE_UNUSED, arelent *reloc_entry,
+_bfd_mips_elf_generic_reloc (bfd *abfd, arelent *reloc_entry,
 			     asymbol *symbol, void *data ATTRIBUTE_UNUSED,
 			     asection *input_section, bfd *output_bfd,
 			     char **error_message ATTRIBUTE_UNUSED)
@@ -2595,9 +2598,19 @@  _bfd_mips_elf_generic_reloc (bfd *abfd ATTRIBUTE_UNUSED, arelent *reloc_entry,
   bfd_reloc_status_type status;
   bool relocatable;
 
+  /* ld -r or gas.  */
   relocatable = (output_bfd != NULL);
 
-  if (reloc_entry->address > bfd_get_section_limit (abfd, input_section))
+  /* We only use bfd_reloc_offset_in_range for final linking because
+     mips object files may use relocations that seem to access beyond
+     section limits.  gas/testsuite/gas/mips/dla-reloc.s is an example
+     that puts R_MIPS_SUB, a 64-bit relocation, on the last
+     instruction in the section.  If final linking that object file
+     the R_MIPS_SUB won't be processed here since it applies to the
+     addend for the next reloc rather than the section contents.  */
+  if (!relocatable
+      && !bfd_reloc_offset_in_range (reloc_entry->howto, abfd,
+				     input_section, reloc_entry->address))
     return bfd_reloc_outofrange;
 
   /* Build up the field adjustment in VAL.  */