diff mbox series

ld.so: Examine GLRO to detect inactive loader [BZ #20204]

Message ID 20171218123414.7096E43994576@oldenburg.str.redhat.com
State New
Headers show
Series ld.so: Examine GLRO to detect inactive loader [BZ #20204] | expand

Commit Message

Florian Weimer Dec. 18, 2017, 12:34 p.m. UTC
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).

2017-12-18  Florian Weimer  <fweimer@redhat.com>

	[BZ #20204]
	ld.so: Harden dl-libc/libdl hooks.
	* sysdeps/generic/ldsodefs.h (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.
	* elf/dl-libc.c (__libc_dlopen_mode, __libc_dlsym)
	(__libc_dlclose): Likewise.

Comments

Carlos O'Donell Dec. 18, 2017, 6:25 p.m. UTC | #1
On 12/18/2017 04:34 AM, Florian Weimer wrote:
> 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).

High level:

The hardening aspect of this change is valuable enough that it should
go in right away.

Design:

Refactoring the symbol check into a function call is great.

Implementation:

Needs 2 additional comments. See notes below.

---

I looked into trying to write a test case for this, but it's not trivial.

OK to checkin if you add the additional comments.

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 (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.
> 	* elf/dl-libc.c (__libc_dlopen_mode, __libc_dlsym)
> 	(__libc_dlclose): Likewise.
> 
> 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>

OK.

>  
>  #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 ())

OK. Nice change.

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

OK.

>  
>  #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 ())

OK.

>      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 ())

OK.

>      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 ())

OK.

>      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 ())

OK.

>      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 ())

OK.

>      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 ())

OK.

>      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 ())

OK.

>      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 ())

OK.

>      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 ())

OK.

>      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 ())

OK.

>      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 ())

OK.

>      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 ())

OK.

>      return _dl_open_hook->dlclose (map);
>  #endif
>    return dlerror_run (do_dlclose, map);
> diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h
> index 196513851f..61313a72ba 100644
> --- a/sysdeps/generic/ldsodefs.h
> +++ b/sysdeps/generic/ldsodefs.h
> @@ -1144,6 +1144,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;

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.

> +}
> +#endif
>  
>  __END_DECLS
>  
>
Florian Weimer Dec. 18, 2017, 7:06 p.m. UTC | #2
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
Carlos O'Donell Dec. 18, 2017, 7:24 p.m. UTC | #3
On 12/18/2017 11:06 AM, Florian Weimer wrote:
> 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.

That's a good point.

I was more focused on asserting the changes you made were being used at all,
and not some other code path. However, functionally, this is no different
from testing the intended external behaviour. So it does look like tststatic2
(with the inner DSO calling dlopen also) should cover this.

>>> +/* 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.)
Your v2 patch looks perfect. Thanks for the additional comments.
diff mbox series

Patch

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/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h
index 196513851f..61313a72ba 100644
--- a/sysdeps/generic/ldsodefs.h
+++ b/sysdeps/generic/ldsodefs.h
@@ -1144,6 +1144,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