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

Message ID 20210915013653.1802776-1-siddhesh@sourceware.org
State New
Headers show
Series
  • [v2] ld.so: Handle read-only dynamic section gracefully [BZ #28340]
Related show

Commit Message

H.J. Lu via Libc-alpha Sept. 15, 2021, 1:36 a.m.
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 l_addr is non-zero, indicating
the need for fixing up entries in .dynamic.  A test case has also been
added to verify that BZ #28340 has been fixed.
---
Changes from v1:
- Assume that all programs with a .dynamic section need a fixup if
  l_addr != 0.  Drop elf_dynamic_info_needs_fixup()  to that effect.
- Rename tst-ro-dynamic.map to tst-ro-dynamic-mod.map for consistency.

 elf/Makefile               | 11 +++++++++--
 elf/dl-load.c              | 14 ++++++++++++++
 elf/rtld.c                 | 12 ++++++++++++
 elf/tst-ro-dynamic-mod.c   |  1 +
 elf/tst-ro-dynamic-mod.map | 13 +++++++++++++
 elf/tst-ro-dynamic.sh      | 36 ++++++++++++++++++++++++++++++++++++
 6 files changed, 85 insertions(+), 2 deletions(-)
 create mode 100644 elf/tst-ro-dynamic-mod.c
 create mode 100644 elf/tst-ro-dynamic-mod.map
 create mode 100644 elf/tst-ro-dynamic.sh

-- 
2.31.1

Comments

H.J. Lu via Libc-alpha Sept. 15, 2021, 10:40 a.m. | #1
* Siddhesh Poyarekar:

> @@ -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;


As far as I can tell, this does not check whether the DYNAMIC segment is
actually covered by a read-write LOAD segment.  I wonder how much value
this imperfect check has.

Would it make more sense to fix the kernel linker script that generates
those bogus vDSO reference files?

Thanks,
Florian
H.J. Lu via Libc-alpha Sept. 15, 2021, 12:10 p.m. | #2
On 9/15/21 4:10 PM, Florian Weimer wrote:
> As far as I can tell, this does not check whether the DYNAMIC segment is

> actually covered by a read-write LOAD segment.  I wonder how much value

> this imperfect check has.


I reckon there's less value in trying to make this corner case work; I 
can't think of a reason for someone (outside of the VDSO use case) to do 
this on purpose.  Read-only DYNAMIC segments in ET_DYN objects shouldn't 
be a supported use case.

> Would it make more sense to fix the kernel linker script that generates

> those bogus vDSO reference files?


The vdso is not meant to be relocated, which is why all segments in it 
are read-only.  setup_vdso only pretends to relocate it in the interest 
of internal consistency in ld.so.

Siddhesh
H.J. Lu via Libc-alpha Sept. 15, 2021, 12:18 p.m. | #3
* Siddhesh Poyarekar:

> On 9/15/21 4:10 PM, Florian Weimer wrote:

>> As far as I can tell, this does not check whether the DYNAMIC segment is

>> actually covered by a read-write LOAD segment.  I wonder how much value

>> this imperfect check has.

>

> I reckon there's less value in trying to make this corner case work; I

> can't think of a reason for someone (outside of the VDSO use case) to

> do this on purpose.  Read-only DYNAMIC segments in ET_DYN objects

> shouldn't be a supported use case.


Sure, but the corresponding LOAD segment is read-only as well, that's
what actually matters.

>> Would it make more sense to fix the kernel linker script that generates

>> those bogus vDSO reference files?

>

> The vdso is not meant to be relocated, which is why all segments in it

> are read-only.  setup_vdso only pretends to relocate it in the

> interest of internal consistency in ld.so.


And they do not want multiple LOAD segments because it can fit all on
the same page.  We would end up with an RWX segment if we made this
single LOAD segment writable, which is probably even more confusing.

So I guess the status quo is probably best.

Maybe someone else can comment on the additional detection.  In the
past, we said that crashes on corrupt ELF files weren't something we
cared about, but I increasingly see th value of good diagnostics for
common special cases.

(See also the discussion about DSOs without entry points.)

Thanks,
Florian
H.J. Lu via Libc-alpha Sept. 15, 2021, 1:04 p.m. | #4
On 9/15/21 5:48 PM, Florian Weimer via Libc-alpha wrote:
> * Siddhesh Poyarekar:

> 

>> On 9/15/21 4:10 PM, Florian Weimer wrote:

>>> As far as I can tell, this does not check whether the DYNAMIC segment is

>>> actually covered by a read-write LOAD segment.  I wonder how much value

>>> this imperfect check has.

>>

>> I reckon there's less value in trying to make this corner case work; I

>> can't think of a reason for someone (outside of the VDSO use case) to

>> do this on purpose.  Read-only DYNAMIC segments in ET_DYN objects

>> shouldn't be a supported use case.

> 

> Sure, but the corresponding LOAD segment is read-only as well, that's

> what actually matters.


I don't understand.  Why isn't the fact that the DYNAMIC segment is 
read-only sufficient to bail out with at error?  Or to rephrase, what 
valid use case would we be supporting by checking flags on the LOAD 
segment containing the .dynamic section to check for write permission?

I know that the crash wouldn't happen if the LOAD was read-write, but I 
don't know if the additional check covers a valid use case.

> And they do not want multiple LOAD segments because it can fit all on

> the same page.  We would end up with an RWX segment if we made this

> single LOAD segment writable, which is probably even more confusing.


Right.

> So I guess the status quo is probably best.


I don't know about that :)

> Maybe someone else can comment on the additional detection.  In the

> past, we said that crashes on corrupt ELF files weren't something we

> cared about, but I increasingly see th value of good diagnostics for

> common special cases.


This seems like a situation where we can add an inexpensive check and 
not crash.

> (See also the discussion about DSOs without entry points.)


I need to collect my thoughts about that.  I had run into it last year 
and I need to see if I had written something down then.  I'd definitely 
like to know if there is a historical perspective to setting .text as 
the entry point by default.

Siddhesh

Patch

diff --git a/elf/Makefile b/elf/Makefile
index 9f3fadc37e..406bb3ad23 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-mod.map
+$(objpfx)tst-ro-dynamic-mod.so: tst-ro-dynamic-mod.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..c33da0467e 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 && l->l_addr != 0)
+    {
+      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/rtld.c b/elf/rtld.c
index 878e6480f4..d8149f2bca 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 && main_map->l_addr != 0)
+	_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-mod.map b/elf/tst-ro-dynamic-mod.map
new file mode 100644
index 0000000000..dac92e0b94
--- /dev/null
+++ b/elf/tst-ro-dynamic-mod.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