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