From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 4133 invoked by alias); 10 Mar 2010 21:11:53 -0000 Received: (qmail 3630 invoked by uid 22791); 10 Mar 2010 21:11:49 -0000 X-SWARE-Spam-Status: No, hits=-7.2 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; Wed, 10 Mar 2010 21:11:43 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o2ALBfbt000480 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Wed, 10 Mar 2010 16:11:41 -0500 Received: from host0.dyn.jankratochvil.net (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id o2ALBdKJ005126 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO) for ; Wed, 10 Mar 2010 16:11:40 -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 o2ALBcM5011549 for ; Wed, 10 Mar 2010 22:11:38 +0100 Received: (from jkratoch@localhost) by host0.dyn.jankratochvil.net (8.14.3/8.14.3/Submit) id o2ALBckX011548 for gdb-patches@sourceware.org; Wed, 10 Mar 2010 22:11:38 +0100 Date: Wed, 10 Mar 2010 21:11:00 -0000 From: Jan Kratochvil To: gdb-patches@sourceware.org Subject: Re: RFC: Verify AT_ENTRY before using it Message-ID: <20100310211138.GA11271@host0.dyn.jankratochvil.net> References: <20100224224913.GA25437@caradoc.them.org> <20100225221620.GA7830@host0.dyn.jankratochvil.net> <20100226211216.GC2630@caradoc.them.org> <20100301200428.GA14079@host0.dyn.jankratochvil.net> <20100308220635.GC2629@caradoc.them.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20100308220635.GC2629@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/msg00395.txt.bz2 On Mon, 08 Mar 2010 23:06:35 +0100, Daniel Jacobowitz wrote: > On Mon, Mar 01, 2010 at 09:04:28PM +0100, Jan Kratochvil wrote: > > 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. > > Thanks for working on this. I agree with all of it except the > warnings. It has been checked-in without the warning() calls. > Since there's no other working way to accomplish (d) [ (d) refers to: [patch] Support gdb --args ld.so progname http://sourceware.org/ml/gdb-patches/2010-02/msg00647.html ] > today, and I have a use case for that which won't go away, I don't > think it's appropriate to warn. Factually I do not understand how the warnings could currently ever be printed with the DYNAMIC/ET_DYN check there (or do you run PIEs with gdbserver?). > Note, if the program headers matched but the alignment was not page > aligned, that would be a different case: that seems worth warning > about. I don't know what that would mean. Is it a case you've > encountered? No. I cannot imagine such a rare case would happen in real world. > Without the warnings, this is OK to check in. If you want the > warnings, we should discuss it a little more. As I (currently) do not use gdbserver where such exec_bfd vs. target memory difference could only happen I am only curious why it did not work. Or do you at least approve simple `if (info_verbose) warning()' patch? I do not have intention to push for the warning() calls more (now). Thanks, Jan http://sourceware.org/ml/gdb-cvs/2010-03/msg00096.html --- src/gdb/ChangeLog 2010/03/10 18:41:37 1.11466 +++ src/gdb/ChangeLog 2010/03/10 20:50:48 1.11467 @@ -1,3 +1,16 @@ +2010-03-10 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. + 2010-03-10 Tom Tromey * dwarf2read.c (struct pubnames_header): Remove. --- src/gdb/solib-svr4.c 2010/03/09 18:09:07 1.127 +++ src/gdb/solib-svr4.c 2010/03/10 20:50:55 1.128 @@ -451,6 +451,9 @@ /* 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 @@ 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; @@ -1625,55 +1633,30 @@ 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); - - 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); + Elf_Internal_Ehdr *ehdr; + gdb_byte *buf; - return 0; + ehdr = elf_elfheader (abfd); + + *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 @@ -1695,23 +1678,93 @@ 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) + 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) + return 0; + } - return svr4_static_exec_displacement (); + return displacement; } /* Relocate the main executable. This function should be called upon