From patchwork Mon Dec 18 19:06:27 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Florian Weimer X-Patchwork-Id: 24994 Received: (qmail 57914 invoked by alias); 18 Dec 2017 19:06:35 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 57904 invoked by uid 89); 18 Dec 2017 19:06:34 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_SHORT, SPF_HELO_PASS, T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy= X-HELO: mx1.redhat.com Subject: Re: [PATCH] ld.so: Examine GLRO to detect inactive loader [BZ #20204] To: Carlos O'Donell , libc-alpha@sourceware.org References: <20171218123414.7096E43994576@oldenburg.str.redhat.com> From: Florian Weimer Message-ID: <3267f325-f0a3-7535-5fd2-5823cd8a6a1a@redhat.com> Date: Mon, 18 Dec 2017 20:06:27 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0 MIME-Version: 1.0 In-Reply-To: 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 2017-12-18 Florian Weimer [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 @@ . */ #include +#include #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 @@ . */ #include +#include #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 #include #include +#include #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