| Message ID | 20191129210327.26434-5-adhemerval.zanella@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/7] linux: Update x86 vDSO symbols | expand |
* 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
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(-) -- 2.17.1