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

[Alan Modra <amodra at gmail dot com>]

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

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;
 	    }
 	}