[PATCH v6 0/4] Args adjustment with ./ld.so exe [BZ #23293]

[PATCH v6 0/4] Args adjustment with ./ld.so exe [BZ #23293]

[Szabolcs Nagy]

The Hurd ld.so start code had to be adjusted. This revealed that
the fix actually cuts across abstraction layers: fiddles with
ELF entry stack layout in generic code, skipping the intermediate
OS sysdep layer. If the OS layer ever wants to do something more
complicated with the args then the interface contract with dl_main
will have to be reevaluated.

I ran cross tests for i686-gnu, but not execution tests, so the
changes in sysdeps/mach/hurd/dl-sysdep.c require a review.

The patches are in the nsz/bug23293-v6 branch.

Szabolcs Nagy (4):
  rtld: Use generic argv adjustment in ld.so [BZ #23293]
  rtld: Remove DL_ARGV_NOT_RELRO and make _dl_skip_args const
  linux: Add a getauxval test [BZ #23293]
  aarch64: Move ld.so _start to separate file and drop _dl_skip_args

 elf/rtld.c                               | 83 ++++++++++++++++++------
 sysdeps/aarch64/Makefile                 |  1 +
 sysdeps/aarch64/dl-machine.h             | 77 +---------------------
 sysdeps/aarch64/dl-start.S               | 53 +++++++++++++++
 sysdeps/aarch64/dl-sysdep.h              |  4 --
 sysdeps/alpha/dl-sysdep.h                | 23 -------
 sysdeps/arc/dl-sysdep.h                  |  4 --
 sysdeps/arm/dl-sysdep.h                  |  4 --
 sysdeps/csky/dl-sysdep.h                 | 23 -------
 sysdeps/generic/ldsodefs.h               | 13 +---
 sysdeps/ia64/dl-sysdep.h                 | 23 -------
 sysdeps/mach/hurd/dl-sysdep.c            | 30 ++++-----
 sysdeps/nios2/dl-sysdep.h                |  4 --
 sysdeps/s390/s390-32/dl-sysdep.h         | 23 -------
 sysdeps/sparc/dl-sysdep.h                | 23 -------
 sysdeps/unix/sysv/linux/Makefile         |  1 +
 sysdeps/unix/sysv/linux/ia64/dl-sysdep.h |  4 --
 sysdeps/unix/sysv/linux/tst-getauxval.c  | 74 +++++++++++++++++++++
 18 files changed, 209 insertions(+), 258 deletions(-)
 create mode 100644 sysdeps/aarch64/dl-start.S
 delete mode 100644 sysdeps/alpha/dl-sysdep.h
 delete mode 100644 sysdeps/csky/dl-sysdep.h
 delete mode 100644 sysdeps/ia64/dl-sysdep.h
 delete mode 100644 sysdeps/s390/s390-32/dl-sysdep.h
 delete mode 100644 sysdeps/sparc/dl-sysdep.h
 create mode 100644 sysdeps/unix/sysv/linux/tst-getauxval.c

-- 
2.25.1

[PATCH v6 1/4] rtld: Use generic argv adjustment in ld.so [BZ #23293]

[Szabolcs Nagy]

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.
---
 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);
+  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
+}
+
 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
     {
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])
 		{
-- 
2.25.1

[PATCH v6 2/4] rtld: Remove DL_ARGV_NOT_RELRO and make _dl_skip_args const

[Szabolcs Nagy]

_dl_skip_args is always 0, so the target specific code that modifies
argv after relro protection is applied is no longer used.

After the patch relro protection is applied to _dl_argv consistently
on all targets.

---
v6:
- const _dl_skip_args.
v4:
- New patch.
Reviewed-by: Florian Weimer <fweimer@redhat.com>
Tested-by: Florian Weimer <fweimer@redhat.com>
---
 elf/rtld.c                               | 10 ++--------
 sysdeps/aarch64/dl-sysdep.h              |  4 ----
 sysdeps/alpha/dl-sysdep.h                | 23 -----------------------
 sysdeps/arc/dl-sysdep.h                  |  4 ----
 sysdeps/arm/dl-sysdep.h                  |  4 ----
 sysdeps/csky/dl-sysdep.h                 | 23 -----------------------
 sysdeps/generic/ldsodefs.h               | 13 +++----------
 sysdeps/ia64/dl-sysdep.h                 | 23 -----------------------
 sysdeps/nios2/dl-sysdep.h                |  4 ----
 sysdeps/s390/s390-32/dl-sysdep.h         | 23 -----------------------
 sysdeps/sparc/dl-sysdep.h                | 23 -----------------------
 sysdeps/unix/sysv/linux/ia64/dl-sysdep.h |  4 ----
 12 files changed, 5 insertions(+), 153 deletions(-)
 delete mode 100644 sysdeps/alpha/dl-sysdep.h
 delete mode 100644 sysdeps/csky/dl-sysdep.h
 delete mode 100644 sysdeps/ia64/dl-sysdep.h
 delete mode 100644 sysdeps/s390/s390-32/dl-sysdep.h
 delete mode 100644 sysdeps/sparc/dl-sysdep.h

diff --git a/elf/rtld.c b/elf/rtld.c
index b5070d453f..71f7095def 100644
--- a/elf/rtld.c
+++ b/elf/rtld.c
@@ -157,16 +157,10 @@ static void dl_main_state_init (struct dl_main_state *state);
 extern char **_environ attribute_hidden;
 static void process_envvars (struct dl_main_state *state);
 
-#ifdef DL_ARGV_NOT_RELRO
-int _dl_argc attribute_hidden;
-char **_dl_argv = NULL;
-/* Nonzero if we were run directly.  */
-unsigned int _dl_skip_args attribute_hidden;
-#else
 int _dl_argc attribute_relro attribute_hidden;
 char **_dl_argv attribute_relro = NULL;
-unsigned int _dl_skip_args attribute_relro attribute_hidden;
-#endif
+/* Always 0, only kept for not-yet-updated target start code.  */
+const unsigned int _dl_skip_args attribute_hidden;
 rtld_hidden_data_def (_dl_argv)
 
 #ifndef THREAD_SET_STACK_GUARD
diff --git a/sysdeps/aarch64/dl-sysdep.h b/sysdeps/aarch64/dl-sysdep.h
index 667786671c..1516dd7d3f 100644
--- a/sysdeps/aarch64/dl-sysdep.h
+++ b/sysdeps/aarch64/dl-sysdep.h
@@ -18,8 +18,4 @@
 
 #include_next <dl-sysdep.h>
 
-/* _dl_argv cannot be attribute_relro, because _dl_start_user
-   might write into it after _dl_start returns.  */
-#define DL_ARGV_NOT_RELRO 1
-
 #define DL_EXTERN_PROTECTED_DATA
diff --git a/sysdeps/alpha/dl-sysdep.h b/sysdeps/alpha/dl-sysdep.h
deleted file mode 100644
index 3099ee419f..0000000000
--- a/sysdeps/alpha/dl-sysdep.h
+++ /dev/null
@@ -1,23 +0,0 @@
-/* System-specific settings for dynamic linker code.  Alpha version.
-   Copyright (C) 2002-2022 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/>.  */
-
-#include_next <dl-sysdep.h>
-
-/* _dl_argv cannot be attribute_relro, because _dl_start_user
-   might write into it after _dl_start returns.  */
-#define DL_ARGV_NOT_RELRO 1
diff --git a/sysdeps/arc/dl-sysdep.h b/sysdeps/arc/dl-sysdep.h
index da060ceeee..cf4d160a73 100644
--- a/sysdeps/arc/dl-sysdep.h
+++ b/sysdeps/arc/dl-sysdep.h
@@ -18,8 +18,4 @@
 
 #include_next <dl-sysdep.h>
 
-/* _dl_argv cannot be attribute_relro, because _dl_start_user
-   might write into it after _dl_start returns.  */
-#define DL_ARGV_NOT_RELRO 1
-
 #define DL_EXTERN_PROTECTED_DATA
diff --git a/sysdeps/arm/dl-sysdep.h b/sysdeps/arm/dl-sysdep.h
index ce7a84a7de..7a99107436 100644
--- a/sysdeps/arm/dl-sysdep.h
+++ b/sysdeps/arm/dl-sysdep.h
@@ -18,8 +18,4 @@
 
 #include_next <dl-sysdep.h>
 
-/* _dl_argv cannot be attribute_relro, because _dl_start_user
-   might write into it after _dl_start returns.  */
-#define DL_ARGV_NOT_RELRO 1
-
 #define DL_EXTERN_PROTECTED_DATA
diff --git a/sysdeps/csky/dl-sysdep.h b/sysdeps/csky/dl-sysdep.h
deleted file mode 100644
index fc8a58b94c..0000000000
--- a/sysdeps/csky/dl-sysdep.h
+++ /dev/null
@@ -1,23 +0,0 @@
-/* System-specific settings for dynamic linker code.  C-SKY version.
-   Copyright (C) 2018-2022 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/>.  */
-
-#include_next <dl-sysdep.h>
-
-/* _dl_argv cannot be attribute_relro, because _dl_start_user
-   might write into it after _dl_start returns.  */
-#define DL_ARGV_NOT_RELRO 1
diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h
index 4a5e698db2..f3d3c78927 100644
--- a/sysdeps/generic/ldsodefs.h
+++ b/sysdeps/generic/ldsodefs.h
@@ -762,18 +762,11 @@ rtld_hidden_proto (__libc_stack_end)
 
 /* Parameters passed to the dynamic linker.  */
 extern int _dl_argc attribute_hidden attribute_relro;
-extern char **_dl_argv
-#ifndef DL_ARGV_NOT_RELRO
-     attribute_relro
-#endif
-     ;
+extern char **_dl_argv attribute_relro;
 rtld_hidden_proto (_dl_argv)
 #if IS_IN (rtld)
-extern unsigned int _dl_skip_args attribute_hidden
-# ifndef DL_ARGV_NOT_RELRO
-     attribute_relro
-# endif
-     ;
+/* Always 0, only kept for not-yet-updated target start code.  */
+extern const unsigned int _dl_skip_args attribute_hidden;
 #endif
 #define rtld_progname _dl_argv[0]
 
diff --git a/sysdeps/ia64/dl-sysdep.h b/sysdeps/ia64/dl-sysdep.h
deleted file mode 100644
index e3a58bec24..0000000000
--- a/sysdeps/ia64/dl-sysdep.h
+++ /dev/null
@@ -1,23 +0,0 @@
-/* System-specific settings for dynamic linker code.  IA-64 version.
-   Copyright (C) 2002-2022 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/>.  */
-
-#include_next <dl-sysdep.h>
-
-/* _dl_argv cannot be attribute_relro, because _dl_start_user
-   might write into it after _dl_start returns.  */
-#define DL_ARGV_NOT_RELRO 1
diff --git a/sysdeps/nios2/dl-sysdep.h b/sysdeps/nios2/dl-sysdep.h
index 0354650042..257b37c258 100644
--- a/sysdeps/nios2/dl-sysdep.h
+++ b/sysdeps/nios2/dl-sysdep.h
@@ -18,8 +18,4 @@
 
 #include_next <dl-sysdep.h>
 
-/* _dl_argv cannot be attribute_relro, because _dl_start_user
-   might write into it after _dl_start returns.  */
-#define DL_ARGV_NOT_RELRO 1
-
 #define DL_EXTERN_PROTECTED_DATA
diff --git a/sysdeps/s390/s390-32/dl-sysdep.h b/sysdeps/s390/s390-32/dl-sysdep.h
deleted file mode 100644
index 699b50f156..0000000000
--- a/sysdeps/s390/s390-32/dl-sysdep.h
+++ /dev/null
@@ -1,23 +0,0 @@
-/* System-specific settings for dynamic linker code.  S/390 version.
-   Copyright (C) 2014-2022 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/>.  */
-
-#include_next <dl-sysdep.h>
-
-/* _dl_argv cannot be attribute_relro, because _dl_start_user
-   might write into it after _dl_start returns.  */
-#define DL_ARGV_NOT_RELRO 1
diff --git a/sysdeps/sparc/dl-sysdep.h b/sysdeps/sparc/dl-sysdep.h
deleted file mode 100644
index f32f16a107..0000000000
--- a/sysdeps/sparc/dl-sysdep.h
+++ /dev/null
@@ -1,23 +0,0 @@
-/* System-specific settings for dynamic linker code.  SPARC version.
-   Copyright (C) 2002-2022 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/>.  */
-
-#include_next <dl-sysdep.h>
-
-/* _dl_argv cannot be attribute_relro, because _dl_start_user
-   might write into it after _dl_start returns.  */
-#define DL_ARGV_NOT_RELRO 1
diff --git a/sysdeps/unix/sysv/linux/ia64/dl-sysdep.h b/sysdeps/unix/sysv/linux/ia64/dl-sysdep.h
index 0d2a1d093a..aa1de6b361 100644
--- a/sysdeps/unix/sysv/linux/ia64/dl-sysdep.h
+++ b/sysdeps/unix/sysv/linux/ia64/dl-sysdep.h
@@ -48,8 +48,4 @@ extern int _dl_sysinfo_break attribute_hidden;
        ".previous");
 #endif
 
-/* _dl_argv cannot be attribute_relro, because _dl_start_user
-   might write into it after _dl_start returns.  */
-#define DL_ARGV_NOT_RELRO 1
-
 #endif	/* dl-sysdep.h */
-- 
2.25.1

[PATCH v6 3/4] linux: Add a getauxval test [BZ #23293]

[Szabolcs Nagy]

This is for bug 23293 and it relies on the glibc test system running
tests via explicit ld.so invokation by default.

Reviewed-by: Florian Weimer <fweimer@redhat.com>

---
v4:
- New patch.
---
 sysdeps/unix/sysv/linux/Makefile        |  1 +
 sysdeps/unix/sysv/linux/tst-getauxval.c | 74 +++++++++++++++++++++++++
 2 files changed, 75 insertions(+)
 create mode 100644 sysdeps/unix/sysv/linux/tst-getauxval.c

diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile
index ca953804d0..89cb005c7d 100644
--- a/sysdeps/unix/sysv/linux/Makefile
+++ b/sysdeps/unix/sysv/linux/Makefile
@@ -126,6 +126,7 @@ tests += tst-clone tst-clone2 tst-clone3 tst-fanotify tst-personality \
   tst-prctl \
   tst-scm_rights \
   tst-epoll \
+  tst-getauxval \
   # tests
 
 # For +depfiles in Makerules.
diff --git a/sysdeps/unix/sysv/linux/tst-getauxval.c b/sysdeps/unix/sysv/linux/tst-getauxval.c
new file mode 100644
index 0000000000..c4b6195743
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/tst-getauxval.c
@@ -0,0 +1,74 @@
+/* Basic test for getauxval.
+   Copyright (C) 2022 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/>.  */
+
+#include <unistd.h>
+#include <stdio.h>
+#include <support/check.h>
+#include <sys/auxv.h>
+
+static int missing;
+static int mismatch;
+
+static void
+check_nonzero (unsigned long t, const char *s)
+{
+  unsigned long v = getauxval (t);
+  printf ("%s: %lu (0x%lx)\n", s, v, v);
+  if (v == 0)
+    missing++;
+}
+
+static void
+check_eq (unsigned long t, const char *s, unsigned long want)
+{
+  unsigned long v = getauxval (t);
+  printf ("%s: %lu want: %lu\n", s, v, want);
+  if (v != want)
+    mismatch++;
+}
+
+#define NZ(x) check_nonzero (x, #x)
+#define EQ(x, want) check_eq (x, #x, want)
+
+static int
+do_test (void)
+{
+  /* These auxv entries should be non-zero on Linux.  */
+  NZ (AT_PHDR);
+  NZ (AT_PHENT);
+  NZ (AT_PHNUM);
+  NZ (AT_PAGESZ);
+  NZ (AT_ENTRY);
+  NZ (AT_CLKTCK);
+  NZ (AT_RANDOM);
+  NZ (AT_EXECFN);
+  if (missing)
+    FAIL_EXIT1 ("Found %d missing auxv entries.\n", missing);
+
+  /* Check against syscalls.  */
+  EQ (AT_UID, getuid ());
+  EQ (AT_EUID, geteuid ());
+  EQ (AT_GID, getgid ());
+  EQ (AT_EGID, getegid ());
+  if (mismatch)
+    FAIL_EXIT1 ("Found %d mismatching auxv entries.\n", mismatch);
+
+  return 0;
+}
+
+#include <support/test-driver.c>
-- 
2.25.1

[PATCH v6 4/4] aarch64: Move ld.so _start to separate file and drop _dl_skip_args

[Szabolcs Nagy]

A separate asm file is easier to maintain than a macro that expands to
inline asm.

The RTLD_START macro is only needed now because _dl_start is local in
rtld.c, but _start has to call it, if _dl_start was made hidden then it
could be empty.

_dl_skip_args is no longer needed.

---
v4:
- adjust commit message about _dl_skip_args.
v3:
- mention _dl_skip_args
v2:
- fix typo in commit message.
---
 sysdeps/aarch64/Makefile     |  1 +
 sysdeps/aarch64/dl-machine.h | 77 +-----------------------------------
 sysdeps/aarch64/dl-start.S   | 53 +++++++++++++++++++++++++
 3 files changed, 56 insertions(+), 75 deletions(-)
 create mode 100644 sysdeps/aarch64/dl-start.S

diff --git a/sysdeps/aarch64/Makefile b/sysdeps/aarch64/Makefile
index 7183895d04..17fb1c5b72 100644
--- a/sysdeps/aarch64/Makefile
+++ b/sysdeps/aarch64/Makefile
@@ -33,6 +33,7 @@ tst-audit27-ENV = LD_AUDIT=$(objpfx)tst-auditmod27.so
 endif
 
 ifeq ($(subdir),elf)
+sysdep-rtld-routines += dl-start
 sysdep-dl-routines += tlsdesc dl-tlsdesc
 gen-as-const-headers += dl-link.sym
 
diff --git a/sysdeps/aarch64/dl-machine.h b/sysdeps/aarch64/dl-machine.h
index b40050a981..fe120bb507 100644
--- a/sysdeps/aarch64/dl-machine.h
+++ b/sysdeps/aarch64/dl-machine.h
@@ -105,81 +105,8 @@ elf_machine_runtime_setup (struct link_map *l, struct r_scope_elem *scope[],
   return lazy;
 }
 
-/* Initial entry point for the dynamic linker. The C function
-   _dl_start is the real entry point, its return value is the user
-   program's entry point */
-#ifdef __LP64__
-# define RTLD_START RTLD_START_1 ("x", "3", "sp")
-#else
-# define RTLD_START RTLD_START_1 ("w", "2", "wsp")
-#endif
-
-
-#define RTLD_START_1(PTR, PTR_SIZE_LOG, PTR_SP) asm ("\
-.text									\n\
-.globl _start								\n\
-.type _start, %function							\n\
-.globl _dl_start_user							\n\
-.type _dl_start_user, %function						\n\
-_start:									\n\
-	// bti c							\n\
-	hint	34							\n\
-	mov	" PTR "0, " PTR_SP "					\n\
-	bl	_dl_start						\n\
-	// returns user entry point in x0				\n\
-	mov	x21, x0							\n\
-_dl_start_user:								\n\
-	// get the original arg count					\n\
-	ldr	" PTR "1, [sp]						\n\
-	// get the argv address						\n\
-	add	" PTR "2, " PTR_SP ", #(1<<"  PTR_SIZE_LOG ")		\n\
-	// get _dl_skip_args to see if we were				\n\
-	// invoked as an executable					\n\
-	adrp	x4, _dl_skip_args					\n\
-        ldr	w4, [x4, #:lo12:_dl_skip_args]				\n\
-	// do we need to adjust argc/argv				\n\
-        cmp	w4, 0							\n\
-	beq	.L_done_stack_adjust					\n\
-	// subtract _dl_skip_args from original arg count		\n\
-	sub	" PTR "1, " PTR "1, " PTR "4				\n\
-	// store adjusted argc back to stack				\n\
-	str	" PTR "1, [sp]						\n\
-	// find the first unskipped argument				\n\
-	mov	" PTR "3, " PTR "2					\n\
-	add	" PTR "4, " PTR "2, " PTR "4, lsl #" PTR_SIZE_LOG "	\n\
-	// shuffle argv down						\n\
-1:	ldr	" PTR "5, [x4], #(1<<"  PTR_SIZE_LOG ")			\n\
-	str	" PTR "5, [x3], #(1<<"  PTR_SIZE_LOG ")			\n\
-	cmp	" PTR "5, #0						\n\
-	bne	1b							\n\
-	// shuffle envp down						\n\
-1:	ldr	" PTR "5, [x4], #(1<<"  PTR_SIZE_LOG ")			\n\
-	str	" PTR "5, [x3], #(1<<"  PTR_SIZE_LOG ")			\n\
-	cmp	" PTR "5, #0						\n\
-	bne	1b							\n\
-	// shuffle auxv down						\n\
-1:	ldp	" PTR "0, " PTR "5, [x4, #(2<<"  PTR_SIZE_LOG ")]!	\n\
-	stp	" PTR "0, " PTR "5, [x3], #(2<<"  PTR_SIZE_LOG ")	\n\
-	cmp	" PTR "0, #0						\n\
-	bne	1b							\n\
-	// Update _dl_argv						\n\
-	adrp	x3, __GI__dl_argv					\n\
-	str	" PTR "2, [x3, #:lo12:__GI__dl_argv]			\n\
-.L_done_stack_adjust:							\n\
-	// compute envp							\n\
-	add	" PTR "3, " PTR "2, " PTR "1, lsl #" PTR_SIZE_LOG "	\n\
-	add	" PTR "3, " PTR "3, #(1<<"  PTR_SIZE_LOG ")		\n\
-	adrp	x16, _rtld_local					\n\
-        add	" PTR "16, " PTR "16, #:lo12:_rtld_local		\n\
-        ldr	" PTR "0, [x16]						\n\
-	bl	_dl_init						\n\
-	// load the finalizer function					\n\
-	adrp	x0, _dl_fini						\n\
-	add	" PTR "0, " PTR "0, #:lo12:_dl_fini			\n\
-	// jump to the user_s entry point				\n\
-	mov     x16, x21						\n\
-	br      x16							\n\
-");
+/* In elf/rtld.c _dl_start should be global so dl-start.S can reference it.  */
+#define RTLD_START asm (".globl _dl_start");
 
 #define elf_machine_type_class(type)					\
   ((((type) == AARCH64_R(JUMP_SLOT)					\
diff --git a/sysdeps/aarch64/dl-start.S b/sysdeps/aarch64/dl-start.S
new file mode 100644
index 0000000000..a3a57bd5a1
--- /dev/null
+++ b/sysdeps/aarch64/dl-start.S
@@ -0,0 +1,53 @@
+/* ld.so _start code.
+   Copyright (C) 2022 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/>.  */
+
+#include <sysdep.h>
+
+ENTRY (_start)
+	/* Create an initial frame with 0 LR and FP */
+	cfi_undefined (x30)
+	mov	x29, #0
+	mov	x30, #0
+
+	mov	x0, sp
+	PTR_ARG (0)
+	bl	_dl_start
+	/* Returns user entry point in x0.  */
+	mov	PTR_REG (21), PTR_REG (0)
+.globl _dl_start_user
+.type _dl_start_user, %function
+_dl_start_user:
+	/* Get argc.  */
+	ldr	PTR_REG (1), [sp]
+	/* Get argv.  */
+	add	x2, sp, PTR_SIZE
+	/* Compute envp.  */
+	add	PTR_REG (3), PTR_REG (2), PTR_REG (1), lsl PTR_LOG_SIZE
+	add	PTR_REG (3), PTR_REG (3), PTR_SIZE
+	adrp	x16, _rtld_local
+	add	PTR_REG (16), PTR_REG (16), :lo12:_rtld_local
+	ldr	PTR_REG (0), [x16]
+	bl	_dl_init
+	/* Load the finalizer function.  */
+	adrp	x0, _dl_fini
+	add	PTR_REG (0), PTR_REG (0), :lo12:_dl_fini
+	/* Jump to the user's entry point.  */
+	mov     x16, x21
+	br      x16
+END (_start)
-- 
2.25.1

Re: [PATCH v6 0/4] Args adjustment with ./ld.so exe [BZ #23293]

[Szabolcs Nagy]

The 05/05/2022 14:58, Szabolcs Nagy via Libc-alpha wrote:
> The Hurd ld.so start code had to be adjusted. This revealed that
> the fix actually cuts across abstraction layers: fiddles with
> ELF entry stack layout in generic code, skipping the intermediate
> OS sysdep layer. If the OS layer ever wants to do something more
> complicated with the args then the interface contract with dl_main
> will have to be reevaluated.

or i can avoid touching hurd code if the arg adjustment
is moved to linux dl-sysdep.c

then _dl_skip_args is kept on hurd (hurd can separately
fix that in its dl-sysdeps.c), so all targets can remove
it other than x86.

Re: [PATCH v6 1/4] rtld: Use generic argv adjustment in ld.so [BZ #23293]

[Adhemerval Zanella]

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.

Re: [PATCH v6 2/4] rtld: Remove DL_ARGV_NOT_RELRO and make _dl_skip_args const

[Adhemerval Zanella]

On 05/05/2022 10:58, Szabolcs Nagy via Libc-alpha wrote:
> _dl_skip_args is always 0, so the target specific code that modifies
> argv after relro protection is applied is no longer used.
> 
> After the patch relro protection is applied to _dl_argv consistently
> on all targets.
> 
> ---
> v6:
> - const _dl_skip_args.
> v4:
> - New patch.
> Reviewed-by: Florian Weimer <fweimer@redhat.com>
> Tested-by: Florian Weimer <fweimer@redhat.com>

LGTM to me as well.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> ---
>  elf/rtld.c                               | 10 ++--------
>  sysdeps/aarch64/dl-sysdep.h              |  4 ----
>  sysdeps/alpha/dl-sysdep.h                | 23 -----------------------
>  sysdeps/arc/dl-sysdep.h                  |  4 ----
>  sysdeps/arm/dl-sysdep.h                  |  4 ----
>  sysdeps/csky/dl-sysdep.h                 | 23 -----------------------
>  sysdeps/generic/ldsodefs.h               | 13 +++----------
>  sysdeps/ia64/dl-sysdep.h                 | 23 -----------------------
>  sysdeps/nios2/dl-sysdep.h                |  4 ----
>  sysdeps/s390/s390-32/dl-sysdep.h         | 23 -----------------------
>  sysdeps/sparc/dl-sysdep.h                | 23 -----------------------
>  sysdeps/unix/sysv/linux/ia64/dl-sysdep.h |  4 ----
>  12 files changed, 5 insertions(+), 153 deletions(-)
>  delete mode 100644 sysdeps/alpha/dl-sysdep.h
>  delete mode 100644 sysdeps/csky/dl-sysdep.h
>  delete mode 100644 sysdeps/ia64/dl-sysdep.h
>  delete mode 100644 sysdeps/s390/s390-32/dl-sysdep.h
>  delete mode 100644 sysdeps/sparc/dl-sysdep.h
> 
> diff --git a/elf/rtld.c b/elf/rtld.c
> index b5070d453f..71f7095def 100644
> --- a/elf/rtld.c
> +++ b/elf/rtld.c
> @@ -157,16 +157,10 @@ static void dl_main_state_init (struct dl_main_state *state);
>  extern char **_environ attribute_hidden;
>  static void process_envvars (struct dl_main_state *state);
>  
> -#ifdef DL_ARGV_NOT_RELRO
> -int _dl_argc attribute_hidden;
> -char **_dl_argv = NULL;
> -/* Nonzero if we were run directly.  */
> -unsigned int _dl_skip_args attribute_hidden;
> -#else
>  int _dl_argc attribute_relro attribute_hidden;
>  char **_dl_argv attribute_relro = NULL;
> -unsigned int _dl_skip_args attribute_relro attribute_hidden;
> -#endif
> +/* Always 0, only kept for not-yet-updated target start code.  */
> +const unsigned int _dl_skip_args attribute_hidden;
>  rtld_hidden_data_def (_dl_argv)
>  
>  #ifndef THREAD_SET_STACK_GUARD
> diff --git a/sysdeps/aarch64/dl-sysdep.h b/sysdeps/aarch64/dl-sysdep.h
> index 667786671c..1516dd7d3f 100644
> --- a/sysdeps/aarch64/dl-sysdep.h
> +++ b/sysdeps/aarch64/dl-sysdep.h
> @@ -18,8 +18,4 @@
>  
>  #include_next <dl-sysdep.h>
>  
> -/* _dl_argv cannot be attribute_relro, because _dl_start_user
> -   might write into it after _dl_start returns.  */
> -#define DL_ARGV_NOT_RELRO 1
> -
>  #define DL_EXTERN_PROTECTED_DATA
> diff --git a/sysdeps/alpha/dl-sysdep.h b/sysdeps/alpha/dl-sysdep.h
> deleted file mode 100644
> index 3099ee419f..0000000000
> --- a/sysdeps/alpha/dl-sysdep.h
> +++ /dev/null
> @@ -1,23 +0,0 @@
> -/* System-specific settings for dynamic linker code.  Alpha version.
> -   Copyright (C) 2002-2022 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/>.  */
> -
> -#include_next <dl-sysdep.h>
> -
> -/* _dl_argv cannot be attribute_relro, because _dl_start_user
> -   might write into it after _dl_start returns.  */
> -#define DL_ARGV_NOT_RELRO 1
> diff --git a/sysdeps/arc/dl-sysdep.h b/sysdeps/arc/dl-sysdep.h
> index da060ceeee..cf4d160a73 100644
> --- a/sysdeps/arc/dl-sysdep.h
> +++ b/sysdeps/arc/dl-sysdep.h
> @@ -18,8 +18,4 @@
>  
>  #include_next <dl-sysdep.h>
>  
> -/* _dl_argv cannot be attribute_relro, because _dl_start_user
> -   might write into it after _dl_start returns.  */
> -#define DL_ARGV_NOT_RELRO 1
> -
>  #define DL_EXTERN_PROTECTED_DATA
> diff --git a/sysdeps/arm/dl-sysdep.h b/sysdeps/arm/dl-sysdep.h
> index ce7a84a7de..7a99107436 100644
> --- a/sysdeps/arm/dl-sysdep.h
> +++ b/sysdeps/arm/dl-sysdep.h
> @@ -18,8 +18,4 @@
>  
>  #include_next <dl-sysdep.h>
>  
> -/* _dl_argv cannot be attribute_relro, because _dl_start_user
> -   might write into it after _dl_start returns.  */
> -#define DL_ARGV_NOT_RELRO 1
> -
>  #define DL_EXTERN_PROTECTED_DATA
> diff --git a/sysdeps/csky/dl-sysdep.h b/sysdeps/csky/dl-sysdep.h
> deleted file mode 100644
> index fc8a58b94c..0000000000
> --- a/sysdeps/csky/dl-sysdep.h
> +++ /dev/null
> @@ -1,23 +0,0 @@
> -/* System-specific settings for dynamic linker code.  C-SKY version.
> -   Copyright (C) 2018-2022 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/>.  */
> -
> -#include_next <dl-sysdep.h>
> -
> -/* _dl_argv cannot be attribute_relro, because _dl_start_user
> -   might write into it after _dl_start returns.  */
> -#define DL_ARGV_NOT_RELRO 1
> diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h
> index 4a5e698db2..f3d3c78927 100644
> --- a/sysdeps/generic/ldsodefs.h
> +++ b/sysdeps/generic/ldsodefs.h
> @@ -762,18 +762,11 @@ rtld_hidden_proto (__libc_stack_end)
>  
>  /* Parameters passed to the dynamic linker.  */
>  extern int _dl_argc attribute_hidden attribute_relro;
> -extern char **_dl_argv
> -#ifndef DL_ARGV_NOT_RELRO
> -     attribute_relro
> -#endif
> -     ;
> +extern char **_dl_argv attribute_relro;
>  rtld_hidden_proto (_dl_argv)
>  #if IS_IN (rtld)
> -extern unsigned int _dl_skip_args attribute_hidden
> -# ifndef DL_ARGV_NOT_RELRO
> -     attribute_relro
> -# endif
> -     ;
> +/* Always 0, only kept for not-yet-updated target start code.  */
> +extern const unsigned int _dl_skip_args attribute_hidden;
>  #endif
>  #define rtld_progname _dl_argv[0]
>  
> diff --git a/sysdeps/ia64/dl-sysdep.h b/sysdeps/ia64/dl-sysdep.h
> deleted file mode 100644
> index e3a58bec24..0000000000
> --- a/sysdeps/ia64/dl-sysdep.h
> +++ /dev/null
> @@ -1,23 +0,0 @@
> -/* System-specific settings for dynamic linker code.  IA-64 version.
> -   Copyright (C) 2002-2022 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/>.  */
> -
> -#include_next <dl-sysdep.h>
> -
> -/* _dl_argv cannot be attribute_relro, because _dl_start_user
> -   might write into it after _dl_start returns.  */
> -#define DL_ARGV_NOT_RELRO 1
> diff --git a/sysdeps/nios2/dl-sysdep.h b/sysdeps/nios2/dl-sysdep.h
> index 0354650042..257b37c258 100644
> --- a/sysdeps/nios2/dl-sysdep.h
> +++ b/sysdeps/nios2/dl-sysdep.h
> @@ -18,8 +18,4 @@
>  
>  #include_next <dl-sysdep.h>
>  
> -/* _dl_argv cannot be attribute_relro, because _dl_start_user
> -   might write into it after _dl_start returns.  */
> -#define DL_ARGV_NOT_RELRO 1
> -
>  #define DL_EXTERN_PROTECTED_DATA
> diff --git a/sysdeps/s390/s390-32/dl-sysdep.h b/sysdeps/s390/s390-32/dl-sysdep.h
> deleted file mode 100644
> index 699b50f156..0000000000
> --- a/sysdeps/s390/s390-32/dl-sysdep.h
> +++ /dev/null
> @@ -1,23 +0,0 @@
> -/* System-specific settings for dynamic linker code.  S/390 version.
> -   Copyright (C) 2014-2022 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/>.  */
> -
> -#include_next <dl-sysdep.h>
> -
> -/* _dl_argv cannot be attribute_relro, because _dl_start_user
> -   might write into it after _dl_start returns.  */
> -#define DL_ARGV_NOT_RELRO 1
> diff --git a/sysdeps/sparc/dl-sysdep.h b/sysdeps/sparc/dl-sysdep.h
> deleted file mode 100644
> index f32f16a107..0000000000
> --- a/sysdeps/sparc/dl-sysdep.h
> +++ /dev/null
> @@ -1,23 +0,0 @@
> -/* System-specific settings for dynamic linker code.  SPARC version.
> -   Copyright (C) 2002-2022 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/>.  */
> -
> -#include_next <dl-sysdep.h>
> -
> -/* _dl_argv cannot be attribute_relro, because _dl_start_user
> -   might write into it after _dl_start returns.  */
> -#define DL_ARGV_NOT_RELRO 1
> diff --git a/sysdeps/unix/sysv/linux/ia64/dl-sysdep.h b/sysdeps/unix/sysv/linux/ia64/dl-sysdep.h
> index 0d2a1d093a..aa1de6b361 100644
> --- a/sysdeps/unix/sysv/linux/ia64/dl-sysdep.h
> +++ b/sysdeps/unix/sysv/linux/ia64/dl-sysdep.h
> @@ -48,8 +48,4 @@ extern int _dl_sysinfo_break attribute_hidden;
>         ".previous");
>  #endif
>  
> -/* _dl_argv cannot be attribute_relro, because _dl_start_user
> -   might write into it after _dl_start returns.  */
> -#define DL_ARGV_NOT_RELRO 1
> -
>  #endif	/* dl-sysdep.h */

Re: [PATCH v6 3/4] linux: Add a getauxval test [BZ #23293]

[Adhemerval Zanella]

On 05/05/2022 10:59, Szabolcs Nagy via Libc-alpha wrote:
> This is for bug 23293 and it relies on the glibc test system running
> tests via explicit ld.so invokation by default.

I think it is 'invocation'.

> 
> Reviewed-by: Florian Weimer <fweimer@redhat.com>

LGTM as well.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> 
> ---
> v4:
> - New patch.
> ---
>  sysdeps/unix/sysv/linux/Makefile        |  1 +
>  sysdeps/unix/sysv/linux/tst-getauxval.c | 74 +++++++++++++++++++++++++
>  2 files changed, 75 insertions(+)
>  create mode 100644 sysdeps/unix/sysv/linux/tst-getauxval.c
> 
> diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile
> index ca953804d0..89cb005c7d 100644
> --- a/sysdeps/unix/sysv/linux/Makefile
> +++ b/sysdeps/unix/sysv/linux/Makefile
> @@ -126,6 +126,7 @@ tests += tst-clone tst-clone2 tst-clone3 tst-fanotify tst-personality \
>    tst-prctl \
>    tst-scm_rights \
>    tst-epoll \
> +  tst-getauxval \
>    # tests
>  
>  # For +depfiles in Makerules.
> diff --git a/sysdeps/unix/sysv/linux/tst-getauxval.c b/sysdeps/unix/sysv/linux/tst-getauxval.c
> new file mode 100644
> index 0000000000..c4b6195743
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/tst-getauxval.c
> @@ -0,0 +1,74 @@
> +/* Basic test for getauxval.
> +   Copyright (C) 2022 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/>.  */
> +
> +#include <unistd.h>
> +#include <stdio.h>
> +#include <support/check.h>
> +#include <sys/auxv.h>
> +
> +static int missing;
> +static int mismatch;
> +
> +static void
> +check_nonzero (unsigned long t, const char *s)
> +{
> +  unsigned long v = getauxval (t);
> +  printf ("%s: %lu (0x%lx)\n", s, v, v);
> +  if (v == 0)
> +    missing++;
> +}
> +
> +static void
> +check_eq (unsigned long t, const char *s, unsigned long want)
> +{
> +  unsigned long v = getauxval (t);
> +  printf ("%s: %lu want: %lu\n", s, v, want);
> +  if (v != want)
> +    mismatch++;
> +}
> +
> +#define NZ(x) check_nonzero (x, #x)
> +#define EQ(x, want) check_eq (x, #x, want)
> +
> +static int
> +do_test (void)
> +{
> +  /* These auxv entries should be non-zero on Linux.  */
> +  NZ (AT_PHDR);
> +  NZ (AT_PHENT);
> +  NZ (AT_PHNUM);
> +  NZ (AT_PAGESZ);
> +  NZ (AT_ENTRY);
> +  NZ (AT_CLKTCK);
> +  NZ (AT_RANDOM);
> +  NZ (AT_EXECFN);
> +  if (missing)
> +    FAIL_EXIT1 ("Found %d missing auxv entries.\n", missing);
> +
> +  /* Check against syscalls.  */
> +  EQ (AT_UID, getuid ());
> +  EQ (AT_EUID, geteuid ());
> +  EQ (AT_GID, getgid ());
> +  EQ (AT_EGID, getegid ());
> +  if (mismatch)
> +    FAIL_EXIT1 ("Found %d mismatching auxv entries.\n", mismatch);
> +
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>

Re: [PATCH v6 4/4] aarch64: Move ld.so _start to separate file and drop _dl_skip_args

[Adhemerval Zanella]

On 05/05/2022 10:59, Szabolcs Nagy via Libc-alpha wrote:
> A separate asm file is easier to maintain than a macro that expands to
> inline asm.
> 
> The RTLD_START macro is only needed now because _dl_start is local in
> rtld.c, but _start has to call it, if _dl_start was made hidden then it
> could be empty.
> 
> _dl_skip_args is no longer needed.

LGTM, thanks.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> 
> ---
> v4:
> - adjust commit message about _dl_skip_args.
> v3:
> - mention _dl_skip_args
> v2:
> - fix typo in commit message.
> ---
>  sysdeps/aarch64/Makefile     |  1 +
>  sysdeps/aarch64/dl-machine.h | 77 +-----------------------------------
>  sysdeps/aarch64/dl-start.S   | 53 +++++++++++++++++++++++++
>  3 files changed, 56 insertions(+), 75 deletions(-)
>  create mode 100644 sysdeps/aarch64/dl-start.S
> 
> diff --git a/sysdeps/aarch64/Makefile b/sysdeps/aarch64/Makefile
> index 7183895d04..17fb1c5b72 100644
> --- a/sysdeps/aarch64/Makefile
> +++ b/sysdeps/aarch64/Makefile
> @@ -33,6 +33,7 @@ tst-audit27-ENV = LD_AUDIT=$(objpfx)tst-auditmod27.so
>  endif
>  
>  ifeq ($(subdir),elf)
> +sysdep-rtld-routines += dl-start
>  sysdep-dl-routines += tlsdesc dl-tlsdesc
>  gen-as-const-headers += dl-link.sym
>  
> diff --git a/sysdeps/aarch64/dl-machine.h b/sysdeps/aarch64/dl-machine.h
> index b40050a981..fe120bb507 100644
> --- a/sysdeps/aarch64/dl-machine.h
> +++ b/sysdeps/aarch64/dl-machine.h
> @@ -105,81 +105,8 @@ elf_machine_runtime_setup (struct link_map *l, struct r_scope_elem *scope[],
>    return lazy;
>  }
>  
> -/* Initial entry point for the dynamic linker. The C function
> -   _dl_start is the real entry point, its return value is the user
> -   program's entry point */
> -#ifdef __LP64__
> -# define RTLD_START RTLD_START_1 ("x", "3", "sp")
> -#else
> -# define RTLD_START RTLD_START_1 ("w", "2", "wsp")
> -#endif
> -
> -
> -#define RTLD_START_1(PTR, PTR_SIZE_LOG, PTR_SP) asm ("\
> -.text									\n\
> -.globl _start								\n\
> -.type _start, %function							\n\
> -.globl _dl_start_user							\n\
> -.type _dl_start_user, %function						\n\
> -_start:									\n\
> -	// bti c							\n\
> -	hint	34							\n\
> -	mov	" PTR "0, " PTR_SP "					\n\
> -	bl	_dl_start						\n\
> -	// returns user entry point in x0				\n\
> -	mov	x21, x0							\n\
> -_dl_start_user:								\n\
> -	// get the original arg count					\n\
> -	ldr	" PTR "1, [sp]						\n\
> -	// get the argv address						\n\
> -	add	" PTR "2, " PTR_SP ", #(1<<"  PTR_SIZE_LOG ")		\n\
> -	// get _dl_skip_args to see if we were				\n\
> -	// invoked as an executable					\n\
> -	adrp	x4, _dl_skip_args					\n\
> -        ldr	w4, [x4, #:lo12:_dl_skip_args]				\n\
> -	// do we need to adjust argc/argv				\n\
> -        cmp	w4, 0							\n\
> -	beq	.L_done_stack_adjust					\n\
> -	// subtract _dl_skip_args from original arg count		\n\
> -	sub	" PTR "1, " PTR "1, " PTR "4				\n\
> -	// store adjusted argc back to stack				\n\
> -	str	" PTR "1, [sp]						\n\
> -	// find the first unskipped argument				\n\
> -	mov	" PTR "3, " PTR "2					\n\
> -	add	" PTR "4, " PTR "2, " PTR "4, lsl #" PTR_SIZE_LOG "	\n\
> -	// shuffle argv down						\n\
> -1:	ldr	" PTR "5, [x4], #(1<<"  PTR_SIZE_LOG ")			\n\
> -	str	" PTR "5, [x3], #(1<<"  PTR_SIZE_LOG ")			\n\
> -	cmp	" PTR "5, #0						\n\
> -	bne	1b							\n\
> -	// shuffle envp down						\n\
> -1:	ldr	" PTR "5, [x4], #(1<<"  PTR_SIZE_LOG ")			\n\
> -	str	" PTR "5, [x3], #(1<<"  PTR_SIZE_LOG ")			\n\
> -	cmp	" PTR "5, #0						\n\
> -	bne	1b							\n\
> -	// shuffle auxv down						\n\
> -1:	ldp	" PTR "0, " PTR "5, [x4, #(2<<"  PTR_SIZE_LOG ")]!	\n\
> -	stp	" PTR "0, " PTR "5, [x3], #(2<<"  PTR_SIZE_LOG ")	\n\
> -	cmp	" PTR "0, #0						\n\
> -	bne	1b							\n\
> -	// Update _dl_argv						\n\
> -	adrp	x3, __GI__dl_argv					\n\
> -	str	" PTR "2, [x3, #:lo12:__GI__dl_argv]			\n\
> -.L_done_stack_adjust:							\n\
> -	// compute envp							\n\
> -	add	" PTR "3, " PTR "2, " PTR "1, lsl #" PTR_SIZE_LOG "	\n\
> -	add	" PTR "3, " PTR "3, #(1<<"  PTR_SIZE_LOG ")		\n\
> -	adrp	x16, _rtld_local					\n\
> -        add	" PTR "16, " PTR "16, #:lo12:_rtld_local		\n\
> -        ldr	" PTR "0, [x16]						\n\
> -	bl	_dl_init						\n\
> -	// load the finalizer function					\n\
> -	adrp	x0, _dl_fini						\n\
> -	add	" PTR "0, " PTR "0, #:lo12:_dl_fini			\n\
> -	// jump to the user_s entry point				\n\
> -	mov     x16, x21						\n\
> -	br      x16							\n\
> -");
> +/* In elf/rtld.c _dl_start should be global so dl-start.S can reference it.  */
> +#define RTLD_START asm (".globl _dl_start");
>  
>  #define elf_machine_type_class(type)					\
>    ((((type) == AARCH64_R(JUMP_SLOT)					\
> diff --git a/sysdeps/aarch64/dl-start.S b/sysdeps/aarch64/dl-start.S
> new file mode 100644
> index 0000000000..a3a57bd5a1
> --- /dev/null
> +++ b/sysdeps/aarch64/dl-start.S
> @@ -0,0 +1,53 @@
> +/* ld.so _start code.
> +   Copyright (C) 2022 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/>.  */
> +
> +#include <sysdep.h>
> +
> +ENTRY (_start)
> +	/* Create an initial frame with 0 LR and FP */
> +	cfi_undefined (x30)
> +	mov	x29, #0
> +	mov	x30, #0
> +
> +	mov	x0, sp
> +	PTR_ARG (0)
> +	bl	_dl_start
> +	/* Returns user entry point in x0.  */
> +	mov	PTR_REG (21), PTR_REG (0)
> +.globl _dl_start_user
> +.type _dl_start_user, %function
> +_dl_start_user:
> +	/* Get argc.  */
> +	ldr	PTR_REG (1), [sp]
> +	/* Get argv.  */
> +	add	x2, sp, PTR_SIZE
> +	/* Compute envp.  */
> +	add	PTR_REG (3), PTR_REG (2), PTR_REG (1), lsl PTR_LOG_SIZE
> +	add	PTR_REG (3), PTR_REG (3), PTR_SIZE
> +	adrp	x16, _rtld_local
> +	add	PTR_REG (16), PTR_REG (16), :lo12:_rtld_local
> +	ldr	PTR_REG (0), [x16]
> +	bl	_dl_init
> +	/* Load the finalizer function.  */
> +	adrp	x0, _dl_fini
> +	add	PTR_REG (0), PTR_REG (0), :lo12:_dl_fini
> +	/* Jump to the user's entry point.  */
> +	mov     x16, x21
> +	br      x16
> +END (_start)

Re: [PATCH v6 1/4] rtld: Use generic argv adjustment in ld.so [BZ #23293]

[Szabolcs Nagy]

[-- Attachment #1: Type: text/plain, Size: 1750 bytes --]

The 05/13/2022 16:56, Adhemerval Zanella wrote:
> On 05/05/2022 10:58, Szabolcs Nagy via Libc-alpha wrote:
> > +_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.

skip_args == 0 should be fine (any skip_args makes sp point to argc).

> > +  /* 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.

i did this differently, but same in spirit.

attaching the patch i committed.

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

committed the patch, if it breaks hurd we can fix/revert later.

[-- Attachment #2: v7-0001-rtld-Use-generic-argv-adjustment-in-ld.so-BZ-2329.patch --]
[-- Type: text/plain, Size: 9234 bytes --]

From ad43cac44a6860eaefcadadfb2acb349921e96bf Mon Sep 17 00:00:00 2001
From: Szabolcs Nagy <szabolcs.nagy@arm.com>
Date: Fri, 15 Jun 2018 16:14:58 +0100
Subject: [COMMITTED v7] rtld: Use generic argv adjustment in ld.so [BZ #23293]

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.

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 578fc14cdb..6e8ed430e2 100644
--- a/elf/rtld.c
+++ b/elf/rtld.c
@@ -1301,6 +1301,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);
+  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. */
+  ElfW(auxv_t) ax;
+  char *oldp = (char *) (p + 1);
+  char *newp = (char *) (sp + 1);
+  do
+    {
+      memcpy (&ax, oldp, sizeof (ax));
+      memcpy (newp, &ax, sizeof (ax));
+      oldp += sizeof (ax);
+      newp += sizeof (ax);
+    }
+  while (ax.a_type != AT_NULL);
+#endif
+}
+
 static void
 dl_main (const ElfW(Phdr) *phdr,
 	 ElfW(Word) phnum,
@@ -1354,6 +1410,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;
@@ -1368,7 +1425,6 @@ dl_main (const ElfW(Phdr) *phdr,
 		GLRO(dl_lazy) = -1;
 	      }
 
-	    ++_dl_skip_args;
 	    --_dl_argc;
 	    ++_dl_argv;
 	  }
@@ -1377,14 +1433,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;
 	  }
@@ -1394,7 +1448,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;
 	  }
@@ -1403,7 +1456,6 @@ dl_main (const ElfW(Phdr) *phdr,
 	  {
 	    GLRO(dl_inhibit_rpath) = _dl_argv[2];
 
-	    _dl_skip_args += 2;
 	    _dl_argc -= 2;
 	    _dl_argv += 2;
 	  }
@@ -1411,14 +1463,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;
 	  }
@@ -1426,7 +1476,6 @@ dl_main (const ElfW(Phdr) *phdr,
 	  {
 	    argv0 = _dl_argv[2];
 
-	    _dl_skip_args += 2;
 	    _dl_argc -= 2;
 	    _dl_argv += 2;
 	  }
@@ -1434,7 +1483,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;
 	  }
@@ -1442,7 +1490,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;
 	  }
@@ -1451,7 +1498,6 @@ dl_main (const ElfW(Phdr) *phdr,
 	  {
 	    state.mode = rtld_mode_list_tunables;
 
-	    ++_dl_skip_args;
 	    --_dl_argc;
 	    ++_dl_argv;
 	  }
@@ -1460,7 +1506,6 @@ dl_main (const ElfW(Phdr) *phdr,
 	  {
 	    state.mode = rtld_mode_list_diagnostics;
 
-	    ++_dl_skip_args;
 	    --_dl_argc;
 	    ++_dl_argv;
 	  }
@@ -1506,7 +1551,6 @@ dl_main (const ElfW(Phdr) *phdr,
 	    _dl_usage (ld_so_name, NULL);
 	}
 
-      ++_dl_skip_args;
       --_dl_argc;
       ++_dl_argv;
 
@@ -1605,6 +1649,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
     {
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])
 		{
-- 
2.25.1