[v2] ld: warn about PE base relocations to sections above .reloc

Message ID 8d236147-8425-627e-a988-83a05c35ed5a@suse.com
State New
Headers show
Series
  • [v2] ld: warn about PE base relocations to sections above .reloc
Related show

Commit Message

Alan Modra via Binutils March 22, 2021, 4:49 p.m.
Due to a bogus linker script, or perhaps because a section doesn't get
placed by a linker script while default placement puts it too high up,
sections can end up above .reloc. Since the process of determining its
contents (and hence its size) happens before final section placement,
relocations needed for such sections would no longer point at the
correct address in the final binary. Warn about this (down the road this
may want to become an error, unless size determination and content
creation for .reloc would get decoupled).

Two of the testcases would actually trigger the warning, because .reloc
gets placed at 0 without mentioning it in their linker scripts. Extend
the two scripts accordingly.

ld/
2021-03-XX  Jan Beulich  <jbeulich@suse.com>

	* pe-dll.c (generate_reloc): Warn when relocated entry is above
	.reloc.
	* testsuite/ld-scripts/pr14962.t, testsuite/ld-scripts/weak.t:
	Place .reloc section.
---
v2: Wrap string literal in _().

Comments

Alan Modra via Binutils March 29, 2021, 3:49 a.m. | #1
On Mon, Mar 22, 2021 at 05:49:10PM +0100, Jan Beulich via Binutils wrote:
> Due to a bogus linker script, or perhaps because a section doesn't get

> placed by a linker script while default placement puts it too high up,

> sections can end up above .reloc. Since the process of determining its

> contents (and hence its size) happens before final section placement,

> relocations needed for such sections would no longer point at the

> correct address in the final binary. Warn about this (down the road this

> may want to become an error, unless size determination and content

> creation for .reloc would get decoupled).

> 

> Two of the testcases would actually trigger the warning, because .reloc

> gets placed at 0 without mentioning it in their linker scripts. Extend

> the two scripts accordingly.


Both of those scripts discard .reloc.  Yes, they get a vma of 0 but
that's not really being "placed".  I think you should probably handle
.reloc being discarded rather than, or perhaps in addition to, this
patch.

-- 
Alan Modra
Australia Development Lab, IBM
Alan Modra via Binutils March 29, 2021, 11:35 a.m. | #2
On 29.03.2021 05:49, Alan Modra wrote:
> On Mon, Mar 22, 2021 at 05:49:10PM +0100, Jan Beulich via Binutils wrote:

>> Due to a bogus linker script, or perhaps because a section doesn't get

>> placed by a linker script while default placement puts it too high up,

>> sections can end up above .reloc. Since the process of determining its

>> contents (and hence its size) happens before final section placement,

>> relocations needed for such sections would no longer point at the

>> correct address in the final binary. Warn about this (down the road this

>> may want to become an error, unless size determination and content

>> creation for .reloc would get decoupled).

>>

>> Two of the testcases would actually trigger the warning, because .reloc

>> gets placed at 0 without mentioning it in their linker scripts. Extend

>> the two scripts accordingly.

> 

> Both of those scripts discard .reloc.  Yes, they get a vma of 0 but

> that's not really being "placed".


Oh, indeed, I'm sorry for not having paid attention.

>  I think you should probably handle

> .reloc being discarded rather than, or perhaps in addition to, this

> patch.


In addition to, yes. This extra hunk would take care:

@@ -1516,7 +1516,7 @@ generate_reloc (bfd *abfd, struct bfd_li
   bfd *b;
   struct bfd_section *s;
 
-  if (reloc_s == NULL)
+  if (reloc_s == NULL || reloc_s->output_section == bfd_abs_section_ptr)
     return;
   total_relocs = 0;
   for (b = info->input_bfds; b; b = b->link.next)

But as it looks pretty unrelated to the purpose of the change and
since likely .edata would want some similar check (albeit there
I'm not sure whether, instead of bailing, we should issue a warning,
as there being exports but .edata getting discarded is unlikely to
be what is intended), I wonder whether this would better be a
separate change. Do you have any suggestion or preference here?

Jan

Patch

--- a/ld/pe-dll.c
+++ b/ld/pe-dll.c
@@ -1627,6 +1627,15 @@  generate_reloc (bfd *abfd, struct bfd_li
 		  reloc_data[total_relocs].vma = sec_vma + relocs[i]->address;
 		  reloc_data[total_relocs].idx = total_relocs;
 
+		  /* Since we're only about to determine .reloc's size,
+		     subsequent output section VMA calculations will shift up
+		     sections at this or higher addresses.  Relocations for
+		     such sections would hence end up not being correct.  */
+		  if (reloc_data[total_relocs].vma
+		      >= reloc_s->output_section->vma)
+		    einfo (_("%P: base relocation for section `%s' above "
+			     ".reloc section\n"), s->output_section->name);
+
 #define BITS_AND_SHIFT(bits, shift) (bits * 1000 | shift)
 
 		  switch BITS_AND_SHIFT (relocs[i]->howto->bitsize,
--- a/ld/testsuite/ld-scripts/pr14962.t
+++ b/ld/testsuite/ld-scripts/pr14962.t
@@ -5,5 +5,6 @@  SECTIONS
   .text : { *(.text .pr) }
   .data : { *(.data .rw) }
   .bss : { *(.bss) }
+  .reloc : { *(.reloc) }
   /DISCARD/ : { *(*) }
 }
--- a/ld/testsuite/ld-scripts/weak.t
+++ b/ld/testsuite/ld-scripts/weak.t
@@ -6,6 +6,9 @@  SECTIONS
   .data 0x2000 : {
     tmpdir/weak2.o(.data)
   }
+  .reloc : {
+     *(.reloc)
+  }
   /DISCARD/ : {
     *(*)
   }