[v2] elf/rtld: Fix auxiliary vector for enable_secure

  Starting with commit
59974938fe1f4add843f5325f78e2a7ccd8db853
elf/rtld: Count skipped environment variables for enable_secure

The new testcase elf/tst-tunables-enable_secure-env segfaults on s390 (31bit).
There _start parses the auxiliary vector for some additional checks.

Therefore it skips over the zeros after the environment variables ...
0x7fffac20:     0x7fffbd17      0x7fffbd32      0x7fffbd69      0x00000000
------------------------------------------------^^^last environment variable

... and then it parses the auxiliary vector and stops at AT_NULL.
0x7fffac30:     0x00000000      0x00000021      0x00000000      0x00000000
--------------------------------^^^AT_SYSINFO_EHDR--------------^^^AT_NULL
----------------^^^newp-----------------------------------------^^^oldp
Afterwards it tries to access AT_PHDR which points to somewhere and segfaults.

Due to not incorporating the skip_env variable in the computation of oldp
when shuffling down the auxv in rtld.c, it just copies one entry with AT_NULL
and value 0x00000021 and stops the loop.  In reality we have skipped
GLIBC_TUNABLES environment variable (=> skip_env=1). Thus we should copy from
here:
0x7fffac40:     0x00000021      0x7ffff000      0x00000010      0x007fffff
----------------^^^fixed-oldp

This patch fixes the computation of oldp when shuffling down auxiliary vector.
It also adds some checks in the testcase.  Those checks also fail on
s390x (64bit) and x86_64 without the fix.

Co-authored-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>
---
 elf/Makefile                         |   9 +-
 elf/rtld.c                           |   2 +-
 elf/tst-tunables-enable_secure-env.c | 127 ++++++++++++++++++++++++++-
 3 files changed, 126 insertions(+), 12 deletions(-)
  

On 02/07/24 10:25, Stefan Liebler wrote:
> Starting with commit
> 59974938fe1f4add843f5325f78e2a7ccd8db853
> elf/rtld: Count skipped environment variables for enable_secure
> 
> The new testcase elf/tst-tunables-enable_secure-env segfaults on s390 (31bit).
> There _start parses the auxiliary vector for some additional checks.
> 
> Therefore it skips over the zeros after the environment variables ...
> 0x7fffac20:     0x7fffbd17      0x7fffbd32      0x7fffbd69      0x00000000
> ------------------------------------------------^^^last environment variable
> 
> ... and then it parses the auxiliary vector and stops at AT_NULL.
> 0x7fffac30:     0x00000000      0x00000021      0x00000000      0x00000000
> --------------------------------^^^AT_SYSINFO_EHDR--------------^^^AT_NULL
> ----------------^^^newp-----------------------------------------^^^oldp
> Afterwards it tries to access AT_PHDR which points to somewhere and segfaults.
> 
> Due to not incorporating the skip_env variable in the computation of oldp
> when shuffling down the auxv in rtld.c, it just copies one entry with AT_NULL
> and value 0x00000021 and stops the loop.  In reality we have skipped
> GLIBC_TUNABLES environment variable (=> skip_env=1). Thus we should copy from
> here:
> 0x7fffac40:     0x00000021      0x7ffff000      0x00000010      0x007fffff
> ----------------^^^fixed-oldp
> 
> This patch fixes the computation of oldp when shuffling down auxiliary vector.
> It also adds some checks in the testcase.  Those checks also fail on
> s390x (64bit) and x86_64 without the fix.
> 
> Co-authored-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

LGTM, thanks.

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

> ---
>  elf/Makefile                         |   9 +-
>  elf/rtld.c                           |   2 +-
>  elf/tst-tunables-enable_secure-env.c | 127 ++++++++++++++++++++++++++-
>  3 files changed, 126 insertions(+), 12 deletions(-)
> 
> diff --git a/elf/Makefile b/elf/Makefile
> index 24ad5221c2..a3475f3fb5 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -1224,7 +1224,6 @@ tests-special += \
>    $(objpfx)tst-trace3.out \
>    $(objpfx)tst-trace4.out \
>    $(objpfx)tst-trace5.out \
> -  $(objpfx)tst-tunables-enable_secure-env.out \
>    $(objpfx)tst-unused-dep-cmp.out \
>    $(objpfx)tst-unused-dep.out \
>    # tests-special
> @@ -2252,13 +2251,7 @@ $(objpfx)tst-unused-dep-cmp.out: $(objpfx)tst-unused-dep.out
>  	cmp $< /dev/null > $@; \
>  	$(evaluate-test)
>  
> -$(objpfx)tst-tunables-enable_secure-env.out: $(objpfx)tst-tunables-enable_secure-env
> -	$(test-wrapper-env) \
> -	GLIBC_TUNABLES=glibc.rtld.enable_secure=1 \
> -	$(rtld-prefix) \
> -	  $< > $@; \
> -	$(evaluate-test)
> -
> +tst-tunables-enable_secure-env-ARGS = -- $(host-test-program-cmd)
>  
>  $(objpfx)tst-audit11.out: $(objpfx)tst-auditmod11.so $(objpfx)tst-audit11mod1.so
>  tst-audit11-ENV = LD_AUDIT=$(objpfx)tst-auditmod11.so
> diff --git a/elf/rtld.c b/elf/rtld.c
> index 6352ba76c5..bfdf632e77 100644
> --- a/elf/rtld.c
> +++ b/elf/rtld.c
> @@ -1327,7 +1327,7 @@ _dl_start_args_adjust (int skip_args, int skip_env)
>  
>    /* Shuffle auxv down. */
>    ElfW(auxv_t) ax;
> -  char *oldp = (char *) (p + 1);
> +  char *oldp = (char *) (p + 1 + skip_env);
>    char *newp = (char *) (sp + 1);
>    do
>      {
> diff --git a/elf/tst-tunables-enable_secure-env.c b/elf/tst-tunables-enable_secure-env.c
> index 24e846f299..01f121efc3 100644
> --- a/elf/tst-tunables-enable_secure-env.c
> +++ b/elf/tst-tunables-enable_secure-env.c
> @@ -17,15 +17,136 @@
>     License along with the GNU C Library; if not, see
>     <https://www.gnu.org/licenses/>.  */
>  
> +#include <array_length.h>
> +#include <errno.h>
> +#include <getopt.h>
> +#include <intprops.h>
> +#include <stdlib.h>
>  #include <support/capture_subprocess.h>
>  #include <support/check.h>
> +#ifdef __linux__
> +# define HAVE_AUXV 1
> +# include <sys/auxv.h>
> +#else
> +# define HAVE_AUXV 0
> +#endif
> +
> +/* Nonzero if the program gets called via `exec'.  */
> +#define CMDLINE_OPTIONS \
> +  { "restart", no_argument, &restart, 1 },
> +static int restart;
> +
> +/* Hold the four initial argument used to respawn the process, plus the extra
> +   '--direct', '--restart', auxiliary vector values, and final NULL.  */
> +static char *spargs[11];
> +
> +#if HAVE_AUXV
> +static void
> +check_auxv (unsigned long type, char *argv)
> +{
> +  char *endptr;
> +  errno = 0;
> +  unsigned long int varg = strtol (argv, &endptr, 10);
> +  TEST_VERIFY_EXIT (errno == 0);
> +  TEST_VERIFY_EXIT (*endptr == '\0');
> +  errno = 0;
> +  unsigned long int v = getauxval (type);
> +  TEST_COMPARE (errno, 0);
> +  TEST_COMPARE (varg, v);
> +}
> +#endif
> +
> +/* Called on process re-execution.  */
> +_Noreturn static void
> +handle_restart (int argc, char *argv[])
> +{
> +  TEST_VERIFY (getenv ("GLIBC_TUNABLES") == NULL);
> +  TEST_VERIFY (getenv ("LD_BIND_NOW") == NULL);
> +
> +#if HAVE_AUXV
> +  TEST_VERIFY_EXIT (argc == 4);
> +  check_auxv (AT_PHENT, argv[0]);
> +  check_auxv (AT_PHNUM, argv[1]);
> +  check_auxv (AT_PAGESZ, argv[2]);
> +  check_auxv (AT_HWCAP, argv[3]);
> +#endif
> +
> +  exit (EXIT_SUCCESS);
> +}
>  
>  static int
>  do_test (int argc, char *argv[])
>  {
> -  /* Ensure that no assertions are hit when a dynamically linked application
> -     runs.  This test requires that GLIBC_TUNABLES=glibc.rtld.enable_secure=1
> -     is set. */
> +  /* We must have either:
> +
> +     - four parameter if called initially:
> +       + path for ld.so            [optional]
> +       + "--library-path"          [optional]
> +       + the library path          [optional]
> +       + the application name
> +
> +     - either parameters left if called through re-execution.
> +       + auxiliary vector value 1
> +       + auxiliary vector value 2
> +       + auxiliary vector value 3
> +       + auxiliary vector value 4
> +  */
> +  if (restart)
> +    handle_restart (argc - 1, &argv[1]);
> +
> +  TEST_VERIFY_EXIT (argc == 2 || argc == 5);
> +
> +#if HAVE_AUXV
> +  struct
> +  {
> +    unsigned long int type;
> +    char str[INT_BUFSIZE_BOUND (unsigned long)];
> +  } auxvals[] =
> +  {
> +    /* Check some auxiliary values that should be constant over process
> +       re-execution.  */
> +    { AT_PHENT },
> +    { AT_PHNUM },
> +    { AT_PAGESZ },
> +    { AT_HWCAP },
> +  };
> +  for (int i = 0; i < array_length (auxvals); i++)
> +  {
> +    unsigned long int v = getauxval (auxvals[i].type);
> +    snprintf (auxvals[i].str, sizeof auxvals[i].str, "%lu", v);
> +  }
> +#endif
> +
> +  {
> +    int i;
> +    for (i = 0; i < argc - 1; i++)
> +      spargs[i] = argv[i + 1];
> +    spargs[i++] = (char *) "--direct";
> +    spargs[i++] = (char *) "--restart";
> +#if HAVE_AUXV
> +    for (int j = 0; j < array_length (auxvals); j++)
> +      spargs[i++] = auxvals[j].str;
> +#endif
> +    spargs[i] = NULL;
> +  }
> +
> +  {
> +    char *envs[] =
> +    {
> +      /* Add some environment variable that should be filtered out.  */
> +      (char *) "GLIBC_TUNABLES=glibc.rtld.enable_secure=1",
> +      (char* ) "LD_BIND_NOW=0",
> +      NULL,
> +    };
> +    struct support_capture_subprocess result
> +      = support_capture_subprogram (spargs[0], spargs, envs);
> +    support_capture_subprocess_check (&result,
> +				      "tst-tunables-enable_secure-env",
> +				      0,
> +				      sc_allow_none);
> +    support_capture_subprocess_free (&result);
> +  }
> +
>    return 0;
>  }
>
  

On 02.07.24 18:57, Adhemerval Zanella Netto wrote:
> 
> 
> On 02/07/24 10:25, Stefan Liebler wrote:
>> Starting with commit
>> 59974938fe1f4add843f5325f78e2a7ccd8db853
>> elf/rtld: Count skipped environment variables for enable_secure
>>
>> The new testcase elf/tst-tunables-enable_secure-env segfaults on s390 (31bit).
>> There _start parses the auxiliary vector for some additional checks.
>>
>> Therefore it skips over the zeros after the environment variables ...
>> 0x7fffac20:     0x7fffbd17      0x7fffbd32      0x7fffbd69      0x00000000
>> ------------------------------------------------^^^last environment variable
>>
>> ... and then it parses the auxiliary vector and stops at AT_NULL.
>> 0x7fffac30:     0x00000000      0x00000021      0x00000000      0x00000000
>> --------------------------------^^^AT_SYSINFO_EHDR--------------^^^AT_NULL
>> ----------------^^^newp-----------------------------------------^^^oldp
>> Afterwards it tries to access AT_PHDR which points to somewhere and segfaults.
>>
>> Due to not incorporating the skip_env variable in the computation of oldp
>> when shuffling down the auxv in rtld.c, it just copies one entry with AT_NULL
>> and value 0x00000021 and stops the loop.  In reality we have skipped
>> GLIBC_TUNABLES environment variable (=> skip_env=1). Thus we should copy from
>> here:
>> 0x7fffac40:     0x00000021      0x7ffff000      0x00000010      0x007fffff
>> ----------------^^^fixed-oldp
>>
>> This patch fixes the computation of oldp when shuffling down auxiliary vector.
>> It also adds some checks in the testcase.  Those checks also fail on
>> s390x (64bit) and x86_64 without the fix.
>>
>> Co-authored-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>
> 
> LGTM, thanks.
> 
> Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>
> 
I've just committed this patch. Thanks, Stefan.