From: Adhemerval Zanella <adhemerval.zanella@linaro.org>
To: Szabolcs Nagy <szabolcs.nagy@arm.com>, libc-alpha@sourceware.org
Subject: Re: [PATCH v6 1/4] rtld: Use generic argv adjustment in ld.so [BZ #23293]
Date: Fri, 13 May 2022 16:56:32 -0300 [thread overview]
Message-ID: <f6d4380e-2ff4-3bb6-6472-f2330577e137@linaro.org> (raw)
In-Reply-To: <a7d9779fde219ba7d64b7a540f994a86914f8d9d.1651757640.git.szabolcs.nagy@arm.com>
On 05/05/2022 10:58, Szabolcs Nagy via Libc-alpha wrote:
> When an executable is invoked as
>
> ./ld.so [ld.so-args] ./exe [exe-args]
>
> then the argv is adujusted in ld.so before calling the entry point of
> the executable so ld.so args are not visible to it. On most targets
> this requires moving argv, env and auxv on the stack to ensure correct
> stack alignment at the entry point. This had several issues:
>
> - The code for this adjustment on the stack is written in asm as part
> of the target specific ld.so _start code which is hard to maintain.
>
> - The adjustment is done after _dl_start returns, where it's too late
> to update GLRO(dl_auxv), as it is already readonly, so it points to
> memory that was clobbered by the adjustment. This is bug 23293.
>
> - _environ is also wrong in ld.so after the adjustment, but it is
> likely not used after _dl_start returns so this is not user visible.
>
> - _dl_argv was updated, but for this it was moved out of relro, which
> changes security properties across targets unnecessarily.
>
> This patch introduces a generic _dl_start_args_adjust function that
> handles the argument adjustments after ld.so processed its own args
> and before relro protection is applied.
>
> The same algorithm is used on all targets, _dl_skip_args is now 0, so
> existing target specific adjustment code is no longer used. The bug
> affects aarch64, alpha, arc, arm, csky, ia64, nios2, s390-32 and sparc,
> other targets don't need the change in principle, only for consistency.
>
> The GNU Hurd start code relied on _dl_skip_args after dl_main returned,
> now it checks directly if args were adjusted and fixes the Hurd startup
> data accordingly.
>
> Follow up patches can remove _dl_skip_args and DL_ARGV_NOT_RELRO.
>
> Tested on aarch64-linux-gnu and cross tested on i686-gnu.
> ---
> v6:
> - don't pass start_argptr to _dl_main, just use _dl_argv-1.
> - add comment for _dl_start_args_adjust.
> - add assert checks to _dl_start_args_adjust and simplify it a bit.
> v5:
> - Hurd specific changes.
> v4:
> - New code is unconditionally used on all targets.
> - Hide auxv adjustments behind HAVE_AUX_VECTOR.
> - DL_NEED_START_ARGS_ADJUST macro is removed.
> - _dl_skip_args is no longer unused.
> - start_argptr is passed down to dl_main instead of using a global.
> - moved aarch64 DL_ARGV_NOT_RELRO removal to separate patch.
> v2:
> - use p != NULL, and a_type != AT_NULL
> - remove the confusing paragraph from the commit message.
Looks ok, just a minor suggestion below.
Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
> ---
> elf/rtld.c | 73 ++++++++++++++++++++++++++++-------
> sysdeps/mach/hurd/dl-sysdep.c | 30 +++++++-------
> 2 files changed, 73 insertions(+), 30 deletions(-)
>
> diff --git a/elf/rtld.c b/elf/rtld.c
> index 3b2e05bf4c..b5070d453f 100644
> --- a/elf/rtld.c
> +++ b/elf/rtld.c
> @@ -1306,6 +1306,62 @@ rtld_setup_main_map (struct link_map *main_map)
> return has_interp;
> }
>
> +/* Adjusts the contents of the stack and related globals for the user
> + entry point. The ld.so processed skip_args arguments and bumped
> + _dl_argv and _dl_argc accordingly. Those arguments are removed from
> + argv here. */
> +static void
> +_dl_start_args_adjust (int skip_args)
> +{
> + void **sp = (void **) (_dl_argv - skip_args - 1);
Is it fully correctly to materialize the address for 'skip_args' equal to 0?
I don't think it would matter anyway.
> + void **p = sp + skip_args;
> +
> + if (skip_args == 0)
> + return;
> +
> + /* Sanity check. */
> + intptr_t argc = (intptr_t) sp[0] - skip_args;
> + assert (argc == _dl_argc);
> +
> + /* Adjust argc on stack. */
> + sp[0] = (void *) (intptr_t) _dl_argc;
> +
> + /* Update globals in rtld. */
> + _dl_argv -= skip_args;
> + _environ -= skip_args;
> +
> + /* Shuffle argv down. */
> + do
> + *++sp = *++p;
> + while (*p != NULL);
> +
> + assert (_environ == (char **) (sp + 1));
> +
> + /* Shuffle envp down. */
> + do
> + *++sp = *++p;
> + while (*p != NULL);
> +
> +#ifdef HAVE_AUX_VECTOR
> + void **auxv = (void **) GLRO(dl_auxv) - skip_args;
> + GLRO(dl_auxv) = (ElfW(auxv_t *)) auxv; /* Aliasing violation. */
> + assert (auxv == sp + 1);
> +
> + /* Shuffle auxv down. */
> + void *a, *b; /* Use a pair of pointers for an auxv entry. */
> + unsigned long a_type;
> + do
> + {
> + a_type = ((ElfW(auxv_t) *) (p + 1))->a_type;
> + a = *++p;
> + b = *++p;
> + *++sp = a;
> + *++sp = b;
> + }> + while (a_type != AT_NULL);
> +#endif
Maybe:
ElfW(auxv_t) ax;
do
{
p = (void**) ((uintptr_t) p + sizeof (ax));
sp = (void**) ((uintptr_t) sp + sizeof (ax));
memcpy (&ax, p, sizeof (ax));
memcpy (sp, &ax, sizeof (ax));
}
while (ax.a_type != AT_NULL);
Most targets will inline memcpy and if they do not we don't enable IFUNC
for ld.so.
> +}
> +
> static void
> dl_main (const ElfW(Phdr) *phdr,
> ElfW(Word) phnum,
> @@ -1359,6 +1415,7 @@ dl_main (const ElfW(Phdr) *phdr,
> rtld_is_main = true;
>
> char *argv0 = NULL;
> + char **orig_argv = _dl_argv;
>
> /* Note the place where the dynamic linker actually came from. */
> GL(dl_rtld_map).l_name = rtld_progname;
> @@ -1373,7 +1430,6 @@ dl_main (const ElfW(Phdr) *phdr,
> GLRO(dl_lazy) = -1;
> }
>
> - ++_dl_skip_args;
> --_dl_argc;
> ++_dl_argv;
> }
> @@ -1382,14 +1438,12 @@ dl_main (const ElfW(Phdr) *phdr,
> if (state.mode != rtld_mode_help)
> state.mode = rtld_mode_verify;
>
> - ++_dl_skip_args;
> --_dl_argc;
> ++_dl_argv;
> }
> else if (! strcmp (_dl_argv[1], "--inhibit-cache"))
> {
> GLRO(dl_inhibit_cache) = 1;
> - ++_dl_skip_args;
> --_dl_argc;
> ++_dl_argv;
> }
> @@ -1399,7 +1453,6 @@ dl_main (const ElfW(Phdr) *phdr,
> state.library_path = _dl_argv[2];
> state.library_path_source = "--library-path";
>
> - _dl_skip_args += 2;
> _dl_argc -= 2;
> _dl_argv += 2;
> }
> @@ -1408,7 +1461,6 @@ dl_main (const ElfW(Phdr) *phdr,
> {
> GLRO(dl_inhibit_rpath) = _dl_argv[2];
>
> - _dl_skip_args += 2;
> _dl_argc -= 2;
> _dl_argv += 2;
> }
> @@ -1416,14 +1468,12 @@ dl_main (const ElfW(Phdr) *phdr,
> {
> audit_list_add_string (&state.audit_list, _dl_argv[2]);
>
> - _dl_skip_args += 2;
> _dl_argc -= 2;
> _dl_argv += 2;
> }
> else if (! strcmp (_dl_argv[1], "--preload") && _dl_argc > 2)
> {
> state.preloadarg = _dl_argv[2];
> - _dl_skip_args += 2;
> _dl_argc -= 2;
> _dl_argv += 2;
> }
> @@ -1431,7 +1481,6 @@ dl_main (const ElfW(Phdr) *phdr,
> {
> argv0 = _dl_argv[2];
>
> - _dl_skip_args += 2;
> _dl_argc -= 2;
> _dl_argv += 2;
> }
> @@ -1439,7 +1488,6 @@ dl_main (const ElfW(Phdr) *phdr,
> && _dl_argc > 2)
> {
> state.glibc_hwcaps_prepend = _dl_argv[2];
> - _dl_skip_args += 2;
> _dl_argc -= 2;
> _dl_argv += 2;
> }
> @@ -1447,7 +1495,6 @@ dl_main (const ElfW(Phdr) *phdr,
> && _dl_argc > 2)
> {
> state.glibc_hwcaps_mask = _dl_argv[2];
> - _dl_skip_args += 2;
> _dl_argc -= 2;
> _dl_argv += 2;
> }
> @@ -1456,7 +1503,6 @@ dl_main (const ElfW(Phdr) *phdr,
> {
> state.mode = rtld_mode_list_tunables;
>
> - ++_dl_skip_args;
> --_dl_argc;
> ++_dl_argv;
> }
> @@ -1465,7 +1511,6 @@ dl_main (const ElfW(Phdr) *phdr,
> {
> state.mode = rtld_mode_list_diagnostics;
>
> - ++_dl_skip_args;
> --_dl_argc;
> ++_dl_argv;
> }
> @@ -1511,7 +1556,6 @@ dl_main (const ElfW(Phdr) *phdr,
> _dl_usage (ld_so_name, NULL);
> }
>
> - ++_dl_skip_args;
> --_dl_argc;
> ++_dl_argv;
>
> @@ -1610,6 +1654,9 @@ dl_main (const ElfW(Phdr) *phdr,
> /* Set the argv[0] string now that we've processed the executable. */
> if (argv0 != NULL)
> _dl_argv[0] = argv0;
> +
> + /* Adjust arguments for the application entry point. */
> + _dl_start_args_adjust (_dl_argv - orig_argv);
> }
> else
> {
Ok.
> diff --git a/sysdeps/mach/hurd/dl-sysdep.c b/sysdeps/mach/hurd/dl-sysdep.c
> index 3cbe075615..8373962e62 100644
> --- a/sysdeps/mach/hurd/dl-sysdep.c
> +++ b/sysdeps/mach/hurd/dl-sysdep.c
> @@ -76,6 +76,7 @@ _dl_sysdep_start (void **start_argptr,
> {
> void go (intptr_t *argdata)
> {
> + char *orig_argv0;
> char **p;
>
> /* Cache the information in various global variables. */
> @@ -84,6 +85,8 @@ _dl_sysdep_start (void **start_argptr,
> _environ = &_dl_argv[_dl_argc + 1];
> for (p = _environ; *p++;); /* Skip environ pointers and terminator. */
>
> + orig_argv0 = _dl_argv[0];
> +
> if ((void *) p == _dl_argv[0])
> {
> static struct hurd_startup_data nodata;
> @@ -173,30 +176,23 @@ _dl_sysdep_start (void **start_argptr,
>
> /* The call above might screw a few things up.
>
> - First of all, if _dl_skip_args is nonzero, we are ignoring
> - the first few arguments. However, if we have no Hurd startup
> - data, it is the magical convention that ARGV[0] == P. The
> + P is the location after the terminating NULL of the list of
> + environment variables. It has to point to the Hurd startup
> + data or if that's missing then P == ARGV[0] must hold. The
> startup code in init-first.c will get confused if this is not
> the case, so we must rearrange things to make it so. We'll
> - overwrite the origional ARGV[0] at P with ARGV[_dl_skip_args].
> + recompute P and move the Hurd data or the new ARGV[0] there.
>
> - Secondly, if we need to be secure, it removes some dangerous
> - environment variables. If we have no Hurd startup date this
> - changes P (since that's the location after the terminating
> - NULL in the list of environment variables). We do the same
> - thing as in the first case but make sure we recalculate P.
> - If we do have Hurd startup data, we have to move the data
> - such that it starts just after the terminating NULL in the
> - environment list.
> + Note: directly invoked ld.so can move arguments and env vars.
>
> We use memmove, since the locations might overlap. */
> - if (__libc_enable_secure || _dl_skip_args)
> - {
> - char **newp;
>
> - for (newp = _environ; *newp++;);
> + char **newp;
> + for (newp = _environ; *newp++;);
>
> - if (_dl_argv[-_dl_skip_args] == (char *) p)
> + if (newp != p || _dl_argv[0] != orig_argv0)
> + {
> + if (orig_argv0 == (char *) p)
> {
> if ((char *) newp != _dl_argv[0])
> {
Looks ok, but I can't really voucher for hurd code.
next prev parent reply other threads:[~2022-05-13 19:56 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-05 13:58 [PATCH v6 0/4] Args adjustment with ./ld.so exe " Szabolcs Nagy
2022-05-05 13:58 ` [PATCH v6 1/4] rtld: Use generic argv adjustment in ld.so " Szabolcs Nagy
2022-05-13 19:56 ` Adhemerval Zanella [this message]
2022-05-17 9:27 ` Szabolcs Nagy
2022-05-05 13:58 ` [PATCH v6 2/4] rtld: Remove DL_ARGV_NOT_RELRO and make _dl_skip_args const Szabolcs Nagy
2022-05-13 20:14 ` Adhemerval Zanella
2022-05-05 13:59 ` [PATCH v6 3/4] linux: Add a getauxval test [BZ #23293] Szabolcs Nagy
2022-05-13 20:21 ` Adhemerval Zanella
2022-05-05 13:59 ` [PATCH v6 4/4] aarch64: Move ld.so _start to separate file and drop _dl_skip_args Szabolcs Nagy
2022-05-13 20:46 ` Adhemerval Zanella
2022-05-06 10:07 ` [PATCH v6 0/4] Args adjustment with ./ld.so exe [BZ #23293] Szabolcs Nagy
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=f6d4380e-2ff4-3bb6-6472-f2330577e137@linaro.org \
--to=adhemerval.zanella@linaro.org \
--cc=libc-alpha@sourceware.org \
--cc=szabolcs.nagy@arm.com \
/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).