[SPARC] Fix GOT relocation overflow

Message ID 4201293.bIne749SvQ@polaris
State New
Headers show
Series
  • [SPARC] Fix GOT relocation overflow
Related show

Commit Message

Eric Botcazou Feb. 8, 2018, 9:48 p.m.
Hi,

there are 2 failures left in the linker testsuite on SPARC64/Linux:

child killed: segmentation violation
FAIL: visibility (hidden_normal) (PIC main)

< main_visibility_checkweak () == 0
---
> main_visibility_checkweak () == 1

child process exited abnormally
FAIL: visibility (hidden_weak)

and they are caused by 2 different issues leading to the same end effect: the 
overflow of the relocation section for the GOT, i.e. the linker generates too 
many dynamic relocations for the GOT wrt the size of the relocation section,
leading to memory corruption and missing relocations in the final binary.

The first issue was introduced by:
  https://sourceware.org/ml/binutils/2017-06/msg00368.html
which makes the linker generate more R_SPARC_RELATIVE relocations for the GOT 
without adjusting the size of the relocation section.  This is fixed by (1) 
preventively adjusting this size in allocate_dynrelocs and (2) generating 
R_SPARC_NONE relocations if needed when R_SPARC_GOTDATA_OP is relaxed.

The second issue is that we generate a GOT relocation for an undefined weak 
symbol with non-default visibility in a PIC binary without accounting for that 
in the size of the relocation section.  My understanding is that the address 
of such an abomination should resolve to 0 at run time, which is achieved by 
not generating the relocation at all, i.e. leaving the GOT entry zeroed.

The patch also contains minor tweaks and adjustments to comments, as well as 
an assertion in sparc_elf_append_rela so as to abort the link on relocation 
overflow instead of corrupting either the memory or the final binary.

Tested on SPARC64/Linux with a GCC bootstrap, OK for mainline and 2.30 branch?


2018-02-08  Eric Botcazou  <ebotcazou@adacore.com>

bfd/
	* elfxx-sparc.c (UNDEFINED_WEAK_RESOLVED_TO_ZERO): Reorder conditions.
	(sparc_elf_append_rela): Assert that there is enough room in the section.
	(_bfd_sparc_elf_copy_indirect_symbol): Fix formatting.
	(_bfd_sparc_elf_adjust_dynamic_symbol): Minor tweak.
	(allocate_dynrelocs): Remove outdated comments and reorder conditions.
	For a symbol subject to a GOT relocation, reserve a slot in the relocation
	section if the symbol isn't dynamic and we are in PIC mode.
	(_bfd_sparc_elf_relocate_section) <R_SPARC_GOTDATA_OP>: If the relocation
	is relaxed and a slot was reserved, generate a R_SPARC_NONE relocation.
	<R_SPARC_GOTDATA_OP_HIX22>: Adjust comments.
	<R_SPARC_PC10>: Reorder conditions.  Remove always-false assertion.
	(_bfd_sparc_elf_finish_dynamic_symbol): Rename local_undefweak into
	resolved_to_zero.  Do not generate a dynamic GOT relocation for an
	undefined weak symbol with non-default visibility.
	Remove superfluous 'else' and fix formatting.

-- 
Eric Botcazou

Comments

Nick Clifton Feb. 9, 2018, 9:40 a.m. | #1
Hi Eric,

> Tested on SPARC64/Linux with a GCC bootstrap, OK for mainline and 2.30 branch?


Approved for both - please apply.

> 2018-02-08  Eric Botcazou  <ebotcazou@adacore.com>

> 

> bfd/

> 	* elfxx-sparc.c (UNDEFINED_WEAK_RESOLVED_TO_ZERO): Reorder conditions.

> 	(sparc_elf_append_rela): Assert that there is enough room in the section.

> 	(_bfd_sparc_elf_copy_indirect_symbol): Fix formatting.

> 	(_bfd_sparc_elf_adjust_dynamic_symbol): Minor tweak.

> 	(allocate_dynrelocs): Remove outdated comments and reorder conditions.

> 	For a symbol subject to a GOT relocation, reserve a slot in the relocation

> 	section if the symbol isn't dynamic and we are in PIC mode.

> 	(_bfd_sparc_elf_relocate_section) <R_SPARC_GOTDATA_OP>: If the relocation

> 	is relaxed and a slot was reserved, generate a R_SPARC_NONE relocation.

> 	<R_SPARC_GOTDATA_OP_HIX22>: Adjust comments.

> 	<R_SPARC_PC10>: Reorder conditions.  Remove always-false assertion.

> 	(_bfd_sparc_elf_finish_dynamic_symbol): Rename local_undefweak into

> 	resolved_to_zero.  Do not generate a dynamic GOT relocation for an

> 	undefined weak symbol with non-default visibility.

> 	Remove superfluous 'else' and fix formatting.

> 


Cheers
  Nick

Patch

diff --git a/bfd/elfxx-sparc.c b/bfd/elfxx-sparc.c
index afd62d4a50..83ead7570b 100644
--- a/bfd/elfxx-sparc.c
+++ b/bfd/elfxx-sparc.c
@@ -673,9 +673,9 @@  _bfd_sparc_elf_info_to_howto (bfd *abfd ATTRIBUTE_UNUSED, arelent *cache_ptr,
   ((EH)->elf.root.type == bfd_link_hash_undefweak		\
    && bfd_link_executable (INFO)				\
    && (_bfd_sparc_elf_hash_table (INFO)->interp == NULL		\
-       || !(EH)->has_got_reloc					\
+       || !(INFO)->dynamic_undefined_weak			\
        || (EH)->has_non_got_reloc				\
-       || !(INFO)->dynamic_undefined_weak))
+       || !(EH)->has_got_reloc))
 
 /* SPARC ELF linker hash entry.  */
 
@@ -750,6 +750,7 @@  sparc_elf_append_rela (bfd *abfd, asection *s, Elf_Internal_Rela *rel)
   bfd_byte *loc;
 
   bed = get_elf_backend_data (abfd);
+  BFD_ASSERT (s->reloc_count * bed->s->sizeof_rela < s->size);
   loc = s->contents + (s->reloc_count++ * bed->s->sizeof_rela);
   bed->s->swap_reloca_out (abfd, rel, loc);
 }
@@ -1310,8 +1311,7 @@  _bfd_sparc_elf_copy_indirect_symbol (struct bfd_link_info *info,
       eind->dyn_relocs = NULL;
     }
 
-  if (ind->root.type == bfd_link_hash_indirect
-      && dir->got.refcount <= 0)
+  if (ind->root.type == bfd_link_hash_indirect && dir->got.refcount <= 0)
     {
       edir->tls_type = eind->tls_type;
       eind->tls_type = GOT_UNKNOWN;
@@ -1968,8 +1968,8 @@  _bfd_sparc_elf_adjust_dynamic_symbol (struct bfd_link_info *info,
       if (h->plt.refcount <= 0
 	  || (h->type != STT_GNU_IFUNC
 	      && (SYMBOL_CALLS_LOCAL (info, h)
-		  || (ELF_ST_VISIBILITY (h->other) != STV_DEFAULT
-		      && h->root.type == bfd_link_hash_undefweak))))
+		  || (h->root.type == bfd_link_hash_undefweak
+		      && ELF_ST_VISIBILITY (h->other) != STV_DEFAULT))))
 	{
 	  /* This case can occur if we saw a WPLT30 reloc in an input
 	     file, but the symbol was never referred to by a dynamic
@@ -2088,12 +2088,11 @@  allocate_dynrelocs (struct elf_link_hash_entry *h, void * inf)
 	  && h->def_regular
 	  && h->ref_regular))
     {
-      /* Make sure this symbol is output as a dynamic symbol.
-	 Undefined weak syms won't yet be marked as dynamic.  */
-      if (h->dynindx == -1
-	  && !h->forced_local
+      /* Undefined weak syms won't yet be marked as dynamic.  */
+      if (h->root.type == bfd_link_hash_undefweak
 	  && !resolved_to_zero
-	  && h->root.type == bfd_link_hash_undefweak)
+	  && h->dynindx == -1
+	  && !h->forced_local)
 	{
 	  if (! bfd_elf_link_record_dynamic_symbol (info, h))
 	    return FALSE;
@@ -2201,12 +2200,11 @@  allocate_dynrelocs (struct elf_link_hash_entry *h, void * inf)
       bfd_boolean dyn;
       int tls_type = _bfd_sparc_elf_hash_entry(h)->tls_type;
 
-      /* Make sure this symbol is output as a dynamic symbol.
-	 Undefined weak syms won't yet be marked as dynamic.  */
-      if (h->dynindx == -1
-	  && !h->forced_local
+      /* Undefined weak syms won't yet be marked as dynamic.  */
+      if (h->root.type == bfd_link_hash_undefweak
 	  && !resolved_to_zero
-	  && h->root.type == bfd_link_hash_undefweak)
+	  && h->dynindx == -1
+	  && !h->forced_local)
 	{
 	  if (! bfd_elf_link_record_dynamic_symbol (info, h))
 	    return FALSE;
@@ -2220,21 +2218,25 @@  allocate_dynrelocs (struct elf_link_hash_entry *h, void * inf)
 	s->size += SPARC_ELF_WORD_BYTES (htab);
       dyn = htab->elf.dynamic_sections_created;
       /* R_SPARC_TLS_IE_{HI22,LO10} needs one dynamic relocation,
-	 R_SPARC_TLS_GD_{HI22,LO10} needs one if local symbol and two if
-	 global.  No dynamic relocations are needed against resolved
-	 undefined weak symbols in an executable.  */
+	 R_SPARC_TLS_GD_{HI22,LO10} needs one if local and two if global.  */
       if ((tls_type == GOT_TLS_GD && h->dynindx == -1)
 	  || tls_type == GOT_TLS_IE
 	  || h->type == STT_GNU_IFUNC)
 	htab->elf.srelgot->size += SPARC_ELF_RELA_BYTES (htab);
       else if (tls_type == GOT_TLS_GD)
 	htab->elf.srelgot->size += 2 * SPARC_ELF_RELA_BYTES (htab);
-      else if (((ELF_ST_VISIBILITY (h->other) == STV_DEFAULT
-		 && !resolved_to_zero)
-		|| h->root.type != bfd_link_hash_undefweak)
-	       && WILL_CALL_FINISH_DYNAMIC_SYMBOL (dyn,
-						   bfd_link_pic (info),
-						   h))
+      else if ((WILL_CALL_FINISH_DYNAMIC_SYMBOL (dyn, bfd_link_pic (info), h)
+		/* Even if the symbol isn't dynamic, we may generate a
+		   reloc for the dynamic linker in PIC mode.  */
+		|| (h->dynindx == -1
+		    && !h->forced_local
+		    && h->root.type != bfd_link_hash_undefweak
+		    && bfd_link_pic (info)))
+	       /* No dynamic relocations are needed against resolved
+		  undefined weak symbols in an executable.  */
+	       && !(h->root.type == bfd_link_hash_undefweak
+		    && (ELF_ST_VISIBILITY (h->other) != STV_DEFAULT
+			|| resolved_to_zero)))
 	htab->elf.srelgot->size += SPARC_ELF_RELA_BYTES (htab);
     }
   else
@@ -2344,12 +2346,11 @@  allocate_dynrelocs (struct elf_link_hash_entry *h, void * inf)
 		  && (h->root.type == bfd_link_hash_undefweak
 		      || h->root.type == bfd_link_hash_undefined))))
 	{
-	  /* Make sure this symbol is output as a dynamic symbol.
-	     Undefined weak syms won't yet be marked as dynamic.  */
-	  if (h->dynindx == -1
-	      && !h->forced_local
+	  /* Undefined weak syms won't yet be marked as dynamic.  */
+	  if (h->root.type == bfd_link_hash_undefweak
 	      && !resolved_to_zero
-	      && h->root.type == bfd_link_hash_undefweak)
+	      && h->dynindx == -1
+	      && !h->forced_local)
 	    {
 	      if (! bfd_elf_link_record_dynamic_symbol (info, h))
 		return FALSE;
@@ -3118,6 +3119,26 @@  _bfd_sparc_elf_relocate_section (bfd *output_bfd,
 	      /* {ld,ldx} [%rs1 + %rs2], %rd --> add %rs1, %rs2, %rd */
 	      relocation = 0x80000000 | (insn & 0x3e07c01f);
 	      bfd_put_32 (output_bfd, relocation, contents + rel->r_offset);
+
+	      /* If the symbol is global but not dynamic, an .rela.* slot has
+		 been allocated for it in the GOT so output R_SPARC_NONE here.
+		 See also the handling of other GOT relocations just below.  */
+	      if (h != NULL
+		  && h->dynindx == -1
+		  && !h->forced_local
+		  && h->root.type != bfd_link_hash_undefweak
+		  && (h->got.offset & 1) == 0
+		  && bfd_link_pic (info))
+		{
+		  asection *s = htab->elf.srelgot;
+		  Elf_Internal_Rela outrel;
+
+		  BFD_ASSERT (s != NULL);
+
+		  memset (&outrel, 0, sizeof outrel);
+		  sparc_elf_append_rela (output_bfd, s, &outrel);
+		  h->got.offset |= 1;
+		}
 	    }
 	  continue;
 	}
@@ -3170,15 +3191,13 @@  _bfd_sparc_elf_relocate_section (bfd *output_bfd,
 		    off &= ~1;
 		  else
 		    {
+		      /* If this symbol isn't dynamic in PIC mode, treat it
+			 like a local symbol in PIC mode below.  */
 		      if (h->dynindx == -1
 			  && !h->forced_local
 			  && h->root.type != bfd_link_hash_undefweak
 			  && bfd_link_pic (info))
-			{
-			  /* If this symbol isn't dynamic in PIC
-			     generate R_SPARC_RELATIVE here.  */
-			  relative_reloc = TRUE;
-			}
+			relative_reloc = TRUE;
 		      else
 			SPARC_ELF_PUT_WORD (htab, output_bfd, relocation,
 					    htab->elf.sgot->contents + off);
@@ -3202,6 +3221,8 @@  _bfd_sparc_elf_relocate_section (bfd *output_bfd,
 		off &= ~1;
 	      else
 		{
+		  /* For a local symbol in PIC mode, we need to generate a
+		     R_SPARC_RELATIVE reloc for the dynamic linker.  */
 		  if (bfd_link_pic (info))
 		    relative_reloc = TRUE;
 		  else
@@ -3213,12 +3234,9 @@  _bfd_sparc_elf_relocate_section (bfd *output_bfd,
 
 	  if (relative_reloc)
 	    {
-	      asection *s;
+	      asection *s = htab->elf.srelgot;
 	      Elf_Internal_Rela outrel;
 
-	      /* We need to generate a R_SPARC_RELATIVE reloc
-		 for the dynamic linker.  */
-	      s = htab->elf.srelgot;
 	      BFD_ASSERT (s != NULL);
 
 	      outrel.r_offset = (htab->elf.sgot->output_section->vma
@@ -3349,9 +3367,9 @@  _bfd_sparc_elf_relocate_section (bfd *output_bfd,
 	     in PIE.  */
 	  if ((bfd_link_pic (info)
 	       && (h == NULL
-		   || ((ELF_ST_VISIBILITY (h->other) == STV_DEFAULT
-			&& !resolved_to_zero)
-		       || h->root.type != bfd_link_hash_undefweak))
+		   || !(h->root.type == bfd_link_hash_undefweak
+			&& (ELF_ST_VISIBILITY (h->other) != STV_DEFAULT
+			    || resolved_to_zero)))
 	       && (! howto->pc_relative
 		   || !SYMBOL_CALLS_LOCAL (info, h)))
 	      || (!bfd_link_pic (info)
@@ -3438,7 +3456,6 @@  _bfd_sparc_elf_relocate_section (bfd *output_bfd,
 			   || !SYMBOLIC_BIND (info, h)
 			   || !h->def_regular))
 		{
-		  BFD_ASSERT (h->dynindx != -1);
 		  outrel.r_info = SPARC_ELF_R_INFO (htab, rel, h->dynindx, r_type);
 		  outrel.r_addend = rel->r_addend;
 		}
@@ -4270,7 +4287,7 @@  _bfd_sparc_elf_finish_dynamic_symbol (bfd *output_bfd,
   struct _bfd_sparc_elf_link_hash_table *htab;
   const struct elf_backend_data *bed;
   struct _bfd_sparc_elf_link_hash_entry  *eh;
-  bfd_boolean local_undefweak;
+  bfd_boolean resolved_to_zero;
 
   htab = _bfd_sparc_elf_hash_table (info);
   BFD_ASSERT (htab != NULL);
@@ -4281,7 +4298,7 @@  _bfd_sparc_elf_finish_dynamic_symbol (bfd *output_bfd,
   /* We keep PLT/GOT entries without dynamic PLT/GOT relocations for
      resolved undefined weak symbols in executable so that their
      references have value 0 at run-time.  */
-  local_undefweak = UNDEFINED_WEAK_RESOLVED_TO_ZERO (info, eh);
+  resolved_to_zero = UNDEFINED_WEAK_RESOLVED_TO_ZERO (info, eh);
 
   if (h->plt.offset != (bfd_vma) -1)
     {
@@ -4406,8 +4423,7 @@  _bfd_sparc_elf_finish_dynamic_symbol (bfd *output_bfd,
       loc += rela_index * bed->s->sizeof_rela;
       bed->s->swap_reloca_out (output_bfd, &rela, loc);
 
-      if (!local_undefweak
-	  && !h->def_regular)
+      if (!resolved_to_zero && !h->def_regular)
 	{
 	  /* Mark the symbol as undefined, rather than as defined in
 	     the .plt section.  Leave the value alone.  */
@@ -4421,12 +4437,14 @@  _bfd_sparc_elf_finish_dynamic_symbol (bfd *output_bfd,
 	}
     }
 
-  /* Don't generate dynamic GOT relocation against undefined weak
-     symbol in executable.  */
+  /* Don't generate dynamic GOT relocation against resolved undefined weak
+     symbols in an executable.  */
   if (h->got.offset != (bfd_vma) -1
       && _bfd_sparc_elf_hash_entry(h)->tls_type != GOT_TLS_GD
       && _bfd_sparc_elf_hash_entry(h)->tls_type != GOT_TLS_IE
-      && !local_undefweak)
+      && !(h->root.type == bfd_link_hash_undefweak
+	   && (ELF_ST_VISIBILITY (h->other) != STV_DEFAULT
+	       || resolved_to_zero)))
     {
       asection *sgot;
       asection *srela;
@@ -4462,8 +4480,8 @@  _bfd_sparc_elf_finish_dynamic_symbol (bfd *output_bfd,
 			      + (h->got.offset & ~(bfd_vma) 1));
 	  return TRUE;
 	}
-      else if (bfd_link_pic (info)
-	       && SYMBOL_REFERENCES_LOCAL (info, h))
+
+      if (bfd_link_pic (info) && SYMBOL_REFERENCES_LOCAL (info, h))
 	{
 	  asection *sec = h->root.u.def.section;
 	  if (h->type == STT_GNU_IFUNC)