[PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340]

[PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340]

[Siddhesh Poyarekar]

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

Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340]

[Florian Weimer]

* 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

Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340]

[Siddhesh Poyarekar]

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

Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340]

[H.J. Lu]

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.

Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340]

[Siddhesh Poyarekar]

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

Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340]

[H.J. Lu]

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.

Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340]

[Siddhesh Poyarekar]

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

Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340]

[H.J. Lu]

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.

Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340]

[Siddhesh Poyarekar]

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

Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340]

[H.J. Lu]

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.

Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340]

[Siddhesh Poyarekar]

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

Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340]

[H.J. Lu]

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.

Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340]

[Siddhesh Poyarekar]

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

Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340]

[H.J. Lu]

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.

Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340]

[Florian Weimer]

* 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

Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340]

[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.  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

Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340]

[Florian Weimer]

* 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

Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340]

[Siddhesh Poyarekar]

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

Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340]

[Carlos O'Donell]

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.

Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340]

[H.J. Lu]

[-- 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

Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340]

[Siddhesh Poyarekar]

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

Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340]

[H.J. Lu]

[-- 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

Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340]

[Siddhesh Poyarekar]

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

Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340]

[H.J. Lu]

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.

Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340]

[Siddhesh Poyarekar]

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

Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340]

[H.J. Lu]

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.

Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340]

[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.

Siddhesh

Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340]

[H.J. Lu]

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.

Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340]

[Siddhesh Poyarekar]

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

Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340]

[Florian Weimer]

* 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

Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340]

[Siddhesh Poyarekar]

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

Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340]

[Florian Weimer]

* 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

Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340]

[H.J. Lu]

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.

Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340]

[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.

Siddhesh

Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340]

[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).

Thanks,
Florian

Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340]

[Florian Weimer]

* 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

[PATCH] ld.so: Remove DL_RO_DYN_SECTION

[H.J. Lu]

[-- 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 --]

Re: [PATCH] ld.so: Remove DL_RO_DYN_SECTION

[David Abdurachmanov]

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.

Re: [PATCH] ld.so: Remove DL_RO_DYN_SECTION

[Siddhesh Poyarekar]

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

Re: [PATCH] ld.so: Remove DL_RO_DYN_SECTION

[H.J. Lu]

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.

Re: [PATCH] ld.so: Handle read-only dynamic section gracefully [BZ #28340]

[Maciej W. Rozycki]

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