diff mbox

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

Message ID 20161209033847.GN10584@bubble.grove.modra.org
State New
Headers show

Commit Message

Alan Modra Dec. 9, 2016, 3:38 a.m. UTC 
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

Comments

Maciej W. Rozycki Dec. 10, 2016, 12:13 a.m. UTC | #1 
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
diff mbox

Patch

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