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
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.
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.
@@ -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);
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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;
@@ -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
@@ -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
@@ -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);
@@ -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. */
@@ -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;
@@ -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