From patchwork Mon Jul 4 17:12:36 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Carlos O'Donell X-Patchwork-Id: 13623 Received: (qmail 32478 invoked by alias); 4 Jul 2016 17:12:52 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 32464 invoked by uid 89); 4 Jul 2016 17:12:52 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.4 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.2 spammy=ls, sorting, Case, tactic X-HELO: mail-qk0-f180.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:to:from:subject:organization:cc:message-id:date :user-agent:mime-version:content-transfer-encoding; bh=VaN7TDj9HbXbMUFI11wsZR6wC5Nfhnc+Pjkr6z+p+Rs=; b=JhwTHJSycgFY0xPlG8jA4hhV+uTtW8oPWMx5WLi3Bhmc+AYlZ7QHB6eldp9roMCMny FMx068vv7IlObfssdjG/8O30zlVWo3135PqFlWH5hMf7giClmvO0J8yV9h7J0/zK2uPK nk9cSW1vcN7p9o3S5Q+sDJ9cM9oywB1y4Vt3SHDxDIRk8w6FCB5Sh+GEkP6xZO/fHKrH frAmbIWvuMeyA4L7rOjhqU/lqVfU9BuqGD0G2nBaowTA2aiKQIhnJge4Snz/hT2iXtwI uIdwjjIRXamIcm33yxE/bot8JwatmZbz53zCjpXBzNB7veji2Ah5l4ClCimmOkDl4XvN Nt+w== X-Gm-Message-State: ALyK8tIFwXX5rgMTw01eXNXK/fC4tqlADIWpZqva9mtCey/vbqrIWhMKDN0XuL4Gs7KftcSq X-Received: by 10.55.157.139 with SMTP id g133mr16617238qke.107.1467652359113; Mon, 04 Jul 2016 10:12:39 -0700 (PDT) To: GNU C Library From: Carlos O'Donell Subject: [PATCH] BZ#20292: Remove redundant statement in _dl_addr_inside_object (refactor and unit test). Cc: Florian Weimer Message-ID: Date: Mon, 4 Jul 2016 13:12:36 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 MIME-Version: 1.0 The following patch refactors two copies of _dl_addr_inside_object into dl-addr-obj.c, and removes the redundant comparison detected by static analysis. The remaining statement: "reladdr - l->l_phdr[n].p_vaddr < l->l_phdr[n].p_memsz" is always sufficient to detect if the unsigned address is inside the load segment of the link map. The copy in dl-addr-obj.c is then subjected to a dynamically linked PIE test where the test driver is linked directly with the built *.os version of the file and the operations exercised. Note that the refactoring creates an additional object file elf/rtld-dl-addr-obj.os needed by the dynamic linker, and an elf/dl-addr-obj.os needed by libc. We test the latter for conformance with the expected behaviours. The goal behind the unit test is _not_ to find bugs, but to exercise the expected behavior of the function. Integration tests are where we should focus our bug finding. Instead the tests here serve to make it precisely clear what the functions should or should not do. I plan to follow the same tactic with the ELF sorting functions and dependency sorting functions (not for this release cycle though). I'll check this fix in before the end of the week if nobody objects. 2016-07-04 Carlos O'Donell [BZ #20292] * elf/Makefile (routines): Add dl-addr-obj. [ifeq (yesyes,$(have-fpie)$(build-shared))] (tests): Add tst-_dl_addr_inside_object. [ifeq (yesyes,$(have-fpie)$(build-shared))] (tests-pie): Likewise. [ifeq (yesyes,$(have-fpie)$(build-shared))] ($(objpfx)tst-_dl_addr_inside_object): Add $(objpfx)dl-addr-obj.os. [ifeq (yesyes,$(have-fpie)$(build-shared))] (CFLAGS-tst-_dl_addr_inside_object.c): Add $(PIE-ccflag). * elf/dl-addr.c: Remove _dl_addr_inside_object function. * elf/dl-open.c: Likewise. * elf/dl-addr-obj.c: New file. * elf/tst-_dl_addr_inside_object.c: New file. diff --git a/elf/Makefile b/elf/Makefile index 210dde9..0b8dde9 100644 --- a/elf/Makefile +++ b/elf/Makefile @@ -23,7 +23,7 @@ include ../Makeconfig headers = elf.h bits/elfclass.h link.h bits/link.h routines = $(all-dl-routines) dl-support dl-iteratephdr \ - dl-addr enbl-secure dl-profstub \ + dl-addr dl-addr-obj enbl-secure dl-profstub \ dl-origin dl-libc dl-sym dl-tsd dl-sysdep # The core dynamic linking functions are in libc for the static and @@ -312,6 +312,13 @@ tests-special += $(objpfx)tst-prelink-cmp.out endif endif +ifeq (yesyes,$(have-fpie)$(build-shared)) +tests += tst-_dl_addr_inside_object +tests-pie += tst-_dl_addr_inside_object +$(objpfx)tst-_dl_addr_inside_object: $(objpfx)dl-addr-obj.os +CFLAGS-tst-_dl_addr_inside_object.c += $(PIE-ccflag) +endif + include ../Rules ifeq (yes,$(build-shared)) diff --git a/elf/dl-addr-obj.c b/elf/dl-addr-obj.c new file mode 100644 index 0000000..3982642 --- /dev/null +++ b/elf/dl-addr-obj.c @@ -0,0 +1,71 @@ +/* Determine if address is inside object load segments. + Copyright (C) 1996-2016 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + . */ + +#include +#include + +/* Return non-zero if ADDR lies within one of L's loadable segments. + We have three cases we care about and we need to avoid overflow. + + Case 1: addr is above a segment. + +==================+<- l_map_end + | |<- addr + |------------------|<- l_addr + p_vaddr + p_memsz + | | + | | + |------------------|<- l_addr + p_vaddr + |------------------|<- l_addr + | | + +==================+<- l_map_start + + Case 2: addr is within a segments. + +==================+<- l_map_end + | | + |------------------|<- l_addr + p_vaddr + p_memsz + | |<- addr + | | + |------------------|<- l_addr + p_vaddr + |------------------|<- l_addr + | | + +==================+<- l_map_start + + Case 3: addr is below a segments. + +==================+<- l_map_end + | | + |------------------|<- l_addr + p_vaddr + p_memsz + | | + | | + |------------------|<- l_addr + p_vaddr + |------------------|<- l_addr + | |<- addr + +==================+<- l_map_start + +*/ +int +internal_function +_dl_addr_inside_object (struct link_map *l, const ElfW(Addr) addr) +{ + int n = l->l_phnum; + const ElfW(Addr) reladdr = addr - l->l_addr; + + while (--n >= 0) + if (l->l_phdr[n].p_type == PT_LOAD + && reladdr - l->l_phdr[n].p_vaddr < l->l_phdr[n].p_memsz) + return 1; + return 0; +} diff --git a/elf/dl-addr.c b/elf/dl-addr.c index 291ff55..5a4ab1e 100644 --- a/elf/dl-addr.c +++ b/elf/dl-addr.c @@ -143,19 +143,3 @@ _dl_addr (const void *address, Dl_info *info, return result; } libc_hidden_def (_dl_addr) - -/* Return non-zero if ADDR lies within one of L's segments. */ -int -internal_function -_dl_addr_inside_object (struct link_map *l, const ElfW(Addr) addr) -{ - int n = l->l_phnum; - const ElfW(Addr) reladdr = addr - l->l_addr; - - while (--n >= 0) - if (l->l_phdr[n].p_type == PT_LOAD - && reladdr - l->l_phdr[n].p_vaddr >= 0 - && reladdr - l->l_phdr[n].p_vaddr < l->l_phdr[n].p_memsz) - return 1; - return 0; -} diff --git a/elf/dl-open.c b/elf/dl-open.c index 6f178b3..70f3f1e 100644 --- a/elf/dl-open.c +++ b/elf/dl-open.c @@ -735,21 +735,3 @@ _dl_show_scope (struct link_map *l, int from) _dl_debug_printf (" no scope\n"); _dl_debug_printf ("\n"); } - -#if IS_IN (rtld) -/* Return non-zero if ADDR lies within one of L's segments. */ -int -internal_function -_dl_addr_inside_object (struct link_map *l, const ElfW(Addr) addr) -{ - int n = l->l_phnum; - const ElfW(Addr) reladdr = addr - l->l_addr; - - while (--n >= 0) - if (l->l_phdr[n].p_type == PT_LOAD - && reladdr - l->l_phdr[n].p_vaddr >= 0 - && reladdr - l->l_phdr[n].p_vaddr < l->l_phdr[n].p_memsz) - return 1; - return 0; -} -#endif diff --git a/elf/tst-_dl_addr_inside_object.c b/elf/tst-_dl_addr_inside_object.c new file mode 100644 index 0000000..580c9b2 --- /dev/null +++ b/elf/tst-_dl_addr_inside_object.c @@ -0,0 +1,196 @@ +/* Unit test for _dl_addr_inside_object. + Copyright (C) 2016 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + . */ + +#include +#include +#include +#include +#include + +extern int internal_function _dl_addr_inside_object (struct link_map *l, + const ElfW(Addr) addr); + +static int +do_test (void) +{ + int ret; + ElfW(Addr) addr; + struct link_map map; + ElfW(Phdr) header; + map.l_phdr = &header; + map.l_phnum = 1; + map.l_addr = 0x0; + /* Segment spans 0x2000 -> 0x4000. */ + header.p_vaddr = 0x2000; + header.p_memsz = 0x2000; + header.p_type = PT_LOAD; + /* Address is above the segment e.g. > 0x4000. */ + addr = 0x5000; + ret = _dl_addr_inside_object (&map, addr); + switch (ret) + { + case 0: + printf ("PASS: Above: Address is detected as outside the segment.\n"); + break; + case 1: + printf ("FAIL: Above: Address is detected as inside the segment.\n"); + exit (1); + break; + default: + printf ("FAIL: Above: Invalid return value.\n"); + exit (1); + } + /* Address is inside the segment e.g. 0x2000 < addr < 0x4000. */ + addr = 0x3000; + ret = _dl_addr_inside_object (&map, addr); + switch (ret) + { + case 0: + printf ("FAIL: Inside: Address is detected as outside the segment.\n"); + exit (1); + break; + case 1: + printf ("PASS: Inside: Address is detected as inside the segment.\n"); + break; + default: + printf ("FAIL: Inside: Invalid return value.\n"); + exit (1); + } + /* Address is below the segment e.g. < 0x2000. */ + addr = 0x1000; + ret = _dl_addr_inside_object (&map, addr); + switch (ret) + { + case 0: + printf ("PASS: Below: Address is detected as outside the segment.\n"); + break; + case 1: + printf ("FAIL: Below: Address is detected as inside the segment.\n"); + exit (1); + break; + default: + printf ("FAIL: Below: Invalid return value.\n"); + exit (1); + } + /* Address is in the segment and addr == p_vaddr. */ + addr = 0x2000; + ret = _dl_addr_inside_object (&map, addr); + switch (ret) + { + case 0: + printf ("FAIL: At p_vaddr: Address is detected as outside the segment.\n"); + exit (1); + break; + case 1: + printf ("PASS: At p_vaddr: Address is detected as inside the segment.\n"); + break; + default: + printf ("FAIL: At p_vaddr: Invalid return value.\n"); + exit (1); + } + /* Address is in the segment and addr == p_vaddr + p_memsz - 1. */ + addr = 0x2000 + 0x2000 - 0x1; + ret = _dl_addr_inside_object (&map, addr); + switch (ret) + { + case 0: + printf ("FAIL: At p_memsz-1: Address is detected as outside the segment.\n"); + exit (1); + break; + case 1: + printf ("PASS: At p_memsz-1: Address is detected as inside the segment.\n"); + break; + default: + printf ("FAIL: At p_memsz-1: Invalid return value.\n"); + exit (1); + } + /* Address is outside the segment and addr == p_vaddr + p_memsz. */ + addr = 0x2000 + 0x2000; + ret = _dl_addr_inside_object (&map, addr); + switch (ret) + { + case 0: + printf ("PASS: At p_memsz: Address is detected as outside the segment.\n"); + break; + case 1: + printf ("FAIL: At p_memsz: Address is detected as inside the segment.\n"); + exit (1); + break; + default: + printf ("FAIL: At p_memsz: Invalid return value.\n"); + exit (1); + } + /* Address is outside the segment and p_vaddr at maximum address. */ + addr = 0x0 - 0x2; + header.p_vaddr = 0x0 - 0x1; + header.p_memsz = 0x1; + ret = _dl_addr_inside_object (&map, addr); + switch (ret) + { + case 0: + printf ("PASS: At max: Address is detected as outside the segment.\n"); + break; + case 1: + printf ("FAIL: At max: Address is detected as inside the segment.\n"); + exit (1); + break; + default: + printf ("FAIL: At max: Invalid return value.\n"); + exit (1); + } + /* Address is outside the segment and p_vaddr at minimum address. */ + addr = 0x1; + header.p_vaddr = 0x0; + header.p_memsz = 0x1; + ret = _dl_addr_inside_object (&map, addr); + switch (ret) + { + case 0: + printf ("PASS: At min: Address is detected as outside the segment.\n"); + break; + case 1: + printf ("FAIL: At min: Address is detected as inside the segment.\n"); + exit (1); + break; + default: + printf ("FAIL: At min: Invalid return value.\n"); + exit (1); + } + /* Address is always inside the segment with p_memsz at max. */ + addr = 0x0; + header.p_vaddr = 0x0; + header.p_memsz = 0x0 - 0x1; + ret = _dl_addr_inside_object (&map, addr); + switch (ret) + { + case 0: + printf ("FAIL: At maxmem: Address is detected as outside the segment.\n"); + exit (1); + break; + case 1: + printf ("PASS: At maxmem: Address is detected as inside the segment.\n"); + break; + default: + printf ("FAIL: At maxmem: Invalid return value.\n"); + exit (1); + } + return 0; +} + +#define TEST_FUNCTION do_test () +#include "../test-skeleton.c"