Re: RFC: Verify AT_ENTRY before using it

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

On Fri, 26 Feb 2010 22:12:16 +0100, Daniel Jacobowitz wrote:
>    if (target_auxv_search (&current_target, AT_ENTRY, &entry_point) == 1)
> -    return entry_point - bfd_get_start_address (exec_bfd);
> +    {
> +      /* Verify that the auxilliary vector describes the same file as
> +	 exec_bfd, by comparing their program headers.  If the program
> +	 headers in the auxilliary vector do not match the program
> +	 headers in the executable, then we are looking at a different
> +	 file than the one used by the kernel - for instance, "gdb
> +	 program" connected to "gdbserver :PORT ld.so program".  */
> +      int phdrs_size, phdrs2_size, ok = 1;
> +      gdb_byte *buf, *buf2;
> +
> +      /* Take a shortcut for the common case.  If the entry addresses
> +	 match, then it is incredibly unlikely that anything
> +	 complicated has happened.  It's not impossible, if the loader
> +	 and executable are both PIE, but it would still require a
> +	 rare conjunction of load addresses.  */
> +      if (entry_point == bfd_get_start_address (exec_bfd))
> +	return 0;
> +
> +      if (bfd_get_flavour (abfd) == bfd_target_elf_flavour)

solib-svr4.c:1757: error: ‘abfd’ undeclared (first use in this function)
solib-svr4.c:1757: error: (Each undeclared identifier is reported only once
solib-svr4.c:1757: error: for each function it appears in.)


> +	{
> +	  buf = read_program_header (-1, &phdrs_size, NULL);
> +	  buf2 = read_program_headers_from_bfd (exec_bfd, &phdrs2_size);
> +	  if (buf != NULL && buf2 != NULL
> +	      && (phdrs_size != phdrs2_size
> +		  || memcmp (buf, buf2, phdrs_size) != 0))
> +	    ok = 0;
> +	}
> +
> +      xfree (buf);
> +      xfree (buf2);

solib-svr4.c:1747: error: ‘buf’ may be used uninitialized in this function
solib-svr4.c:1747: note: ‘buf’ was declared here
solib-svr4.c:1747: error: ‘buf2’ may be used uninitialized in this function
solib-svr4.c:1747: note: ‘buf2’ was declared here


> +      if (ok)
> +	return entry_point - bfd_get_start_address (exec_bfd);
> +    }
>  
>    return svr4_static_exec_displacement ();
>  }

The previous alignment-based variant
	Re: [patch] Sanity check PIE displacement #2
	http://sourceware.org/ml/gdb-patches/2010-02/msg00346.html

was returning 0 if the PIE check failed.  It has been done intentionally as
svr4_static_exec_displacement is buggy for the case:

* auxv AT_ENTRY is not found
  (it is present in a live spawned or attached process or even in a core file)
  [ therefore svr4_static_exec_displacement is executed at all ]

* exec_bfd is static (it has no .interp)

* exec_bfd is PIE (=ET_DYN).

* GDB does a process attachment.

Reproducer of the svr4_static_exec_displacement bug was posted as:
	Re: [patch 08/15] PIE: Base functionality
	http://sourceware.org/ml/gdb-patches/2010-01/msg00383.html

I had various wrong assumptions about svr4_static_exec_displacement before
(such as it is for some embedded targets) and so I wanted to keep it in place.
But found now the function svr4_static_exec_displacement comes from:
	[PATCH RFA] solib-svr4.c patch for dynamic executables
	http://sourceware.org/ml/gdb-patches/2000-11/msg00040.html
	af21058b0321e272e6d0ad2877f402b286b0fb18

And its goal was ld.so on GNU/Linux so this functionality has been already
superseded by the current checked-in PIE patchset and
svr4_static_exec_displacement can be safely dropped.

Its advantage was the idea of DYNAMIC (ET_DYN) check - imported it now which
in fact removes any regression possibilities as ET_DYN executables have not
worked in GDB at all before anyway.

Its disadvantage was that it was using regcache_read_pc instead of auxv
AT_ENTRY which did not work for attached processes.


The Daniel J.'s problem of wrong PIE offset has multiple solutions:
	./gdbserver/gdbserver :2222 /lib64/ld-linux-x86-64.so.2 ./gdb -nx
+
	./gdb -nx -ex 'target remote localhost:2222' -ex c ./gdb

(a) The most safe + cheap is the DYNAMIC (ET_DYN) check in this patch taken
    from former variant from year 2000.

(b) Mostly reliable (but being silent in the case of non-matching executable)
    PHDRs checking patch:
    RFC: Verify AT_ENTRY before using it
    http://sourceware.org/ml/gdb-patches/2010-02/msg00612.html

(c) non-matching executable check based on alignment,
    less reliable variant of (b)
    Re: [patch] Sanity check PIE displacement #2
    http://sourceware.org/ml/gdb-patches/2010-02/msg00346.html

(d) Using /lib64/ld-linux-x86-64.so.2 as the argument to the client gdb which
    IMO makes it just whole more correct:
    [patch] Support gdb --args ld.so progname
    http://sourceware.org/ml/gdb-patches/2010-02/msg00647.html

The patch below combines (a)+(c)+(b) (in this order).  It also implements
a warning on non-matching exec_bfd vs. target memory.  (d) is still left as
applicable independent patch for approval.

This patch or some its subset should got for gdb_7_1-branch.

No regressions on {x86_64,x86_64-m32,i686}-fedora12-linux-gnu.


Thanks,
Jan


gdb/
2010-03-01  Jan Kratochvil  <jan.kratochvil@redhat.com>
	    Daniel Jacobowitz  <dan@codesourcery.com>

	* solib-svr4.c (read_program_header): Support type == -1 to read
	all program headers.
	(read_program_headers_from_bfd): New function.
	(svr4_static_exec_displacement): Remove and move the comment ...
	(svr4_exec_displacement): ... here.  Remove variable found.  New
	variable displacement.  Check also DYNAMIC.  Verify DISPLACEMENT
	alignment for ELF targets.  Compare target vs. exec_bfd PHDRs for ELF
	targets using read_program_headers_from_bfd.  Remove the call of
	svr4_static_exec_displacement.

--- a/gdb/solib-svr4.c
+++ b/gdb/solib-svr4.c
@@ -451,6 +451,9 @@ bfd_lookup_symbol (bfd *abfd, char *symname)
 /* Read program header TYPE from inferior memory.  The header is found
    by scanning the OS auxillary vector.
 
+   If TYPE == -1, return the program headers instead of the contents of
+   one program header.
+
    Return a pointer to allocated memory holding the program header contents,
    or NULL on failure.  If sucessful, and unless P_SECT_SIZE is NULL, the
    size of those contents is returned to P_SECT_SIZE.  Likewise, the target
@@ -483,8 +486,13 @@ read_program_header (int type, int *p_sect_size, int *p_arch_size)
   else
     return 0;
 
-  /* Find .dynamic section via the PT_DYNAMIC PHDR.  */
-  if (arch_size == 32)
+  /* Find the requested segment.  */
+  if (type == -1)
+    {
+      sect_addr = at_phdr;
+      sect_size = at_phent * at_phnum;
+    }
+  else if (arch_size == 32)
     {
       Elf32_External_Phdr phdr;
       int i;
@@ -1618,55 +1626,30 @@ svr4_special_symbol_handling (void)
   svr4_relocate_main_executable ();
 }
 
-/* Decide if the objfile needs to be relocated.  As indicated above,
-   we will only be here when execution is stopped at the beginning
-   of the program.  Relocation is necessary if the address at which
-   we are presently stopped differs from the start address stored in
-   the executable AND there's no interpreter section.  The condition
-   regarding the interpreter section is very important because if
-   there *is* an interpreter section, execution will begin there
-   instead.  When there is an interpreter section, the start address
-   is (presumably) used by the interpreter at some point to start
-   execution of the program.
-
-   If there is an interpreter, it is normal for it to be set to an
-   arbitrary address at the outset.  The job of finding it is
-   handled in enable_break().
-
-   So, to summarize, relocations are necessary when there is no
-   interpreter section and the start address obtained from the
-   executable is different from the address at which GDB is
-   currently stopped.
-   
-   [ The astute reader will note that we also test to make sure that
-     the executable in question has the DYNAMIC flag set.  It is my
-     opinion that this test is unnecessary (undesirable even).  It
-     was added to avoid inadvertent relocation of an executable
-     whose e_type member in the ELF header is not ET_DYN.  There may
-     be a time in the future when it is desirable to do relocations
-     on other types of files as well in which case this condition
-     should either be removed or modified to accomodate the new file
-     type.  (E.g, an ET_EXEC executable which has been built to be
-     position-independent could safely be relocated by the OS if
-     desired.  It is true that this violates the ABI, but the ABI
-     has been known to be bent from time to time.)  - Kevin, Nov 2000. ]
-   */
+/* Read the ELF program headers from ABFD.  Return the contents and
+   set *PHDRS_SIZE to the size of the program headers.  */
 
-static CORE_ADDR
-svr4_static_exec_displacement (void)
+static gdb_byte *
+read_program_headers_from_bfd (bfd *abfd, int *phdrs_size)
 {
-  asection *interp_sect;
-  struct regcache *regcache
-    = get_thread_arch_regcache (inferior_ptid, target_gdbarch);
-  CORE_ADDR pc = regcache_read_pc (regcache);
+  Elf_Internal_Ehdr *ehdr;
+  gdb_byte *buf;
 
-  interp_sect = bfd_get_section_by_name (exec_bfd, ".interp");
-  if (interp_sect == NULL 
-      && (bfd_get_file_flags (exec_bfd) & DYNAMIC) != 0
-      && (exec_entry_point (exec_bfd, &exec_ops) != pc))
-    return pc - exec_entry_point (exec_bfd, &exec_ops);
+  ehdr = elf_elfheader (abfd);
 
-  return 0;
+  *phdrs_size = ehdr->e_phnum * ehdr->e_phentsize;
+  if (*phdrs_size == 0)
+    return NULL;
+
+  buf = xmalloc (*phdrs_size);
+  if (bfd_seek (abfd, ehdr->e_phoff, SEEK_SET) != 0
+      || bfd_bread (buf, *phdrs_size, abfd) != *phdrs_size)
+    {
+      xfree (buf);
+      return NULL;
+    }
+
+  return buf;
 }
 
 /* We relocate all of the sections by the same amount.  This
@@ -1688,23 +1671,109 @@ svr4_static_exec_displacement (void)
      memory image of the program during dynamic linking.
 
    The same language also appears in Edition 4.0 of the System V
-   ABI and is left unspecified in some of the earlier editions.  */
+   ABI and is left unspecified in some of the earlier editions.
+
+   Decide if the objfile needs to be relocated.  As indicated above, we will
+   only be here when execution is stopped.  But during attachment PC can be at
+   arbitrary address therefore regcache_read_pc can be misleading (contrary to
+   the auxv AT_ENTRY value).  Moreover for executable with interpreter section
+   regcache_read_pc would point to the interpreter and not the main executable.
+
+   So, to summarize, relocations are necessary when the start address obtained
+   from the executable is different from the address in auxv AT_ENTRY entry.
+   
+   [ The astute reader will note that we also test to make sure that
+     the executable in question has the DYNAMIC flag set.  It is my
+     opinion that this test is unnecessary (undesirable even).  It
+     was added to avoid inadvertent relocation of an executable
+     whose e_type member in the ELF header is not ET_DYN.  There may
+     be a time in the future when it is desirable to do relocations
+     on other types of files as well in which case this condition
+     should either be removed or modified to accomodate the new file
+     type.  - Kevin, Nov 2000. ]  */
 
 static CORE_ADDR
 svr4_exec_displacement (void)
 {
-  int found;
   /* ENTRY_POINT is a possible function descriptor - before
      a call to gdbarch_convert_from_func_ptr_addr.  */
-  CORE_ADDR entry_point;
+  CORE_ADDR entry_point, displacement;
 
   if (exec_bfd == NULL)
     return 0;
 
-  if (target_auxv_search (&current_target, AT_ENTRY, &entry_point) == 1)
-    return entry_point - bfd_get_start_address (exec_bfd);
+  /* Therefore for ELF it is ET_EXEC and not ET_DYN.  Both shared libraries
+     being executed themselves and PIE (Position Independent Executable)
+     executables are ET_DYN.  */
+
+  if ((bfd_get_file_flags (exec_bfd) & DYNAMIC) == 0)
+    return 0;
+
+  if (target_auxv_search (&current_target, AT_ENTRY, &entry_point) <= 0)
+    return 0;
+
+  displacement = entry_point - bfd_get_start_address (exec_bfd);
+
+  /* Verify the DISPLACEMENT candidate complies with the required page
+     alignment.  It is cheaper than the program headers comparison below.  */
+
+  if (bfd_get_flavour (exec_bfd) == bfd_target_elf_flavour)
+    {
+      const struct elf_backend_data *elf = get_elf_backend_data (exec_bfd);
+
+      /* p_align of PT_LOAD segments does not specify any alignment but
+	 only congruency of addresses:
+	   p_offset % p_align == p_vaddr % p_align
+	 Kernel is free to load the executable with lower alignment.  */
+
+      if ((displacement & (elf->minpagesize - 1)) != 0)
+	{
+	  warning (_("PIE (Position Independent Executable) displacement "
+		     "%s is not aligned to the minimal page size %s "
+		     "for \"%s\" (wrong executable or version mismatch?)"),
+		   paddress (target_gdbarch, displacement),
+		   paddress (target_gdbarch, elf->minpagesize),
+		   bfd_get_filename (exec_bfd));
+	  return 0;
+	}
+    }
+
+  /* Verify that the auxilliary vector describes the same file as exec_bfd, by
+     comparing their program headers.  If the program headers in the auxilliary
+     vector do not match the program headers in the executable, then we are
+     looking at a different file than the one used by the kernel - for
+     instance, "gdb program" connected to "gdbserver :PORT ld.so program".  */
+
+  if (bfd_get_flavour (exec_bfd) == bfd_target_elf_flavour)
+    {
+      /* Be optimistic and clear OK only if GDB was able to verify the headers
+	 really do not match.  */
+      int phdrs_size, phdrs2_size, ok = 1;
+      gdb_byte *buf, *buf2;
+
+      buf = read_program_header (-1, &phdrs_size, NULL);
+      buf2 = read_program_headers_from_bfd (exec_bfd, &phdrs2_size);
+      if (buf != NULL && buf2 != NULL
+	  && (phdrs_size != phdrs2_size
+	      || memcmp (buf, buf2, phdrs_size) != 0))
+	ok = 0;
+
+      xfree (buf);
+      xfree (buf2);
+
+      if (!ok)
+	{
+	  warning (_("PIE (Position Independent Executable) displacement "
+		     "%s is ingored as invalid as the target program headers "
+		     "do not match those in file \"%s\" (wrong executable or "
+		     "version mismatch?)"),
+		   paddress (target_gdbarch, displacement),
+		   bfd_get_filename (exec_bfd));
+	  return 0;
+	}
+    }
 
-  return svr4_static_exec_displacement ();
+  return displacement;
 }
 
 /* Relocate the main executable.  This function should be called upon