Message ID | 20191129210327.26434-5-adhemerval.zanella@linaro.org |
---|---|
State | Dropped |
Headers |
Received: (qmail 72392 invoked by alias); 29 Nov 2019 21:03:44 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 72334 invoked by uid 89); 29 Nov 2019 21:03:43 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-21.6 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.1 spammy=sk:sparc64 X-HELO: mail-qk1-f195.google.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=from:to:subject:date:message-id:in-reply-to:references; bh=uZ7ljY/tPkpsfNmkSLtnbeMdTkZ8/CcTaqA/754S96I=; b=T/qG1jGaDm1rpRD6rX9fMr0FRuCwXewIEnSnanmw8jJe0qXWmA+Pt1LnQoyL2a87on 8nIgEIOFJiTTffWkDKTc/DhTWC/hUCUK0QPossAMKK35zlTViLVTZfAnwXFKsIkDEz1e vTm/HLX9BR2LtLwXGBMXhOhR7OjzVOv5iOw7sEMsPK+R++vXa7xEcZx2BOBRKN/nF4Qo 7h3LCRH9Wja1EFDlLLacb07q8VzV4XHROkdwPs/V3MumG8LQnitc9Rfj/7UnApZ+eGiN 4DoJ0VmNbdo/5UXumSvakSnq5LkFHhxa/bAhA2IKdDfAgZBFWpdc6XxwldEIF6VLofED PU4A== Return-Path: <adhemerval.zanella@linaro.org> From: Adhemerval Zanella <adhemerval.zanella@linaro.org> To: libc-alpha@sourceware.org Subject: [PATCH 5/7] elf: Enable relro for static build Date: Fri, 29 Nov 2019 18:03:25 -0300 Message-Id: <20191129210327.26434-5-adhemerval.zanella@linaro.org> In-Reply-To: <20191129210327.26434-1-adhemerval.zanella@linaro.org> References: <20191129210327.26434-1-adhemerval.zanella@linaro.org> |
Commit Message
Adhemerval Zanella Netto
Nov. 29, 2019, 9:03 p.m. UTC
The code is similar to the one at rtld.c, where its check for the PT_GNU_RELRO header values from program headers and call _dl_protected_relro with the updated l_relro_{addr,size} values. Checked on x86_64-linux-gnu, i686-linux-gnu, powerpc64le-linux-gnu, aarch64-linux-gnu, s390x-linux-gnu, and sparc64-linux-gnu. --- elf/dl-support.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)
Comments
* Adhemerval Zanella: > The code is similar to the one at rtld.c, where its check for the > PT_GNU_RELRO header values from program headers and call > _dl_protected_relro with the updated l_relro_{addr,size} values. This is not the actual code that does RELRO in most cases, it's only used with prelink. _dl_relocate_object is what is used. > diff --git a/elf/dl-support.c b/elf/dl-support.c > index 5526d5ee6e..bdb5c2ae91 100644 > --- a/elf/dl-support.c > +++ b/elf/dl-support.c > @@ -367,14 +367,24 @@ _dl_non_dynamic_init (void) > if (_dl_platform != NULL) > _dl_platformlen = strlen (_dl_platform); > > - /* Scan for a program header telling us the stack is nonexecutable. */ > if (_dl_phdr != NULL) > - for (uint_fast16_t i = 0; i < _dl_phnum; ++i) > - if (_dl_phdr[i].p_type == PT_GNU_STACK) > + for (const ElfW(Phdr) *ph = _dl_phdr; ph < &_dl_phdr[_dl_phnum]; ++ph) > + switch (ph->p_type) > { > - _dl_stack_flags = _dl_phdr[i].p_flags; > + /* Check if the stack is nonexecutable. */ > + case PT_GNU_STACK: > + _dl_stack_flags = ph->p_flags; > + break; > + > + case PT_GNU_RELRO: > + _dl_main_map.l_relro_addr = ph->p_vaddr; > + _dl_main_map.l_relro_size = ph->p_memsz; > break; > } > + > + /* Setup relro on the binary itself. */ > + if (_dl_main_map.l_relro_size) > + _dl_protect_relro (&_dl_main_map); Please use an explicit comparison with != 0. I have a test case for this which I can post. Somewhat bizarrely, full RELRO for statically linked binaries requires linking with -z now.
On 01/12/2019 06:55, Florian Weimer wrote: > * Adhemerval Zanella: > >> The code is similar to the one at rtld.c, where its check for the >> PT_GNU_RELRO header values from program headers and call >> _dl_protected_relro with the updated l_relro_{addr,size} values. > > This is not the actual code that does RELRO in most cases, it's only > used with prelink. _dl_relocate_object is what is used. Ack, I changed the commit message to: The code is similar to the one at elf/dl-reloc.c, where it checks for the l_relro_size from the link_map (obtained from PT_GNU_RELRO header from program headers) and calls_dl_protected_relro. > >> diff --git a/elf/dl-support.c b/elf/dl-support.c >> index 5526d5ee6e..bdb5c2ae91 100644 >> --- a/elf/dl-support.c >> +++ b/elf/dl-support.c >> @@ -367,14 +367,24 @@ _dl_non_dynamic_init (void) >> if (_dl_platform != NULL) >> _dl_platformlen = strlen (_dl_platform); >> >> - /* Scan for a program header telling us the stack is nonexecutable. */ >> if (_dl_phdr != NULL) >> - for (uint_fast16_t i = 0; i < _dl_phnum; ++i) >> - if (_dl_phdr[i].p_type == PT_GNU_STACK) >> + for (const ElfW(Phdr) *ph = _dl_phdr; ph < &_dl_phdr[_dl_phnum]; ++ph) >> + switch (ph->p_type) >> { >> - _dl_stack_flags = _dl_phdr[i].p_flags; >> + /* Check if the stack is nonexecutable. */ >> + case PT_GNU_STACK: >> + _dl_stack_flags = ph->p_flags; >> + break; >> + >> + case PT_GNU_RELRO: >> + _dl_main_map.l_relro_addr = ph->p_vaddr; >> + _dl_main_map.l_relro_size = ph->p_memsz; >> break; >> } >> + >> + /* Setup relro on the binary itself. */ >> + if (_dl_main_map.l_relro_size) >> + _dl_protect_relro (&_dl_main_map); > > Please use an explicit comparison with != 0. Ack. > > I have a test case for this which I can post. Sure, I can attach on the patch itself. > Somewhat bizarrely, > full RELRO for statically linked binaries requires linking with -z now. > My understanding it is arch-specific and also depends on how bintuils was build. For instance, with my system ld (GNU ld (GNU Binutils for Ubuntu) 2.30) seemed to be built with DEFAULT_LD_Z_RELRO (set by --enable-relro) which sets relro by default. With this binutils I could only disable relro by explicit add norelro, the -z {lazy,now} did not change the GNU_RELRO header creation. Also the code in ld/emultempl/elf.em does seem to select different linker scripts for both link_info.relro and (link_info.flags & DF_BIND_NOW), however it does no have a special case for link_info.relro and !(link_info.flags & DF_BIND_NOW). I don't see how -relro is requiring -z now on ld code.
* Adhemerval Zanella: >> Somewhat bizarrely, full RELRO for statically linked binaries >> requires linking with -z now. > My understanding it is arch-specific and also depends on how > bintuils was build. For instance, with my system ld (GNU ld (GNU > Binutils for Ubuntu) 2.30) seemed to be built with > DEFAULT_LD_Z_RELRO (set by --enable-relro) which sets relro by > default. With this binutils I could only disable relro by explicit > add norelro, the -z {lazy,now} did not change the GNU_RELRO header > creation. Whether -z relro gives you full RELRO depends somewhat on the architecture and what relocations can be eliminated from the static link. Objects built with -fPIC tend to leave relocations behind, though, and to protect them, you need -z now.
On 02/12/2019 15:25, Florian Weimer wrote: > * Adhemerval Zanella: > >>> Somewhat bizarrely, full RELRO for statically linked binaries >>> requires linking with -z now. > >> My understanding it is arch-specific and also depends on how >> bintuils was build. For instance, with my system ld (GNU ld (GNU >> Binutils for Ubuntu) 2.30) seemed to be built with >> DEFAULT_LD_Z_RELRO (set by --enable-relro) which sets relro by >> default. With this binutils I could only disable relro by explicit >> add norelro, the -z {lazy,now} did not change the GNU_RELRO header >> creation. > > Whether -z relro gives you full RELRO depends somewhat on the > architecture and what relocations can be eliminated from the static > link. Objects built with -fPIC tend to leave relocations behind, > though, and to protect them, you need -z now. > I was investigating in fact if binutils is requiring the -z now to actually enable full RELRO, but it seems that there is no option to actually set 'full RELRO' besides the combination of -z now and -z relro. And I think it is worth to check for static PIE as well. At least for partial relro, .data.rel.ro seems to protect the required data. About testing, I am not sure what kind of coverage we are aiming here. My initial approach would to check if a write on a variable set to .data.rel.so does trigger a SEGSEGV signal and check with some different combinations (-z now, -z lazy, static, dynamic, pie, nopie). Do you have something more elaborated in mind?
* Adhemerval Zanella: > About testing, I am not sure what kind of coverage we are aiming > here. My initial approach would to check if a write on a variable > set to .data.rel.so does trigger a SEGSEGV signal and check with > some different combinations (-z now, -z lazy, static, dynamic, > pie, nopie). Do you have something more elaborated in mind? Yes, that's what my tests do. I will post them once we've stabilized the dynamic linker again.
On 02/12/2019 16:02, Florian Weimer wrote: > * Adhemerval Zanella: > >> About testing, I am not sure what kind of coverage we are aiming >> here. My initial approach would to check if a write on a variable >> set to .data.rel.so does trigger a SEGSEGV signal and check with >> some different combinations (-z now, -z lazy, static, dynamic, >> pie, nopie). Do you have something more elaborated in mind? > > Yes, that's what my tests do. I will post them once we've stabilized > the dynamic linker again. > I will update the patch with the tests I have done so far.
diff --git a/elf/dl-support.c b/elf/dl-support.c index 5526d5ee6e..bdb5c2ae91 100644 --- a/elf/dl-support.c +++ b/elf/dl-support.c @@ -367,14 +367,24 @@ _dl_non_dynamic_init (void) if (_dl_platform != NULL) _dl_platformlen = strlen (_dl_platform); - /* Scan for a program header telling us the stack is nonexecutable. */ if (_dl_phdr != NULL) - for (uint_fast16_t i = 0; i < _dl_phnum; ++i) - if (_dl_phdr[i].p_type == PT_GNU_STACK) + for (const ElfW(Phdr) *ph = _dl_phdr; ph < &_dl_phdr[_dl_phnum]; ++ph) + switch (ph->p_type) { - _dl_stack_flags = _dl_phdr[i].p_flags; + /* Check if the stack is nonexecutable. */ + case PT_GNU_STACK: + _dl_stack_flags = ph->p_flags; + break; + + case PT_GNU_RELRO: + _dl_main_map.l_relro_addr = ph->p_vaddr; + _dl_main_map.l_relro_size = ph->p_memsz; break; } + + /* Setup relro on the binary itself. */ + if (_dl_main_map.l_relro_size) + _dl_protect_relro (&_dl_main_map); } #ifdef DL_SYSINFO_IMPLEMENTATION