* [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340] @ 2021-09-14 19:09 Siddhesh Poyarekar 2021-09-14 19:15 ` Florian Weimer 2021-09-15 14:35 ` H.J. Lu 0 siblings, 2 replies; 41+ messages in thread From: Siddhesh Poyarekar @ 2021-09-14 19:09 UTC (permalink / raw) To: libc-alpha; +Cc: fweimer, carlos ld.so on x86_64 (and possibly for all other architectures that don't have a read-only dynamic section by default but I haven't checked on all of them), when invoked with a DSO that has a read-only .dynamic section, crashes when trying to update gotplt, symtab, etc. entries with the load address. This crash happens even during verification, i.e. when it is invoked with --verify or with LD_TRACE_LOADED_OBJECTS. This is seen with vdso64.so from the Linux kernel on x86_64 for example. Handle this case more gracefully by bailing out early when a read-only PT_DYNAMIC segment is encountered and if the dynamic section has entries that would need to be updated on relocation. A test case has also been added to verify that BZ #28340 has been fixed. --- elf/Makefile | 11 +++++++++-- elf/dl-load.c | 14 ++++++++++++++ elf/get-dynamic-info.h | 41 ++++++++++++++++++++++++++++++++++++++++ elf/rtld.c | 12 ++++++++++++ elf/tst-ro-dynamic-mod.c | 1 + elf/tst-ro-dynamic.map | 13 +++++++++++++ elf/tst-ro-dynamic.sh | 36 +++++++++++++++++++++++++++++++++++ 7 files changed, 126 insertions(+), 2 deletions(-) create mode 100644 elf/tst-ro-dynamic-mod.c create mode 100644 elf/tst-ro-dynamic.map create mode 100644 elf/tst-ro-dynamic.sh diff --git a/elf/Makefile b/elf/Makefile index 9f3fadc37e..0bed622040 100644 --- a/elf/Makefile +++ b/elf/Makefile @@ -223,7 +223,7 @@ tests += restest1 preloadtest loadfail multiload origtest resolvfail \ tst-tls-ie tst-tls-ie-dlmopen argv0test \ tst-glibc-hwcaps tst-glibc-hwcaps-prepend tst-glibc-hwcaps-mask \ tst-tls20 tst-tls21 tst-dlmopen-dlerror tst-dlmopen-gethostbyname \ - tst-dl-is_dso + tst-dl-is_dso tst-ro-dynamic # reldep9 tests-internal += loadtest unload unload2 circleload1 \ neededtest neededtest2 neededtest3 neededtest4 \ @@ -359,7 +359,7 @@ modules-names = testobj1 testobj2 testobj3 testobj4 testobj5 testobj6 \ libmarkermod4-1 libmarkermod4-2 libmarkermod4-3 libmarkermod4-4 \ tst-tls20mod-bad tst-tls21mod tst-dlmopen-dlerror-mod \ tst-auxvalmod \ - tst-dlmopen-gethostbyname-mod \ + tst-dlmopen-gethostbyname-mod tst-ro-dynamic-mod \ # Most modules build with _ISOMAC defined, but those filtered out # depend on internal headers. @@ -1908,3 +1908,10 @@ $(objpfx)tst-getauxval-static.out: $(objpfx)tst-auxvalmod.so tst-getauxval-static-ENV = LD_LIBRARY_PATH=$(objpfx):$(common-objpfx) $(objpfx)tst-dlmopen-gethostbyname.out: $(objpfx)tst-dlmopen-gethostbyname-mod.so + +LDFLAGS-tst-ro-dynamic-mod = -Wl,--version-script=tst-ro-dynamic.map +$(objpfx)tst-ro-dynamic-mod.so: tst-ro-dynamic.map +$(objpfx)tst-ro-dynamic.out: tst-ro-dynamic.sh $(objpfx)tst-ro-dynamic-mod.so \ + $(objpfx)ld.so + $(SHELL) $^ '$(test-wrapper-env)' '$(run_program_env)' > $@; \ + $(evaluate-test) diff --git a/elf/dl-load.c b/elf/dl-load.c index 650e4edc35..5bac007a01 100644 --- a/elf/dl-load.c +++ b/elf/dl-load.c @@ -1124,6 +1124,9 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd, * executable. Other platforms default to a nonexecutable stack and don't * need PT_GNU_STACK to do so. */ uint_fast16_t stack_flags = DEFAULT_STACK_PERMS; +#ifndef DL_RO_DYN_SECTION + bool readonly_dynamic = false; +#endif { /* Scan the program header table, collecting its load commands. */ @@ -1149,6 +1152,9 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd, such a segment to avoid a crash later. */ l->l_ld = (void *) ph->p_vaddr; l->l_ldnum = ph->p_memsz / sizeof (ElfW(Dyn)); +#ifndef DL_RO_DYN_SECTION + readonly_dynamic = (ph->p_flags & PF_W) == 0; +#endif } break; @@ -1292,6 +1298,14 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd, else l->l_ld = (ElfW(Dyn) *) ((ElfW(Addr)) l->l_ld + l->l_addr); +#ifndef DL_RO_DYN_SECTION + if (readonly_dynamic && elf_dynamic_info_needs_fixup (l)) + { + errstring = N_("dynamic section of object file not writable"); + goto lose; + } +#endif + elf_get_dynamic_info (l, NULL); /* Make sure we are not dlopen'ing an object that has the diff --git a/elf/get-dynamic-info.h b/elf/get-dynamic-info.h index d8ec32377d..6072696afb 100644 --- a/elf/get-dynamic-info.h +++ b/elf/get-dynamic-info.h @@ -22,6 +22,47 @@ #include <assert.h> #include <libc-diag.h> +#ifndef RESOLVE_MAP +static +#else +auto +#endif +#ifndef DL_RO_DYN_SECTION +inline bool __attribute__ ((unused, always_inline)) +elf_dynamic_info_needs_fixup (struct link_map *l) +{ +#if __ELF_NATIVE_CLASS == 32 + typedef Elf32_Word d_tag_utype; +#elif __ELF_NATIVE_CLASS == 64 + typedef Elf64_Xword d_tag_utype; +#endif + + if (l->l_addr == 0) + return false; + + for (ElfW(Dyn) *dyn = l->l_ld; dyn->d_tag != DT_NULL; dyn++) + switch ((d_tag_utype) dyn->d_tag) + { + case DT_HASH: + case DT_PLTGOT: + case DT_STRTAB: + case DT_SYMTAB: +# if !ELF_MACHINE_NO_RELA + case DT_RELA: +# endif +# if !ELF_MACHINE_NO_REL + case DT_REL: +# endif + case DT_JMPREL: + case VERSYMIDX (DT_VERSYM): + case ADDRIDX (DT_GNU_HASH): + return true; + } + + return false; +} +#endif + #ifndef RESOLVE_MAP static #else diff --git a/elf/rtld.c b/elf/rtld.c index 878e6480f4..8067a01088 100644 --- a/elf/rtld.c +++ b/elf/rtld.c @@ -1456,6 +1456,10 @@ dl_main (const ElfW(Phdr) *phdr, /* And it was opened directly. */ ++main_map->l_direct_opencount; +#ifndef DL_RO_DYN_SECTION + bool readonly_dynamic = false; +#endif + /* Scan the program header table for the dynamic section. */ for (ph = phdr; ph < &phdr[phnum]; ++ph) switch (ph->p_type) @@ -1468,6 +1472,9 @@ dl_main (const ElfW(Phdr) *phdr, /* This tells us where to find the dynamic section, which tells us everything we need to do. */ main_map->l_ld = (void *) main_map->l_addr + ph->p_vaddr; +#ifndef DL_RO_DYN_SECTION + readonly_dynamic = (ph->p_flags & PF_W) == 0; +#endif break; case PT_INTERP: /* This "interpreter segment" was used by the program loader to @@ -1612,6 +1619,11 @@ dl_main (const ElfW(Phdr) *phdr, if (! rtld_is_main) { +#ifndef DL_RO_DYN_SECTION + if (readonly_dynamic && elf_dynamic_info_needs_fixup (main_map)) + _dl_fatal_printf ("dynamic section of executable is not writable\n"); +#endif + /* Extract the contents of the dynamic section for easy access. */ elf_get_dynamic_info (main_map, NULL); diff --git a/elf/tst-ro-dynamic-mod.c b/elf/tst-ro-dynamic-mod.c new file mode 100644 index 0000000000..2bc87d0c3c --- /dev/null +++ b/elf/tst-ro-dynamic-mod.c @@ -0,0 +1 @@ +int foo = 0; diff --git a/elf/tst-ro-dynamic.map b/elf/tst-ro-dynamic.map new file mode 100644 index 0000000000..dac92e0b94 --- /dev/null +++ b/elf/tst-ro-dynamic.map @@ -0,0 +1,13 @@ +SECTIONS +{ + .data : { *(.data) } :text + .bss : { *(.bss) } :text + .dynamic : { *(.dynamic) } :text :dynamic + .text : { *(.text) } :text +} + +PHDRS +{ + text PT_LOAD FLAGS(5) FILEHDR PHDRS; + dynamic PT_DYNAMIC FLAGS(4); +} diff --git a/elf/tst-ro-dynamic.sh b/elf/tst-ro-dynamic.sh new file mode 100644 index 0000000000..3d7144e0e2 --- /dev/null +++ b/elf/tst-ro-dynamic.sh @@ -0,0 +1,36 @@ +#!/bin/sh +# Ensure ld.so does not crash when verifying DSO with read-only dynamic +# section. +# Copyright (C) 2021 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 +# <https://www.gnu.org/licenses/>. + +dso=$1 +rtld=$2 +test_wrapper_env=$3 +run_program_env=$4 + +${test_wrapper_env} \ +${run_program_env} \ +$rtld --verify ${dso} + +ret=$? + +# ld.so should fail gracefully by returning 1. +if [ $ret -ne 1 ]; then + echo "ld.so returned $ret" + exit 1 +fi -- 2.31.1 ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340] 2021-09-14 19:09 [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340] Siddhesh Poyarekar @ 2021-09-14 19:15 ` Florian Weimer 2021-09-15 1:14 ` Siddhesh Poyarekar 2021-09-15 14:35 ` H.J. Lu 1 sibling, 1 reply; 41+ messages in thread From: Florian Weimer @ 2021-09-14 19:15 UTC (permalink / raw) To: Siddhesh Poyarekar; +Cc: libc-alpha, carlos * Siddhesh Poyarekar: > +inline bool __attribute__ ((unused, always_inline)) > +elf_dynamic_info_needs_fixup (struct link_map *l) > +{ > +#if __ELF_NATIVE_CLASS == 32 > + typedef Elf32_Word d_tag_utype; > +#elif __ELF_NATIVE_CLASS == 64 > + typedef Elf64_Xword d_tag_utype; > +#endif > + > + if (l->l_addr == 0) > + return false; > + > + for (ElfW(Dyn) *dyn = l->l_ld; dyn->d_tag != DT_NULL; dyn++) > + switch ((d_tag_utype) dyn->d_tag) > + { > + case DT_HASH: > + case DT_PLTGOT: > + case DT_STRTAB: > + case DT_SYMTAB: > +# if !ELF_MACHINE_NO_RELA > + case DT_RELA: > +# endif > +# if !ELF_MACHINE_NO_REL > + case DT_REL: > +# endif > + case DT_JMPREL: > + case VERSYMIDX (DT_VERSYM): > + case ADDRIDX (DT_GNU_HASH): > + return true; > + } > + > + return false; > +} A valid dynamic object must have DT_STRTAB. So I think we can assume that all objects need fixup, and we do not need this helper function. See figure 5.10 in <http://www.sco.com/developers/gabi/latest/ch5.dynamic.html>. Thanks, Florian ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340] 2021-09-14 19:15 ` Florian Weimer @ 2021-09-15 1:14 ` Siddhesh Poyarekar 0 siblings, 0 replies; 41+ messages in thread From: Siddhesh Poyarekar @ 2021-09-15 1:14 UTC (permalink / raw) To: Florian Weimer; +Cc: libc-alpha, carlos On 9/15/21 12:45 AM, Florian Weimer wrote: > A valid dynamic object must have DT_STRTAB. So I think we can assume > that all objects need fixup, and we do not need this helper function. > > See figure 5.10 in > <http://www.sco.com/developers/gabi/latest/ch5.dynamic.html>. Ahh good, thanks, I'll send a v2. Siddhesh ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340] 2021-09-14 19:09 [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340] Siddhesh Poyarekar 2021-09-14 19:15 ` Florian Weimer @ 2021-09-15 14:35 ` H.J. Lu 2021-09-15 15:42 ` Siddhesh Poyarekar 1 sibling, 1 reply; 41+ messages in thread From: H.J. Lu @ 2021-09-15 14:35 UTC (permalink / raw) To: Siddhesh Poyarekar; +Cc: GNU C Library, Florian Weimer On Tue, Sep 14, 2021 at 12:09 PM Siddhesh Poyarekar via Libc-alpha <libc-alpha@sourceware.org> wrote: > > ld.so on x86_64 (and possibly for all other architectures that don't > have a read-only dynamic section by default but I haven't checked on > all of them), when invoked with a DSO that has a read-only .dynamic > section, crashes when trying to update gotplt, symtab, etc. entries > with the load address. This crash happens even during verification, There are no relocations in vDSO. Why does ld.so try to update gotplt, symtab, etc. entries? > i.e. when it is invoked with --verify or with LD_TRACE_LOADED_OBJECTS. > > This is seen with vdso64.so from the Linux kernel on x86_64 for > example. > > Handle this case more gracefully by bailing out early when a read-only > PT_DYNAMIC segment is encountered and if the dynamic section has > entries that would need to be updated on relocation. A test case has > also been added to verify that BZ #28340 has been fixed. H.J. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340] 2021-09-15 14:35 ` H.J. Lu @ 2021-09-15 15:42 ` Siddhesh Poyarekar 2021-09-15 16:13 ` H.J. Lu 0 siblings, 1 reply; 41+ messages in thread From: Siddhesh Poyarekar @ 2021-09-15 15:42 UTC (permalink / raw) To: H.J. Lu; +Cc: GNU C Library, Florian Weimer On 9/15/21 8:05 PM, H.J. Lu wrote: > On Tue, Sep 14, 2021 at 12:09 PM Siddhesh Poyarekar via Libc-alpha > <libc-alpha@sourceware.org> wrote: >> >> ld.so on x86_64 (and possibly for all other architectures that don't >> have a read-only dynamic section by default but I haven't checked on >> all of them), when invoked with a DSO that has a read-only .dynamic >> section, crashes when trying to update gotplt, symtab, etc. entries >> with the load address. This crash happens even during verification, > > There are no relocations in vDSO. Why does ld.so try to update > gotplt, symtab, etc. entries? elf_get_dynamic_info adjusts the dynamic entries whenever l->l_addr is non-zero, regardless of whether there are relocations or not. It does so even when it is called through setup_vdso, except that the adjustment is done in a special static array for the vdso and not in the dynamic section itself. Siddhesh ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340] 2021-09-15 15:42 ` Siddhesh Poyarekar @ 2021-09-15 16:13 ` H.J. Lu 2021-09-15 16:24 ` Siddhesh Poyarekar 0 siblings, 1 reply; 41+ messages in thread From: H.J. Lu @ 2021-09-15 16:13 UTC (permalink / raw) To: Siddhesh Poyarekar; +Cc: GNU C Library, Florian Weimer On Wed, Sep 15, 2021 at 8:42 AM Siddhesh Poyarekar <siddhesh@sourceware.org> wrote: > > On 9/15/21 8:05 PM, H.J. Lu wrote: > > On Tue, Sep 14, 2021 at 12:09 PM Siddhesh Poyarekar via Libc-alpha > > <libc-alpha@sourceware.org> wrote: > >> > >> ld.so on x86_64 (and possibly for all other architectures that don't > >> have a read-only dynamic section by default but I haven't checked on > >> all of them), when invoked with a DSO that has a read-only .dynamic > >> section, crashes when trying to update gotplt, symtab, etc. entries > >> with the load address. This crash happens even during verification, > > > > There are no relocations in vDSO. Why does ld.so try to update > > gotplt, symtab, etc. entries? > > elf_get_dynamic_info adjusts the dynamic entries whenever l->l_addr is > non-zero, regardless of whether there are relocations or not. It does > so even when it is called through setup_vdso, except that the adjustment > is done in a special static array for the vdso and not in the dynamic > section itself. > > Siddhesh Can you extract logic from setup_vdso to handle DSOs without relocations? -- H.J. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340] 2021-09-15 16:13 ` H.J. Lu @ 2021-09-15 16:24 ` Siddhesh Poyarekar 2021-09-15 16:34 ` H.J. Lu 0 siblings, 1 reply; 41+ messages in thread From: Siddhesh Poyarekar @ 2021-09-15 16:24 UTC (permalink / raw) To: H.J. Lu; +Cc: Florian Weimer, GNU C Library On 9/15/21 9:43 PM, H.J. Lu via Libc-alpha wrote: > Can you extract logic from setup_vdso to handle DSOs without > relocations? The logic is already in elf_get_dynamic_info; setup_vdso only supplies the static temp buffer to store the adjustments. We could dynamically allocate memory for every DSO that has a read-only dynamic segment and put the adjustments there, pointing l->l_info pointers to it, but is it really necessary? What's the use case for a DSO with a read-only dynamic segment? Siddhesh ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340] 2021-09-15 16:24 ` Siddhesh Poyarekar @ 2021-09-15 16:34 ` H.J. Lu 2021-09-16 1:43 ` Siddhesh Poyarekar 0 siblings, 1 reply; 41+ messages in thread From: H.J. Lu @ 2021-09-15 16:34 UTC (permalink / raw) To: Siddhesh Poyarekar; +Cc: Florian Weimer, GNU C Library On Wed, Sep 15, 2021 at 9:24 AM Siddhesh Poyarekar <siddhesh@sourceware.org> wrote: > > On 9/15/21 9:43 PM, H.J. Lu via Libc-alpha wrote: > > Can you extract logic from setup_vdso to handle DSOs without > > relocations? > > The logic is already in elf_get_dynamic_info; setup_vdso only supplies > the static temp buffer to store the adjustments. We could dynamically > allocate memory for every DSO that has a read-only dynamic segment and > put the adjustments there, pointing l->l_info pointers to it, but is it > really necessary? What's the use case for a DSO with a read-only > dynamic segment? > > Siddhesh I think there are 2 separate, but related cases: read-only dynamic segment and DSOs without relocations. -- H.J. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340] 2021-09-15 16:34 ` H.J. Lu @ 2021-09-16 1:43 ` Siddhesh Poyarekar 2021-09-16 2:23 ` H.J. Lu 0 siblings, 1 reply; 41+ messages in thread From: Siddhesh Poyarekar @ 2021-09-16 1:43 UTC (permalink / raw) To: H.J. Lu; +Cc: Florian Weimer, GNU C Library On 9/15/21 10:04 PM, H.J. Lu wrote: > On Wed, Sep 15, 2021 at 9:24 AM Siddhesh Poyarekar > <siddhesh@sourceware.org> wrote: >> >> On 9/15/21 9:43 PM, H.J. Lu via Libc-alpha wrote: >>> Can you extract logic from setup_vdso to handle DSOs without >>> relocations? >> >> The logic is already in elf_get_dynamic_info; setup_vdso only supplies >> the static temp buffer to store the adjustments. We could dynamically >> allocate memory for every DSO that has a read-only dynamic segment and >> put the adjustments there, pointing l->l_info pointers to it, but is it >> really necessary? What's the use case for a DSO with a read-only >> dynamic segment? >> >> Siddhesh > > I think there are 2 separate, but related cases: read-only dynamic > segment and DSOs without relocations. What's the use case for read-only dynamic segments beyond VDSO? That is, in what situations would one need to load an relocatable DSO (or execute a PIE program) that has a read-only dynamic segment? On the other hand for DSOs without relocations, in what situations do you see a valid object having a read-only DYNAMIC segment? Thanks, Siddhesh ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340] 2021-09-16 1:43 ` Siddhesh Poyarekar @ 2021-09-16 2:23 ` H.J. Lu 2021-09-16 3:46 ` Siddhesh Poyarekar 2021-09-16 4:48 ` Florian Weimer 0 siblings, 2 replies; 41+ messages in thread From: H.J. Lu @ 2021-09-16 2:23 UTC (permalink / raw) To: Siddhesh Poyarekar; +Cc: Florian Weimer, GNU C Library On Wed, Sep 15, 2021 at 6:44 PM Siddhesh Poyarekar <siddhesh@sourceware.org> wrote: > > On 9/15/21 10:04 PM, H.J. Lu wrote: > > On Wed, Sep 15, 2021 at 9:24 AM Siddhesh Poyarekar > > <siddhesh@sourceware.org> wrote: > >> > >> On 9/15/21 9:43 PM, H.J. Lu via Libc-alpha wrote: > >>> Can you extract logic from setup_vdso to handle DSOs without > >>> relocations? > >> > >> The logic is already in elf_get_dynamic_info; setup_vdso only supplies > >> the static temp buffer to store the adjustments. We could dynamically > >> allocate memory for every DSO that has a read-only dynamic segment and > >> put the adjustments there, pointing l->l_info pointers to it, but is it > >> really necessary? What's the use case for a DSO with a read-only > >> dynamic segment? > >> > >> Siddhesh > > > > I think there are 2 separate, but related cases: read-only dynamic > > segment and DSOs without relocations. > > What's the use case for read-only dynamic segments beyond VDSO? That > is, in what situations would one need to load an relocatable DSO (or > execute a PIE program) that has a read-only dynamic segment? On the > other hand for DSOs without relocations, in what situations do you see a > valid object having a read-only DYNAMIC segment? There is nothing wrong with read-only dynamic segment. -- H.J. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340] 2021-09-16 2:23 ` H.J. Lu @ 2021-09-16 3:46 ` Siddhesh Poyarekar 2021-09-16 4:26 ` H.J. Lu 2021-09-16 4:48 ` Florian Weimer 1 sibling, 1 reply; 41+ messages in thread From: Siddhesh Poyarekar @ 2021-09-16 3:46 UTC (permalink / raw) To: H.J. Lu; +Cc: Florian Weimer, GNU C Library On 9/16/21 7:53 AM, H.J. Lu wrote: >> What's the use case for read-only dynamic segments beyond VDSO? That >> is, in what situations would one need to load an relocatable DSO (or >> execute a PIE program) that has a read-only dynamic segment? On the >> other hand for DSOs without relocations, in what situations do you see a >> valid object having a read-only DYNAMIC segment? > > There is nothing wrong with read-only dynamic segment. Do you mean to say that the dynamic linker should always try to read such DSOs correctly and, like the vdso, allocate a writable dynamic array to work around the .dynamic section being read-only? I suppose if ld.so needs to handle it correctly, then it would also have to read the flags of the LOAD segment that covers the DYNAMIC segment to make sure that it has the right permission flags. Siddhesh ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340] 2021-09-16 3:46 ` Siddhesh Poyarekar @ 2021-09-16 4:26 ` H.J. Lu 2021-09-16 4:28 ` Siddhesh Poyarekar 0 siblings, 1 reply; 41+ messages in thread From: H.J. Lu @ 2021-09-16 4:26 UTC (permalink / raw) To: Siddhesh Poyarekar; +Cc: Florian Weimer, GNU C Library On Wed, Sep 15, 2021 at 8:46 PM Siddhesh Poyarekar <siddhesh@sourceware.org> wrote: > > On 9/16/21 7:53 AM, H.J. Lu wrote: > >> What's the use case for read-only dynamic segments beyond VDSO? That > >> is, in what situations would one need to load an relocatable DSO (or > >> execute a PIE program) that has a read-only dynamic segment? On the > >> other hand for DSOs without relocations, in what situations do you see a > >> valid object having a read-only DYNAMIC segment? > > > > There is nothing wrong with read-only dynamic segment. > > Do you mean to say that the dynamic linker should always try to read > such DSOs correctly and, like the vdso, allocate a writable dynamic > array to work around the .dynamic section being read-only? > > I suppose if ld.so needs to handle it correctly, then it would also have > to read the flags of the LOAD segment that covers the DYNAMIC segment to > make sure that it has the right permission flags. What if DL_RO_DYN_SECTION is always 1? -- H.J. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340] 2021-09-16 4:26 ` H.J. Lu @ 2021-09-16 4:28 ` Siddhesh Poyarekar 2021-09-16 4:30 ` H.J. Lu 0 siblings, 1 reply; 41+ messages in thread From: Siddhesh Poyarekar @ 2021-09-16 4:28 UTC (permalink / raw) To: H.J. Lu; +Cc: Florian Weimer, GNU C Library On 9/16/21 9:56 AM, H.J. Lu wrote: > On Wed, Sep 15, 2021 at 8:46 PM Siddhesh Poyarekar > <siddhesh@sourceware.org> wrote: >> >> On 9/16/21 7:53 AM, H.J. Lu wrote: >>>> What's the use case for read-only dynamic segments beyond VDSO? That >>>> is, in what situations would one need to load an relocatable DSO (or >>>> execute a PIE program) that has a read-only dynamic segment? On the >>>> other hand for DSOs without relocations, in what situations do you see a >>>> valid object having a read-only DYNAMIC segment? >>> >>> There is nothing wrong with read-only dynamic segment. >> >> Do you mean to say that the dynamic linker should always try to read >> such DSOs correctly and, like the vdso, allocate a writable dynamic >> array to work around the .dynamic section being read-only? >> >> I suppose if ld.so needs to handle it correctly, then it would also have >> to read the flags of the LOAD segment that covers the DYNAMIC segment to >> make sure that it has the right permission flags. > > What if DL_RO_DYN_SECTION is always 1? > That's only true for riscv and mips. The addresses in the dynamic section are not updated in that case. Siddhesh ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340] 2021-09-16 4:28 ` Siddhesh Poyarekar @ 2021-09-16 4:30 ` H.J. Lu 0 siblings, 0 replies; 41+ messages in thread From: H.J. Lu @ 2021-09-16 4:30 UTC (permalink / raw) To: Siddhesh Poyarekar; +Cc: Florian Weimer, GNU C Library On Wed, Sep 15, 2021 at 9:28 PM Siddhesh Poyarekar <siddhesh@sourceware.org> wrote: > > On 9/16/21 9:56 AM, H.J. Lu wrote: > > On Wed, Sep 15, 2021 at 8:46 PM Siddhesh Poyarekar > > <siddhesh@sourceware.org> wrote: > >> > >> On 9/16/21 7:53 AM, H.J. Lu wrote: > >>>> What's the use case for read-only dynamic segments beyond VDSO? That > >>>> is, in what situations would one need to load an relocatable DSO (or > >>>> execute a PIE program) that has a read-only dynamic segment? On the > >>>> other hand for DSOs without relocations, in what situations do you see a > >>>> valid object having a read-only DYNAMIC segment? > >>> > >>> There is nothing wrong with read-only dynamic segment. > >> > >> Do you mean to say that the dynamic linker should always try to read > >> such DSOs correctly and, like the vdso, allocate a writable dynamic > >> array to work around the .dynamic section being read-only? > >> > >> I suppose if ld.so needs to handle it correctly, then it would also have > >> to read the flags of the LOAD segment that covers the DYNAMIC segment to > >> make sure that it has the right permission flags. > > > > What if DL_RO_DYN_SECTION is always 1? > > > > That's only true for riscv and mips. The addresses in the dynamic > section are not updated in that case. > We can remove DL_RO_DYN_SECTION and delete !DL_RO_DYN_SECTION codes. -- H.J. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340] 2021-09-16 2:23 ` H.J. Lu 2021-09-16 3:46 ` Siddhesh Poyarekar @ 2021-09-16 4:48 ` Florian Weimer 2021-09-16 5:36 ` Siddhesh Poyarekar 2021-09-16 14:11 ` Carlos O'Donell 1 sibling, 2 replies; 41+ messages in thread From: Florian Weimer @ 2021-09-16 4:48 UTC (permalink / raw) To: H.J. Lu; +Cc: Siddhesh Poyarekar, GNU C Library * H. J. Lu: > There is nothing wrong with read-only dynamic segment. A relocated DYNAMIC array is part of the ABI for !DL_RO_DYN_SECTION. ELF requires that DT_STRTAB is present. DT_STRTAB needs relocation. This means that for !DL_RO_DYN_SECTION, the dynamic segment cannot be in a read-ony LOAD segment for a valid ELF file. Thanks, Florian ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340] 2021-09-16 4:48 ` Florian Weimer @ 2021-09-16 5:36 ` Siddhesh Poyarekar 2021-09-16 5:46 ` Florian Weimer 2021-09-16 14:11 ` Carlos O'Donell 1 sibling, 1 reply; 41+ messages in thread From: Siddhesh Poyarekar @ 2021-09-16 5:36 UTC (permalink / raw) To: Florian Weimer, H.J. Lu; +Cc: GNU C Library On 9/16/21 10:18 AM, Florian Weimer wrote: > * H. J. Lu: > >> There is nothing wrong with read-only dynamic segment. > > A relocated DYNAMIC array is part of the ABI for !DL_RO_DYN_SECTION. > ELF requires that DT_STRTAB is present. DT_STRTAB needs relocation. > This means that for !DL_RO_DYN_SECTION, the dynamic segment cannot be in > a read-ony LOAD segment for a valid ELF file. I think what H. J. means here is that we just never adjust DT_STRTAB and add l_addr to it every time we use it. Basically, D_PTR is then always defined as: # define D_PTR(map, i) ((map)->i->d_un.d_ptr + (map)->l_addr) which could work (I think, I need to see if it breaks anything else) but will add an overhead every time these entries have to be accessed. Siddhesh ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340] 2021-09-16 5:36 ` Siddhesh Poyarekar @ 2021-09-16 5:46 ` Florian Weimer 2021-09-16 6:04 ` Siddhesh Poyarekar 0 siblings, 1 reply; 41+ messages in thread From: Florian Weimer @ 2021-09-16 5:46 UTC (permalink / raw) To: Siddhesh Poyarekar; +Cc: H.J. Lu, GNU C Library * Siddhesh Poyarekar: > On 9/16/21 10:18 AM, Florian Weimer wrote: >> * H. J. Lu: >> >>> There is nothing wrong with read-only dynamic segment. >> A relocated DYNAMIC array is part of the ABI for !DL_RO_DYN_SECTION. >> ELF requires that DT_STRTAB is present. DT_STRTAB needs relocation. >> This means that for !DL_RO_DYN_SECTION, the dynamic segment cannot be in >> a read-ony LOAD segment for a valid ELF file. > > I think what H. J. means here is that we just never adjust DT_STRTAB > and add l_addr to it every time we use it. I understood that. But applications expected that DT_STRTAB value has been relocated. See the discussion about the setting of DL_RO_DYN_SECTION for RISC-V and libphobos. Quoting libphobos/libdruntime/gcc/sections/elf.d: | if (dyn.d_tag == DT_STRTAB) | { | version (CRuntime_Musl) | strtab = cast(const(char)*)(info.dlpi_addr + dyn.d_un.d_ptr); // relocate | else version (linux) | { | // This might change in future glibc releases (after 2.29) as dynamic sections | // are not required to be read-only on RISC-V. This was copy & pasted from MIPS | // while upstreaming RISC-V support. Otherwise MIPS is the only arch which sets | // in glibc: #define DL_RO_DYN_SECTION 1 | version (RISCV_Any) | strtab = cast(const(char)*)(info.dlpi_addr + dyn.d_un.d_ptr); // relocate | else version (MIPS_Any) | strtab = cast(const(char)*)(info.dlpi_addr + dyn.d_un.d_ptr); // relocate | else | strtab = cast(const(char)*)dyn.d_un.d_ptr; | } Thanks, Florian ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340] 2021-09-16 5:46 ` Florian Weimer @ 2021-09-16 6:04 ` Siddhesh Poyarekar 0 siblings, 0 replies; 41+ messages in thread From: Siddhesh Poyarekar @ 2021-09-16 6:04 UTC (permalink / raw) To: Florian Weimer; +Cc: H.J. Lu, GNU C Library On 9/16/21 11:16 AM, Florian Weimer wrote: > * Siddhesh Poyarekar: > >> On 9/16/21 10:18 AM, Florian Weimer wrote: >>> * H. J. Lu: >>> >>>> There is nothing wrong with read-only dynamic segment. >>> A relocated DYNAMIC array is part of the ABI for !DL_RO_DYN_SECTION. >>> ELF requires that DT_STRTAB is present. DT_STRTAB needs relocation. >>> This means that for !DL_RO_DYN_SECTION, the dynamic segment cannot be in >>> a read-ony LOAD segment for a valid ELF file. >> >> I think what H. J. means here is that we just never adjust DT_STRTAB >> and add l_addr to it every time we use it. > > I understood that. But applications expected that DT_STRTAB value has > been relocated. > > See the discussion about the setting of DL_RO_DYN_SECTION for RISC-V and > libphobos. Quoting libphobos/libdruntime/gcc/sections/elf.d: > > | if (dyn.d_tag == DT_STRTAB) > | { > | version (CRuntime_Musl) > | strtab = cast(const(char)*)(info.dlpi_addr + dyn.d_un.d_ptr); // relocate > | else version (linux) > | { > | // This might change in future glibc releases (after 2.29) as dynamic sections > | // are not required to be read-only on RISC-V. This was copy & pasted from MIPS > | // while upstreaming RISC-V support. Otherwise MIPS is the only arch which sets > | // in glibc: #define DL_RO_DYN_SECTION 1 > | version (RISCV_Any) > | strtab = cast(const(char)*)(info.dlpi_addr + dyn.d_un.d_ptr); // relocate > | else version (MIPS_Any) > | strtab = cast(const(char)*)(info.dlpi_addr + dyn.d_un.d_ptr); // relocate > | else > | strtab = cast(const(char)*)dyn.d_un.d_ptr; > | } Yeah you're right, basically anything that uses dl_iterate_phdr and subsequently reaches the DYNAMIC segment will be affected by this change. So if we have to support read-only DYNAMIC then we must allocate dynamically and relocate there, like we do for vdso. Siddhesh ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340] 2021-09-16 4:48 ` Florian Weimer 2021-09-16 5:36 ` Siddhesh Poyarekar @ 2021-09-16 14:11 ` Carlos O'Donell 2021-09-16 15:18 ` H.J. Lu 1 sibling, 1 reply; 41+ messages in thread From: Carlos O'Donell @ 2021-09-16 14:11 UTC (permalink / raw) To: Florian Weimer, H.J. Lu; +Cc: Siddhesh Poyarekar, GNU C Library On 9/16/21 00:48, Florian Weimer via Libc-alpha wrote: > * H. J. Lu: > >> There is nothing wrong with read-only dynamic segment. > > A relocated DYNAMIC array is part of the ABI for !DL_RO_DYN_SECTION. > ELF requires that DT_STRTAB is present. DT_STRTAB needs relocation. > This means that for !DL_RO_DYN_SECTION, the dynamic segment cannot be in > a read-ony LOAD segment for a valid ELF file. I agree strongly with this position. Even with PT_GNU_RELRO, we must only go in one direction from RW -> RO (to avoid other security issues e.g. hardening not loosening the restrictions). In theory the vDSO is invalid. In practice it is a DL_RO_DYN_SECTION DSO but selected dynamically at runtime rather than statically at compile time for the target. -- Cheers, Carlos. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340] 2021-09-16 14:11 ` Carlos O'Donell @ 2021-09-16 15:18 ` H.J. Lu 2021-09-16 16:45 ` Siddhesh Poyarekar 0 siblings, 1 reply; 41+ messages in thread From: H.J. Lu @ 2021-09-16 15:18 UTC (permalink / raw) To: Carlos O'Donell; +Cc: Florian Weimer, Siddhesh Poyarekar, GNU C Library [-- Attachment #1: Type: text/plain, Size: 972 bytes --] On Thu, Sep 16, 2021 at 7:11 AM Carlos O'Donell <carlos@redhat.com> wrote: > > On 9/16/21 00:48, Florian Weimer via Libc-alpha wrote: > > * H. J. Lu: > > > >> There is nothing wrong with read-only dynamic segment. > > > > A relocated DYNAMIC array is part of the ABI for !DL_RO_DYN_SECTION. > > ELF requires that DT_STRTAB is present. DT_STRTAB needs relocation. > > This means that for !DL_RO_DYN_SECTION, the dynamic segment cannot be in > > a read-ony LOAD segment for a valid ELF file. > > I agree strongly with this position. > > Even with PT_GNU_RELRO, we must only go in one direction from RW -> RO (to avoid > other security issues e.g. hardening not loosening the restrictions). > > In theory the vDSO is invalid. > > In practice it is a DL_RO_DYN_SECTION DSO but selected dynamically at runtime > rather than statically at compile time for the target. > This patch makes DL_RO_DYN_SECTION semi-dynamic. We can also simply remove DL_RO_DYN_SECTION. -- H.J. [-- Attachment #2: 0001-ld.so-Change-DL_RO_DYN_SECTION-to-semi-dynamic.patch --] [-- Type: text/x-patch, Size: 4889 bytes --] From dc8d0fbc17024696cfa9e1ed8df2ecee4bad4696 Mon Sep 17 00:00:00 2001 From: "H.J. Lu" <hjl.tools@gmail.com> Date: Thu, 16 Sep 2021 08:15:29 -0700 Subject: [PATCH] ld.so: Change DL_RO_DYN_SECTION to semi-dynamic --- elf/dl-load.c | 1 + elf/dl-reloc-static-pie.c | 10 ++++++++++ elf/get-dynamic-info.h | 4 +--- elf/rtld.c | 2 ++ include/link.h | 1 + sysdeps/generic/ldsodefs.h | 12 +++++++----- 6 files changed, 22 insertions(+), 8 deletions(-) diff --git a/elf/dl-load.c b/elf/dl-load.c index 650e4edc35..292e921292 100644 --- a/elf/dl-load.c +++ b/elf/dl-load.c @@ -1149,6 +1149,7 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd, such a segment to avoid a crash later. */ l->l_ld = (void *) ph->p_vaddr; l->l_ldnum = ph->p_memsz / sizeof (ElfW(Dyn)); + l->l_ld_readonly = (ph->p_flags & PF_W) == 0; } break; diff --git a/elf/dl-reloc-static-pie.c b/elf/dl-reloc-static-pie.c index d5bd2f31e9..d5fbeb137c 100644 --- a/elf/dl-reloc-static-pie.c +++ b/elf/dl-reloc-static-pie.c @@ -40,6 +40,16 @@ _dl_relocate_static_pie (void) /* Read our own dynamic section and fill in the info array. */ main_map->l_ld = ((void *) main_map->l_addr + elf_machine_dynamic ()); + + const ElfW(Phdr) *ph, *phdr = GL(dl_phdr); + size_t phnum = GL(dl_phnum); + for (ph = phdr; ph < &phdr[phnum]; ++ph) + if (ph->p_type == PT_DYNAMIC) + { + main_map->l_ld_readonly = (ph->p_flags & PF_W) == 0; + break; + } + elf_get_dynamic_info (main_map, NULL); # ifdef ELF_MACHINE_BEFORE_RTLD_RELOC diff --git a/elf/get-dynamic-info.h b/elf/get-dynamic-info.h index d8ec32377d..02dc281ec5 100644 --- a/elf/get-dynamic-info.h +++ b/elf/get-dynamic-info.h @@ -71,9 +71,8 @@ elf_get_dynamic_info (struct link_map *l, ElfW(Dyn) *temp) #define DL_RO_DYN_TEMP_CNT 8 -#ifndef DL_RO_DYN_SECTION /* Don't adjust .dynamic unnecessarily. */ - if (l->l_addr != 0) + if (l->l_addr != 0 && !l->l_ld_readonly) { ElfW(Addr) l_addr = l->l_addr; int cnt = 0; @@ -109,7 +108,6 @@ elf_get_dynamic_info (struct link_map *l, ElfW(Dyn) *temp) # undef ADJUST_DYN_INFO assert (cnt <= DL_RO_DYN_TEMP_CNT); } -#endif if (info[DT_PLTREL] != NULL) { #if ELF_MACHINE_NO_RELA diff --git a/elf/rtld.c b/elf/rtld.c index 878e6480f4..c7818586a2 100644 --- a/elf/rtld.c +++ b/elf/rtld.c @@ -463,6 +463,7 @@ _dl_start_final (void *arg, struct dl_start_final_info *info) #ifndef DONT_USE_BOOTSTRAP_MAP GL(dl_rtld_map).l_addr = info->l.l_addr; GL(dl_rtld_map).l_ld = info->l.l_ld; + GL(dl_rtld_map).l_ld_readonly = info->l.l_ld_readonly; memcpy (GL(dl_rtld_map).l_info, info->l.l_info, sizeof GL(dl_rtld_map).l_info); GL(dl_rtld_map).l_mach = info->l.l_mach; @@ -1468,6 +1469,7 @@ dl_main (const ElfW(Phdr) *phdr, /* This tells us where to find the dynamic section, which tells us everything we need to do. */ main_map->l_ld = (void *) main_map->l_addr + ph->p_vaddr; + main_map->l_ld_readonly = (ph->p_flags & PF_W) == 0; break; case PT_INTERP: /* This "interpreter segment" was used by the program loader to diff --git a/include/link.h b/include/link.h index 4af16cb596..963a9f0147 100644 --- a/include/link.h +++ b/include/link.h @@ -205,6 +205,7 @@ struct link_map unsigned int l_free_initfini:1; /* Nonzero if l_initfini can be freed, ie. not allocated with the dummy malloc in ld.so. */ + unsigned int l_ld_readonly; /* Nonzero if dynamic section is readonly. */ /* NODELETE status of the map. Only valid for maps of type lt_loaded. Lazy binding sets l_nodelete_active directly, diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h index fd67871f4b..3710d7ea79 100644 --- a/sysdeps/generic/ldsodefs.h +++ b/sysdeps/generic/ldsodefs.h @@ -69,17 +69,19 @@ __BEGIN_DECLS `ElfW(TYPE)' is used in place of `Elf32_TYPE' or `Elf64_TYPE'. */ #define ELFW(type) _ElfW (ELF, __ELF_NATIVE_CLASS, type) +#ifndef DL_RO_DYN_SECTION +# define DL_RO_DYN_SECTION 0 +#endif + /* All references to the value of l_info[DT_PLTGOT], l_info[DT_STRTAB], l_info[DT_SYMTAB], l_info[DT_RELA], l_info[DT_REL], l_info[DT_JMPREL], and l_info[VERSYMIDX (DT_VERSYM)] have to be accessed via the D_PTR macro. The macro is needed since for most architectures the entry is already relocated - but for some not and we need to relocate at access time. */ -#ifdef DL_RO_DYN_SECTION -# define D_PTR(map, i) ((map)->i->d_un.d_ptr + (map)->l_addr) -#else -# define D_PTR(map, i) (map)->i->d_un.d_ptr -#endif +#define D_PTR(map, i) \ + ((map)->i->d_un.d_ptr \ + + (DL_RO_DYN_SECTION || (map)->l_ld_readonly ? (map)->l_addr : 0)) /* Result of the lookup functions and how to retrieve the base address. */ typedef struct link_map *lookup_t; -- 2.31.1 ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340] 2021-09-16 15:18 ` H.J. Lu @ 2021-09-16 16:45 ` Siddhesh Poyarekar 2021-09-16 17:38 ` H.J. Lu 0 siblings, 1 reply; 41+ messages in thread From: Siddhesh Poyarekar @ 2021-09-16 16:45 UTC (permalink / raw) To: H.J. Lu, Carlos O'Donell; +Cc: Florian Weimer, GNU C Library On 9/16/21 8:48 PM, H.J. Lu wrote: > On Thu, Sep 16, 2021 at 7:11 AM Carlos O'Donell <carlos@redhat.com> wrote: >> >> On 9/16/21 00:48, Florian Weimer via Libc-alpha wrote: >>> * H. J. Lu: >>> >>>> There is nothing wrong with read-only dynamic segment. >>> >>> A relocated DYNAMIC array is part of the ABI for !DL_RO_DYN_SECTION. >>> ELF requires that DT_STRTAB is present. DT_STRTAB needs relocation. >>> This means that for !DL_RO_DYN_SECTION, the dynamic segment cannot be in >>> a read-ony LOAD segment for a valid ELF file. >> >> I agree strongly with this position. >> >> Even with PT_GNU_RELRO, we must only go in one direction from RW -> RO (to avoid >> other security issues e.g. hardening not loosening the restrictions). >> >> In theory the vDSO is invalid. >> >> In practice it is a DL_RO_DYN_SECTION DSO but selected dynamically at runtime >> rather than statically at compile time for the target. Actually it isn't. The dynamic section in the vdso is already relocated by the kernel when it's mapped in. DL_RO_DYN_SECTION DSOs are not relocated because of which any references to pointers written in the .dynamic section need to be relocated. > > This patch makes DL_RO_DYN_SECTION semi-dynamic. We can also simply > remove DL_RO_DYN_SECTION. > > > From dc8d0fbc17024696cfa9e1ed8df2ecee4bad4696 Mon Sep 17 00:00:00 2001 > From: "H.J. Lu" <hjl.tools@gmail.com> > Date: Thu, 16 Sep 2021 08:15:29 -0700 > Subject: [PATCH] ld.so: Change DL_RO_DYN_SECTION to semi-dynamic > > --- > elf/dl-load.c | 1 + > elf/dl-reloc-static-pie.c | 10 ++++++++++ > elf/get-dynamic-info.h | 4 +--- > elf/rtld.c | 2 ++ > include/link.h | 1 + > sysdeps/generic/ldsodefs.h | 12 +++++++----- > 6 files changed, 22 insertions(+), 8 deletions(-) > > diff --git a/elf/dl-load.c b/elf/dl-load.c > index 650e4edc35..292e921292 100644 > --- a/elf/dl-load.c > +++ b/elf/dl-load.c > @@ -1149,6 +1149,7 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd, > such a segment to avoid a crash later. */ > l->l_ld = (void *) ph->p_vaddr; > l->l_ldnum = ph->p_memsz / sizeof (ElfW(Dyn)); > + l->l_ld_readonly = (ph->p_flags & PF_W) == 0; One concern Florian had was that the read-only decision should be made based on the LOAD segment flags and not the DYNAMIC segment flags. It works for vdso since those permissions are in sync as far as write is concerned (RX and R for LOAD and DYNAMIC respectively) but if we want to write to .dynamic when LOAD is RW (but DYNAMIC isn't for some reason) then we should look for the LOAD segment that overlaps with this. > } > break; > > diff --git a/elf/dl-reloc-static-pie.c b/elf/dl-reloc-static-pie.c > index d5bd2f31e9..d5fbeb137c 100644 > --- a/elf/dl-reloc-static-pie.c > +++ b/elf/dl-reloc-static-pie.c > @@ -40,6 +40,16 @@ _dl_relocate_static_pie (void) > > /* Read our own dynamic section and fill in the info array. */ > main_map->l_ld = ((void *) main_map->l_addr + elf_machine_dynamic ()); > + > + const ElfW(Phdr) *ph, *phdr = GL(dl_phdr); > + size_t phnum = GL(dl_phnum); > + for (ph = phdr; ph < &phdr[phnum]; ++ph) > + if (ph->p_type == PT_DYNAMIC) > + { > + main_map->l_ld_readonly = (ph->p_flags & PF_W) == 0; > + break; > + } > + > elf_get_dynamic_info (main_map, NULL); Same question here. > > # ifdef ELF_MACHINE_BEFORE_RTLD_RELOC > diff --git a/elf/get-dynamic-info.h b/elf/get-dynamic-info.h > index d8ec32377d..02dc281ec5 100644 > --- a/elf/get-dynamic-info.h > +++ b/elf/get-dynamic-info.h > @@ -71,9 +71,8 @@ elf_get_dynamic_info (struct link_map *l, ElfW(Dyn) *temp) > > #define DL_RO_DYN_TEMP_CNT 8 > > -#ifndef DL_RO_DYN_SECTION > /* Don't adjust .dynamic unnecessarily. */ > - if (l->l_addr != 0) > + if (l->l_addr != 0 && !l->l_ld_readonly) This still has the concern that applications will incorrectly assume that these entries are relocated. If we want to continue to load DSOs with read-only DYNAMIC, shouldn't we just allocate memory (like we do for setup_vdso, but dynamically per DSO) and write the relocations there instead? > { > ElfW(Addr) l_addr = l->l_addr; > int cnt = 0; > @@ -109,7 +108,6 @@ elf_get_dynamic_info (struct link_map *l, ElfW(Dyn) *temp) > # undef ADJUST_DYN_INFO > assert (cnt <= DL_RO_DYN_TEMP_CNT); > } > -#endif > if (info[DT_PLTREL] != NULL) > { > #if ELF_MACHINE_NO_RELA > diff --git a/elf/rtld.c b/elf/rtld.c > index 878e6480f4..c7818586a2 100644 > --- a/elf/rtld.c > +++ b/elf/rtld.c > @@ -463,6 +463,7 @@ _dl_start_final (void *arg, struct dl_start_final_info *info) > #ifndef DONT_USE_BOOTSTRAP_MAP > GL(dl_rtld_map).l_addr = info->l.l_addr; > GL(dl_rtld_map).l_ld = info->l.l_ld; > + GL(dl_rtld_map).l_ld_readonly = info->l.l_ld_readonly; > memcpy (GL(dl_rtld_map).l_info, info->l.l_info, > sizeof GL(dl_rtld_map).l_info); > GL(dl_rtld_map).l_mach = info->l.l_mach; > @@ -1468,6 +1469,7 @@ dl_main (const ElfW(Phdr) *phdr, > /* This tells us where to find the dynamic section, > which tells us everything we need to do. */ > main_map->l_ld = (void *) main_map->l_addr + ph->p_vaddr; > + main_map->l_ld_readonly = (ph->p_flags & PF_W) == 0; > break; > case PT_INTERP: > /* This "interpreter segment" was used by the program loader to > diff --git a/include/link.h b/include/link.h > index 4af16cb596..963a9f0147 100644 > --- a/include/link.h > +++ b/include/link.h > @@ -205,6 +205,7 @@ struct link_map > unsigned int l_free_initfini:1; /* Nonzero if l_initfini can be > freed, ie. not allocated with > the dummy malloc in ld.so. */ > + unsigned int l_ld_readonly; /* Nonzero if dynamic section is readonly. */ > > /* NODELETE status of the map. Only valid for maps of type > lt_loaded. Lazy binding sets l_nodelete_active directly, > diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h > index fd67871f4b..3710d7ea79 100644 > --- a/sysdeps/generic/ldsodefs.h > +++ b/sysdeps/generic/ldsodefs.h > @@ -69,17 +69,19 @@ __BEGIN_DECLS > `ElfW(TYPE)' is used in place of `Elf32_TYPE' or `Elf64_TYPE'. */ > #define ELFW(type) _ElfW (ELF, __ELF_NATIVE_CLASS, type) > > +#ifndef DL_RO_DYN_SECTION > +# define DL_RO_DYN_SECTION 0 > +#endif > + > /* All references to the value of l_info[DT_PLTGOT], > l_info[DT_STRTAB], l_info[DT_SYMTAB], l_info[DT_RELA], > l_info[DT_REL], l_info[DT_JMPREL], and l_info[VERSYMIDX (DT_VERSYM)] > have to be accessed via the D_PTR macro. The macro is needed since for > most architectures the entry is already relocated - but for some not > and we need to relocate at access time. */ > -#ifdef DL_RO_DYN_SECTION > -# define D_PTR(map, i) ((map)->i->d_un.d_ptr + (map)->l_addr) > -#else > -# define D_PTR(map, i) (map)->i->d_un.d_ptr > -#endif > +#define D_PTR(map, i) \ > + ((map)->i->d_un.d_ptr \ > + + (DL_RO_DYN_SECTION || (map)->l_ld_readonly ? (map)->l_addr : 0)) OK for vDSO since it doesn't get marked as readonly in setup_vdso. > > /* Result of the lookup functions and how to retrieve the base address. */ > typedef struct link_map *lookup_t; > -- > 2.31.1 ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340] 2021-09-16 16:45 ` Siddhesh Poyarekar @ 2021-09-16 17:38 ` H.J. Lu 2021-09-16 17:58 ` Siddhesh Poyarekar 2021-09-16 18:03 ` Florian Weimer 0 siblings, 2 replies; 41+ messages in thread From: H.J. Lu @ 2021-09-16 17:38 UTC (permalink / raw) To: Siddhesh Poyarekar; +Cc: Carlos O'Donell, Florian Weimer, GNU C Library [-- Attachment #1: Type: text/plain, Size: 1429 bytes --] On Thu, Sep 16, 2021 at 9:45 AM Siddhesh Poyarekar <siddhesh@sourceware.org> wrote: > > On 9/16/21 8:48 PM, H.J. Lu wrote: > > On Thu, Sep 16, 2021 at 7:11 AM Carlos O'Donell <carlos@redhat.com> wrote: > >> > >> On 9/16/21 00:48, Florian Weimer via Libc-alpha wrote: > >>> * H. J. Lu: > >>> > >>>> There is nothing wrong with read-only dynamic segment. > >>> > >>> A relocated DYNAMIC array is part of the ABI for !DL_RO_DYN_SECTION. > >>> ELF requires that DT_STRTAB is present. DT_STRTAB needs relocation. > >>> This means that for !DL_RO_DYN_SECTION, the dynamic segment cannot be in > >>> a read-ony LOAD segment for a valid ELF file. > >> > >> I agree strongly with this position. > >> > >> Even with PT_GNU_RELRO, we must only go in one direction from RW -> RO (to avoid > >> other security issues e.g. hardening not loosening the restrictions). > >> > >> In theory the vDSO is invalid. > >> > >> In practice it is a DL_RO_DYN_SECTION DSO but selected dynamically at runtime > >> rather than statically at compile time for the target. > > Actually it isn't. The dynamic section in the vdso is already relocated > by the kernel when it's mapped in. DL_RO_DYN_SECTION DSOs are not > relocated because of which any references to pointers written in the > .dynamic section need to be relocated. No. Relocation of vDSO dynamic section is done by elf_get_dynamic_info. Here is a patch to remove the hack for vDSO. -- H.J. [-- Attachment #2: 0001-ld.so-Remoe-DL_RO_DYN_SECTION.patch --] [-- Type: text/x-patch, Size: 8760 bytes --] From b3d1a60ce6daef343b94d3c8dae5094f37c31c09 Mon Sep 17 00:00:00 2001 From: "H.J. Lu" <hjl.tools@gmail.com> Date: Thu, 16 Sep 2021 08:15:29 -0700 Subject: [PATCH] ld.so: Remoe DL_RO_DYN_SECTION --- elf/dl-load.c | 3 ++- elf/dl-reloc-static-pie.c | 12 +++++++++++- elf/get-dynamic-info.h | 17 +++-------------- elf/rtld.c | 6 ++++-- elf/setup-vdso.h | 5 ++--- include/link.h | 1 + sysdeps/generic/ldsodefs.h | 7 ++----- sysdeps/mips/ldsodefs.h | 4 ---- sysdeps/riscv/ldsodefs.h | 5 ----- 9 files changed, 25 insertions(+), 35 deletions(-) diff --git a/elf/dl-load.c b/elf/dl-load.c index 650e4edc35..4445c28ef3 100644 --- a/elf/dl-load.c +++ b/elf/dl-load.c @@ -1149,6 +1149,7 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd, such a segment to avoid a crash later. */ l->l_ld = (void *) ph->p_vaddr; l->l_ldnum = ph->p_memsz / sizeof (ElfW(Dyn)); + l->l_ld_readonly = (ph->p_flags & PF_W) == 0; } break; @@ -1292,7 +1293,7 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd, else l->l_ld = (ElfW(Dyn) *) ((ElfW(Addr)) l->l_ld + l->l_addr); - elf_get_dynamic_info (l, NULL); + elf_get_dynamic_info (l); /* Make sure we are not dlopen'ing an object that has the DF_1_NOOPEN flag set, or a PIE object. */ diff --git a/elf/dl-reloc-static-pie.c b/elf/dl-reloc-static-pie.c index d5bd2f31e9..2fb02d7276 100644 --- a/elf/dl-reloc-static-pie.c +++ b/elf/dl-reloc-static-pie.c @@ -40,7 +40,17 @@ _dl_relocate_static_pie (void) /* Read our own dynamic section and fill in the info array. */ main_map->l_ld = ((void *) main_map->l_addr + elf_machine_dynamic ()); - elf_get_dynamic_info (main_map, NULL); + + const ElfW(Phdr) *ph, *phdr = GL(dl_phdr); + size_t phnum = GL(dl_phnum); + for (ph = phdr; ph < &phdr[phnum]; ++ph) + if (ph->p_type == PT_DYNAMIC) + { + main_map->l_ld_readonly = (ph->p_flags & PF_W) == 0; + break; + } + + elf_get_dynamic_info (main_map); # ifdef ELF_MACHINE_BEFORE_RTLD_RELOC ELF_MACHINE_BEFORE_RTLD_RELOC (main_map->l_info); diff --git a/elf/get-dynamic-info.h b/elf/get-dynamic-info.h index d8ec32377d..7e6c0f3987 100644 --- a/elf/get-dynamic-info.h +++ b/elf/get-dynamic-info.h @@ -28,7 +28,7 @@ static auto #endif inline void __attribute__ ((unused, always_inline)) -elf_get_dynamic_info (struct link_map *l, ElfW(Dyn) *temp) +elf_get_dynamic_info (struct link_map *l) { #if __ELF_NATIVE_CLASS == 32 typedef Elf32_Word d_tag_utype; @@ -71,9 +71,8 @@ elf_get_dynamic_info (struct link_map *l, ElfW(Dyn) *temp) #define DL_RO_DYN_TEMP_CNT 8 -#ifndef DL_RO_DYN_SECTION /* Don't adjust .dynamic unnecessarily. */ - if (l->l_addr != 0) + if (l->l_addr != 0 && !l->l_ld_readonly) { ElfW(Addr) l_addr = l->l_addr; int cnt = 0; @@ -81,16 +80,7 @@ elf_get_dynamic_info (struct link_map *l, ElfW(Dyn) *temp) # define ADJUST_DYN_INFO(tag) \ do \ if (info[tag] != NULL) \ - { \ - if (temp) \ - { \ - temp[cnt].d_tag = info[tag]->d_tag; \ - temp[cnt].d_un.d_ptr = info[tag]->d_un.d_ptr + l_addr; \ - info[tag] = temp + cnt++; \ - } \ - else \ - info[tag]->d_un.d_ptr += l_addr; \ - } \ + info[tag]->d_un.d_ptr += l_addr; \ while (0) ADJUST_DYN_INFO (DT_HASH); @@ -109,7 +99,6 @@ elf_get_dynamic_info (struct link_map *l, ElfW(Dyn) *temp) # undef ADJUST_DYN_INFO assert (cnt <= DL_RO_DYN_TEMP_CNT); } -#endif if (info[DT_PLTREL] != NULL) { #if ELF_MACHINE_NO_RELA diff --git a/elf/rtld.c b/elf/rtld.c index 878e6480f4..88a78326c8 100644 --- a/elf/rtld.c +++ b/elf/rtld.c @@ -463,6 +463,7 @@ _dl_start_final (void *arg, struct dl_start_final_info *info) #ifndef DONT_USE_BOOTSTRAP_MAP GL(dl_rtld_map).l_addr = info->l.l_addr; GL(dl_rtld_map).l_ld = info->l.l_ld; + GL(dl_rtld_map).l_ld_readonly = info->l.l_ld_readonly; memcpy (GL(dl_rtld_map).l_info, info->l.l_info, sizeof GL(dl_rtld_map).l_info); GL(dl_rtld_map).l_mach = info->l.l_mach; @@ -546,7 +547,7 @@ _dl_start (void *arg) /* Read our own dynamic section and fill in the info array. */ bootstrap_map.l_ld = (void *) bootstrap_map.l_addr + elf_machine_dynamic (); - elf_get_dynamic_info (&bootstrap_map, NULL); + elf_get_dynamic_info (&bootstrap_map); #if NO_TLS_OFFSET != 0 bootstrap_map.l_tls_offset = NO_TLS_OFFSET; @@ -1468,6 +1469,7 @@ dl_main (const ElfW(Phdr) *phdr, /* This tells us where to find the dynamic section, which tells us everything we need to do. */ main_map->l_ld = (void *) main_map->l_addr + ph->p_vaddr; + main_map->l_ld_readonly = (ph->p_flags & PF_W) == 0; break; case PT_INTERP: /* This "interpreter segment" was used by the program loader to @@ -1613,7 +1615,7 @@ dl_main (const ElfW(Phdr) *phdr, if (! rtld_is_main) { /* Extract the contents of the dynamic section for easy access. */ - elf_get_dynamic_info (main_map, NULL); + elf_get_dynamic_info (main_map); /* If the main map is libc.so, update the base namespace to refer to this map. If libc.so is loaded later, this happens diff --git a/elf/setup-vdso.h b/elf/setup-vdso.h index 86c491e49c..f44748bc98 100644 --- a/elf/setup-vdso.h +++ b/elf/setup-vdso.h @@ -33,8 +33,6 @@ setup_vdso (struct link_map *main_map __attribute__ ((unused)), 0, LM_ID_BASE); if (__glibc_likely (l != NULL)) { - static ElfW(Dyn) dyn_temp[DL_RO_DYN_TEMP_CNT] attribute_relro; - l->l_phdr = ((const void *) GLRO(dl_sysinfo_dso) + GLRO(dl_sysinfo_dso)->e_phoff); l->l_phnum = GLRO(dl_sysinfo_dso)->e_phnum; @@ -45,6 +43,7 @@ setup_vdso (struct link_map *main_map __attribute__ ((unused)), { l->l_ld = (void *) ph->p_vaddr; l->l_ldnum = ph->p_memsz / sizeof (ElfW(Dyn)); + l->l_ld_readonly = (ph->p_flags & PF_W) == 0; } else if (ph->p_type == PT_LOAD) { @@ -65,7 +64,7 @@ setup_vdso (struct link_map *main_map __attribute__ ((unused)), l->l_map_end += l->l_addr; l->l_text_end += l->l_addr; l->l_ld = (void *) ((ElfW(Addr)) l->l_ld + l->l_addr); - elf_get_dynamic_info (l, dyn_temp); + elf_get_dynamic_info (l); _dl_setup_hash (l); l->l_relocated = 1; diff --git a/include/link.h b/include/link.h index 4af16cb596..963a9f0147 100644 --- a/include/link.h +++ b/include/link.h @@ -205,6 +205,7 @@ struct link_map unsigned int l_free_initfini:1; /* Nonzero if l_initfini can be freed, ie. not allocated with the dummy malloc in ld.so. */ + unsigned int l_ld_readonly; /* Nonzero if dynamic section is readonly. */ /* NODELETE status of the map. Only valid for maps of type lt_loaded. Lazy binding sets l_nodelete_active directly, diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h index fd67871f4b..e51a62a5da 100644 --- a/sysdeps/generic/ldsodefs.h +++ b/sysdeps/generic/ldsodefs.h @@ -75,11 +75,8 @@ __BEGIN_DECLS have to be accessed via the D_PTR macro. The macro is needed since for most architectures the entry is already relocated - but for some not and we need to relocate at access time. */ -#ifdef DL_RO_DYN_SECTION -# define D_PTR(map, i) ((map)->i->d_un.d_ptr + (map)->l_addr) -#else -# define D_PTR(map, i) (map)->i->d_un.d_ptr -#endif +#define D_PTR(map, i) \ + ((map)->i->d_un.d_ptr + ((map)->l_ld_readonly ? (map)->l_addr : 0)) /* Result of the lookup functions and how to retrieve the base address. */ typedef struct link_map *lookup_t; diff --git a/sysdeps/mips/ldsodefs.h b/sysdeps/mips/ldsodefs.h index 4db7c60e38..36fd09a8bd 100644 --- a/sysdeps/mips/ldsodefs.h +++ b/sysdeps/mips/ldsodefs.h @@ -75,10 +75,6 @@ struct La_mips_64_retval; struct La_mips_64_retval *, \ const char *); -/* The MIPS ABI specifies that the dynamic section has to be read-only. */ - -#define DL_RO_DYN_SECTION 1 - #include_next <ldsodefs.h> /* The 64-bit MIPS ELF ABI uses an unusual reloc format. Each diff --git a/sysdeps/riscv/ldsodefs.h b/sysdeps/riscv/ldsodefs.h index 0c696714a7..8947ffe4b5 100644 --- a/sysdeps/riscv/ldsodefs.h +++ b/sysdeps/riscv/ldsodefs.h @@ -38,11 +38,6 @@ struct La_riscv_retval; struct La_riscv_retval *, \ const char *); -/* Although the RISC-V ABI does not specify that the dynamic section has - to be read-only, it needs to be kept for ABI compatibility. */ - -#define DL_RO_DYN_SECTION 1 - #include_next <ldsodefs.h> #endif -- 2.31.1 ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340] 2021-09-16 17:38 ` H.J. Lu @ 2021-09-16 17:58 ` Siddhesh Poyarekar 2021-09-16 22:11 ` H.J. Lu 2021-09-16 18:03 ` Florian Weimer 1 sibling, 1 reply; 41+ messages in thread From: Siddhesh Poyarekar @ 2021-09-16 17:58 UTC (permalink / raw) To: H.J. Lu; +Cc: Carlos O'Donell, Florian Weimer, GNU C Library On 9/16/21 11:08 PM, H.J. Lu wrote: > On Thu, Sep 16, 2021 at 9:45 AM Siddhesh Poyarekar > <siddhesh@sourceware.org> wrote: >> >> On 9/16/21 8:48 PM, H.J. Lu wrote: >>> On Thu, Sep 16, 2021 at 7:11 AM Carlos O'Donell <carlos@redhat.com> wrote: >>>> >>>> On 9/16/21 00:48, Florian Weimer via Libc-alpha wrote: >>>>> * H. J. Lu: >>>>> >>>>>> There is nothing wrong with read-only dynamic segment. >>>>> >>>>> A relocated DYNAMIC array is part of the ABI for !DL_RO_DYN_SECTION. >>>>> ELF requires that DT_STRTAB is present. DT_STRTAB needs relocation. >>>>> This means that for !DL_RO_DYN_SECTION, the dynamic segment cannot be in >>>>> a read-ony LOAD segment for a valid ELF file. >>>> >>>> I agree strongly with this position. >>>> >>>> Even with PT_GNU_RELRO, we must only go in one direction from RW -> RO (to avoid >>>> other security issues e.g. hardening not loosening the restrictions). >>>> >>>> In theory the vDSO is invalid. >>>> >>>> In practice it is a DL_RO_DYN_SECTION DSO but selected dynamically at runtime >>>> rather than statically at compile time for the target. >> >> Actually it isn't. The dynamic section in the vdso is already relocated >> by the kernel when it's mapped in. DL_RO_DYN_SECTION DSOs are not >> relocated because of which any references to pointers written in the >> .dynamic section need to be relocated. > > No. Relocation of vDSO dynamic section is done by elf_get_dynamic_info. > Here is a patch to remove the hack for vDSO. > Ahh, I misunderstood the comment in setup_vdso, sorry. I verified by running under the debugger that the kernel doesn't adjust .dynamic entries before mapping the vdso. This updated patch is definitely better IMO but it still doesn't resolve the two outstanding questions posed so far. I'm on the fence about the first one (it's imprecise but the cost of imprecision doesn't seem high enough to warrant the extra check) but slanted slightly towards allocating memory and writing out relocated addresses in the interest of keeping the user experience with dl_iterate_phdr and friends consistent. 1. Should the readonly decision be based solely on DYNAMIC flags or also consider flags on the encompassing LOAD segment? 2. Do we want to leave .dynamic unrelocated for read-only DYNAMIC or should we instead allocate an array to write relocated addresses in there, like we did for vdso? Siddhesh ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340] 2021-09-16 17:58 ` Siddhesh Poyarekar @ 2021-09-16 22:11 ` H.J. Lu 2021-09-17 2:47 ` Siddhesh Poyarekar 0 siblings, 1 reply; 41+ messages in thread From: H.J. Lu @ 2021-09-16 22:11 UTC (permalink / raw) To: Siddhesh Poyarekar; +Cc: Carlos O'Donell, Florian Weimer, GNU C Library On Thu, Sep 16, 2021 at 10:59 AM Siddhesh Poyarekar <siddhesh@sourceware.org> wrote: > > On 9/16/21 11:08 PM, H.J. Lu wrote: > > On Thu, Sep 16, 2021 at 9:45 AM Siddhesh Poyarekar > > <siddhesh@sourceware.org> wrote: > >> > >> On 9/16/21 8:48 PM, H.J. Lu wrote: > >>> On Thu, Sep 16, 2021 at 7:11 AM Carlos O'Donell <carlos@redhat.com> wrote: > >>>> > >>>> On 9/16/21 00:48, Florian Weimer via Libc-alpha wrote: > >>>>> * H. J. Lu: > >>>>> > >>>>>> There is nothing wrong with read-only dynamic segment. > >>>>> > >>>>> A relocated DYNAMIC array is part of the ABI for !DL_RO_DYN_SECTION. > >>>>> ELF requires that DT_STRTAB is present. DT_STRTAB needs relocation. > >>>>> This means that for !DL_RO_DYN_SECTION, the dynamic segment cannot be in > >>>>> a read-ony LOAD segment for a valid ELF file. > >>>> > >>>> I agree strongly with this position. > >>>> > >>>> Even with PT_GNU_RELRO, we must only go in one direction from RW -> RO (to avoid > >>>> other security issues e.g. hardening not loosening the restrictions). > >>>> > >>>> In theory the vDSO is invalid. > >>>> > >>>> In practice it is a DL_RO_DYN_SECTION DSO but selected dynamically at runtime > >>>> rather than statically at compile time for the target. > >> > >> Actually it isn't. The dynamic section in the vdso is already relocated > >> by the kernel when it's mapped in. DL_RO_DYN_SECTION DSOs are not > >> relocated because of which any references to pointers written in the > >> .dynamic section need to be relocated. > > > > No. Relocation of vDSO dynamic section is done by elf_get_dynamic_info. > > Here is a patch to remove the hack for vDSO. > > > > Ahh, I misunderstood the comment in setup_vdso, sorry. I verified by > running under the debugger that the kernel doesn't adjust .dynamic > entries before mapping the vdso. > > This updated patch is definitely better IMO but it still doesn't resolve > the two outstanding questions posed so far. I'm on the fence about the > first one (it's imprecise but the cost of imprecision doesn't seem high > enough to warrant the extra check) but slanted slightly towards > allocating memory and writing out relocated addresses in the interest of > keeping the user experience with dl_iterate_phdr and friends consistent. > > 1. Should the readonly decision be based solely on DYNAMIC flags or also > consider flags on the encompassing LOAD segment? It should purely depend on PT_DYNAMIC segment. > 2. Do we want to leave .dynamic unrelocated for read-only DYNAMIC or > should we instead allocate an array to write relocated addresses in > there, like we did for vdso? My patch removed the hack for vDSO to leave the read-only PT_DYNAMIC segment unrelocated. -- H.J. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340] 2021-09-16 22:11 ` H.J. Lu @ 2021-09-17 2:47 ` Siddhesh Poyarekar 2021-09-17 2:59 ` H.J. Lu 0 siblings, 1 reply; 41+ messages in thread From: Siddhesh Poyarekar @ 2021-09-17 2:47 UTC (permalink / raw) To: H.J. Lu; +Cc: Carlos O'Donell, Florian Weimer, GNU C Library On 9/17/21 3:41 AM, H.J. Lu wrote: >> 2. Do we want to leave .dynamic unrelocated for read-only DYNAMIC or >> should we instead allocate an array to write relocated addresses in >> there, like we did for vdso? > > My patch removed the hack for vDSO to leave the read-only > PT_DYNAMIC segment unrelocated. Presuming that your rationale for this is your response from your other email: > Non-glibc consumers of > PT_DYNAMIC segment need to handle it differently for these non-ABI > conforming binaries. could you qualify this a bit more? My reading of the spec[1] suggests that the spec is silent on whether the .dynamic entries should be writable, it merely says that the dynamic linker reads d_un.d_ptr and adds the memory base address to it when referring to those addresses. So calling them non-conforming seems wrong. And if they're conforming, there seems to be little reason to make them different from DSOs with writable DYNAMIC segment as far as l_info data returned from dl_iterate_phdr is concerned. Siddhesh [1] https://refspecs.linuxbase.org/elf/gabi4+/ch5.dynamic.html ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340] 2021-09-17 2:47 ` Siddhesh Poyarekar @ 2021-09-17 2:59 ` H.J. Lu 2021-09-17 3:36 ` Siddhesh Poyarekar 0 siblings, 1 reply; 41+ messages in thread From: H.J. Lu @ 2021-09-17 2:59 UTC (permalink / raw) To: Siddhesh Poyarekar; +Cc: Carlos O'Donell, Florian Weimer, GNU C Library On Thu, Sep 16, 2021 at 7:47 PM Siddhesh Poyarekar <siddhesh@sourceware.org> wrote: > > On 9/17/21 3:41 AM, H.J. Lu wrote: > >> 2. Do we want to leave .dynamic unrelocated for read-only DYNAMIC or > >> should we instead allocate an array to write relocated addresses in > >> there, like we did for vdso? > > > > My patch removed the hack for vDSO to leave the read-only > > PT_DYNAMIC segment unrelocated. > > Presuming that your rationale for this is your response from your other > email: > > > Non-glibc consumers of > > PT_DYNAMIC segment need to handle it differently for these non-ABI > > conforming binaries. > > could you qualify this a bit more? My reading of the spec[1] suggests > that the spec is silent on whether the .dynamic entries should be > writable, it merely says that the dynamic linker reads d_un.d_ptr and > adds the memory base address to it when referring to those addresses. > So calling them non-conforming seems wrong. > I was referring to extern ElfW(Dyn) _DYNAMIC[]; On x86-64, _DYNAMIC entries are normally relocated. But for read-only DSOs, they are unrelocated. It isn't a problem for ld.so since it doesn't use it. > And if they're conforming, there seems to be little reason to make them > different from DSOs with writable DYNAMIC segment as far as l_info data > returned from dl_iterate_phdr is concerned. > > Siddhesh > > [1] https://refspecs.linuxbase.org/elf/gabi4+/ch5.dynamic.html -- H.J. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340] 2021-09-17 2:59 ` H.J. Lu @ 2021-09-17 3:36 ` Siddhesh Poyarekar 2021-09-17 3:42 ` H.J. Lu 2021-09-17 3:44 ` Florian Weimer 0 siblings, 2 replies; 41+ messages in thread From: Siddhesh Poyarekar @ 2021-09-17 3:36 UTC (permalink / raw) To: H.J. Lu; +Cc: Carlos O'Donell, Florian Weimer, GNU C Library On 9/17/21 8:29 AM, H.J. Lu wrote: > On Thu, Sep 16, 2021 at 7:47 PM Siddhesh Poyarekar > <siddhesh@sourceware.org> wrote: >> >> On 9/17/21 3:41 AM, H.J. Lu wrote: >>>> 2. Do we want to leave .dynamic unrelocated for read-only DYNAMIC or >>>> should we instead allocate an array to write relocated addresses in >>>> there, like we did for vdso? >>> >>> My patch removed the hack for vDSO to leave the read-only >>> PT_DYNAMIC segment unrelocated. >> >> Presuming that your rationale for this is your response from your other >> email: >> >>> Non-glibc consumers of >>> PT_DYNAMIC segment need to handle it differently for these non-ABI >>> conforming binaries. >> >> could you qualify this a bit more? My reading of the spec[1] suggests >> that the spec is silent on whether the .dynamic entries should be >> writable, it merely says that the dynamic linker reads d_un.d_ptr and >> adds the memory base address to it when referring to those addresses. >> So calling them non-conforming seems wrong. >> > > I was referring to > > extern ElfW(Dyn) _DYNAMIC[]; > > On x86-64, _DYNAMIC entries are normally relocated. But for read-only > DSOs, they are unrelocated. It isn't a problem for ld.so since it doesn't > use it. OK, then we've traditionally had an inconsistency between the contents of _DYNAMIC and l_info and this change would make that difference go away. It's still a user-visible change as far as consumers of l_info (through dl_iterate_phdr) are concerned. Why not just allocate a writable table for read-only DSOs and put the relocated entries there, pointing the l_info[...] pointer to it? That will ensure that the change is user-invisible. Siddhesh ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340] 2021-09-17 3:36 ` Siddhesh Poyarekar @ 2021-09-17 3:42 ` H.J. Lu 2021-09-17 3:44 ` Siddhesh Poyarekar 2021-09-17 3:44 ` Florian Weimer 1 sibling, 1 reply; 41+ messages in thread From: H.J. Lu @ 2021-09-17 3:42 UTC (permalink / raw) To: Siddhesh Poyarekar; +Cc: Carlos O'Donell, Florian Weimer, GNU C Library On Thu, Sep 16, 2021 at 8:36 PM Siddhesh Poyarekar <siddhesh@sourceware.org> wrote: > > On 9/17/21 8:29 AM, H.J. Lu wrote: > > On Thu, Sep 16, 2021 at 7:47 PM Siddhesh Poyarekar > > <siddhesh@sourceware.org> wrote: > >> > >> On 9/17/21 3:41 AM, H.J. Lu wrote: > >>>> 2. Do we want to leave .dynamic unrelocated for read-only DYNAMIC or > >>>> should we instead allocate an array to write relocated addresses in > >>>> there, like we did for vdso? > >>> > >>> My patch removed the hack for vDSO to leave the read-only > >>> PT_DYNAMIC segment unrelocated. > >> > >> Presuming that your rationale for this is your response from your other > >> email: > >> > >>> Non-glibc consumers of > >>> PT_DYNAMIC segment need to handle it differently for these non-ABI > >>> conforming binaries. > >> > >> could you qualify this a bit more? My reading of the spec[1] suggests > >> that the spec is silent on whether the .dynamic entries should be > >> writable, it merely says that the dynamic linker reads d_un.d_ptr and > >> adds the memory base address to it when referring to those addresses. > >> So calling them non-conforming seems wrong. > >> > > > > I was referring to > > > > extern ElfW(Dyn) _DYNAMIC[]; > > > > On x86-64, _DYNAMIC entries are normally relocated. But for read-only > > DSOs, they are unrelocated. It isn't a problem for ld.so since it doesn't > > use it. > > OK, then we've traditionally had an inconsistency between the contents > of _DYNAMIC and l_info and this change would make that difference go > away. It's still a user-visible change as far as consumers of l_info > (through dl_iterate_phdr) are concerned. > > Why not just allocate a writable table for read-only DSOs and put the > relocated entries there, pointing the l_info[...] pointer to it? That > will ensure that the change is user-invisible. > We could do that INSIDE OF ld.so. But it won't change _DYNAMIC since it is allocated by linker. -- H.J. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340] 2021-09-17 3:42 ` H.J. Lu @ 2021-09-17 3:44 ` Siddhesh Poyarekar 0 siblings, 0 replies; 41+ messages in thread From: Siddhesh Poyarekar @ 2021-09-17 3:44 UTC (permalink / raw) To: H.J. Lu; +Cc: Carlos O'Donell, Florian Weimer, GNU C Library On 9/17/21 9:12 AM, H.J. Lu wrote: > We could do that INSIDE OF ld.so. But it won't change _DYNAMIC since > it is allocated by linker. Yes, I reckon that is fine because it's always been that way. Siddhesh ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340] 2021-09-17 3:36 ` Siddhesh Poyarekar 2021-09-17 3:42 ` H.J. Lu @ 2021-09-17 3:44 ` Florian Weimer 2021-09-17 3:51 ` Siddhesh Poyarekar 1 sibling, 1 reply; 41+ messages in thread From: Florian Weimer @ 2021-09-17 3:44 UTC (permalink / raw) To: Siddhesh Poyarekar; +Cc: H.J. Lu, Carlos O'Donell, GNU C Library * Siddhesh Poyarekar: > On 9/17/21 8:29 AM, H.J. Lu wrote: >> On Thu, Sep 16, 2021 at 7:47 PM Siddhesh Poyarekar >> <siddhesh@sourceware.org> wrote: >>> >>> On 9/17/21 3:41 AM, H.J. Lu wrote: >>>>> 2. Do we want to leave .dynamic unrelocated for read-only DYNAMIC or >>>>> should we instead allocate an array to write relocated addresses in >>>>> there, like we did for vdso? >>>> >>>> My patch removed the hack for vDSO to leave the read-only >>>> PT_DYNAMIC segment unrelocated. >>> >>> Presuming that your rationale for this is your response from your other >>> email: >>> >>>> Non-glibc consumers of >>>> PT_DYNAMIC segment need to handle it differently for these non-ABI >>>> conforming binaries. >>> >>> could you qualify this a bit more? My reading of the spec[1] suggests >>> that the spec is silent on whether the .dynamic entries should be >>> writable, it merely says that the dynamic linker reads d_un.d_ptr and >>> adds the memory base address to it when referring to those addresses. >>> So calling them non-conforming seems wrong. >>> >> I was referring to >> extern ElfW(Dyn) _DYNAMIC[]; >> On x86-64, _DYNAMIC entries are normally relocated. But for >> read-only >> DSOs, they are unrelocated. It isn't a problem for ld.so since it doesn't >> use it. > > OK, then we've traditionally had an inconsistency between the contents > of _DYNAMIC and l_info and this change would make that difference go > away. It's still a user-visible change as far as consumers of l_info > (through dl_iterate_phdr) are concerned. > > Why not just allocate a writable table for read-only DSOs and put the > relocated entries there, pointing the l_info[...] pointer to it? That > will ensure that the change is user-invisible. l_info isn't an exported field, is it? So this wouldn't make a difference. Thanks, Florian ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340] 2021-09-17 3:44 ` Florian Weimer @ 2021-09-17 3:51 ` Siddhesh Poyarekar 0 siblings, 0 replies; 41+ messages in thread From: Siddhesh Poyarekar @ 2021-09-17 3:51 UTC (permalink / raw) To: Florian Weimer; +Cc: H.J. Lu, Carlos O'Donell, GNU C Library On 9/17/21 9:14 AM, Florian Weimer wrote: > * Siddhesh Poyarekar: > >> On 9/17/21 8:29 AM, H.J. Lu wrote: >>> On Thu, Sep 16, 2021 at 7:47 PM Siddhesh Poyarekar >>> <siddhesh@sourceware.org> wrote: >>>> >>>> On 9/17/21 3:41 AM, H.J. Lu wrote: >>>>>> 2. Do we want to leave .dynamic unrelocated for read-only DYNAMIC or >>>>>> should we instead allocate an array to write relocated addresses in >>>>>> there, like we did for vdso? >>>>> >>>>> My patch removed the hack for vDSO to leave the read-only >>>>> PT_DYNAMIC segment unrelocated. >>>> >>>> Presuming that your rationale for this is your response from your other >>>> email: >>>> >>>>> Non-glibc consumers of >>>>> PT_DYNAMIC segment need to handle it differently for these non-ABI >>>>> conforming binaries. >>>> >>>> could you qualify this a bit more? My reading of the spec[1] suggests >>>> that the spec is silent on whether the .dynamic entries should be >>>> writable, it merely says that the dynamic linker reads d_un.d_ptr and >>>> adds the memory base address to it when referring to those addresses. >>>> So calling them non-conforming seems wrong. >>>> >>> I was referring to >>> extern ElfW(Dyn) _DYNAMIC[]; >>> On x86-64, _DYNAMIC entries are normally relocated. But for >>> read-only >>> DSOs, they are unrelocated. It isn't a problem for ld.so since it doesn't >>> use it. >> >> OK, then we've traditionally had an inconsistency between the contents >> of _DYNAMIC and l_info and this change would make that difference go >> away. It's still a user-visible change as far as consumers of l_info >> (through dl_iterate_phdr) are concerned. >> >> Why not just allocate a writable table for read-only DSOs and put the >> relocated entries there, pointing the l_info[...] pointer to it? That >> will ensure that the change is user-invisible. > > l_info isn't an exported field, is it? So this wouldn't make a > difference. Hmm, I thought it was but then on re-reading dl_iterate_phdr, it's only possible to read through _DYNAMIC via the program headers. In that case I think H. J.'s latest patch seems fine in principle. I could do a proper review if you don't see any other issues here. Siddhesh ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340] 2021-09-16 17:38 ` H.J. Lu 2021-09-16 17:58 ` Siddhesh Poyarekar @ 2021-09-16 18:03 ` Florian Weimer 2021-09-16 22:14 ` H.J. Lu 2021-09-17 2:58 ` Siddhesh Poyarekar 1 sibling, 2 replies; 41+ messages in thread From: Florian Weimer @ 2021-09-16 18:03 UTC (permalink / raw) To: H.J. Lu; +Cc: Siddhesh Poyarekar, Carlos O'Donell, GNU C Library * H. J. Lu: > No. Relocation of vDSO dynamic section is done by elf_get_dynamic_info. > Here is a patch to remove the hack for vDSO. I have concerns that this does not actually work on MIPS. What happens if the object has a single RWX LOAD segment? What kind of flags will the link editor set on the DYNAMIC segment in this case? RWX or R, RX? I suspect it will be RWX, which means that we actually need the special case. Thanks, Florian ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340] 2021-09-16 18:03 ` Florian Weimer @ 2021-09-16 22:14 ` H.J. Lu 2021-09-17 2:58 ` Siddhesh Poyarekar 1 sibling, 0 replies; 41+ messages in thread From: H.J. Lu @ 2021-09-16 22:14 UTC (permalink / raw) To: Florian Weimer; +Cc: Siddhesh Poyarekar, Carlos O'Donell, GNU C Library On Thu, Sep 16, 2021 at 11:04 AM Florian Weimer <fweimer@redhat.com> wrote: > > * H. J. Lu: > > > No. Relocation of vDSO dynamic section is done by elf_get_dynamic_info. > > Here is a patch to remove the hack for vDSO. > > I have concerns that this does not actually work on MIPS. What happens > if the object has a single RWX LOAD segment? What kind of flags will > the link editor set on the DYNAMIC segment in this case? RWX or R, RX? > I suspect it will be RWX, which means that we actually need the special > case. > My patch should handle it in ld.so properly. Non-glibc consumers of PT_DYNAMIC segment need to handle it differently for these non-ABI conforming binaries. -- H.J. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340] 2021-09-16 18:03 ` Florian Weimer 2021-09-16 22:14 ` H.J. Lu @ 2021-09-17 2:58 ` Siddhesh Poyarekar 2021-09-17 3:46 ` Florian Weimer 1 sibling, 1 reply; 41+ messages in thread From: Siddhesh Poyarekar @ 2021-09-17 2:58 UTC (permalink / raw) To: Florian Weimer, H.J. Lu; +Cc: Carlos O'Donell, GNU C Library On 9/16/21 11:33 PM, Florian Weimer wrote: > * H. J. Lu: > >> No. Relocation of vDSO dynamic section is done by elf_get_dynamic_info. >> Here is a patch to remove the hack for vDSO. > > I have concerns that this does not actually work on MIPS. What happens > if the object has a single RWX LOAD segment? What kind of flags will > the link editor set on the DYNAMIC segment in this case? RWX or R, RX? > I suspect it will be RWX, which means that we actually need the special > case. The DYNAMIC segment flags will be based on .dynamic, which is read-only for MIPS. Siddhesh ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340] 2021-09-17 2:58 ` Siddhesh Poyarekar @ 2021-09-17 3:46 ` Florian Weimer 2021-09-17 4:00 ` Florian Weimer 2021-10-14 12:36 ` [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340] Maciej W. Rozycki 0 siblings, 2 replies; 41+ messages in thread From: Florian Weimer @ 2021-09-17 3:46 UTC (permalink / raw) To: Siddhesh Poyarekar; +Cc: H.J. Lu, Carlos O'Donell, GNU C Library * Siddhesh Poyarekar: > On 9/16/21 11:33 PM, Florian Weimer wrote: >> * H. J. Lu: >> >>> No. Relocation of vDSO dynamic section is done by elf_get_dynamic_info. >>> Here is a patch to remove the hack for vDSO. >> I have concerns that this does not actually work on MIPS. What >> happens >> if the object has a single RWX LOAD segment? What kind of flags will >> the link editor set on the DYNAMIC segment in this case? RWX or R, RX? >> I suspect it will be RWX, which means that we actually need the special >> case. > > The DYNAMIC segment flags will be based on .dynamic, which is > read-only for MIPS. Right, as far as I can tell BFD ld does not copy the W bit to the DYNAMIC segment even if it is located inside an RW LOAD segment. So unless there are old binaries floating around which were linked differently, the change should be correct for MIPS, too (and likewise for RISC-V, although I did not check that). Thanks, Florian ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340] 2021-09-17 3:46 ` Florian Weimer @ 2021-09-17 4:00 ` Florian Weimer 2021-09-17 4:12 ` [PATCH] ld.so: Remove DL_RO_DYN_SECTION H.J. Lu 2021-10-14 12:36 ` [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340] Maciej W. Rozycki 1 sibling, 1 reply; 41+ messages in thread From: Florian Weimer @ 2021-09-17 4:00 UTC (permalink / raw) To: Siddhesh Poyarekar; +Cc: H.J. Lu, Carlos O'Donell, GNU C Library * Florian Weimer: > * Siddhesh Poyarekar: > >> On 9/16/21 11:33 PM, Florian Weimer wrote: >>> * H. J. Lu: >>> >>>> No. Relocation of vDSO dynamic section is done by elf_get_dynamic_info. >>>> Here is a patch to remove the hack for vDSO. >>> I have concerns that this does not actually work on MIPS. What >>> happens >>> if the object has a single RWX LOAD segment? What kind of flags will >>> the link editor set on the DYNAMIC segment in this case? RWX or R, RX? >>> I suspect it will be RWX, which means that we actually need the special >>> case. >> >> The DYNAMIC segment flags will be based on .dynamic, which is >> read-only for MIPS. > > Right, as far as I can tell BFD ld does not copy the W bit to the > DYNAMIC segment even if it is located inside an RW LOAD segment. So > unless there are old binaries floating around which were linked > differently, the change should be correct for MIPS, too (and likewise > for RISC-V, although I did not check that). *sigh* I checked riscv64-linux-gnu-rv64imafdc-lp64d now, and binutils BFD ld is actually inconsistent with glibc there. There, DYNAMIC is RW: LOAD 0x0003f0 0x00000000000003f0 0x00000000000003f0 0x0001c0 0x0001c0 RW 0x1000 DYNAMIC 0x000440 0x0000000000000440 0x0000000000000440 0x000170 0x000170 RW 0x8 So libphobos and others would have to change if we adopt H.J.'s patch, I think. With it, RISC-V would be just like all the other non-MIPS targets. Thanks, Florian ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH] ld.so: Remove DL_RO_DYN_SECTION 2021-09-17 4:00 ` Florian Weimer @ 2021-09-17 4:12 ` H.J. Lu 2021-09-17 6:54 ` David Abdurachmanov 2021-09-17 9:01 ` Siddhesh Poyarekar 0 siblings, 2 replies; 41+ messages in thread From: H.J. Lu @ 2021-09-17 4:12 UTC (permalink / raw) To: Florian Weimer; +Cc: Siddhesh Poyarekar, Carlos O'Donell, GNU C Library [-- Attachment #1: Type: text/plain, Size: 1711 bytes --] On Thu, Sep 16, 2021 at 9:01 PM Florian Weimer <fweimer@redhat.com> wrote: > > * Florian Weimer: > > > * Siddhesh Poyarekar: > > > >> On 9/16/21 11:33 PM, Florian Weimer wrote: > >>> * H. J. Lu: > >>> > >>>> No. Relocation of vDSO dynamic section is done by elf_get_dynamic_info. > >>>> Here is a patch to remove the hack for vDSO. > >>> I have concerns that this does not actually work on MIPS. What > >>> happens > >>> if the object has a single RWX LOAD segment? What kind of flags will > >>> the link editor set on the DYNAMIC segment in this case? RWX or R, RX? > >>> I suspect it will be RWX, which means that we actually need the special > >>> case. > >> > >> The DYNAMIC segment flags will be based on .dynamic, which is > >> read-only for MIPS. > > > > Right, as far as I can tell BFD ld does not copy the W bit to the > > DYNAMIC segment even if it is located inside an RW LOAD segment. So > > unless there are old binaries floating around which were linked > > differently, the change should be correct for MIPS, too (and likewise > > for RISC-V, although I did not check that). > > *sigh* > > I checked riscv64-linux-gnu-rv64imafdc-lp64d now, and binutils BFD ld is > actually inconsistent with glibc there. There, DYNAMIC is RW: > > LOAD 0x0003f0 0x00000000000003f0 0x00000000000003f0 0x0001c0 0x0001c0 RW 0x1000 > DYNAMIC 0x000440 0x0000000000000440 0x0000000000000440 0x000170 0x000170 RW 0x8 > > So libphobos and others would have to change if we adopt H.J.'s patch, I > think. With it, RISC-V would be just like all the other non-MIPS > targets. > Glibc is wrong here. My patch will make it correct. Here is the updated patch with the new commit log. -- H.J. [-- Attachment #2: 0001-ld.so-Remove-DL_RO_DYN_SECTION.patch --] [-- Type: application/x-patch, Size: 9157 bytes --] ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] ld.so: Remove DL_RO_DYN_SECTION 2021-09-17 4:12 ` [PATCH] ld.so: Remove DL_RO_DYN_SECTION H.J. Lu @ 2021-09-17 6:54 ` David Abdurachmanov 2021-09-17 9:01 ` Siddhesh Poyarekar 1 sibling, 0 replies; 41+ messages in thread From: David Abdurachmanov @ 2021-09-17 6:54 UTC (permalink / raw) To: H.J. Lu; +Cc: Florian Weimer, Siddhesh Poyarekar, GNU C Library On Fri, Sep 17, 2021 at 7:12 AM H.J. Lu via Libc-alpha <libc-alpha@sourceware.org> wrote: > > On Thu, Sep 16, 2021 at 9:01 PM Florian Weimer <fweimer@redhat.com> wrote: > > > > * Florian Weimer: > > > > > * Siddhesh Poyarekar: > > > > > >> On 9/16/21 11:33 PM, Florian Weimer wrote: > > >>> * H. J. Lu: > > >>> > > >>>> No. Relocation of vDSO dynamic section is done by elf_get_dynamic_info. > > >>>> Here is a patch to remove the hack for vDSO. > > >>> I have concerns that this does not actually work on MIPS. What > > >>> happens > > >>> if the object has a single RWX LOAD segment? What kind of flags will > > >>> the link editor set on the DYNAMIC segment in this case? RWX or R, RX? > > >>> I suspect it will be RWX, which means that we actually need the special > > >>> case. > > >> > > >> The DYNAMIC segment flags will be based on .dynamic, which is > > >> read-only for MIPS. > > > > > > Right, as far as I can tell BFD ld does not copy the W bit to the > > > DYNAMIC segment even if it is located inside an RW LOAD segment. So > > > unless there are old binaries floating around which were linked > > > differently, the change should be correct for MIPS, too (and likewise > > > for RISC-V, although I did not check that). > > > > *sigh* > > > > I checked riscv64-linux-gnu-rv64imafdc-lp64d now, and binutils BFD ld is > > actually inconsistent with glibc there. There, DYNAMIC is RW: > > > > LOAD 0x0003f0 0x00000000000003f0 0x00000000000003f0 0x0001c0 0x0001c0 RW 0x1000 > > DYNAMIC 0x000440 0x0000000000000440 0x0000000000000440 0x000170 0x000170 RW 0x8 > > > > So libphobos and others would have to change if we adopt H.J.'s patch, I > > think. With it, RISC-V would be just like all the other non-MIPS > > targets. > > > > Glibc is wrong here. My patch will make it correct. > > Here is the updated patch with the new commit log. Back in 2019 I tried to remove DL_RO_DYN_SECTION for riscv while working on libphobos as it was a copy & paste mistake during the initial porting. IIRC there was no way in libphobos to dynamically check for glibc version and do different things. https://github.com/gcc-mirror/gcc/blob/master/libphobos/libdruntime/gcc/sections/elf.d#L763 Sadly the change was reverted back in the same 2019 as ABI change. https://sourceware.org/bugzilla/show_bug.cgi?id=24484 david > > -- > H.J. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] ld.so: Remove DL_RO_DYN_SECTION 2021-09-17 4:12 ` [PATCH] ld.so: Remove DL_RO_DYN_SECTION H.J. Lu 2021-09-17 6:54 ` David Abdurachmanov @ 2021-09-17 9:01 ` Siddhesh Poyarekar 2021-09-17 15:40 ` H.J. Lu 1 sibling, 1 reply; 41+ messages in thread From: Siddhesh Poyarekar @ 2021-09-17 9:01 UTC (permalink / raw) To: H.J. Lu, Florian Weimer; +Cc: GNU C Library On 9/17/21 9:42 AM, H.J. Lu via Libc-alpha wrote: > Glibc is wrong here. My patch will make it correct. > > Here is the updated patch with the new commit log. > As an aside, please send patches with fresh subject line; patchwork does not capture replies. Thanks, Siddhesh ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] ld.so: Remove DL_RO_DYN_SECTION 2021-09-17 9:01 ` Siddhesh Poyarekar @ 2021-09-17 15:40 ` H.J. Lu 0 siblings, 0 replies; 41+ messages in thread From: H.J. Lu @ 2021-09-17 15:40 UTC (permalink / raw) To: Siddhesh Poyarekar; +Cc: Florian Weimer, GNU C Library On Fri, Sep 17, 2021 at 2:02 AM Siddhesh Poyarekar <siddhesh@sourceware.org> wrote: > > On 9/17/21 9:42 AM, H.J. Lu via Libc-alpha wrote: > > Glibc is wrong here. My patch will make it correct. > > > > Here is the updated patch with the new commit log. > > > > As an aside, please send patches with fresh subject line; patchwork > does not capture replies. Done. I sent out the v2 patch. -- H.J. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340] 2021-09-17 3:46 ` Florian Weimer 2021-09-17 4:00 ` Florian Weimer @ 2021-10-14 12:36 ` Maciej W. Rozycki 1 sibling, 0 replies; 41+ messages in thread From: Maciej W. Rozycki @ 2021-10-14 12:36 UTC (permalink / raw) To: Florian Weimer; +Cc: Siddhesh Poyarekar, GNU C Library On Fri, 17 Sep 2021, Florian Weimer via Libc-alpha wrote: > >> I have concerns that this does not actually work on MIPS. What > >> happens > >> if the object has a single RWX LOAD segment? What kind of flags will > >> the link editor set on the DYNAMIC segment in this case? RWX or R, RX? > >> I suspect it will be RWX, which means that we actually need the special > >> case. > > > > The DYNAMIC segment flags will be based on .dynamic, which is > > read-only for MIPS. > > Right, as far as I can tell BFD ld does not copy the W bit to the > DYNAMIC segment even if it is located inside an RW LOAD segment. So > unless there are old binaries floating around which were linked > differently, the change should be correct for MIPS, too (and likewise > for RISC-V, although I did not check that). For the record AFAICT the MIPS/Linux target has always had the DYNAMIC segment read-only and the `.dynamic' section also mapped to a read-only LOAD segment along with `.text', `.rodata', etc. Here's output from the oldest MIPS/Linux DSO I could find on my system: $ readelf -l libcrack.so.2.0.7 Elf file type is DYN (Shared object file) Entry point 0x5ffe0eb0 There are 5 program headers, starting at offset 52 Program Headers: Type Offset VirtAddr PhysAddr FileSiz MemSiz Flg Align REGINFO 0x0000e0 0x5ffe00e0 0x5ffe00e0 0x00018 0x00018 R 0x10 LOAD 0x000000 0x5ffe0000 0x5ffe0000 0x07500 0x07500 R E 0x1000 LOAD 0x007500 0x60027500 0x60027500 0x007b4 0x03c10 RW 0x1000 DYNAMIC 0x000100 0x5ffe0100 0x5ffe0100 0x00c27 0x00c27 R 0x10 RTPROC 0x000000 0x00000000 0x00000000 0x00000 0x00000 R 0x4 Section to Segment mapping: Segment Sections... 00 .reginfo 01 .reginfo .dynamic .hash .dynsym .dynstr .gnu.version .gnu.version_r .init .text .fini .rodata .rel.dyn 02 .data .eh_frame .ctors .dtors .got .sbss .bss 03 .dynamic .hash .dynsym .dynstr 04 $ ls -l libcrack.so.2.0.7 -rwxr-xr-x 1 root root 41004 May 2 2001 libcrack.so.2.0.7 $ Likewise the oldest executable: $ readelf -l uuencode Elf file type is EXEC (Executable file) Entry point 0x4009f0 There are 7 program headers, starting at offset 52 Program Headers: Type Offset VirtAddr PhysAddr FileSiz MemSiz Flg Align PHDR 0x000034 0x00400034 0x00400034 0x000e0 0x000e0 R E 0x4 INTERP 0x000134 0x00400134 0x00400134 0x0000d 0x0000d R 0x1 [Requesting program interpreter: /lib/ld.so.1] REGINFO 0x000170 0x00400170 0x00400170 0x00018 0x00018 R 0x10 LOAD 0x000000 0x00400000 0x00400000 0x01f40 0x01f40 R E 0x1000 LOAD 0x002000 0x10000000 0x10000000 0x00160 0x00190 RW 0x1000 DYNAMIC 0x000190 0x00400190 0x00400190 0x00797 0x00797 R 0x10 NOTE 0x000150 0x00400150 0x00400150 0x00020 0x00020 R 0x10 Section to Segment mapping: Segment Sections... 00 01 .interp 02 .reginfo 03 .interp .note.ABI-tag .reginfo .dynamic .hash .dynsym .dynstr .gnu.version .gnu.version_r .init .text .fini .rodata 04 .data .rld_map .eh_frame .ctors .dtors .got .sbss .bss 05 .dynamic .hash .dynsym .dynstr 06 .note.ABI-tag $ ls -l uuencode -rwxr-xr-x 1 root root 10076 Jul 26 2000 uuencode $ These are over 20 years old now, so I guess we need not look further back. FWIW, Maciej ^ permalink raw reply [flat|nested] 41+ messages in thread
end of thread, other threads:[~2021-10-14 12:36 UTC | newest] Thread overview: 41+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-14 19:09 [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340] Siddhesh Poyarekar 2021-09-14 19:15 ` Florian Weimer 2021-09-15 1:14 ` Siddhesh Poyarekar 2021-09-15 14:35 ` H.J. Lu 2021-09-15 15:42 ` Siddhesh Poyarekar 2021-09-15 16:13 ` H.J. Lu 2021-09-15 16:24 ` Siddhesh Poyarekar 2021-09-15 16:34 ` H.J. Lu 2021-09-16 1:43 ` Siddhesh Poyarekar 2021-09-16 2:23 ` H.J. Lu 2021-09-16 3:46 ` Siddhesh Poyarekar 2021-09-16 4:26 ` H.J. Lu 2021-09-16 4:28 ` Siddhesh Poyarekar 2021-09-16 4:30 ` H.J. Lu 2021-09-16 4:48 ` Florian Weimer 2021-09-16 5:36 ` Siddhesh Poyarekar 2021-09-16 5:46 ` Florian Weimer 2021-09-16 6:04 ` Siddhesh Poyarekar 2021-09-16 14:11 ` Carlos O'Donell 2021-09-16 15:18 ` H.J. Lu 2021-09-16 16:45 ` Siddhesh Poyarekar 2021-09-16 17:38 ` H.J. Lu 2021-09-16 17:58 ` Siddhesh Poyarekar 2021-09-16 22:11 ` H.J. Lu 2021-09-17 2:47 ` Siddhesh Poyarekar 2021-09-17 2:59 ` H.J. Lu 2021-09-17 3:36 ` Siddhesh Poyarekar 2021-09-17 3:42 ` H.J. Lu 2021-09-17 3:44 ` Siddhesh Poyarekar 2021-09-17 3:44 ` Florian Weimer 2021-09-17 3:51 ` Siddhesh Poyarekar 2021-09-16 18:03 ` Florian Weimer 2021-09-16 22:14 ` H.J. Lu 2021-09-17 2:58 ` Siddhesh Poyarekar 2021-09-17 3:46 ` Florian Weimer 2021-09-17 4:00 ` Florian Weimer 2021-09-17 4:12 ` [PATCH] ld.so: Remove DL_RO_DYN_SECTION H.J. Lu 2021-09-17 6:54 ` David Abdurachmanov 2021-09-17 9:01 ` Siddhesh Poyarekar 2021-09-17 15:40 ` H.J. Lu 2021-10-14 12:36 ` [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340] Maciej W. Rozycki
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).