This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] ld.so: Examine GLRO to detect inactive loader [BZ #20204]
On 12/18/2017 07:25 PM, Carlos O'Donell wrote:
I looked into trying to write a test case for this, but it's not trivial.
There is an existing test case, it's dlfcn/tststatic2, I think. It
calls dlopen etc. from an inner DSO, so it needs the dlfcn hooks.
In both directions, it is self-asserting: non-static dlopen will never
call the hook functions, so a wrong rtld_active result in a fully
dynamic application will cause a null pointer dereference. Similarly,
failure to correctly detect an incative ld.so in a static dlopen
situation will lead to a non-working dynamic linker.
+/* Return true if the ld.so copy in this namespace is actually active
+ and working. If false, the dl_open/dlfcn hooks have to be used to
+ call into the outer dynamic linker (which happens after static
+ dlopen). */
+#ifdef SHARED
+static inline bool
+rtld_active (void)
+{
+ /* The default-initialized variable does not have a non-zero
+ dl_init_all_dirs member, so this allows us to recognize an
+ initialized and active ld.so copy. */
+ return GLRO(dl_init_all_dirs) != NULL;
We need to add a comment to the definition of GLRO(dl_init_all_dirs) that
it is being used as the initialization marker, then at assignment in rtld.c
we need a similar comment. This ties the definition, assignment, and usage
together in a meaningful way.
Sure, makes sense.
I'm testing the attached patch now. It also changes the hook check used
for libio vtable compatibility across multiple namespaces. I'll commit
this if testing passes. (We have some minimal test coverage for the
libio vtables check, too.)
Thanks,
Florian
Subject: [PATCH] ld.so: Examine GLRO to detect inactive loader [BZ #20204]
To: libc-alpha@sourceware.org
GLRO (_rtld_global_ro) is read-only after initialization and can
therefore not be patched at run time, unlike the hook table addresses
and their contents, so this is a desirable hardening feature.
The hooks are only needed if ld.so has not been initialized, and this
happens only after static dlopen (dlmopen uses a single ld.so object
across all namespaces).
Reviewed-by: Carlos O'Donell <carlos@redhat.com>
2017-12-18 Florian Weimer <fweimer@redhat.com>
[BZ #20204]
ld.so: Harden dl-libc/libdl hooks.
* sysdeps/generic/ldsodefs.h (_dl_init_all_dirs): Update comment.
(rtld_active): New function.
* dlfcn/dladdr.c (__dladdr): Call it.
* dlfcn/dladdr1.c (__dladdr1): Likewise.
* dlfcn/dlclose.c (__dlcose): Likewise.
* dlfcn/dlerror.c (__dlerror): Likewise.
* dlfcn/dlinfo.c (__dlinfo): Likewise.
* dlfcn/dlmopen.c (__dlmopen): Likewise.
* dlfcn/dlopen.c (__dlopen): Likewise.
* dlfcn/dlopenold.c (__dlopen_nocheck): Likewise.
* dlfcn/dlsym.c (__dlsym): Likewise.
* dlfcn/dlvsym.c (__dlvsym): Likewise.
* libio/vtables.c (_IO_vtable_check): Likewise.
* elf/dl-libc.c (__libc_dlopen_mode, __libc_dlsym)
(__libc_dlclose): Likewise.
* elf/rtld.c (dl_main): Update comment on the _dl_init_all_dirs
assignment.
diff --git a/dlfcn/dladdr.c b/dlfcn/dladdr.c
index 1753434160..1bebd00240 100644
--- a/dlfcn/dladdr.c
+++ b/dlfcn/dladdr.c
@@ -17,6 +17,7 @@
<http://www.gnu.org/licenses/>. */
#include <dlfcn.h>
+#include <ldsodefs.h>
#if !defined SHARED && IS_IN (libdl)
@@ -32,7 +33,7 @@ int
__dladdr (const void *address, Dl_info *info)
{
# ifdef SHARED
- if (__glibc_unlikely (_dlfcn_hook != NULL))
+ if (!rtld_active ())
return _dlfcn_hook->dladdr (address, info);
# endif
return _dl_addr (address, info, NULL, NULL);
diff --git a/dlfcn/dladdr1.c b/dlfcn/dladdr1.c
index a19f9fdea2..901cf43f6b 100644
--- a/dlfcn/dladdr1.c
+++ b/dlfcn/dladdr1.c
@@ -17,6 +17,7 @@
<http://www.gnu.org/licenses/>. */
#include <dlfcn.h>
+#include <ldsodefs.h>
#if !defined SHARED && IS_IN (libdl)
@@ -32,7 +33,7 @@ int
__dladdr1 (const void *address, Dl_info *info, void **extra, int flags)
{
# ifdef SHARED
- if (__glibc_unlikely (_dlfcn_hook != NULL))
+ if (!rtld_active ())
return _dlfcn_hook->dladdr1 (address, info, extra, flags);
# endif
diff --git a/dlfcn/dlclose.c b/dlfcn/dlclose.c
index da66e20488..223887d338 100644
--- a/dlfcn/dlclose.c
+++ b/dlfcn/dlclose.c
@@ -39,7 +39,7 @@ int
__dlclose (void *handle)
{
# ifdef SHARED
- if (__glibc_unlikely (_dlfcn_hook != NULL))
+ if (!rtld_active ())
return _dlfcn_hook->dlclose (handle);
# endif
diff --git a/dlfcn/dlerror.c b/dlfcn/dlerror.c
index fb5012ee85..b33c05095a 100644
--- a/dlfcn/dlerror.c
+++ b/dlfcn/dlerror.c
@@ -63,7 +63,7 @@ __dlerror (void)
struct dl_action_result *result;
# ifdef SHARED
- if (__glibc_unlikely (_dlfcn_hook != NULL))
+ if (!rtld_active ())
return _dlfcn_hook->dlerror ();
# endif
diff --git a/dlfcn/dlinfo.c b/dlfcn/dlinfo.c
index a34e947ed3..b011257468 100644
--- a/dlfcn/dlinfo.c
+++ b/dlfcn/dlinfo.c
@@ -111,7 +111,7 @@ int
__dlinfo (void *handle, int request, void *arg DL_CALLER_DECL)
{
# ifdef SHARED
- if (__glibc_unlikely (_dlfcn_hook != NULL))
+ if (!rtld_active ())
return _dlfcn_hook->dlinfo (handle, request, arg,
DL_CALLER);
# endif
diff --git a/dlfcn/dlmopen.c b/dlfcn/dlmopen.c
index 07d59ade30..58f88bb7c6 100644
--- a/dlfcn/dlmopen.c
+++ b/dlfcn/dlmopen.c
@@ -79,7 +79,7 @@ void *
__dlmopen (Lmid_t nsid, const char *file, int mode DL_CALLER_DECL)
{
# ifdef SHARED
- if (__glibc_unlikely (_dlfcn_hook != NULL))
+ if (!rtld_active ())
return _dlfcn_hook->dlmopen (nsid, file, mode, RETURN_ADDRESS (0));
# endif
diff --git a/dlfcn/dlopen.c b/dlfcn/dlopen.c
index 22120655d2..73651a8430 100644
--- a/dlfcn/dlopen.c
+++ b/dlfcn/dlopen.c
@@ -74,7 +74,7 @@ void *
__dlopen (const char *file, int mode DL_CALLER_DECL)
{
# ifdef SHARED
- if (__glibc_unlikely (_dlfcn_hook != NULL))
+ if (!rtld_active ())
return _dlfcn_hook->dlopen (file, mode, DL_CALLER);
# endif
diff --git a/dlfcn/dlopenold.c b/dlfcn/dlopenold.c
index a3db500705..d899c4e890 100644
--- a/dlfcn/dlopenold.c
+++ b/dlfcn/dlopenold.c
@@ -70,7 +70,7 @@ __dlopen_nocheck (const char *file, int mode)
mode |= RTLD_LAZY;
args.mode = mode;
- if (__glibc_unlikely (_dlfcn_hook != NULL))
+ if (!rtld_active ())
return _dlfcn_hook->dlopen (file, mode, RETURN_ADDRESS (0));
return _dlerror_run (dlopen_doit, &args) ? NULL : args.new;
diff --git a/dlfcn/dlsym.c b/dlfcn/dlsym.c
index 7976c5f75c..19733a0f19 100644
--- a/dlfcn/dlsym.c
+++ b/dlfcn/dlsym.c
@@ -55,7 +55,7 @@ void *
__dlsym (void *handle, const char *name DL_CALLER_DECL)
{
# ifdef SHARED
- if (__glibc_unlikely (_dlfcn_hook != NULL))
+ if (!rtld_active ())
return _dlfcn_hook->dlsym (handle, name, DL_CALLER);
# endif
diff --git a/dlfcn/dlvsym.c b/dlfcn/dlvsym.c
index 5ed220b77c..ad46b65023 100644
--- a/dlfcn/dlvsym.c
+++ b/dlfcn/dlvsym.c
@@ -58,7 +58,7 @@ __dlvsym (void *handle, const char *name, const char *version_str
DL_CALLER_DECL)
{
# ifdef SHARED
- if (__glibc_unlikely (_dlfcn_hook != NULL))
+ if (!rtld_active ())
return _dlfcn_hook->dlvsym (handle, name, version_str, DL_CALLER);
# endif
diff --git a/elf/dl-libc.c b/elf/dl-libc.c
index bd3c18d20f..7d9a8948f3 100644
--- a/elf/dl-libc.c
+++ b/elf/dl-libc.c
@@ -157,7 +157,7 @@ __libc_dlopen_mode (const char *name, int mode)
args.caller_dlopen = RETURN_ADDRESS (0);
#ifdef SHARED
- if (__glibc_unlikely (_dl_open_hook != NULL))
+ if (!rtld_active ())
return _dl_open_hook->dlopen_mode (name, mode);
return (dlerror_run (do_dlopen, &args) ? NULL : (void *) args.map);
#else
@@ -203,7 +203,7 @@ __libc_dlsym (void *map, const char *name)
args.name = name;
#ifdef SHARED
- if (__glibc_unlikely (_dl_open_hook != NULL))
+ if (!rtld_active ())
return _dl_open_hook->dlsym (map, name);
#endif
return (dlerror_run (do_dlsym, &args) ? NULL
@@ -215,7 +215,7 @@ int
__libc_dlclose (void *map)
{
#ifdef SHARED
- if (__glibc_unlikely (_dl_open_hook != NULL))
+ if (!rtld_active ())
return _dl_open_hook->dlclose (map);
#endif
return dlerror_run (do_dlclose, map);
diff --git a/elf/rtld.c b/elf/rtld.c
index cfd3729b8e..c01b7e3641 100644
--- a/elf/rtld.c
+++ b/elf/rtld.c
@@ -2096,7 +2096,9 @@ ERROR: ld.so: object '%s' cannot be loaded as audit interface: %s; ignored.\n",
GLRO(dl_initial_searchlist) = *GL(dl_ns)[LM_ID_BASE]._ns_main_searchlist;
/* Remember the last search directory added at startup, now that
- malloc will no longer be the one from dl-minimal.c. */
+ malloc will no longer be the one from dl-minimal.c. As a side
+ effect, this marks ld.so as initialized, so that the rtld_active
+ function returns true from now on. */
GLRO(dl_init_all_dirs) = GL(dl_all_dirs);
/* Print scope information. */
diff --git a/libio/vtables.c b/libio/vtables.c
index 41b48db98c..4d4afa2efc 100644
--- a/libio/vtables.c
+++ b/libio/vtables.c
@@ -19,6 +19,7 @@
#include <dlfcn.h>
#include <libioP.h>
#include <stdio.h>
+#include <ldsodefs.h>
#ifdef SHARED
@@ -54,7 +55,7 @@ _IO_vtable_check (void)
{
Dl_info di;
struct link_map *l;
- if (_dl_open_hook != NULL
+ if (!rtld_active ()
|| (_dl_addr (_IO_vtable_check, &di, &l, NULL) != 0
&& l->l_ns != LM_ID_BASE))
return;
diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h
index 196513851f..658a4f20b4 100644
--- a/sysdeps/generic/ldsodefs.h
+++ b/sysdeps/generic/ldsodefs.h
@@ -558,7 +558,11 @@ struct rtld_global_ro
/* Map of shared object to be prelink traced. */
EXTERN struct link_map *_dl_trace_prelink_map;
- /* All search directories defined at startup. */
+ /* All search directories defined at startup. This is assigned a
+ non-NULL pointer by the ld.so startup code (after initialization
+ to NULL), so this can also serve as an indicator whether a copy
+ of ld.so is initialized and active. See the rtld_active function
+ below. */
EXTERN struct r_search_path_elem *_dl_init_all_dirs;
#ifdef NEED_DL_SYSINFO
@@ -1144,6 +1148,20 @@ extern void _dl_non_dynamic_init (void)
extern void _dl_aux_init (ElfW(auxv_t) *av)
attribute_hidden;
+/* Return true if the ld.so copy in this namespace is actually active
+ and working. If false, the dl_open/dlfcn hooks have to be used to
+ call into the outer dynamic linker (which happens after static
+ dlopen). */
+#ifdef SHARED
+static inline bool
+rtld_active (void)
+{
+ /* The default-initialized variable does not have a non-zero
+ dl_init_all_dirs member, so this allows us to recognize an
+ initialized and active ld.so copy. */
+ return GLRO(dl_init_all_dirs) != NULL;
+}
+#endif
__END_DECLS