From: Carlos O'Donell <carlos@redhat.com>
To: libc-alpha <libc-alpha@sourceware.org>
Cc: Florian Weimer <fweimer@redhat.com>
Subject: Re: [PATCH] elf: Always set l in _dl_init_paths (Bug 23462).
Date: Tue, 15 Oct 2019 16:24:00 -0000 [thread overview]
Message-ID: <916e86c1-dd59-6784-1318-856309aebd95@redhat.com> (raw)
In-Reply-To: <ca723e73-c8eb-ca4c-1ca0-5fbf2b88edcd@redhat.com>
On 10/8/19 8:01 PM, Carlos O'Donell wrote:
> I was doing a Fedora triage with Florian and we caught that this
> issue had slipped through the cracks. Here is the patch with a
> very simple test case.
Ping.
> 8< --- 8< --- 8<
> After d1d5471579eb0426671bf94f2d71e61dfb204c30 we always setup the
> link map l to make the static and shared cases the same. The bug
> is that in elf/dl-load.c (_dl_init_paths) we conditionally set l
> only in the #ifdef SHARED case, but unconditionally use it later.
> The simple solution is to remove the #ifdef SHARED conditional,
> because it's no longer needed, and unconditionally setup l for
> both the static and shared cases. A regression test is added to
> run a static binary with LD_LIBRARY_PATH='$ORIGIN' which crashes
> before the fix and runs after the fix.
>
> Regression tested on x86_64.
> ---
> ChangeLog | 9 ++++++
> elf/Makefile | 5 +++-
> elf/dl-load.c | 67 +++++++++++++++++++++-----------------------
> elf/tst-dst-static.c | 30 ++++++++++++++++++++
> 4 files changed, 75 insertions(+), 36 deletions(-)
> create mode 100644 elf/tst-dst-static.c
>
> diff --git a/ChangeLog b/ChangeLog
> index 2ce48223f4..cf198257ba 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,12 @@
> +2019-10-08 Carlos O'Donell <carlos@redhat.com>
> +
> + [BZ #23462]
> + * elf/Makefile (tests-static-normal): tst-dst-static.
> + (tst-dst-static-ENV): Define.
> + * elf/dl-load.c (_dl_init_paths): Remove #ifdef SHARED, and refactor
> + to always assume l is non-NULL.
> + * elf/tst-dst-static.c: New file.
> +
> 2019-10-07 Carlos O'Donell <carlos@redhat.com>
>
> * nptl/cancellation.c: Document that all functions here must be
> diff --git a/elf/Makefile b/elf/Makefile
> index dea51ca182..bb55baaa1f 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -148,7 +148,8 @@ endif
> tests-static-normal := tst-leaks1-static tst-array1-static tst-array5-static \
> tst-dl-iter-static \
> tst-tlsalign-static tst-tlsalign-extern-static \
> - tst-linkall-static tst-env-setuid tst-env-setuid-tunables
> + tst-linkall-static tst-env-setuid tst-env-setuid-tunables \
> + tst-dst-static
> tests-static-internal := tst-tls1-static tst-tls2-static \
> tst-ptrguard1-static tst-stackguard1-static \
> tst-tls1-static-non-pie tst-libc_dlvsym-static
> @@ -1557,3 +1558,5 @@ $(objpfx)tst-big-note-lib.so: $(objpfx)tst-big-note-lib.o
> $(objpfx)tst-unwind-ctor: $(objpfx)tst-unwind-ctor-lib.so
>
> CFLAGS-tst-unwind-main.c += -funwind-tables -DUSE_PTHREADS=0
> +
> +tst-dst-static-ENV = LD_LIBRARY_PATH='$$ORIGIN'
> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index 438793e53d..903f8af13a 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -748,50 +748,47 @@ _dl_init_paths (const char *llp)
> max_dirnamelen = SYSTEM_DIRS_MAX_LEN;
> *aelem = NULL;
>
> -#ifdef SHARED
> - /* This points to the map of the main object. */
> + /* This points to the map of the main object. It is always non-NULL
> + since we have purposely made the dynamic and static cases look the
> + same. */
> l = GL(dl_ns)[LM_ID_BASE]._ns_loaded;
> - if (l != NULL)
> + assert (l->l_type != lt_loaded);
> +
> + if (l->l_info[DT_RUNPATH])
> + {
> + /* Allocate room for the search path and fill in information
> + from RUNPATH. */
> + decompose_rpath (&l->l_runpath_dirs,
> + (const void *) (D_PTR (l, l_info[DT_STRTAB])
> + + l->l_info[DT_RUNPATH]->d_un.d_val),
> + l, "RUNPATH");
> + /* During rtld init the memory is allocated by the stub malloc,
> + prevent any attempt to free it by the normal malloc. */
> + l->l_runpath_dirs.malloced = 0;
> +
> + /* The RPATH is ignored. */
> + l->l_rpath_dirs.dirs = (void *) -1;
> + }
> + else
> {
> - assert (l->l_type != lt_loaded);
> + l->l_runpath_dirs.dirs = (void *) -1;
>
> - if (l->l_info[DT_RUNPATH])
> + if (l->l_info[DT_RPATH])
> {
> /* Allocate room for the search path and fill in information
> - from RUNPATH. */
> - decompose_rpath (&l->l_runpath_dirs,
> + from RPATH. */
> + decompose_rpath (&l->l_rpath_dirs,
> (const void *) (D_PTR (l, l_info[DT_STRTAB])
> - + l->l_info[DT_RUNPATH]->d_un.d_val),
> - l, "RUNPATH");
> - /* During rtld init the memory is allocated by the stub malloc,
> - prevent any attempt to free it by the normal malloc. */
> - l->l_runpath_dirs.malloced = 0;
> -
> - /* The RPATH is ignored. */
> - l->l_rpath_dirs.dirs = (void *) -1;
> + + l->l_info[DT_RPATH]->d_un.d_val),
> + l, "RPATH");
> + /* During rtld init the memory is allocated by the stub
> + malloc, prevent any attempt to free it by the normal
> + malloc. */
> + l->l_rpath_dirs.malloced = 0;
> }
> else
> - {
> - l->l_runpath_dirs.dirs = (void *) -1;
> -
> - if (l->l_info[DT_RPATH])
> - {
> - /* Allocate room for the search path and fill in information
> - from RPATH. */
> - decompose_rpath (&l->l_rpath_dirs,
> - (const void *) (D_PTR (l, l_info[DT_STRTAB])
> - + l->l_info[DT_RPATH]->d_un.d_val),
> - l, "RPATH");
> - /* During rtld init the memory is allocated by the stub
> - malloc, prevent any attempt to free it by the normal
> - malloc. */
> - l->l_rpath_dirs.malloced = 0;
> - }
> - else
> - l->l_rpath_dirs.dirs = (void *) -1;
> - }
> + l->l_rpath_dirs.dirs = (void *) -1;
> }
> -#endif /* SHARED */
>
> if (llp != NULL && *llp != '\0')
> {
> diff --git a/elf/tst-dst-static.c b/elf/tst-dst-static.c
> new file mode 100644
> index 0000000000..7092f24a8e
> --- /dev/null
> +++ b/elf/tst-dst-static.c
> @@ -0,0 +1,30 @@
> +/* Test DST expansion for static binaries doesn't carsh. Bug 23462.
> + Copyright (C) 2019 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/>. */
> +
> +/* The purpose of this test is to exercise the code in elf/dl-loac.c
> + (_dl_init_paths) or thereabout and ensure that static binaries
> + don't crash when expanding DSTs. */
> +static int
> +do_test (void)
> +{
> + /* If the dynamic loader code linked into the static binary cannot
> + handle expanding the DSTs e.g. null-deref on an incomplete link
> + map, then it will crash before reaching main. */
> + return 0;
> +}
> +#include <support/test-driver.c>
>
--
Cheers,
Carlos.
next prev parent reply other threads:[~2019-10-15 16:24 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-09 0:01 Carlos O'Donell
2019-10-15 16:24 ` Carlos O'Donell [this message]
2019-10-16 9:54 ` Florian Weimer
2019-10-16 11:15 ` Carlos O'Donell
2019-10-16 11:23 ` Florian Weimer
2019-10-16 13:31 ` Carlos O'Donell
2019-10-25 1:16 ` Carlos O'Donell
2019-10-25 8:37 ` Florian Weimer
2019-10-25 12:55 ` Carlos O'Donell
2021-03-05 18:25 [PATCH] elf: Always set l in _dl_init_paths (bug 23462) Florian Weimer
2021-03-08 22:26 ` Carlos O'Donell
2021-03-09 5:39 ` Navin P
2021-03-09 6:05 ` Florian Weimer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=916e86c1-dd59-6784-1318-856309aebd95@redhat.com \
--to=carlos@redhat.com \
--cc=fweimer@redhat.com \
--cc=libc-alpha@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).