Message ID | 20230112144412.3102134-1-adhemerval.zanella@linaro.org |
---|---|
State | New |
Headers | show |
Series | [v2] elf: Fix GL(dl_phdr) and GL(dl_phnum) for static builds [BZ #29864] | expand |
* Adhemerval Zanella: > The 73fc4e28b9464f0e refactor did not add the GL(dl_phdr) and > GL(dl_phnum) for static build, relying on the __ehdr_start symbol, > which is always added by the static linker, to get the correct values. > > This is problematic in some ways: > > - The segment may see its in-memory size differ from its in-file > size (or the binary may have holes). The Linux has fixed is to > provide concise values for both AT_PHDR and AT_PHNUM (commit > 0da1d5002745c - "fs/binfmt_elf: Fix AT_PHDR for unusual ELF files") > > - Some archs (alpha for instance) the hidden weak reference is not > correctly pulled by the static linker and __ehdr_start address > end up being 0, which makes GL(dl_phdr) and GL(dl_phnum) have both > invalid values (and triggering a segfault later on libc.so while > accessing TLS variables). > > The safer fix is to just restore the previous behavior to setup > GL(dl_phdr) and GL(dl_phnum) for static based on kernel auxv. The > __ehdr_start fallback can also be simplified by not assuming weak > linkage (as for PIE). > > The libc-static.c auxv init logic is moved to dl-support.c, since > the later is build without SHARED and then GLRO macro is defined > to access the variables directly. > > The _dl_phdr is also assumed to be always non NULL, since an invalid > NULL values does not trigger TLS initialization (which is used in > various libc systems). > > Checked on aarch64-linux-gnu, x86_64-linux-gnu, and i686-linux-gnu. > > * Changes from v1: > - Removed auxv_values implicit initialization. > - Removed unused header inclusion. This version looks okay to me. Reviewed-by: Florian Weimer <fweimer@redhat.com> Thanks, Florian
Hi Adhemveral! On Thu, 2023-01-12 at 11:44 -0300, Adhemerval Zanella wrote: > The 73fc4e28b9464f0e refactor did not add the GL(dl_phdr) and > GL(dl_phnum) for static build, relying on the __ehdr_start symbol, > which is always added by the static linker, to get the correct values. > > This is problematic in some ways: > > - The segment may see its in-memory size differ from its in-file > size (or the binary may have holes). The Linux has fixed is to > provide concise values for both AT_PHDR and AT_PHNUM (commit > 0da1d5002745c - "fs/binfmt_elf: Fix AT_PHDR for unusual ELF files") > > - Some archs (alpha for instance) the hidden weak reference is not > correctly pulled by the static linker and __ehdr_start address > end up being 0, which makes GL(dl_phdr) and GL(dl_phnum) have both > invalid values (and triggering a segfault later on libc.so while > accessing TLS variables). > > The safer fix is to just restore the previous behavior to setup > GL(dl_phdr) and GL(dl_phnum) for static based on kernel auxv. The > __ehdr_start fallback can also be simplified by not assuming weak > linkage (as for PIE). > > The libc-static.c auxv init logic is moved to dl-support.c, since > the later is build without SHARED and then GLRO macro is defined > to access the variables directly. > > The _dl_phdr is also assumed to be always non NULL, since an invalid > NULL values does not trigger TLS initialization (which is used in > various libc systems). > > Checked on aarch64-linux-gnu, x86_64-linux-gnu, and i686-linux-gnu. > > * Changes from v1: > - Removed auxv_values implicit initialization. > - Removed unused header inclusion. Any chance this fix can be backported to 2.36? Thanks, Adrian
On 02/02/23 06:36, John Paul Adrian Glaubitz wrote: > Hi Adhemveral! > > On Thu, 2023-01-12 at 11:44 -0300, Adhemerval Zanella wrote: >> The 73fc4e28b9464f0e refactor did not add the GL(dl_phdr) and >> GL(dl_phnum) for static build, relying on the __ehdr_start symbol, >> which is always added by the static linker, to get the correct values. >> >> This is problematic in some ways: >> >> - The segment may see its in-memory size differ from its in-file >> size (or the binary may have holes). The Linux has fixed is to >> provide concise values for both AT_PHDR and AT_PHNUM (commit >> 0da1d5002745c - "fs/binfmt_elf: Fix AT_PHDR for unusual ELF files") >> >> - Some archs (alpha for instance) the hidden weak reference is not >> correctly pulled by the static linker and __ehdr_start address >> end up being 0, which makes GL(dl_phdr) and GL(dl_phnum) have both >> invalid values (and triggering a segfault later on libc.so while >> accessing TLS variables). >> >> The safer fix is to just restore the previous behavior to setup >> GL(dl_phdr) and GL(dl_phnum) for static based on kernel auxv. The >> __ehdr_start fallback can also be simplified by not assuming weak >> linkage (as for PIE). >> >> The libc-static.c auxv init logic is moved to dl-support.c, since >> the later is build without SHARED and then GLRO macro is defined >> to access the variables directly. >> >> The _dl_phdr is also assumed to be always non NULL, since an invalid >> NULL values does not trigger TLS initialization (which is used in >> various libc systems). >> >> Checked on aarch64-linux-gnu, x86_64-linux-gnu, and i686-linux-gnu. >> >> * Changes from v1: >> - Removed auxv_values implicit initialization. >> - Removed unused header inclusion. > > Any chance this fix can be backported to 2.36? Done.
diff --git a/csu/libc-start.c b/csu/libc-start.c index ef26a89bc4..c3bb6d09bc 100644 --- a/csu/libc-start.c +++ b/csu/libc-start.c @@ -262,28 +262,7 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL), } # endif _dl_aux_init (auxvec); - if (GL(dl_phdr) == NULL) # endif - { - /* Starting from binutils-2.23, the linker will define the - magic symbol __ehdr_start to point to our own ELF header - if it is visible in a segment that also includes the phdrs. - So we can set up _dl_phdr and _dl_phnum even without any - information from auxv. */ - - extern const ElfW(Ehdr) __ehdr_start -# if BUILD_PIE_DEFAULT - __attribute__ ((visibility ("hidden"))); -# else - __attribute__ ((weak, visibility ("hidden"))); - if (&__ehdr_start != NULL) -# endif - { - assert (__ehdr_start.e_phentsize == sizeof *GL(dl_phdr)); - GL(dl_phdr) = (const void *) &__ehdr_start + __ehdr_start.e_phoff; - GL(dl_phnum) = __ehdr_start.e_phnum; - } - } __tunables_init (__environ); diff --git a/csu/libc-tls.c b/csu/libc-tls.c index 8dabf95bcc..cdf6442c02 100644 --- a/csu/libc-tls.c +++ b/csu/libc-tls.c @@ -119,19 +119,18 @@ __libc_setup_tls (void) __tls_pre_init_tp (); /* Look through the TLS segment if there is any. */ - if (_dl_phdr != NULL) - for (phdr = _dl_phdr; phdr < &_dl_phdr[_dl_phnum]; ++phdr) - if (phdr->p_type == PT_TLS) - { - /* Remember the values we need. */ - memsz = phdr->p_memsz; - filesz = phdr->p_filesz; - initimage = (void *) phdr->p_vaddr + main_map->l_addr; - align = phdr->p_align; - if (phdr->p_align > max_align) - max_align = phdr->p_align; - break; - } + for (phdr = _dl_phdr; phdr < &_dl_phdr[_dl_phnum]; ++phdr) + if (phdr->p_type == PT_TLS) + { + /* Remember the values we need. */ + memsz = phdr->p_memsz; + filesz = phdr->p_filesz; + initimage = (void *) phdr->p_vaddr + main_map->l_addr; + align = phdr->p_align; + if (phdr->p_align > max_align) + max_align = phdr->p_align; + break; + } /* Calculate the size of the static TLS surplus, with 0 auditors. */ _dl_tls_static_surplus_init (0); diff --git a/elf/dl-support.c b/elf/dl-support.c index ef0e5591e3..9714f75db0 100644 --- a/elf/dl-support.c +++ b/elf/dl-support.c @@ -256,6 +256,25 @@ _dl_aux_init (ElfW(auxv_t) *av) for (int i = 0; i < array_length (auxv_values); ++i) auxv_values[i] = 0; _dl_parse_auxv (av, auxv_values); + + _dl_phdr = (void*) auxv_values[AT_PHDR]; + _dl_phnum = auxv_values[AT_PHNUM]; + + if (_dl_phdr == NULL) + { + /* Starting from binutils-2.23, the linker will define the + magic symbol __ehdr_start to point to our own ELF header + if it is visible in a segment that also includes the phdrs. + So we can set up _dl_phdr and _dl_phnum even without any + information from auxv. */ + + extern const ElfW(Ehdr) __ehdr_start attribute_hidden; + assert (__ehdr_start.e_phentsize == sizeof *GL(dl_phdr)); + _dl_phdr = (const void *) &__ehdr_start + __ehdr_start.e_phoff; + _dl_phnum = __ehdr_start.e_phnum; + } + + assert (_dl_phdr != NULL); } #endif @@ -324,20 +343,19 @@ _dl_non_dynamic_init (void) if (_dl_platform != NULL) _dl_platformlen = strlen (_dl_platform); - if (_dl_phdr != NULL) - for (const ElfW(Phdr) *ph = _dl_phdr; ph < &_dl_phdr[_dl_phnum]; ++ph) - switch (ph->p_type) - { - /* 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; - } + for (const ElfW(Phdr) *ph = _dl_phdr; ph < &_dl_phdr[_dl_phnum]; ++ph) + switch (ph->p_type) + { + /* 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; + } call_function_static_weak (_dl_find_object_init);