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

[Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>]

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"
>>   }