Message ID | 20210914190919.1728320-1-siddhesh@sourceware.org |
---|---|
State | New |
Headers | show |
Series | ld.so: Handle read-only dynamic section gracefully [BZ #28340] | expand |
* 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
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
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.
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
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?
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
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.
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
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.
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
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?
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
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. 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
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
* 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
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
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.
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.
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
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.
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
* 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
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.
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.
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
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
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
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
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.
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
* 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
* 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
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
* 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
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.
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.
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
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.
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
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