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

[Siddhesh Poyarekar <siddhesh@sourceware.org>]

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