From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22524 invoked by alias); 1 Mar 2010 20:04:41 -0000 Received: (qmail 22259 invoked by uid 22791); 1 Mar 2010 20:04:38 -0000 X-SWARE-Spam-Status: No, hits=-7.0 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 01 Mar 2010 20:04:33 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o21K4VWR021593 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 1 Mar 2010 15:04:31 -0500 Received: from host0.dyn.jankratochvil.net (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id o21K4SxY026122 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO) for ; Mon, 1 Mar 2010 15:04:30 -0500 Received: from host0.dyn.jankratochvil.net (localhost [127.0.0.1]) by host0.dyn.jankratochvil.net (8.14.3/8.14.3) with ESMTP id o21K4SaX030783 for ; Mon, 1 Mar 2010 21:04:28 +0100 Received: (from jkratoch@localhost) by host0.dyn.jankratochvil.net (8.14.3/8.14.3/Submit) id o21K4SeK030782 for gdb-patches@sourceware.org; Mon, 1 Mar 2010 21:04:28 +0100 Date: Mon, 01 Mar 2010 20:04:00 -0000 From: Jan Kratochvil To: gdb-patches@sourceware.org Subject: Re: RFC: Verify AT_ENTRY before using it Message-ID: <20100301200428.GA14079@host0.dyn.jankratochvil.net> References: <20100224224913.GA25437@caradoc.them.org> <20100225221620.GA7830@host0.dyn.jankratochvil.net> <20100226211216.GC2630@caradoc.them.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-2022-jp Content-Disposition: inline In-Reply-To: <20100226211216.GC2630@caradoc.them.org> User-Agent: Mutt/1.5.20 (2009-08-17) X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2010-03/txt/msg00030.txt.bz2 On Fri, 26 Feb 2010 22:12:16 +0100, Daniel Jacobowitz wrote: > if (target_auxv_search (¤t_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 Daniel Jacobowitz * 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 (¤t_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 (¤t_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