diff mbox series

[4/4] elf: Make glibc.rtld.enable_secure ignore alias environment variables

Message ID 20240430192739.1032549-5-adhemerval.zanella@linaro.org
State New
Headers show
Series More tunable fixes | expand

Commit Message

Adhemerval Zanella Netto April 30, 2024, 7:25 p.m. UTC
So tunable with environment variables aliases are also ignored if
glibc.rtld.enable_secure is enabled.  The tunable parsing is also
optimized a bit, where the loop that checks each environment variable
only checks for the tunables with aliases instead of all tables.

Checked on aarch64-linux-gnu and x86_64-linux-gnu.
---
 elf/dl-tunables.c                |  34 ++++++--
 elf/tst-tunables-enable_secure.c | 131 +++++++++++++++++++++++++++----
 scripts/gen-tunables.awk         |  64 ++++++++++-----
 3 files changed, 189 insertions(+), 40 deletions(-)

Comments

Siddhesh Poyarekar May 1, 2024, 5:40 p.m. UTC | #1
On 2024-04-30 15:25, Adhemerval Zanella wrote:
> So tunable with environment variables aliases are also ignored if
> glibc.rtld.enable_secure is enabled.  The tunable parsing is also
> optimized a bit, where the loop that checks each environment variable
> only checks for the tunables with aliases instead of all tables.
> 
> Checked on aarch64-linux-gnu and x86_64-linux-gnu.

Changing the sorting of the list of tunables ends up breaking internal 
ABI.  It's only a problem that crops up intermittently, e.g. when doing 
updates where a system may, for a very short time, have a different 
dynamic linker and libc.

How about, instead, something like this:

1. Run through envp twice, first time just for tunables and the second 
time for envvar aliases.

2. Generate a second, separate array of only the tunables with env 
aliases for the second envvar alias run.  It could be simply:

tunable_envalias_t
{
   const char *name;
   tunable_t *t;
};

3. Skip the second run if enable_secure is set.

Thanks,
Sid

> ---
>   elf/dl-tunables.c                |  34 ++++++--
>   elf/tst-tunables-enable_secure.c | 131 +++++++++++++++++++++++++++----
>   scripts/gen-tunables.awk         |  64 ++++++++++-----
>   3 files changed, 189 insertions(+), 40 deletions(-)
> 
> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
> index 63cf8c7ab5..c1a1d1a2e3 100644
> --- a/elf/dl-tunables.c
> +++ b/elf/dl-tunables.c
> @@ -300,6 +300,10 @@ __tunables_init (char **envp)
>     if (__libc_enable_secure)
>       return;
>   
> +  /* The tunable with environment variable alias are placed at the start of
> +     tunable array.  */
> +  struct tunable_toset_t tunables_env_alias[TUNABLE_NUM_ENV_ALIAS] = { 0 };
> +
>     while ((envp = get_next_env (envp, &envname, &envval, &prev_envp)) != NULL)
>       {
>         /* The environment variable is allocated on the stack by the kernel, so
> @@ -311,29 +315,43 @@ __tunables_init (char **envp)
>   	  continue;
>   	}
>   
> -      for (int i = 0; i < tunables_list_size; i++)
> +      for (int i = 0; i < TUNABLE_NUM_ENV_ALIAS; i++)
>   	{
>   	  tunable_t *cur = &tunable_list[i];
> +	  const char *name = cur->env_alias;
>   
> -	  /* Skip over tunables that have either been set already or should be
> -	     skipped.  */
> -	  if (cur->initialized || cur->env_alias[0] == '\0')
> +	  if (name[0] == '\0')
>   	    continue;
>   
> -	  const char *name = cur->env_alias;
> -
> -	  /* We have a match.  Initialize and move on to the next line.  */
>   	  if (tunable_is_name (name, envname))
>   	    {
>   	      size_t envvallen = 0;
>   	      /* The environment variable is always null-terminated.  */
>   	      for (const char *p = envval; *p != '\0'; p++, envvallen++);
>   
> -	      tunable_initialize (cur, envval, envvallen);
> +	      tunables_env_alias[i] =
> +		(struct tunable_toset_t) { cur, envval, envvallen };
>   	      break;
>   	    }
>   	}
>       }
> +
> +  /* Check if glibc.rtld.enable_secure was set and skip over the environment
> +     variables aliases.  */
> +  if (__libc_enable_secure)
> +    return;
> +
> +  for (int i = 0; i < TUNABLE_NUM_ENV_ALIAS; i++)
> +    {
> +      if (tunables_env_alias[i].t == NULL
> +	  || tunables_env_alias[i].t->initialized)
> +	continue;
> +
> +      if (!tunable_initialize (tunables_env_alias[i].t,
> +			       tunables_env_alias[i].value,
> +			       tunables_env_alias[i].len))
> +	parse_tunable_print_error (&tunables_env_alias[i]);
> +    }
>   }
>   
>   void
> diff --git a/elf/tst-tunables-enable_secure.c b/elf/tst-tunables-enable_secure.c
> index f5db1c84e9..d4938a2e5c 100644
> --- a/elf/tst-tunables-enable_secure.c
> +++ b/elf/tst-tunables-enable_secure.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>
> @@ -34,6 +35,8 @@ static int restart;
>   static const struct test_t
>   {
>     const char *env;
> +  const char *extraenv;
> +  bool check_multiple;
>     int32_t expected_malloc_check;
>     int32_t expected_enable_secure;
>   } tests[] =
> @@ -41,39 +44,124 @@ static const struct test_t
>     /* Expected tunable format.  */
>     /* Tunables should be ignored if enable_secure is set. */
>     {
> -    "glibc.malloc.check=2:glibc.rtld.enable_secure=1",
> +    "GLIBC_TUNABLES=glibc.malloc.check=2:glibc.rtld.enable_secure=1",
> +    NULL,
> +    false,
>       0,
>       1,
>     },
>     /* Tunables should be ignored if enable_secure is set. */
>     {
> -    "glibc.rtld.enable_secure=1:glibc.malloc.check=2",
> +    "GLIBC_TUNABLES=glibc.rtld.enable_secure=1:glibc.malloc.check=2",
> +    NULL,
> +    false,
>       0,
>       1,
>     },
>     /* Tunables should be set if enable_secure is unset. */
>     {
> -    "glibc.rtld.enable_secure=0:glibc.malloc.check=2",
> +    "GLIBC_TUNABLES=glibc.rtld.enable_secure=0:glibc.malloc.check=2",
> +    NULL,
> +    false,
>       2,
>       0,
>     },
> +  /* Tunables should be ignored if enable_secure is set. */
> +  {
> +    "GLIBC_TUNABLES=glibc.malloc.check=2:glibc.rtld.enable_secure=1",
> +    "MALLOC_CHECK_=2",
> +    false,
> +    0,
> +    1,
> +  },
> +  /* Same as before, but with enviroment alias prior GLIBC_TUNABLES.  */
> +  {
> +    "MALLOC_CHECK_=2",
> +    "GLIBC_TUNABLES=glibc.malloc.check=2:glibc.rtld.enable_secure=1",
> +    false,
> +    0,
> +    1,
> +  },
> +  /* Tunables should be ignored if enable_secure is set. */
> +  {
> +    "GLIBC_TUNABLES=glibc.rtld.enable_secure=1:glibc.malloc.check=2",
> +    "MALLOC_CHECK_=2",
> +    false,
> +    0,
> +    1,
> +  },
> +  {
> +    "MALLOC_CHECK_=2",
> +    "GLIBC_TUNABLES=glibc.rtld.enable_secure=1:glibc.malloc.check=2",
> +    false,
> +    0,
> +    1,
> +  },
> +  /* Tunables should be set if enable_secure is unset. */
> +  {
> +    "GLIBC_TUNABLES=glibc.rtld.enable_secure=0:glibc.malloc.check=2",
> +    /* Tunable have precedence over the environment variable.  */
> +    "MALLOC_CHECK_=1",
> +    false,
> +    2,
> +    0,
> +  },
> +  {
> +    "MALLOC_CHECK_=1",
> +    "GLIBC_TUNABLES=glibc.rtld.enable_secure=0:glibc.malloc.check=2",
> +    /* Tunable have precedence over the environment variable.  */
> +    false,
> +    2,
> +    0,
> +  },
> +  /* Tunables should be set if enable_secure is unset. */
> +  {
> +    "GLIBC_TUNABLES=glibc.rtld.enable_secure=0",
> +    /* Tunable have precedence over the environment variable.  */
> +    "MALLOC_CHECK_=1",
> +    false,
> +    1,
> +    0,
> +  },
> +  /* Tunables should be set if enable_secure is unset. */
> +  {
> +    "GLIBC_TUNABLES=glibc.rtld.enable_secure=0",
> +    /* Tunable have precedence over the environment variable.  */
> +    "MALLOC_CHECK_=1",
> +    false,
> +    1,
> +    0,
> +  },
> +  /* Check with tunables environment variable alias set multiple times.  */
> +  {
> +    "GLIBC_TUNABLES=glibc.rtld.enable_secure=1:glibc.malloc.check=2",
> +    "MALLOC_CHECK_=2",
> +    true,
> +    0,
> +    1,
> +  },
> +  /* Tunables should be set if enable_secure is unset. */
> +  {
> +    "GLIBC_TUNABLES=glibc.rtld.enable_secure=0",
> +    /* Tunable have precedence over the environment variable.  */
> +    "MALLOC_CHECK_=1",
> +    true,
> +    1,
> +    0,
> +  },
>   };
>   
>   static int
>   handle_restart (int i)
>   {
>     if (tests[i].expected_enable_secure == 1)
> -    {
> -      TEST_COMPARE (1, __libc_enable_secure);
> -    }
> +    TEST_COMPARE (1, __libc_enable_secure);
>     else
> -    {
> -      TEST_COMPARE (tests[i].expected_malloc_check,
> -		    TUNABLE_GET_FULL (glibc, malloc, check, int32_t, NULL));
> -      TEST_COMPARE (tests[i].expected_enable_secure,
> -		    TUNABLE_GET_FULL (glibc, rtld, enable_secure, int32_t,
> -		    NULL));
> -    }
> +    TEST_COMPARE (tests[i].expected_enable_secure,
> +		  TUNABLE_GET_FULL (glibc, rtld, enable_secure, int32_t,
> +				     NULL));
> +  TEST_COMPARE (tests[i].expected_malloc_check,
> +		TUNABLE_GET_FULL (glibc, malloc, check, int32_t, NULL));
>     return 0;
>   }
>   
> @@ -112,8 +200,23 @@ do_test (int argc, char *argv[])
>   
>         printf ("[%d] Spawned test for %s\n", i, tests[i].env);
>         setenv ("GLIBC_TUNABLES", tests[i].env, 1);
> +
> +      char *envp[2 + TUNABLE_NUM_ENV_ALIAS + 1] =
> +      {
> +	(char *) tests[i].env,
> +	(char *) tests[i].extraenv,
> +	NULL,
> +      };
> +      if (tests[i].check_multiple)
> +	{
> +	  int j;
> +	  for (j=0; j < TUNABLE_NUM_ENV_ALIAS; j++)
> +	    envp[j + 2] = (char *) tests[i].extraenv;
> +	  envp[j + 2] = NULL;
> +	}
> +
>         struct support_capture_subprocess result
> -	= support_capture_subprogram (spargv[0], spargv, NULL);
> +	= support_capture_subprogram (spargv[0], spargv, envp);
>         support_capture_subprocess_check (&result, "tst-tunables-enable_secure",
>   		                        0, sc_allow_stderr);
>         support_capture_subprocess_free (&result);
> diff --git a/scripts/gen-tunables.awk b/scripts/gen-tunables.awk
> index 9f5336381e..9a18aa6861 100644
> --- a/scripts/gen-tunables.awk
> +++ b/scripts/gen-tunables.awk
> @@ -14,6 +14,7 @@ BEGIN {
>     top_ns=""
>     max_name_len=0
>     max_alias_len=0
> +  num_env_alias=0
>   }
>   
>   # Skip over blank lines and comments.
> @@ -60,6 +61,8 @@ $1 == "}" {
>       }
>       if (!env_alias[top_ns,ns,tunable]) {
>         env_alias[top_ns,ns,tunable] = "{0}"
> +    } else {
> +      num_env_alias = num_env_alias + 1
>       }
>       len = length(top_ns"."ns"."tunable)
>       if (len > max_name_len)
> @@ -125,6 +128,39 @@ $1 == "}" {
>     }
>   }
>   
> +function print_tunable_id_t (envfirst)
> +{
> +  for (tnm in types) {
> +    split (tnm, indices, SUBSEP);
> +    t = indices[1];
> +    n = indices[2];
> +    m = indices[3];
> +    if ((envfirst && env_alias[t,n,m] == "{0}") \
> +	|| (!envfirst && env_alias[t,n,m] != "{0}")) {
> +      continue;
> +    }
> +    printf ("  TUNABLE_ENUM_NAME(%s, %s, %s),\n", t, n, m);
> +  }
> +}
> +
> +function print_tunable_entry (envfirst)
> +{
> +  for (tnm in types) {
> +    split (tnm, indices, SUBSEP);
> +    t = indices[1];
> +    n = indices[2];
> +    m = indices[3];
> +    if ((envfirst && env_alias[t,n,m] == "{0}") \
> +	|| (!envfirst && env_alias[t,n,m] != "{0}")) {
> +      continue;
> +    }
> +    printf ("  {TUNABLE_NAME_S(%s, %s, %s)", t, n, m)
> +    printf (", {TUNABLE_TYPE_%s, %s, %s}, {%s}, {%s}, false, %s},\n",
> +	    types[t,n,m], minvals[t,n,m], maxvals[t,n,m], default_val[t,n,m],
> +	    default_val[t,n,m], env_alias[t,n,m]);
> +  }
> +}
> +
>   END {
>     if (ns != "") {
>       print "Unterminated namespace.  Is a closing brace missing?"
> @@ -138,35 +174,27 @@ END {
>     print "#endif"
>     print "#include <dl-procinfo.h>\n"
>   
> +  # asort (types)
> +
>     # Now, the enum names
>     print "\ntypedef enum"
>     print "{"
> -  for (tnm in types) {
> -    split (tnm, indices, SUBSEP);
> -    t = indices[1];
> -    n = indices[2];
> -    m = indices[3];
> -    printf ("  TUNABLE_ENUM_NAME(%s, %s, %s),\n", t, n, m);
> -  }
> +  # Make the tunable with environment variables aliases first, their index
> +  # will be used in the tunable parsing.
> +  print_tunable_id_t(1)
> +  print_tunable_id_t(0)
>     print "} tunable_id_t;\n"
>   
>     print "\n#ifdef TUNABLES_INTERNAL"
>     # Internal definitions.
>     print "# define TUNABLE_NAME_MAX " (max_name_len + 1)
>     print "# define TUNABLE_ALIAS_MAX " (max_alias_len + 1)
> +  print "# define TUNABLE_NUM_ENV_ALIAS " (num_env_alias)
>     print "# include \"dl-tunable-types.h\""
>     # Finally, the tunable list.
> -  print "static tunable_t tunable_list[] attribute_relro = {"
> -  for (tnm in types) {
> -    split (tnm, indices, SUBSEP);
> -    t = indices[1];
> -    n = indices[2];
> -    m = indices[3];
> -    printf ("  {TUNABLE_NAME_S(%s, %s, %s)", t, n, m)
> -    printf (", {TUNABLE_TYPE_%s, %s, %s}, {%s}, {%s}, false, %s},\n",
> -	    types[t,n,m], minvals[t,n,m], maxvals[t,n,m], default_val[t,n,m],
> -	    default_val[t,n,m], env_alias[t,n,m]);
> -  }
> +  print "static tunable_t tunable_list[] attribute_relro __attribute_used__ = {"
> +  print_tunable_entry(1)
> +  print_tunable_entry(0)
>     print "};"
>     print "#endif"
>   }
Adhemerval Zanella Netto May 1, 2024, 6 p.m. UTC | #2
On 01/05/24 14:40, Siddhesh Poyarekar wrote:
> On 2024-04-30 15:25, Adhemerval Zanella wrote:
>> So tunable with environment variables aliases are also ignored if
>> glibc.rtld.enable_secure is enabled.  The tunable parsing is also
>> optimized a bit, where the loop that checks each environment variable
>> only checks for the tunables with aliases instead of all tables.
>>
>> Checked on aarch64-linux-gnu and x86_64-linux-gnu.
> 
> Changing the sorting of the list of tunables ends up breaking internal ABI.  It's only a problem that crops up intermittently, e.g. when doing updates where a system may, for a very short time, have a different dynamic linker and libc.
> 
> How about, instead, something like this:
> 
> 1. Run through envp twice, first time just for tunables and the second time for envvar aliases.
> 
> 2. Generate a second, separate array of only the tunables with env aliases for the second envvar alias run.  It could be simply:
> 
> tunable_envalias_t
> {
>   const char *name;
>   tunable_t *t;
> };
> 
> 3. Skip the second run if enable_secure is set.

I though about it and I decided to change tunables sorting because this is
add extra overhead to a routine that is issue for every new process.  Updating
the system libc was never atomically due the current constraints (maybe in
the future if/when we move all libc to ld.so), so I am not really convinced 
that we should make the tunables ordering list an ABI constraint.

> 
> Thanks,
> Sid
> 
>> ---
>>   elf/dl-tunables.c                |  34 ++++++--
>>   elf/tst-tunables-enable_secure.c | 131 +++++++++++++++++++++++++++----
>>   scripts/gen-tunables.awk         |  64 ++++++++++-----
>>   3 files changed, 189 insertions(+), 40 deletions(-)
>>
>> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
>> index 63cf8c7ab5..c1a1d1a2e3 100644
>> --- a/elf/dl-tunables.c
>> +++ b/elf/dl-tunables.c
>> @@ -300,6 +300,10 @@ __tunables_init (char **envp)
>>     if (__libc_enable_secure)
>>       return;
>>   +  /* The tunable with environment variable alias are placed at the start of
>> +     tunable array.  */
>> +  struct tunable_toset_t tunables_env_alias[TUNABLE_NUM_ENV_ALIAS] = { 0 };
>> +
>>     while ((envp = get_next_env (envp, &envname, &envval, &prev_envp)) != NULL)
>>       {
>>         /* The environment variable is allocated on the stack by the kernel, so
>> @@ -311,29 +315,43 @@ __tunables_init (char **envp)
>>         continue;
>>       }
>>   -      for (int i = 0; i < tunables_list_size; i++)
>> +      for (int i = 0; i < TUNABLE_NUM_ENV_ALIAS; i++)
>>       {
>>         tunable_t *cur = &tunable_list[i];
>> +      const char *name = cur->env_alias;
>>   -      /* Skip over tunables that have either been set already or should be
>> -         skipped.  */
>> -      if (cur->initialized || cur->env_alias[0] == '\0')
>> +      if (name[0] == '\0')
>>           continue;
>>   -      const char *name = cur->env_alias;
>> -
>> -      /* We have a match.  Initialize and move on to the next line.  */
>>         if (tunable_is_name (name, envname))
>>           {
>>             size_t envvallen = 0;
>>             /* The environment variable is always null-terminated.  */
>>             for (const char *p = envval; *p != '\0'; p++, envvallen++);
>>   -          tunable_initialize (cur, envval, envvallen);
>> +          tunables_env_alias[i] =
>> +        (struct tunable_toset_t) { cur, envval, envvallen };
>>             break;
>>           }
>>       }
>>       }
>> +
>> +  /* Check if glibc.rtld.enable_secure was set and skip over the environment
>> +     variables aliases.  */
>> +  if (__libc_enable_secure)
>> +    return;
>> +
>> +  for (int i = 0; i < TUNABLE_NUM_ENV_ALIAS; i++)
>> +    {
>> +      if (tunables_env_alias[i].t == NULL
>> +      || tunables_env_alias[i].t->initialized)
>> +    continue;
>> +
>> +      if (!tunable_initialize (tunables_env_alias[i].t,
>> +                   tunables_env_alias[i].value,
>> +                   tunables_env_alias[i].len))
>> +    parse_tunable_print_error (&tunables_env_alias[i]);
>> +    }
>>   }
>>     void
>> diff --git a/elf/tst-tunables-enable_secure.c b/elf/tst-tunables-enable_secure.c
>> index f5db1c84e9..d4938a2e5c 100644
>> --- a/elf/tst-tunables-enable_secure.c
>> +++ b/elf/tst-tunables-enable_secure.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>
>> @@ -34,6 +35,8 @@ static int restart;
>>   static const struct test_t
>>   {
>>     const char *env;
>> +  const char *extraenv;
>> +  bool check_multiple;
>>     int32_t expected_malloc_check;
>>     int32_t expected_enable_secure;
>>   } tests[] =
>> @@ -41,39 +44,124 @@ static const struct test_t
>>     /* Expected tunable format.  */
>>     /* Tunables should be ignored if enable_secure is set. */
>>     {
>> -    "glibc.malloc.check=2:glibc.rtld.enable_secure=1",
>> +    "GLIBC_TUNABLES=glibc.malloc.check=2:glibc.rtld.enable_secure=1",
>> +    NULL,
>> +    false,
>>       0,
>>       1,
>>     },
>>     /* Tunables should be ignored if enable_secure is set. */
>>     {
>> -    "glibc.rtld.enable_secure=1:glibc.malloc.check=2",
>> +    "GLIBC_TUNABLES=glibc.rtld.enable_secure=1:glibc.malloc.check=2",
>> +    NULL,
>> +    false,
>>       0,
>>       1,
>>     },
>>     /* Tunables should be set if enable_secure is unset. */
>>     {
>> -    "glibc.rtld.enable_secure=0:glibc.malloc.check=2",
>> +    "GLIBC_TUNABLES=glibc.rtld.enable_secure=0:glibc.malloc.check=2",
>> +    NULL,
>> +    false,
>>       2,
>>       0,
>>     },
>> +  /* Tunables should be ignored if enable_secure is set. */
>> +  {
>> +    "GLIBC_TUNABLES=glibc.malloc.check=2:glibc.rtld.enable_secure=1",
>> +    "MALLOC_CHECK_=2",
>> +    false,
>> +    0,
>> +    1,
>> +  },
>> +  /* Same as before, but with enviroment alias prior GLIBC_TUNABLES.  */
>> +  {
>> +    "MALLOC_CHECK_=2",
>> +    "GLIBC_TUNABLES=glibc.malloc.check=2:glibc.rtld.enable_secure=1",
>> +    false,
>> +    0,
>> +    1,
>> +  },
>> +  /* Tunables should be ignored if enable_secure is set. */
>> +  {
>> +    "GLIBC_TUNABLES=glibc.rtld.enable_secure=1:glibc.malloc.check=2",
>> +    "MALLOC_CHECK_=2",
>> +    false,
>> +    0,
>> +    1,
>> +  },
>> +  {
>> +    "MALLOC_CHECK_=2",
>> +    "GLIBC_TUNABLES=glibc.rtld.enable_secure=1:glibc.malloc.check=2",
>> +    false,
>> +    0,
>> +    1,
>> +  },
>> +  /* Tunables should be set if enable_secure is unset. */
>> +  {
>> +    "GLIBC_TUNABLES=glibc.rtld.enable_secure=0:glibc.malloc.check=2",
>> +    /* Tunable have precedence over the environment variable.  */
>> +    "MALLOC_CHECK_=1",
>> +    false,
>> +    2,
>> +    0,
>> +  },
>> +  {
>> +    "MALLOC_CHECK_=1",
>> +    "GLIBC_TUNABLES=glibc.rtld.enable_secure=0:glibc.malloc.check=2",
>> +    /* Tunable have precedence over the environment variable.  */
>> +    false,
>> +    2,
>> +    0,
>> +  },
>> +  /* Tunables should be set if enable_secure is unset. */
>> +  {
>> +    "GLIBC_TUNABLES=glibc.rtld.enable_secure=0",
>> +    /* Tunable have precedence over the environment variable.  */
>> +    "MALLOC_CHECK_=1",
>> +    false,
>> +    1,
>> +    0,
>> +  },
>> +  /* Tunables should be set if enable_secure is unset. */
>> +  {
>> +    "GLIBC_TUNABLES=glibc.rtld.enable_secure=0",
>> +    /* Tunable have precedence over the environment variable.  */
>> +    "MALLOC_CHECK_=1",
>> +    false,
>> +    1,
>> +    0,
>> +  },
>> +  /* Check with tunables environment variable alias set multiple times.  */
>> +  {
>> +    "GLIBC_TUNABLES=glibc.rtld.enable_secure=1:glibc.malloc.check=2",
>> +    "MALLOC_CHECK_=2",
>> +    true,
>> +    0,
>> +    1,
>> +  },
>> +  /* Tunables should be set if enable_secure is unset. */
>> +  {
>> +    "GLIBC_TUNABLES=glibc.rtld.enable_secure=0",
>> +    /* Tunable have precedence over the environment variable.  */
>> +    "MALLOC_CHECK_=1",
>> +    true,
>> +    1,
>> +    0,
>> +  },
>>   };
>>     static int
>>   handle_restart (int i)
>>   {
>>     if (tests[i].expected_enable_secure == 1)
>> -    {
>> -      TEST_COMPARE (1, __libc_enable_secure);
>> -    }
>> +    TEST_COMPARE (1, __libc_enable_secure);
>>     else
>> -    {
>> -      TEST_COMPARE (tests[i].expected_malloc_check,
>> -            TUNABLE_GET_FULL (glibc, malloc, check, int32_t, NULL));
>> -      TEST_COMPARE (tests[i].expected_enable_secure,
>> -            TUNABLE_GET_FULL (glibc, rtld, enable_secure, int32_t,
>> -            NULL));
>> -    }
>> +    TEST_COMPARE (tests[i].expected_enable_secure,
>> +          TUNABLE_GET_FULL (glibc, rtld, enable_secure, int32_t,
>> +                     NULL));
>> +  TEST_COMPARE (tests[i].expected_malloc_check,
>> +        TUNABLE_GET_FULL (glibc, malloc, check, int32_t, NULL));
>>     return 0;
>>   }
>>   @@ -112,8 +200,23 @@ do_test (int argc, char *argv[])
>>           printf ("[%d] Spawned test for %s\n", i, tests[i].env);
>>         setenv ("GLIBC_TUNABLES", tests[i].env, 1);
>> +
>> +      char *envp[2 + TUNABLE_NUM_ENV_ALIAS + 1] =
>> +      {
>> +    (char *) tests[i].env,
>> +    (char *) tests[i].extraenv,
>> +    NULL,
>> +      };
>> +      if (tests[i].check_multiple)
>> +    {
>> +      int j;
>> +      for (j=0; j < TUNABLE_NUM_ENV_ALIAS; j++)
>> +        envp[j + 2] = (char *) tests[i].extraenv;
>> +      envp[j + 2] = NULL;
>> +    }
>> +
>>         struct support_capture_subprocess result
>> -    = support_capture_subprogram (spargv[0], spargv, NULL);
>> +    = support_capture_subprogram (spargv[0], spargv, envp);
>>         support_capture_subprocess_check (&result, "tst-tunables-enable_secure",
>>                                   0, sc_allow_stderr);
>>         support_capture_subprocess_free (&result);
>> diff --git a/scripts/gen-tunables.awk b/scripts/gen-tunables.awk
>> index 9f5336381e..9a18aa6861 100644
>> --- a/scripts/gen-tunables.awk
>> +++ b/scripts/gen-tunables.awk
>> @@ -14,6 +14,7 @@ BEGIN {
>>     top_ns=""
>>     max_name_len=0
>>     max_alias_len=0
>> +  num_env_alias=0
>>   }
>>     # Skip over blank lines and comments.
>> @@ -60,6 +61,8 @@ $1 == "}" {
>>       }
>>       if (!env_alias[top_ns,ns,tunable]) {
>>         env_alias[top_ns,ns,tunable] = "{0}"
>> +    } else {
>> +      num_env_alias = num_env_alias + 1
>>       }
>>       len = length(top_ns"."ns"."tunable)
>>       if (len > max_name_len)
>> @@ -125,6 +128,39 @@ $1 == "}" {
>>     }
>>   }
>>   +function print_tunable_id_t (envfirst)
>> +{
>> +  for (tnm in types) {
>> +    split (tnm, indices, SUBSEP);
>> +    t = indices[1];
>> +    n = indices[2];
>> +    m = indices[3];
>> +    if ((envfirst && env_alias[t,n,m] == "{0}") \
>> +    || (!envfirst && env_alias[t,n,m] != "{0}")) {
>> +      continue;
>> +    }
>> +    printf ("  TUNABLE_ENUM_NAME(%s, %s, %s),\n", t, n, m);
>> +  }
>> +}
>> +
>> +function print_tunable_entry (envfirst)
>> +{
>> +  for (tnm in types) {
>> +    split (tnm, indices, SUBSEP);
>> +    t = indices[1];
>> +    n = indices[2];
>> +    m = indices[3];
>> +    if ((envfirst && env_alias[t,n,m] == "{0}") \
>> +    || (!envfirst && env_alias[t,n,m] != "{0}")) {
>> +      continue;
>> +    }
>> +    printf ("  {TUNABLE_NAME_S(%s, %s, %s)", t, n, m)
>> +    printf (", {TUNABLE_TYPE_%s, %s, %s}, {%s}, {%s}, false, %s},\n",
>> +        types[t,n,m], minvals[t,n,m], maxvals[t,n,m], default_val[t,n,m],
>> +        default_val[t,n,m], env_alias[t,n,m]);
>> +  }
>> +}
>> +
>>   END {
>>     if (ns != "") {
>>       print "Unterminated namespace.  Is a closing brace missing?"
>> @@ -138,35 +174,27 @@ END {
>>     print "#endif"
>>     print "#include <dl-procinfo.h>\n"
>>   +  # asort (types)
>> +
>>     # Now, the enum names
>>     print "\ntypedef enum"
>>     print "{"
>> -  for (tnm in types) {
>> -    split (tnm, indices, SUBSEP);
>> -    t = indices[1];
>> -    n = indices[2];
>> -    m = indices[3];
>> -    printf ("  TUNABLE_ENUM_NAME(%s, %s, %s),\n", t, n, m);
>> -  }
>> +  # Make the tunable with environment variables aliases first, their index
>> +  # will be used in the tunable parsing.
>> +  print_tunable_id_t(1)
>> +  print_tunable_id_t(0)
>>     print "} tunable_id_t;\n"
>>       print "\n#ifdef TUNABLES_INTERNAL"
>>     # Internal definitions.
>>     print "# define TUNABLE_NAME_MAX " (max_name_len + 1)
>>     print "# define TUNABLE_ALIAS_MAX " (max_alias_len + 1)
>> +  print "# define TUNABLE_NUM_ENV_ALIAS " (num_env_alias)
>>     print "# include \"dl-tunable-types.h\""
>>     # Finally, the tunable list.
>> -  print "static tunable_t tunable_list[] attribute_relro = {"
>> -  for (tnm in types) {
>> -    split (tnm, indices, SUBSEP);
>> -    t = indices[1];
>> -    n = indices[2];
>> -    m = indices[3];
>> -    printf ("  {TUNABLE_NAME_S(%s, %s, %s)", t, n, m)
>> -    printf (", {TUNABLE_TYPE_%s, %s, %s}, {%s}, {%s}, false, %s},\n",
>> -        types[t,n,m], minvals[t,n,m], maxvals[t,n,m], default_val[t,n,m],
>> -        default_val[t,n,m], env_alias[t,n,m]);
>> -  }
>> +  print "static tunable_t tunable_list[] attribute_relro __attribute_used__ = {"
>> +  print_tunable_entry(1)
>> +  print_tunable_entry(0)
>>     print "};"
>>     print "#endif"
>>   }
Siddhesh Poyarekar May 2, 2024, 11:03 a.m. UTC | #3
On 2024-05-01 14:00, Adhemerval Zanella Netto wrote:
> 
> 
> On 01/05/24 14:40, Siddhesh Poyarekar wrote:
>> On 2024-04-30 15:25, Adhemerval Zanella wrote:
>>> So tunable with environment variables aliases are also ignored if
>>> glibc.rtld.enable_secure is enabled.  The tunable parsing is also
>>> optimized a bit, where the loop that checks each environment variable
>>> only checks for the tunables with aliases instead of all tables.
>>>
>>> Checked on aarch64-linux-gnu and x86_64-linux-gnu.
>>
>> Changing the sorting of the list of tunables ends up breaking internal ABI.  It's only a problem that crops up intermittently, e.g. when doing updates where a system may, for a very short time, have a different dynamic linker and libc.
>>
>> How about, instead, something like this:
>>
>> 1. Run through envp twice, first time just for tunables and the second time for envvar aliases.
>>
>> 2. Generate a second, separate array of only the tunables with env aliases for the second envvar alias run.  It could be simply:
>>
>> tunable_envalias_t
>> {
>>    const char *name;
>>    tunable_t *t;
>> };
>>
>> 3. Skip the second run if enable_secure is set.
> 
> I though about it and I decided to change tunables sorting because this is
> add extra overhead to a routine that is issue for every new process.  Updating
> the system libc was never atomically due the current constraints (maybe in
> the future if/when we move all libc to ld.so), so I am not really convinced
> that we should make the tunables ordering list an ABI constraint.

I don't have a really strong opinion either way, but it's something 
we've run into often enough in Fedora for it to be pointed out as a 
potential problem.  The right solution for this problem is in fact to 
make the TUNABLE_GET API independent of the tunable ID so that the 
tunables can move around as needed, but I don't think this patch needs 
to block on that.

Florian, do you have an opinion on this?

> 
>>
>> Thanks,
>> Sid
>>
>>> ---
>>>    elf/dl-tunables.c                |  34 ++++++--
>>>    elf/tst-tunables-enable_secure.c | 131 +++++++++++++++++++++++++++----
>>>    scripts/gen-tunables.awk         |  64 ++++++++++-----
>>>    3 files changed, 189 insertions(+), 40 deletions(-)
>>>
>>> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
>>> index 63cf8c7ab5..c1a1d1a2e3 100644
>>> --- a/elf/dl-tunables.c
>>> +++ b/elf/dl-tunables.c
>>> @@ -300,6 +300,10 @@ __tunables_init (char **envp)
>>>      if (__libc_enable_secure)
>>>        return;
>>>    +  /* The tunable with environment variable alias are placed at the start of
>>> +     tunable array.  */
>>> +  struct tunable_toset_t tunables_env_alias[TUNABLE_NUM_ENV_ALIAS] = { 0 };
>>> +
>>>      while ((envp = get_next_env (envp, &envname, &envval, &prev_envp)) != NULL)
>>>        {
>>>          /* The environment variable is allocated on the stack by the kernel, so
>>> @@ -311,29 +315,43 @@ __tunables_init (char **envp)
>>>          continue;
>>>        }
>>>    -      for (int i = 0; i < tunables_list_size; i++)
>>> +      for (int i = 0; i < TUNABLE_NUM_ENV_ALIAS; i++)
>>>        {
>>>          tunable_t *cur = &tunable_list[i];
>>> +      const char *name = cur->env_alias;
>>>    -      /* Skip over tunables that have either been set already or should be
>>> -         skipped.  */
>>> -      if (cur->initialized || cur->env_alias[0] == '\0')
>>> +      if (name[0] == '\0')
>>>            continue;
>>>    -      const char *name = cur->env_alias;
>>> -
>>> -      /* We have a match.  Initialize and move on to the next line.  */
>>>          if (tunable_is_name (name, envname))
>>>            {
>>>              size_t envvallen = 0;
>>>              /* The environment variable is always null-terminated.  */
>>>              for (const char *p = envval; *p != '\0'; p++, envvallen++);
>>>    -          tunable_initialize (cur, envval, envvallen);
>>> +          tunables_env_alias[i] =
>>> +        (struct tunable_toset_t) { cur, envval, envvallen };
>>>              break;
>>>            }
>>>        }
>>>        }
>>> +
>>> +  /* Check if glibc.rtld.enable_secure was set and skip over the environment
>>> +     variables aliases.  */
>>> +  if (__libc_enable_secure)
>>> +    return;
>>> +
>>> +  for (int i = 0; i < TUNABLE_NUM_ENV_ALIAS; i++)
>>> +    {
>>> +      if (tunables_env_alias[i].t == NULL
>>> +      || tunables_env_alias[i].t->initialized)
>>> +    continue;
>>> +
>>> +      if (!tunable_initialize (tunables_env_alias[i].t,
>>> +                   tunables_env_alias[i].value,
>>> +                   tunables_env_alias[i].len))
>>> +    parse_tunable_print_error (&tunables_env_alias[i]);
>>> +    }
>>>    }
>>>      void
>>> diff --git a/elf/tst-tunables-enable_secure.c b/elf/tst-tunables-enable_secure.c
>>> index f5db1c84e9..d4938a2e5c 100644
>>> --- a/elf/tst-tunables-enable_secure.c
>>> +++ b/elf/tst-tunables-enable_secure.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>
>>> @@ -34,6 +35,8 @@ static int restart;
>>>    static const struct test_t
>>>    {
>>>      const char *env;
>>> +  const char *extraenv;
>>> +  bool check_multiple;
>>>      int32_t expected_malloc_check;
>>>      int32_t expected_enable_secure;
>>>    } tests[] =
>>> @@ -41,39 +44,124 @@ static const struct test_t
>>>      /* Expected tunable format.  */
>>>      /* Tunables should be ignored if enable_secure is set. */
>>>      {
>>> -    "glibc.malloc.check=2:glibc.rtld.enable_secure=1",
>>> +    "GLIBC_TUNABLES=glibc.malloc.check=2:glibc.rtld.enable_secure=1",
>>> +    NULL,
>>> +    false,
>>>        0,
>>>        1,
>>>      },
>>>      /* Tunables should be ignored if enable_secure is set. */
>>>      {
>>> -    "glibc.rtld.enable_secure=1:glibc.malloc.check=2",
>>> +    "GLIBC_TUNABLES=glibc.rtld.enable_secure=1:glibc.malloc.check=2",
>>> +    NULL,
>>> +    false,
>>>        0,
>>>        1,
>>>      },
>>>      /* Tunables should be set if enable_secure is unset. */
>>>      {
>>> -    "glibc.rtld.enable_secure=0:glibc.malloc.check=2",
>>> +    "GLIBC_TUNABLES=glibc.rtld.enable_secure=0:glibc.malloc.check=2",
>>> +    NULL,
>>> +    false,
>>>        2,
>>>        0,
>>>      },
>>> +  /* Tunables should be ignored if enable_secure is set. */
>>> +  {
>>> +    "GLIBC_TUNABLES=glibc.malloc.check=2:glibc.rtld.enable_secure=1",
>>> +    "MALLOC_CHECK_=2",
>>> +    false,
>>> +    0,
>>> +    1,
>>> +  },
>>> +  /* Same as before, but with enviroment alias prior GLIBC_TUNABLES.  */
>>> +  {
>>> +    "MALLOC_CHECK_=2",
>>> +    "GLIBC_TUNABLES=glibc.malloc.check=2:glibc.rtld.enable_secure=1",
>>> +    false,
>>> +    0,
>>> +    1,
>>> +  },
>>> +  /* Tunables should be ignored if enable_secure is set. */
>>> +  {
>>> +    "GLIBC_TUNABLES=glibc.rtld.enable_secure=1:glibc.malloc.check=2",
>>> +    "MALLOC_CHECK_=2",
>>> +    false,
>>> +    0,
>>> +    1,
>>> +  },
>>> +  {
>>> +    "MALLOC_CHECK_=2",
>>> +    "GLIBC_TUNABLES=glibc.rtld.enable_secure=1:glibc.malloc.check=2",
>>> +    false,
>>> +    0,
>>> +    1,
>>> +  },
>>> +  /* Tunables should be set if enable_secure is unset. */
>>> +  {
>>> +    "GLIBC_TUNABLES=glibc.rtld.enable_secure=0:glibc.malloc.check=2",
>>> +    /* Tunable have precedence over the environment variable.  */

s/Tunable/Tunables/

>>> +    "MALLOC_CHECK_=1",
>>> +    false,
>>> +    2,
>>> +    0,
>>> +  },
>>> +  {
>>> +    "MALLOC_CHECK_=1",
>>> +    "GLIBC_TUNABLES=glibc.rtld.enable_secure=0:glibc.malloc.check=2",
>>> +    /* Tunable have precedence over the environment variable.  */
>>> +    false,
>>> +    2,
>>> +    0,
>>> +  },
>>> +  /* Tunables should be set if enable_secure is unset. */
>>> +  {
>>> +    "GLIBC_TUNABLES=glibc.rtld.enable_secure=0",

This gives the false impression that enable_secure can be disabled.  The 
only valid value to set this tunable is 1; 0 should be ignored.  In 
fact, now I wonder if setting enable_secure to 0 should emit a warning, 
like for other invalid tunables.  That's independent of this change of 
course.

>>> +    /* Tunable have precedence over the environment variable.  */
>>> +    "MALLOC_CHECK_=1",
>>> +    false,
>>> +    1,
>>> +    0,
>>> +  },
>>> +  /* Tunables should be set if enable_secure is unset. */
>>> +  {
>>> +    "GLIBC_TUNABLES=glibc.rtld.enable_secure=0",
>>> +    /* Tunable have precedence over the environment variable.  */
>>> +    "MALLOC_CHECK_=1",
>>> +    false,
>>> +    1,
>>> +    0,
>>> +  },
>>> +  /* Check with tunables environment variable alias set multiple times.  */
>>> +  {
>>> +    "GLIBC_TUNABLES=glibc.rtld.enable_secure=1:glibc.malloc.check=2",
>>> +    "MALLOC_CHECK_=2",
>>> +    true,
>>> +    0,
>>> +    1,
>>> +  },
>>> +  /* Tunables should be set if enable_secure is unset. */
>>> +  {
>>> +    "GLIBC_TUNABLES=glibc.rtld.enable_secure=0",
>>> +    /* Tunable have precedence over the environment variable.  */
>>> +    "MALLOC_CHECK_=1",
>>> +    true,
>>> +    1,
>>> +    0,
>>> +  },
>>>    };
>>>      static int
>>>    handle_restart (int i)
>>>    {
>>>      if (tests[i].expected_enable_secure == 1)
>>> -    {
>>> -      TEST_COMPARE (1, __libc_enable_secure);
>>> -    }
>>> +    TEST_COMPARE (1, __libc_enable_secure);
>>>      else
>>> -    {
>>> -      TEST_COMPARE (tests[i].expected_malloc_check,
>>> -            TUNABLE_GET_FULL (glibc, malloc, check, int32_t, NULL));
>>> -      TEST_COMPARE (tests[i].expected_enable_secure,
>>> -            TUNABLE_GET_FULL (glibc, rtld, enable_secure, int32_t,
>>> -            NULL));
>>> -    }
>>> +    TEST_COMPARE (tests[i].expected_enable_secure,
>>> +          TUNABLE_GET_FULL (glibc, rtld, enable_secure, int32_t,
>>> +                     NULL));
>>> +  TEST_COMPARE (tests[i].expected_malloc_check,
>>> +        TUNABLE_GET_FULL (glibc, malloc, check, int32_t, NULL));
>>>      return 0;
>>>    }
>>>    @@ -112,8 +200,23 @@ do_test (int argc, char *argv[])
>>>            printf ("[%d] Spawned test for %s\n", i, tests[i].env);
>>>          setenv ("GLIBC_TUNABLES", tests[i].env, 1);
>>> +
>>> +      char *envp[2 + TUNABLE_NUM_ENV_ALIAS + 1] =
>>> +      {
>>> +    (char *) tests[i].env,
>>> +    (char *) tests[i].extraenv,
>>> +    NULL,
>>> +      };
>>> +      if (tests[i].check_multiple)
>>> +    {
>>> +      int j;
>>> +      for (j=0; j < TUNABLE_NUM_ENV_ALIAS; j++)
>>> +        envp[j + 2] = (char *) tests[i].extraenv;
>>> +      envp[j + 2] = NULL;
>>> +    }
>>> +
>>>          struct support_capture_subprocess result
>>> -    = support_capture_subprogram (spargv[0], spargv, NULL);
>>> +    = support_capture_subprogram (spargv[0], spargv, envp);
>>>          support_capture_subprocess_check (&result, "tst-tunables-enable_secure",
>>>                                    0, sc_allow_stderr);
>>>          support_capture_subprocess_free (&result);
>>> diff --git a/scripts/gen-tunables.awk b/scripts/gen-tunables.awk
>>> index 9f5336381e..9a18aa6861 100644
>>> --- a/scripts/gen-tunables.awk
>>> +++ b/scripts/gen-tunables.awk
>>> @@ -14,6 +14,7 @@ BEGIN {
>>>      top_ns=""
>>>      max_name_len=0
>>>      max_alias_len=0
>>> +  num_env_alias=0
>>>    }
>>>      # Skip over blank lines and comments.
>>> @@ -60,6 +61,8 @@ $1 == "}" {
>>>        }
>>>        if (!env_alias[top_ns,ns,tunable]) {
>>>          env_alias[top_ns,ns,tunable] = "{0}"
>>> +    } else {
>>> +      num_env_alias = num_env_alias + 1
>>>        }
>>>        len = length(top_ns"."ns"."tunable)
>>>        if (len > max_name_len)
>>> @@ -125,6 +128,39 @@ $1 == "}" {
>>>      }
>>>    }
>>>    +function print_tunable_id_t (envfirst)
>>> +{
>>> +  for (tnm in types) {
>>> +    split (tnm, indices, SUBSEP);
>>> +    t = indices[1];
>>> +    n = indices[2];
>>> +    m = indices[3];
>>> +    if ((envfirst && env_alias[t,n,m] == "{0}") \
>>> +    || (!envfirst && env_alias[t,n,m] != "{0}")) {
>>> +      continue;
>>> +    }
>>> +    printf ("  TUNABLE_ENUM_NAME(%s, %s, %s),\n", t, n, m);
>>> +  }
>>> +}
>>> +
>>> +function print_tunable_entry (envfirst)
>>> +{
>>> +  for (tnm in types) {
>>> +    split (tnm, indices, SUBSEP);
>>> +    t = indices[1];
>>> +    n = indices[2];
>>> +    m = indices[3];
>>> +    if ((envfirst && env_alias[t,n,m] == "{0}") \
>>> +    || (!envfirst && env_alias[t,n,m] != "{0}")) {
>>> +      continue;
>>> +    }
>>> +    printf ("  {TUNABLE_NAME_S(%s, %s, %s)", t, n, m)
>>> +    printf (", {TUNABLE_TYPE_%s, %s, %s}, {%s}, {%s}, false, %s},\n",
>>> +        types[t,n,m], minvals[t,n,m], maxvals[t,n,m], default_val[t,n,m],
>>> +        default_val[t,n,m], env_alias[t,n,m]);
>>> +  }
>>> +}
>>> +
>>>    END {
>>>      if (ns != "") {
>>>        print "Unterminated namespace.  Is a closing brace missing?"
>>> @@ -138,35 +174,27 @@ END {
>>>      print "#endif"
>>>      print "#include <dl-procinfo.h>\n"
>>>    +  # asort (types)
>>> +
>>>      # Now, the enum names
>>>      print "\ntypedef enum"
>>>      print "{"
>>> -  for (tnm in types) {
>>> -    split (tnm, indices, SUBSEP);
>>> -    t = indices[1];
>>> -    n = indices[2];
>>> -    m = indices[3];
>>> -    printf ("  TUNABLE_ENUM_NAME(%s, %s, %s),\n", t, n, m);
>>> -  }
>>> +  # Make the tunable with environment variables aliases first, their index
>>> +  # will be used in the tunable parsing.
>>> +  print_tunable_id_t(1)
>>> +  print_tunable_id_t(0)
>>>      print "} tunable_id_t;\n"
>>>        print "\n#ifdef TUNABLES_INTERNAL"
>>>      # Internal definitions.
>>>      print "# define TUNABLE_NAME_MAX " (max_name_len + 1)
>>>      print "# define TUNABLE_ALIAS_MAX " (max_alias_len + 1)
>>> +  print "# define TUNABLE_NUM_ENV_ALIAS " (num_env_alias)
>>>      print "# include \"dl-tunable-types.h\""
>>>      # Finally, the tunable list.
>>> -  print "static tunable_t tunable_list[] attribute_relro = {"
>>> -  for (tnm in types) {
>>> -    split (tnm, indices, SUBSEP);
>>> -    t = indices[1];
>>> -    n = indices[2];
>>> -    m = indices[3];
>>> -    printf ("  {TUNABLE_NAME_S(%s, %s, %s)", t, n, m)
>>> -    printf (", {TUNABLE_TYPE_%s, %s, %s}, {%s}, {%s}, false, %s},\n",
>>> -        types[t,n,m], minvals[t,n,m], maxvals[t,n,m], default_val[t,n,m],
>>> -        default_val[t,n,m], env_alias[t,n,m]);
>>> -  }
>>> +  print "static tunable_t tunable_list[] attribute_relro __attribute_used__ = {"
>>> +  print_tunable_entry(1)
>>> +  print_tunable_entry(0)
>>>      print "};"
>>>      print "#endif"
>>>    }
>
diff mbox series

Patch

diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
index 63cf8c7ab5..c1a1d1a2e3 100644
--- a/elf/dl-tunables.c
+++ b/elf/dl-tunables.c
@@ -300,6 +300,10 @@  __tunables_init (char **envp)
   if (__libc_enable_secure)
     return;
 
+  /* The tunable with environment variable alias are placed at the start of
+     tunable array.  */
+  struct tunable_toset_t tunables_env_alias[TUNABLE_NUM_ENV_ALIAS] = { 0 };
+
   while ((envp = get_next_env (envp, &envname, &envval, &prev_envp)) != NULL)
     {
       /* The environment variable is allocated on the stack by the kernel, so
@@ -311,29 +315,43 @@  __tunables_init (char **envp)
 	  continue;
 	}
 
-      for (int i = 0; i < tunables_list_size; i++)
+      for (int i = 0; i < TUNABLE_NUM_ENV_ALIAS; i++)
 	{
 	  tunable_t *cur = &tunable_list[i];
+	  const char *name = cur->env_alias;
 
-	  /* Skip over tunables that have either been set already or should be
-	     skipped.  */
-	  if (cur->initialized || cur->env_alias[0] == '\0')
+	  if (name[0] == '\0')
 	    continue;
 
-	  const char *name = cur->env_alias;
-
-	  /* We have a match.  Initialize and move on to the next line.  */
 	  if (tunable_is_name (name, envname))
 	    {
 	      size_t envvallen = 0;
 	      /* The environment variable is always null-terminated.  */
 	      for (const char *p = envval; *p != '\0'; p++, envvallen++);
 
-	      tunable_initialize (cur, envval, envvallen);
+	      tunables_env_alias[i] =
+		(struct tunable_toset_t) { cur, envval, envvallen };
 	      break;
 	    }
 	}
     }
+
+  /* Check if glibc.rtld.enable_secure was set and skip over the environment
+     variables aliases.  */
+  if (__libc_enable_secure)
+    return;
+
+  for (int i = 0; i < TUNABLE_NUM_ENV_ALIAS; i++)
+    {
+      if (tunables_env_alias[i].t == NULL
+	  || tunables_env_alias[i].t->initialized)
+	continue;
+
+      if (!tunable_initialize (tunables_env_alias[i].t,
+			       tunables_env_alias[i].value,
+			       tunables_env_alias[i].len))
+	parse_tunable_print_error (&tunables_env_alias[i]);
+    }
 }
 
 void
diff --git a/elf/tst-tunables-enable_secure.c b/elf/tst-tunables-enable_secure.c
index f5db1c84e9..d4938a2e5c 100644
--- a/elf/tst-tunables-enable_secure.c
+++ b/elf/tst-tunables-enable_secure.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>
@@ -34,6 +35,8 @@  static int restart;
 static const struct test_t
 {
   const char *env;
+  const char *extraenv;
+  bool check_multiple;
   int32_t expected_malloc_check;
   int32_t expected_enable_secure;
 } tests[] =
@@ -41,39 +44,124 @@  static const struct test_t
   /* Expected tunable format.  */
   /* Tunables should be ignored if enable_secure is set. */
   {
-    "glibc.malloc.check=2:glibc.rtld.enable_secure=1",
+    "GLIBC_TUNABLES=glibc.malloc.check=2:glibc.rtld.enable_secure=1",
+    NULL,
+    false,
     0,
     1,
   },
   /* Tunables should be ignored if enable_secure is set. */
   {
-    "glibc.rtld.enable_secure=1:glibc.malloc.check=2",
+    "GLIBC_TUNABLES=glibc.rtld.enable_secure=1:glibc.malloc.check=2",
+    NULL,
+    false,
     0,
     1,
   },
   /* Tunables should be set if enable_secure is unset. */
   {
-    "glibc.rtld.enable_secure=0:glibc.malloc.check=2",
+    "GLIBC_TUNABLES=glibc.rtld.enable_secure=0:glibc.malloc.check=2",
+    NULL,
+    false,
     2,
     0,
   },
+  /* Tunables should be ignored if enable_secure is set. */
+  {
+    "GLIBC_TUNABLES=glibc.malloc.check=2:glibc.rtld.enable_secure=1",
+    "MALLOC_CHECK_=2",
+    false,
+    0,
+    1,
+  },
+  /* Same as before, but with enviroment alias prior GLIBC_TUNABLES.  */
+  {
+    "MALLOC_CHECK_=2",
+    "GLIBC_TUNABLES=glibc.malloc.check=2:glibc.rtld.enable_secure=1",
+    false,
+    0,
+    1,
+  },
+  /* Tunables should be ignored if enable_secure is set. */
+  {
+    "GLIBC_TUNABLES=glibc.rtld.enable_secure=1:glibc.malloc.check=2",
+    "MALLOC_CHECK_=2",
+    false,
+    0,
+    1,
+  },
+  {
+    "MALLOC_CHECK_=2",
+    "GLIBC_TUNABLES=glibc.rtld.enable_secure=1:glibc.malloc.check=2",
+    false,
+    0,
+    1,
+  },
+  /* Tunables should be set if enable_secure is unset. */
+  {
+    "GLIBC_TUNABLES=glibc.rtld.enable_secure=0:glibc.malloc.check=2",
+    /* Tunable have precedence over the environment variable.  */
+    "MALLOC_CHECK_=1",
+    false,
+    2,
+    0,
+  },
+  {
+    "MALLOC_CHECK_=1",
+    "GLIBC_TUNABLES=glibc.rtld.enable_secure=0:glibc.malloc.check=2",
+    /* Tunable have precedence over the environment variable.  */
+    false,
+    2,
+    0,
+  },
+  /* Tunables should be set if enable_secure is unset. */
+  {
+    "GLIBC_TUNABLES=glibc.rtld.enable_secure=0",
+    /* Tunable have precedence over the environment variable.  */
+    "MALLOC_CHECK_=1",
+    false,
+    1,
+    0,
+  },
+  /* Tunables should be set if enable_secure is unset. */
+  {
+    "GLIBC_TUNABLES=glibc.rtld.enable_secure=0",
+    /* Tunable have precedence over the environment variable.  */
+    "MALLOC_CHECK_=1",
+    false,
+    1,
+    0,
+  },
+  /* Check with tunables environment variable alias set multiple times.  */
+  {
+    "GLIBC_TUNABLES=glibc.rtld.enable_secure=1:glibc.malloc.check=2",
+    "MALLOC_CHECK_=2",
+    true,
+    0,
+    1,
+  },
+  /* Tunables should be set if enable_secure is unset. */
+  {
+    "GLIBC_TUNABLES=glibc.rtld.enable_secure=0",
+    /* Tunable have precedence over the environment variable.  */
+    "MALLOC_CHECK_=1",
+    true,
+    1,
+    0,
+  },
 };
 
 static int
 handle_restart (int i)
 {
   if (tests[i].expected_enable_secure == 1)
-    {
-      TEST_COMPARE (1, __libc_enable_secure);
-    }
+    TEST_COMPARE (1, __libc_enable_secure);
   else
-    {
-      TEST_COMPARE (tests[i].expected_malloc_check,
-		    TUNABLE_GET_FULL (glibc, malloc, check, int32_t, NULL));
-      TEST_COMPARE (tests[i].expected_enable_secure,
-		    TUNABLE_GET_FULL (glibc, rtld, enable_secure, int32_t,
-		    NULL));
-    }
+    TEST_COMPARE (tests[i].expected_enable_secure,
+		  TUNABLE_GET_FULL (glibc, rtld, enable_secure, int32_t,
+				     NULL));
+  TEST_COMPARE (tests[i].expected_malloc_check,
+		TUNABLE_GET_FULL (glibc, malloc, check, int32_t, NULL));
   return 0;
 }
 
@@ -112,8 +200,23 @@  do_test (int argc, char *argv[])
 
       printf ("[%d] Spawned test for %s\n", i, tests[i].env);
       setenv ("GLIBC_TUNABLES", tests[i].env, 1);
+
+      char *envp[2 + TUNABLE_NUM_ENV_ALIAS + 1] =
+      {
+	(char *) tests[i].env,
+	(char *) tests[i].extraenv,
+	NULL,
+      };
+      if (tests[i].check_multiple)
+	{
+	  int j;
+	  for (j=0; j < TUNABLE_NUM_ENV_ALIAS; j++)
+	    envp[j + 2] = (char *) tests[i].extraenv;
+	  envp[j + 2] = NULL;
+	}
+
       struct support_capture_subprocess result
-	= support_capture_subprogram (spargv[0], spargv, NULL);
+	= support_capture_subprogram (spargv[0], spargv, envp);
       support_capture_subprocess_check (&result, "tst-tunables-enable_secure",
 		                        0, sc_allow_stderr);
       support_capture_subprocess_free (&result);
diff --git a/scripts/gen-tunables.awk b/scripts/gen-tunables.awk
index 9f5336381e..9a18aa6861 100644
--- a/scripts/gen-tunables.awk
+++ b/scripts/gen-tunables.awk
@@ -14,6 +14,7 @@  BEGIN {
   top_ns=""
   max_name_len=0
   max_alias_len=0
+  num_env_alias=0
 }
 
 # Skip over blank lines and comments.
@@ -60,6 +61,8 @@  $1 == "}" {
     }
     if (!env_alias[top_ns,ns,tunable]) {
       env_alias[top_ns,ns,tunable] = "{0}"
+    } else {
+      num_env_alias = num_env_alias + 1
     }
     len = length(top_ns"."ns"."tunable)
     if (len > max_name_len)
@@ -125,6 +128,39 @@  $1 == "}" {
   }
 }
 
+function print_tunable_id_t (envfirst)
+{
+  for (tnm in types) {
+    split (tnm, indices, SUBSEP);
+    t = indices[1];
+    n = indices[2];
+    m = indices[3];
+    if ((envfirst && env_alias[t,n,m] == "{0}") \
+	|| (!envfirst && env_alias[t,n,m] != "{0}")) {
+      continue;
+    }
+    printf ("  TUNABLE_ENUM_NAME(%s, %s, %s),\n", t, n, m);
+  }
+}
+
+function print_tunable_entry (envfirst)
+{
+  for (tnm in types) {
+    split (tnm, indices, SUBSEP);
+    t = indices[1];
+    n = indices[2];
+    m = indices[3];
+    if ((envfirst && env_alias[t,n,m] == "{0}") \
+	|| (!envfirst && env_alias[t,n,m] != "{0}")) {
+      continue;
+    }
+    printf ("  {TUNABLE_NAME_S(%s, %s, %s)", t, n, m)
+    printf (", {TUNABLE_TYPE_%s, %s, %s}, {%s}, {%s}, false, %s},\n",
+	    types[t,n,m], minvals[t,n,m], maxvals[t,n,m], default_val[t,n,m],
+	    default_val[t,n,m], env_alias[t,n,m]);
+  }
+}
+
 END {
   if (ns != "") {
     print "Unterminated namespace.  Is a closing brace missing?"
@@ -138,35 +174,27 @@  END {
   print "#endif"
   print "#include <dl-procinfo.h>\n"
 
+  # asort (types)
+
   # Now, the enum names
   print "\ntypedef enum"
   print "{"
-  for (tnm in types) {
-    split (tnm, indices, SUBSEP);
-    t = indices[1];
-    n = indices[2];
-    m = indices[3];
-    printf ("  TUNABLE_ENUM_NAME(%s, %s, %s),\n", t, n, m);
-  }
+  # Make the tunable with environment variables aliases first, their index
+  # will be used in the tunable parsing.
+  print_tunable_id_t(1)
+  print_tunable_id_t(0)
   print "} tunable_id_t;\n"
 
   print "\n#ifdef TUNABLES_INTERNAL"
   # Internal definitions.
   print "# define TUNABLE_NAME_MAX " (max_name_len + 1)
   print "# define TUNABLE_ALIAS_MAX " (max_alias_len + 1)
+  print "# define TUNABLE_NUM_ENV_ALIAS " (num_env_alias)
   print "# include \"dl-tunable-types.h\""
   # Finally, the tunable list.
-  print "static tunable_t tunable_list[] attribute_relro = {"
-  for (tnm in types) {
-    split (tnm, indices, SUBSEP);
-    t = indices[1];
-    n = indices[2];
-    m = indices[3];
-    printf ("  {TUNABLE_NAME_S(%s, %s, %s)", t, n, m)
-    printf (", {TUNABLE_TYPE_%s, %s, %s}, {%s}, {%s}, false, %s},\n",
-	    types[t,n,m], minvals[t,n,m], maxvals[t,n,m], default_val[t,n,m],
-	    default_val[t,n,m], env_alias[t,n,m]);
-  }
+  print "static tunable_t tunable_list[] attribute_relro __attribute_used__ = {"
+  print_tunable_entry(1)
+  print_tunable_entry(0)
   print "};"
   print "#endif"
 }