[1/4] elf: Only process multiple tunable once (BZ 31686)

  The parse_tunables_string is a tunable entry on the 'tunable_list' list
to be set later without checking if the entry is already present.  If
leads to a stack overflow if the tunable is set multiple times,
for instance:

  GLIBC_TUNABLES=glibc.malloc.check=2:... (repeat over the number of
  total support for different tunable).

Instead, use the index of the tunable list to get the expected tunable
entry.  Since now the initial list is zero-initialized, the compiler
might emit an extra memset and this requires some minor adjustment
on some ports.

Checked on x86_64-linux-gnu and aarch64-linux-gnu.

Reported-by: Yuto Maeda <maeda@cyberdefense.jp>
---
 elf/dl-tunables.c                          | 30 ++++++-----
 elf/tst-tunables.c                         | 59 +++++++++++++++++++++-
 sysdeps/aarch64/multiarch/memset_generic.S |  4 ++
 sysdeps/sparc/sparc64/rtld-memset.c        |  3 ++
 4 files changed, 82 insertions(+), 14 deletions(-)
  

* Adhemerval Zanella:

> The parse_tunables_string is a tunable entry on the 'tunable_list' list
> to be set later without checking if the entry is already present.  If
> leads to a stack overflow if the tunable is set multiple times,
> for instance:
>
>   GLIBC_TUNABLES=glibc.malloc.check=2:... (repeat over the number of
>   total support for different tunable).
>
> Instead, use the index of the tunable list to get the expected tunable
> entry.  Since now the initial list is zero-initialized, the compiler
> might emit an extra memset and this requires some minor adjustment
> on some ports.
>
> Checked on x86_64-linux-gnu and aarch64-linux-gnu.
>
> Reported-by: Yuto Maeda <maeda@cyberdefense.jp>

Is this fixing a particular commit?  If it does, could you mention that
in the commit message?

Thanks,
Florian
  

On 01/05/24 09:54, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> The parse_tunables_string is a tunable entry on the 'tunable_list' list
>> to be set later without checking if the entry is already present.  If
>> leads to a stack overflow if the tunable is set multiple times,
>> for instance:
>>
>>   GLIBC_TUNABLES=glibc.malloc.check=2:... (repeat over the number of
>>   total support for different tunable).
>>
>> Instead, use the index of the tunable list to get the expected tunable
>> entry.  Since now the initial list is zero-initialized, the compiler
>> might emit an extra memset and this requires some minor adjustment
>> on some ports.
>>
>> Checked on x86_64-linux-gnu and aarch64-linux-gnu.
>>
>> Reported-by: Yuto Maeda <maeda@cyberdefense.jp>
> 
> Is this fixing a particular commit?  If it does, could you mention that
> in the commit message?

Yes, I added on the cover-letter (680c597e9c3).  I will add it on the
comment message as well.
  

On 2024-04-30 15:25, Adhemerval Zanella wrote:
> The parse_tunables_string is a tunable entry on the 'tunable_list' list
> to be set later without checking if the entry is already present.  If
> leads to a stack overflow if the tunable is set multiple times,
> for instance:
> 
>    GLIBC_TUNABLES=glibc.malloc.check=2:... (repeat over the number of
>    total support for different tunable).
> 
> Instead, use the index of the tunable list to get the expected tunable
> entry.  Since now the initial list is zero-initialized, the compiler
> might emit an extra memset and this requires some minor adjustment
> on some ports.
> 
> Checked on x86_64-linux-gnu and aarch64-linux-gnu.
> 
> Reported-by: Yuto Maeda <maeda@cyberdefense.jp>

Please also credit Yutaro Shimizu <shimizu@cyberdefense.jp> in reporters.

> ---
>   elf/dl-tunables.c                          | 30 ++++++-----
>   elf/tst-tunables.c                         | 59 +++++++++++++++++++++-
>   sysdeps/aarch64/multiarch/memset_generic.S |  4 ++
>   sysdeps/sparc/sparc64/rtld-memset.c        |  3 ++
>   4 files changed, 82 insertions(+), 14 deletions(-)
> 
> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
> index d3ccd2ecd4..1db80e0f92 100644
> --- a/elf/dl-tunables.c
> +++ b/elf/dl-tunables.c
> @@ -32,6 +32,7 @@
>   #include <ldsodefs.h>
>   #include <array_length.h>
>   #include <dl-minimal-malloc.h>
> +#include <dl-symbol-redir-ifunc.h>
>   
>   #define TUNABLES_INTERNAL 1
>   #include "dl-tunables.h"
> @@ -221,8 +222,7 @@ parse_tunables_string (const char *valstring, struct tunable_toset_t *tunables)
>   
>   	  if (tunable_is_name (cur->name, name))
>   	    {
> -	      tunables[ntunables++] =
> -		(struct tunable_toset_t) { cur, value, p - value };
> +	      tunables[i] = (struct tunable_toset_t) { cur, value, p - value };

Uninitialized tunables stay as NULL, tunables appearing later in the 
string will persist in case of duplicates.  OK.

>   
>   	      /* Ignore tunables if enable_secure is set */
>   	      if (tunable_is_name ("glibc.rtld.enable_secure", name))
> @@ -245,23 +245,27 @@ parse_tunables_string (const char *valstring, struct tunable_toset_t *tunables)
>   static void
>   parse_tunables (const char *valstring)
>   {
> -  struct tunable_toset_t tunables[tunables_list_size];
> -  int ntunables = parse_tunables_string (valstring, tunables);
> -  if (ntunables == -1)
> +  struct tunable_toset_t tunables[tunables_list_size] = { 0 };

zero-initialized, so tunables[i].t is NULL unless set above.  OK.

> +  if (parse_tunables_string (valstring, tunables) == -1)
>       {
>         _dl_error_printf (
>           "WARNING: ld.so: invalid GLIBC_TUNABLES `%s': ignored.\n", valstring);
>         return;
>       }
>   
> -  for (int i = 0; i < ntunables; i++)
> -    if (!tunable_initialize (tunables[i].t, tunables[i].value,
> -			     tunables[i].len))
> -      _dl_error_printf ("WARNING: ld.so: invalid GLIBC_TUNABLES value `%.*s' "
> -		       "for option `%s': ignored.\n",
> -		       (int) tunables[i].len,
> -		       tunables[i].value,
> -		       tunables[i].t->name);
> +  for (int i = 0; i < tunables_list_size; i++)
> +    {
> +      if (tunables[i].t == NULL)
> +	continue;

Skip over NULLs, initializing only those that are set.  OK.

> +
> +      if (!tunable_initialize (tunables[i].t, tunables[i].value,
> +			       tunables[i].len))
> +	_dl_error_printf ("WARNING: ld.so: invalid GLIBC_TUNABLES value `%.*s' "
> +			  "for option `%s': ignored.\n",
> +			  (int) tunables[i].len,
> +			  tunables[i].value,
> +			  tunables[i].t->name);
> +    }
>   }
>   
>   /* Initialize the tunables list from the environment.  For now we only use the
> diff --git a/elf/tst-tunables.c b/elf/tst-tunables.c
> index 095b5c81d9..ce5f62f777 100644
> --- a/elf/tst-tunables.c
> +++ b/elf/tst-tunables.c
> @@ -17,6 +17,7 @@
>      <https://www.gnu.org/licenses/>.  */
>   
>   #include <array_length.h>
> +#define TUNABLES_INTERNAL 1
>   #include <dl-tunables.h>
>   #include <getopt.h>
>   #include <intprops.h>
> @@ -24,12 +25,13 @@
>   #include <stdlib.h>
>   #include <support/capture_subprocess.h>
>   #include <support/check.h>
> +#include <support/support.h>
>   
>   static int restart;
>   #define CMDLINE_OPTIONS \
>     { "restart", no_argument, &restart, 1 },
>   
> -static const struct test_t
> +static struct test_t
>   {
>     const char *name;
>     const char *value;
> @@ -284,6 +286,29 @@ static const struct test_t
>       0,
>       0,
>     },
> +  /* Also check for repeated tunables with a count larger than the total number
> +     of tunables.  */
> +  {
> +    "GLIBC_TUNABLES",
> +    NULL,
> +    2,
> +    0,
> +    0,
> +  },
> +  {
> +    "GLIBC_TUNABLES",
> +    NULL,
> +    1,
> +    0,
> +    0,
> +  },
> +  {
> +    "GLIBC_TUNABLES",
> +    NULL,
> +    0,
> +    0,
> +    0,
> +  },
>   };
>   
>   static int
> @@ -316,6 +341,7 @@ do_test (int argc, char *argv[])
>   
>     char nteststr[INT_BUFSIZE_BOUND (int)];
>   
> +

Spurious newline.

>     char *spargv[10];
>     {
>       int i = 0;
> @@ -327,6 +353,37 @@ do_test (int argc, char *argv[])
>       spargv[i] = NULL;
>     }
>   
> +  /* Create a tunable line with the duplicate values with a total number
> +     larger than the different number of tunables.  */
> +  {
> +    enum { tunables_list_size = array_length (tunable_list) };
> +    const char *value = "";
> +    for (int i = 0; i < tunables_list_size; i++)
> +      value = xasprintf ("%sglibc.malloc.check=2%c",
> +			 value,
> +			 i == (tunables_list_size - 1) ? '\0' : ':');
> +    tests[33].value = value;
> +  }
> +  /* Same as before, but the last tunable vallues is differen than the
> +     rest.  */
> +  {
> +    enum { tunables_list_size = array_length (tunable_list) };
> +    const char *value = "";
> +    for (int i = 0; i < tunables_list_size - 1; i++)
> +      value = xasprintf ("%sglibc.malloc.check=2:", value);
> +    value = xasprintf ("%sglibc.malloc.check=1", value);
> +    tests[34].value = value;
> +  }
> +  /* Same as before, but with an invalid last entry.  */
> +  {
> +    enum { tunables_list_size = array_length (tunable_list) };
> +    const char *value = "";
> +    for (int i = 0; i < tunables_list_size - 1; i++)
> +      value = xasprintf ("%sglibc.malloc.check=2:", value);
> +    value = xasprintf ("%sglibc.malloc.check=1=1", value);
> +    tests[35].value = value;
> +  }
> +
>     for (int i = 0; i < array_length (tests); i++)
>       {
>         snprintf (nteststr, sizeof nteststr, "%d", i);
> diff --git a/sysdeps/aarch64/multiarch/memset_generic.S b/sysdeps/aarch64/multiarch/memset_generic.S
> index 81748bdbce..e125a5ed85 100644
> --- a/sysdeps/aarch64/multiarch/memset_generic.S
> +++ b/sysdeps/aarch64/multiarch/memset_generic.S
> @@ -33,3 +33,7 @@
>   #endif
>   
>   #include <../memset.S>
> +
> +#if IS_IN (rtld)
> +strong_alias (memset, __memset_generic)
> +#endif

The extra memset you mentioned.  OK.

> diff --git a/sysdeps/sparc/sparc64/rtld-memset.c b/sysdeps/sparc/sparc64/rtld-memset.c
> index 55f3835790..a19202a620 100644
> --- a/sysdeps/sparc/sparc64/rtld-memset.c
> +++ b/sysdeps/sparc/sparc64/rtld-memset.c
> @@ -1 +1,4 @@
>   #include <string/memset.c>
> +#if IS_IN(rtld)
> +strong_alias (memset, __memset_ultra1)
> +#endif

Likewise.

Thanks,
Sid