Re: [binutils-gdb] Fix the linker so that it will not silently generate ELF binaries with invalid program headers. Fix

Re: [binutils-gdb] Fix the linker so that it will not silently generate ELF binaries with invalid program headers. Fix

[Maciej W. Rozycki]

Hi Nick,

On Wed, 23 Nov 2016, Nick Clifton wrote:

> commit 1a9ccd70f9a75dc6b48d340059f28ef3550c107b
> Author: Nick Clifton <nickc@redhat.com>
> Date:   Wed Nov 23 11:10:39 2016 +0000
> 
>     Fix the linker so that it will not silently generate ELF binaries with invalid program headers.  Fix readelf to report such invalid binaries.

 This change has caused a `mips-vxworks' and `mipsel-vxworks' target 
regression:

FAIL: VxWorks executable test 2 (dynamic)

and I cannot figure out either from the commit description or the lengthy 
Bugzilla discussion whether it is an actual functional VxWorks regression 
or whether we can just adjust the regexps in the test case and consider it 
handled.

 Your commit changed output from this test case from:

Elf file type is EXEC (Executable file)
Entry point 0x80400
There are 5 program headers, starting at offset 52

Program Headers:
  Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
  PHDR           0x000034 0x00001034 0x00000000 0x000a0 0x000a0 R E 0x4
  INTERP         0x001000 0x00080000 0x00080000 0x00013 0x00013 R   0x1
      [Requesting program interpreter: /usr/lib/libc.so.1]
  LOAD           0x001000 0x00080000 0x00080000 0x00408 0x00408 R E 0x1000
  LOAD           0x002000 0x00081000 0x00081000 0x00804 0x00c00 RW  0x1000
  DYNAMIC        0x002000 0x00081000 0x00081000 0x00078 0x00078 RW  0x4

 Section to Segment mapping:
  Segment Sections...
   00
   01     .interp
   02     .interp .hash .dynsym .dynstr .text
   03     .dynamic .got .data .rld_map
   04     .dynamic

to:

Elf file type is EXEC (Executable file)
Entry point 0x80400
There are 5 program headers, starting at offset 52

Program Headers:
  Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
  PHDR           0x000034 0x0007f034 0x0007f034 0x000a0 0x000a0 R E 0x4
  INTERP         0x001000 0x00080000 0x00080000 0x00013 0x00013 R   0x1
      [Requesting program interpreter: /usr/lib/libc.so.1]
  LOAD           0x000000 0x0007f000 0x0007f000 0x01408 0x01408 R E 0x1000
  LOAD           0x002000 0x00081000 0x00081000 0x00804 0x00c00 RW  0x1000
  DYNAMIC        0x002000 0x00081000 0x00081000 0x00078 0x00078 RW  0x4

 Section to Segment mapping:
  Segment Sections...
   00
   01     .interp
   02     .interp .hash .dynsym .dynstr .text
   03     .dynamic .got .data .rld_map
   04     .dynamic

This places the first LOAD segment below the location set with the linker 
script, which I fear may affect run-time interpretation.  The test case 
has the addresses expected spelled out in full for a reason I suppose.

 Please help with resolving this problem.

  Maciej

Re: [binutils-gdb] Fix the linker so that it will not silently generate ELF binaries with invalid program headers. Fix

[Alan Modra]

On Thu, Dec 08, 2016 at 09:30:04AM +0000, Maciej W. Rozycki wrote:
> Program Headers:
>   Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
>   PHDR           0x000034 0x00001034 0x00000000 0x000a0 0x000a0 R E 0x4
>   INTERP         0x001000 0x00080000 0x00080000 0x00013 0x00013 R   0x1
>       [Requesting program interpreter: /usr/lib/libc.so.1]
>   LOAD           0x001000 0x00080000 0x00080000 0x00408 0x00408 R E 0x1000
>   LOAD           0x002000 0x00081000 0x00081000 0x00804 0x00c00 RW  0x1000
>   DYNAMIC        0x002000 0x00081000 0x00081000 0x00078 0x00078 RW  0x4

The gABI says:

PT_PHDR
    The array element, if present, specifies the location and size of
    the program header table itself, both in the file and in the
    memory image of the program. This segment type may not occur more
    than once in a file. Moreover, it may occur only if the program
    header table is part of the memory image of the program. If it is
    present, it must precede any loadable segment entry.

The above clearly violates this part of the spec because PT_PHDR is
present yet is not part of the memory image.

Nick's patch forced the first PT_LOAD to cover the program headers.  I
think an equally valid and somewhat better fix would have been to not
emit PT_PHDR when no PT_LOAD header covers the program headers.  The
reason I say that is because PT_PHDR is optional.  A loader can read
the program headers itself from file using info in the ELF header.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [binutils-gdb] Fix the linker so that it will not silently generate ELF binaries with invalid program headers. Fix

[Nick Clifton]

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

Hi Maciej,

> Program Headers:
>   Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
>   PHDR           0x000034 0x00001034 0x00000000 0x000a0 0x000a0 R E 0x4
>   INTERP         0x001000 0x00080000 0x00080000 0x00013 0x00013 R   0x1
>       [Requesting program interpreter: /usr/lib/libc.so.1]
>   LOAD           0x001000 0x00080000 0x00080000 0x00408 0x00408 R E 0x1000
>   LOAD           0x002000 0x00081000 0x00081000 0x00804 0x00c00 RW  0x1000
>   DYNAMIC        0x002000 0x00081000 0x00081000 0x00078 0x00078 RW  0x4

The problem with this layout is that the ELF specification says in its description
of the program header types:

  PT_PHDR  The array element, if present, specifies the location 
           and size of the program header table itself, both in 
           the file and in the memory image of the program. This 
           segment type may not occur more than once in a file.
           Moreover, it may occur only if the program header table 
           is part of the memory image of the program. If it is
           present, it must precede any loadable segment entry.

Hence, if a PT_PHDR segment is present in a binary it must also be part of
a PT_LOAD segment, so that it gets loaded into the runtime memory image of
the executable.

The layout above breaks this requirement by placing the PT_PHDR segment at
0x1034 but starting the first load segment at 0x80000.

> Program Headers:
>   Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
>   PHDR           0x000034 0x0007f034 0x0007f034 0x000a0 0x000a0 R E 0x4
>   INTERP         0x001000 0x00080000 0x00080000 0x00013 0x00013 R   0x1
>       [Requesting program interpreter: /usr/lib/libc.so.1]
>   LOAD           0x000000 0x0007f000 0x0007f000 0x01408 0x01408 R E 0x1000
>   LOAD           0x002000 0x00081000 0x00081000 0x00804 0x00c00 RW  0x1000
>   DYNAMIC        0x002000 0x00081000 0x00081000 0x00078 0x00078 RW  0x4

This is the linker's attempt to resolve the problem, by moving the start
address of the first LOAD segment down to 0x7f0000.  (The linker cannot just
create another LOAD segment just for the program headers as there is code
inside the linker that will discard any LOAD segments that do not have any
sections mapped into them).


> This places the first LOAD segment below the location set with the linker 
> script, which I fear may affect run-time interpretation.  The test case 
> has the addresses expected spelled out in full for a reason I suppose.

True.  You could fix the problem by adding --no-dynamic-linker to the test's
linker command line, since then the linker will not create PHDR or INTERP
segments, and so the LOAD segments will have the expected addresses.  I doubt
if you want this solution however as I assume that the whole point of this
test is to create a working dynamic executable.

The correct solution, I think, is to define the program headers in the linker
script, so that their placement can be specifically controlled by the user.
See the vxworks1.ld script attached, which does this.

But...this causes a problem for the "VxWorks executable test 2 (static)" test
as now there is a DYNAMIC segment in the program headers but no .dynamic
section in the section headers, and readelf complains.  *sigh*  So you need
a second linker script, just for static executables.

So the result is the attached patch, which I think resolves the problem.  (For
MIPS based vxworks targets anyway.  It looks like similar changes may be needed
for other vxworks architectures).  What do you think ?

Cheers
  Nick

[-- Attachment #2: vxworks1.ld --]
[-- Type: text/plain, Size: 815 bytes --]

PHDRS
{
  headers      PT_PHDR PHDRS;
  header_load  PT_LOAD PHDRS;
  interp       PT_INTERP;
  code         PT_LOAD;
  data         PT_LOAD;
  dynamic      PT_DYNAMIC;
}

SECTIONS
{
  . = 0x80000;
  . = . + SIZEOF_HEADERS;
  .interp : { *(.interp) } :interp :headers :header_load
  .hash : { *(.hash) } :code
  .dynsym : { *(.dynsym) }
  .dynstr : { *(.dynstr) }

  . = ALIGN (0x400);
  .rela.dyn : { *(.rela.dyn) }
  .rela.plt : { *(.rela.plt) }

  . = ALIGN (0x400);
  .plt : { *(.plt) }

  . = ALIGN (0x400);
  .text : { *(.text) }

  . = ALIGN (0x1000);
  .dynamic : { *(.dynamic) } :data :dynamic

  . = ALIGN (0x400);
  .got : { *(.got.plt) *(.got) } :data

  . = ALIGN (0x400);
  .data : { *(.data) }

  . = ALIGN (0x400);
  .bss : { *(.bss) *(.dynbss) }


  /DISCARD/ : { *(.reginfo) *(.MIPS.abiflags) }
}

[-- Attachment #3: mips.vxworks.ld-tests.patch --]
[-- Type: text/x-patch, Size: 2635 bytes --]

diff --git a/ld/testsuite/ld-mips-elf/mips-elf.exp b/ld/testsuite/ld-mips-elf/mips-elf.exp
index fd79e80..b12d1b7 100644
--- a/ld/testsuite/ld-mips-elf/mips-elf.exp
+++ b/ld/testsuite/ld-mips-elf/mips-elf.exp
@@ -37,7 +37,7 @@ if {[istarget "mips*-*-vxworks"]} {
 	 {{readelf --segments vxworks2.sd}}
 	 "vxworks2"}
 	{"VxWorks executable test 2 (static)"
-	 "-Tvxworks1.ld" ""
+	 "-Tvxworks2-static.ld" ""
 	 "-mips2" {vxworks2.s}
 	 {{readelf --segments vxworks2-static.sd}}
 	 "vxworks2"}
diff --git a/ld/testsuite/ld-mips-elf/vxworks1.ld b/ld/testsuite/ld-mips-elf/vxworks1.ld
index d9f8621..428564c 100644
--- a/ld/testsuite/ld-mips-elf/vxworks1.ld
+++ b/ld/testsuite/ld-mips-elf/vxworks1.ld
@@ -1,8 +1,19 @@
+PHDRS
+{
+  headers      PT_PHDR PHDRS;
+  header_load  PT_LOAD PHDRS;
+  interp       PT_INTERP;
+  code         PT_LOAD;
+  data         PT_LOAD;
+  dynamic      PT_DYNAMIC;
+}
+
 SECTIONS
 {
   . = 0x80000;
-  .interp : { *(.interp) }
-  .hash : { *(.hash) }
+  . = . + SIZEOF_HEADERS;
+  .interp : { *(.interp) } :interp :headers :header_load
+  .hash : { *(.hash) } :code
   .dynsym : { *(.dynsym) }
   .dynstr : { *(.dynstr) }
 
@@ -17,10 +28,10 @@ SECTIONS
   .text : { *(.text) }
 
   . = ALIGN (0x1000);
-  .dynamic : { *(.dynamic) }
+  .dynamic : { *(.dynamic) } :data :dynamic
 
   . = ALIGN (0x400);
-  .got : { *(.got.plt) *(.got) }
+  .got : { *(.got.plt) *(.got) } :data
 
   . = ALIGN (0x400);
   .data : { *(.data) }
@@ -28,5 +39,6 @@ SECTIONS
   . = ALIGN (0x400);
   .bss : { *(.bss) *(.dynbss) }
 
+
   /DISCARD/ : { *(.reginfo) *(.MIPS.abiflags) }
 }
diff --git a/ld/testsuite/ld-mips-elf/vxworks2.sd b/ld/testsuite/ld-mips-elf/vxworks2.sd
index 5ff87d3..01301ba 100644
--- a/ld/testsuite/ld-mips-elf/vxworks2.sd
+++ b/ld/testsuite/ld-mips-elf/vxworks2.sd
@@ -6,8 +6,8 @@ Program Headers:
   Type .*
   PHDR .*
 #...
-  LOAD .* 0x00080000 0x00080000 .* R E 0x1000
-  LOAD .* 0x00081000 0x00081000 .* RW  0x1000
+  LOAD .* 0x00080... 0x00080... .* R E 0x1000
+  LOAD .* 0x00081... 0x00081... .* RW  0x1000
   DYNAMIC .*
 
 #...
--- /dev/null	2016-12-08 07:35:33.885870158 +0000
+++ ld/testsuite/ld-mips-elf/vxworks2-static.ld	2016-12-08 12:04:17.091239541 +0000
@@ -0,0 +1,25 @@
+SECTIONS
+{
+  . = 0x80000;
+  .hash : { *(.hash) }
+
+  . = ALIGN (0x400);
+  .rela.plt : { *(.rela.plt) }
+
+  . = ALIGN (0x400);
+  .plt : { *(.plt) }
+
+  . = ALIGN (0x400);
+  .text : { *(.text) }
+
+  . = ALIGN (0x1000);
+  .got : { *(.got.plt) *(.got) }
+
+  . = ALIGN (0x400);
+  .data : { *(.data) }
+
+  . = ALIGN (0x400);
+  .bss : { *(.bss) *(.dynbss) }
+
+  /DISCARD/ : { *(.reginfo) *(.MIPS.abiflags) }
+}

Re: [binutils-gdb] Fix the linker so that it will not silently generate ELF binaries with invalid program headers. Fix

[Nick Clifton]

Hi Alan,

> I think an equally valid and somewhat better fix would have been to not
> emit PT_PHDR when no PT_LOAD header covers the program headers.  The
> reason I say that is because PT_PHDR is optional.  A loader can read
> the program headers itself from file using info in the ELF header.

That would be nicer, but there is explicit code in _bfd_elf_map_sections_to_segments()
to create a PHDR segment whenever we create an INTERP segment.  I assumed
that this was a requirement and hence could not be dropped.  (I did not
explore why this was needed, I just assumed that I would break something,
probably the kernel, if I dropped the requirement).

Cheers
  Nick

Re: [binutils-gdb] Fix the linker so that it will not silently generate ELF binaries with invalid program headers. Fix

[Alan Modra]

On Thu, Dec 08, 2016 at 12:14:49PM +0000, Nick Clifton wrote:
> Hi Alan,
> 
> > I think an equally valid and somewhat better fix would have been to not
> > emit PT_PHDR when no PT_LOAD header covers the program headers.  The
> > reason I say that is because PT_PHDR is optional.  A loader can read
> > the program headers itself from file using info in the ELF header.
> 
> That would be nicer, but there is explicit code in _bfd_elf_map_sections_to_segments()
> to create a PHDR segment whenever we create an INTERP segment.  I assumed
> that this was a requirement and hence could not be dropped.  (I did not
> explore why this was needed, I just assumed that I would break something,
> probably the kernel, if I dropped the requirement).

OK, so I experimented with a patch that omits PHDR when a script
doesn't leave enough room, and "hello world" no longer runs on Ubuntu
16.04.  Segfault in dl_main.  So it seems INTERP really does need
PHDR, at least with current glibc ld.so.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [binutils-gdb] Fix the linker so that it will not silently generate ELF binaries with invalid program headers. Fix

[Maciej W. Rozycki]

On Thu, 8 Dec 2016, Alan Modra wrote:

> > Program Headers:
> >   Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
> >   PHDR           0x000034 0x00001034 0x00000000 0x000a0 0x000a0 R E 0x4
> >   INTERP         0x001000 0x00080000 0x00080000 0x00013 0x00013 R   0x1
> >       [Requesting program interpreter: /usr/lib/libc.so.1]
> >   LOAD           0x001000 0x00080000 0x00080000 0x00408 0x00408 R E 0x1000
> >   LOAD           0x002000 0x00081000 0x00081000 0x00804 0x00c00 RW  0x1000
> >   DYNAMIC        0x002000 0x00081000 0x00081000 0x00078 0x00078 RW  0x4
> 
> The gABI says:
> 
> PT_PHDR
>     The array element, if present, specifies the location and size of
>     the program header table itself, both in the file and in the
>     memory image of the program. This segment type may not occur more
>     than once in a file. Moreover, it may occur only if the program
>     header table is part of the memory image of the program. If it is
>     present, it must precede any loadable segment entry.
> 
> The above clearly violates this part of the spec because PT_PHDR is
> present yet is not part of the memory image.

 Well, TBH I think it's all but clear, given that nowhere in the ELF gABI 
that I can find there is an actual definition of what "the memory image" 
is.

 It might be possible to prove it by contradiction from the "Base Address" 
section, where it is specified that it is `p_vaddr' of the lowest PT_LOAD 
segment that is used to calculate the base address or the relocation 
amount for the whole image, that no other segment type is a part of the 
memory image without a corresponding PT_LOAD shadow.  But I wouldn't be 
that convinced about it as arguably the definition does not actually 
*require* that there is no mapped area below the lowest PT_LOAD segment -- 
it merely specifies the calculation algorithm which needs a point of 
reference, and which may well apply to any other segment types, even if 
mapped below the lowest PT_LOAD segment.

 It may also be important to note that based on the change bars in the 
latest 4.1 PDF version of the gABI it can be inferred that the "Base 
Address" section was updated relatively late in the course of the 
document's processing and it cannot be completely ruled out that parts 
elsewhere were missed out and not revised accordingly.

 Contrariwise, it is specified that `p_memsz' gives "the number of bytes 
in the memory image of the segment" without limiting it to PT_LOAD 
segments, so one could argue all segments whose this value is non-zero 
comprise the memory image, regardless of whether they are shadowed by a 
PT_LOAD segment or not.

 Intuitively I think that you are correct in that it was the intent of the 
gABI authors that only PT_LOAD segments comprise the memory image, but I 
also think that intuition and guessing the intent is not the right way of 
interpreting a technical specification, as intuition varies among people 
and some implementers may have understood the document differently and 
still be formally right in their interpretation.

> Nick's patch forced the first PT_LOAD to cover the program headers.  I
> think an equally valid and somewhat better fix would have been to not
> emit PT_PHDR when no PT_LOAD header covers the program headers.  The
> reason I say that is because PT_PHDR is optional.  A loader can read
> the program headers itself from file using info in the ELF header.

 There may be no loader available to load the program headers or the ELF 
file header if a bare-metal ELF image has been externally loaded according 
to some interpretation of the headers, and the binary wants to access its 
own structures at run time.  Existing environments may rely on our current 
semantics even if it is not strictly compliant.

 I think my inability to actually decide which way is correct for MIPS 
VxWorks (for which I have no environment available to verify) just shows 
that changing semantics which we had since forever just to match a piece 
of (virtual) paper from 20 years ago may not be the greatest idea, and 
certainly needs a lot of care and consideration if it is given a go-ahead.  
There may well be environments out there which have relied on our 
technically broken semantics for an equally long period of time and we may 
well have thus established the de-facto standard.  There may be parts of 
the environments we have no control over and which may be difficult to 
correct to match the written standard.

 I mean if there is a case of actual breakage that our implementation 
causes, as it has been in this case, then we certainly do ought to address 
it, one way or another.  So we found a way to fix Linux, but are we 
confident it didn't break anything else?  Maybe we need a different fix.  
Or at least a linker script or command-line option to chicken out and 
resort to the old semantics?

 But perhaps I'm just being overly cautious.  So is anyone out there who 
could actually regression-test say GCC with current binutils on MIPS 
VxWorks, or run whatever is usually done for toolchain validation with 
that target?  After all what matters is whether the environment continues 
operating correctly and not whether a cryptic test case from 10 years ago 
passes unchanged or not.  It might be that the test case is too strict or 
genuinely wrong in some way.

  Maciej

Re: [binutils-gdb] Fix the linker so that it will not silently generate ELF binaries with invalid program headers. Fix

[Alan Modra]

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

On Fri, Dec 09, 2016 at 12:17:31AM +0000, Maciej W. Rozycki wrote:
> On Thu, 8 Dec 2016, Alan Modra wrote:
> 
> > > Program Headers:
> > >   Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
> > >   PHDR           0x000034 0x00001034 0x00000000 0x000a0 0x000a0 R E 0x4
> > >   INTERP         0x001000 0x00080000 0x00080000 0x00013 0x00013 R   0x1
> > >       [Requesting program interpreter: /usr/lib/libc.so.1]
> > >   LOAD           0x001000 0x00080000 0x00080000 0x00408 0x00408 R E 0x1000
> > >   LOAD           0x002000 0x00081000 0x00081000 0x00804 0x00c00 RW  0x1000
> > >   DYNAMIC        0x002000 0x00081000 0x00081000 0x00078 0x00078 RW  0x4
> > 
> > The gABI says:
> > 
> > PT_PHDR
> >     The array element, if present, specifies the location and size of
> >     the program header table itself, both in the file and in the
> >     memory image of the program. This segment type may not occur more
> >     than once in a file. Moreover, it may occur only if the program
> >     header table is part of the memory image of the program. If it is
> >     present, it must precede any loadable segment entry.
> > 
> > The above clearly violates this part of the spec because PT_PHDR is
> > present yet is not part of the memory image.
> 
>  Well, TBH I think it's all but clear, given that nowhere in the ELF gABI 
> that I can find there is an actual definition of what "the memory image" 
> is.

I'm not going to argue this point except to note that all specs I've
ever read rely on some background knowledge and require
interpretation.  Those that try to specify endless detail might
satisfy lawyers, but often become so unwieldy that they aren't as
useful as they should be to engineers.

> > Nick's patch forced the first PT_LOAD to cover the program headers.  I
> > think an equally valid and somewhat better fix would have been to not
> > emit PT_PHDR when no PT_LOAD header covers the program headers.  The
> > reason I say that is because PT_PHDR is optional.  A loader can read
> > the program headers itself from file using info in the ELF header.
> 
>  There may be no loader available to load the program headers or the ELF 
> file header if a bare-metal ELF image has been externally loaded according 
> to some interpretation of the headers, and the binary wants to access its 
> own structures at run time.  Existing environments may rely on our current 
> semantics even if it is not strictly compliant.

This is true but I was arguing about the validity of the ELF file.  I
mentioned a loader only to say what might be done.

We do know that on reasonably up to date Linux with glibc, that
linking a simple program with a script that leaves no room for headers
results in a segfault before Nick's patch, but runs after Nick's
patch.  My suggestion to remove PHDR (patch attached) results in the
same ld.so segfault.  It would certainly be useful to know how vxworks
behaves with similar tests.

We've also found that Nick's patch broke linux kernel builds, which is
why I was exploring alternative fixes.

-- 
Alan Modra
Australia Development Lab, IBM

[-- Attachment #2: dropphdr.diff --]
[-- Type: text/x-diff, Size: 1572 bytes --]

diff --git a/bfd/elf.c b/bfd/elf.c
index 678c043..d7fbca1 100644
--- a/bfd/elf.c
+++ b/bfd/elf.c
@@ -4507,7 +4507,6 @@ _bfd_elf_map_sections_to_segments (bfd *abfd, struct bfd_link_info *info)
       asection *dynsec, *eh_frame_hdr;
       bfd_size_type amt;
       bfd_vma addr_mask, wrap_to = 0;
-      bfd_boolean linker_created_pt_phdr_segment = FALSE;
 
       /* Select the allocated sections, and sort them.  */
 
@@ -4560,7 +4559,6 @@ _bfd_elf_map_sections_to_segments (bfd *abfd, struct bfd_link_info *info)
 	  m->p_flags = PF_R | PF_X;
 	  m->p_flags_valid = 1;
 	  m->includes_phdrs = 1;
-	  linker_created_pt_phdr_segment = TRUE;
 	  *pm = m;
 	  pm = &m->next;
 
@@ -4612,17 +4610,13 @@ _bfd_elf_map_sections_to_segments (bfd *abfd, struct bfd_link_info *info)
 		  < phdr_size % maxpagesize)
 	      || (sections[0]->lma & addr_mask & -maxpagesize) < wrap_to)
 	    {
+	      phdr_in_segment = FALSE;
 	      /* PR 20815: The ELF standard says that a PT_PHDR segment, if
 		 present, must be included as part of the memory image of the
 		 program.  Ie it must be part of a PT_LOAD segment as well.
-		 If we have had to create our own PT_PHDR segment, but it is
-		 not going to be covered by the first PT_LOAD segment, then
-		 force the inclusion if we can...  */
-	      if ((abfd->flags & D_PAGED) != 0
-		  && linker_created_pt_phdr_segment)
-		phdr_in_segment = TRUE;
-	      else
-		phdr_in_segment = FALSE;
+		 If PHDR won't be part of the memory image, remove it.  */
+	      if (mfirst->p_type == PT_PHDR)
+		mfirst = mfirst->next;
 	    }
 	}
 

Re: [binutils-gdb] Fix the linker so that it will not silently generate ELF binaries with invalid program headers. Fix

[Maciej W. Rozycki]

On Fri, 9 Dec 2016, Alan Modra wrote:

> >  Well, TBH I think it's all but clear, given that nowhere in the ELF gABI 
> > that I can find there is an actual definition of what "the memory image" 
> > is.
> 
> I'm not going to argue this point except to note that all specs I've
> ever read rely on some background knowledge and require
> interpretation.  Those that try to specify endless detail might
> satisfy lawyers, but often become so unwieldy that they aren't as
> useful as they should be to engineers.

 True, we've been applying common sense over the years to the original 
MIPS SVR psABI as well, which has areas which are grey to say the least 
if not outright broken.  All I have written is that I think we need to 
be careful stating that something is clear where indeed I think we can 
have serious doubts about it.

 There is this also this note in the "Program Header" section:

"Some entries describe process segments; others give supplementary 
information and do not contribute to the process image."

-- if that said say:

"Loadable entries describe process segments..."

or something similar to the same effect, then it would make things 
clear about PT_LOAD segments being *the* process image, at least to me.

 Otherwise we can of course make a *decision* to interpret a 
specification one way or another, or declare that our (or someone 
else's) implementation has become the de-facto standard, and then follow 
that choice.  I think it would also be great to have such decisions and 
any backing analysis recorded with commits themselves as well, now that 
we've departed from the "ChangeLog entry only for commit logs" rule.

 NB I'm not arguing about the actual decision made here, it does seem 
reasonable to me.

> >  There may be no loader available to load the program headers or the ELF 
> > file header if a bare-metal ELF image has been externally loaded according 
> > to some interpretation of the headers, and the binary wants to access its 
> > own structures at run time.  Existing environments may rely on our current 
> > semantics even if it is not strictly compliant.
> 
> This is true but I was arguing about the validity of the ELF file.  I
> mentioned a loader only to say what might be done.
> 
> We do know that on reasonably up to date Linux with glibc, that
> linking a simple program with a script that leaves no room for headers
> results in a segfault before Nick's patch, but runs after Nick's
> patch.  My suggestion to remove PHDR (patch attached) results in the
> same ld.so segfault.  It would certainly be useful to know how vxworks
> behaves with similar tests.
> 
> We've also found that Nick's patch broke linux kernel builds, which is
> why I was exploring alternative fixes.

 Thanks for your and Nick's effort; I'll be looking into these patches 
once I have offloaded my immediately pending queue, which has to take 
precedence over any VxWorks cleanups.  As I say, what would really help 
is if someone who has a suitable test environment available verified the 
recent PHDR changes, and if they are indeed fine, then we can then deal 
with test suite adjustments ourselves.

  Maciej

Re: [binutils-gdb] Fix the linker so that it will not silently generate ELF binaries with invalid program headers. Fix

[Alan Modra]

On Sat, Dec 10, 2016 at 12:13:46AM +0000, Maciej W. Rozycki wrote:
>  Thanks for your and Nick's effort; I'll be looking into these patches 
> once I have offloaded my immediately pending queue, which has to take 

I should note that the patch I posted was just a quick hack to
experiment with removing the PT_PHDR header.  A proper patch minus
testsuite tweaking is more like the following, due to elf-nacl.c
fiddling with segments.

diff --git a/bfd/elf.c b/bfd/elf.c
index 678c043..9c8a167 100644
--- a/bfd/elf.c
+++ b/bfd/elf.c
@@ -4483,6 +4483,7 @@ _bfd_elf_map_sections_to_segments (bfd *abfd, struct bfd_link_info *info)
   asection **sections = NULL;
   const struct elf_backend_data *bed = get_elf_backend_data (abfd);
   bfd_boolean no_user_phdrs;
+  bfd_boolean phdr_in_segment = TRUE;
 
   no_user_phdrs = elf_seg_map (abfd) == NULL;
 
@@ -4500,14 +4501,12 @@ _bfd_elf_map_sections_to_segments (bfd *abfd, struct bfd_link_info *info)
       unsigned int phdr_index;
       bfd_vma maxpagesize;
       asection **hdrpp;
-      bfd_boolean phdr_in_segment = TRUE;
       bfd_boolean writable;
       int tls_count = 0;
       asection *first_tls = NULL;
       asection *dynsec, *eh_frame_hdr;
       bfd_size_type amt;
       bfd_vma addr_mask, wrap_to = 0;
-      bfd_boolean linker_created_pt_phdr_segment = FALSE;
 
       /* Select the allocated sections, and sort them.  */
 
@@ -4560,7 +4559,6 @@ _bfd_elf_map_sections_to_segments (bfd *abfd, struct bfd_link_info *info)
 	  m->p_flags = PF_R | PF_X;
 	  m->p_flags_valid = 1;
 	  m->includes_phdrs = 1;
-	  linker_created_pt_phdr_segment = TRUE;
 	  *pm = m;
 	  pm = &m->next;
 
@@ -4611,19 +4609,7 @@ _bfd_elf_map_sections_to_segments (bfd *abfd, struct bfd_link_info *info)
 	      || ((sections[0]->lma & addr_mask) % maxpagesize
 		  < phdr_size % maxpagesize)
 	      || (sections[0]->lma & addr_mask & -maxpagesize) < wrap_to)
-	    {
-	      /* PR 20815: The ELF standard says that a PT_PHDR segment, if
-		 present, must be included as part of the memory image of the
-		 program.  Ie it must be part of a PT_LOAD segment as well.
-		 If we have had to create our own PT_PHDR segment, but it is
-		 not going to be covered by the first PT_LOAD segment, then
-		 force the inclusion if we can...  */
-	      if ((abfd->flags & D_PAGED) != 0
-		  && linker_created_pt_phdr_segment)
-		phdr_in_segment = TRUE;
-	      else
-		phdr_in_segment = FALSE;
-	    }
+	    phdr_in_segment = FALSE;
 	}
 
       for (i = 0, hdrpp = sections; i < count; i++, hdrpp++)
@@ -4965,8 +4951,26 @@ _bfd_elf_map_sections_to_segments (bfd *abfd, struct bfd_link_info *info)
   if (!elf_modify_segment_map (abfd, info, no_user_phdrs))
     return FALSE;
 
+  phdr_in_segment = FALSE;
   for (count = 0, m = elf_seg_map (abfd); m != NULL; m = m->next)
-    ++count;
+    {
+      if (m->p_type == PT_LOAD && m->includes_phdrs)
+	phdr_in_segment = TRUE;
+      ++count;
+    }
+  if (no_user_phdrs && !phdr_in_segment && count != 0)
+    {
+      /* PR 20815: The ELF standard says that a PT_PHDR segment, if
+	 present, must be included as part of the memory image of the
+	 program.  Ie it must be part of a PT_LOAD segment as well.
+	 If PHDR won't be part of the memory image, remove it.  */
+      m = elf_seg_map (abfd);
+      if (m->p_type == PT_PHDR)
+	{
+	  elf_seg_map (abfd) = m->next;
+	  --count;
+	}
+    }
   elf_program_header_size (abfd) = count * bed->s->sizeof_phdr;
 
   return TRUE;

-- 
Alan Modra
Australia Development Lab, IBM