diff mbox series

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

Message ID 20240506161955.1570278-5-adhemerval.zanella@linaro.org
State Accepted
Commit eb59c7b43dd5c64c38e4c3cd21e7ad75d8d29cb0
Headers show
Series More tunable fixes | expand

Commit Message

Adhemerval Zanella Netto May 6, 2024, 4:18 p.m. UTC
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                |  36 ++++++--
 elf/tst-tunables-enable_secure.c | 136 +++++++++++++++++++++++++++----
 scripts/gen-tunables.awk         |  16 +++-
 3 files changed, 164 insertions(+), 24 deletions(-)

Comments

Siddhesh Poyarekar May 7, 2024, 2:33 p.m. UTC | #1
On 2024-05-06 12:18, Adhemerval Zanella wrote:
> 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.
> ---

LGTM.

Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org>


>   elf/dl-tunables.c                |  36 ++++++--
>   elf/tst-tunables-enable_secure.c | 136 +++++++++++++++++++++++++++----
>   scripts/gen-tunables.awk         |  16 +++-
>   3 files changed, 164 insertions(+), 24 deletions(-)
> 
> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
> index 63cf8c7ab5..147cc4cf23 100644
> --- a/elf/dl-tunables.c
> +++ b/elf/dl-tunables.c
> @@ -300,6 +300,9 @@ __tunables_init (char **envp)
>     if (__libc_enable_secure)
>       return;
>   
> +  enum { tunable_num_env_alias = array_length (tunable_env_alias_list) };
> +  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 +314,44 @@ __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];
> +	  tunable_t *cur = &tunable_list[tunable_env_alias_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++)
> +    {
> +      /* Skip over tunables that have either been set or already initialized.  */
> +      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..99a065e8db 100644
> --- a/elf/tst-tunables-enable_secure.c
> +++ b/elf/tst-tunables-enable_secure.c
> @@ -17,6 +17,10 @@
>      <https://www.gnu.org/licenses/>.  */
>   
>   #include <array_length.h>
> +/* The test uses the tunable_env_alias_list size, which is only exported for
> +   ld.so.  This will result in a copy of tunable_list and
> +   tunable_env_alias_list, which is ununsed by the test itself.  */
> +#define TUNABLES_INTERNAL 1
>   #include <dl-tunables.h>
>   #include <getopt.h>
>   #include <intprops.h>
> @@ -34,6 +38,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 +47,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;
>   }
>   
> @@ -106,14 +197,31 @@ do_test (int argc, char *argv[])
>       spargv[i] = NULL;
>     }
>   
> +  enum { tunable_num_env_alias = array_length (tunable_env_alias_list) };
> +
>     for (int i = 0; i < array_length (tests); i++)
>       {
>         snprintf (nteststr, sizeof nteststr, "%d", i);
>   
>         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..fc3b41376f 100644
> --- a/scripts/gen-tunables.awk
> +++ b/scripts/gen-tunables.awk
> @@ -156,7 +156,7 @@ END {
>     print "# define TUNABLE_ALIAS_MAX " (max_alias_len + 1)
>     print "# include \"dl-tunable-types.h\""
>     # Finally, the tunable list.
> -  print "static tunable_t tunable_list[] attribute_relro = {"
> +  print "static tunable_t tunable_list[] attribute_relro __attribute_used__ = {"
>     for (tnm in types) {
>       split (tnm, indices, SUBSEP);
>       t = indices[1];
> @@ -168,5 +168,19 @@ END {
>   	    default_val[t,n,m], env_alias[t,n,m]);
>     }
>     print "};"
> +
> +  # Map of tunable with environment variables aliases used during parsing.  */
> +  print "\nstatic const tunable_id_t tunable_env_alias_list[] ="
> +  printf "{\n"
> +  for (tnm in types) {
> +    split (tnm, indices, SUBSEP);
> +    t = indices[1];
> +    n = indices[2];
> +    m = indices[3];
> +    if (env_alias[t,n,m] != "{0}") {
> +      printf ("  TUNABLE_ENUM_NAME(%s, %s, %s),\n", t, n, m);
> +    }
> +  }
> +  printf "};\n"
>     print "#endif"
>   }
diff mbox series

Patch

diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
index 63cf8c7ab5..147cc4cf23 100644
--- a/elf/dl-tunables.c
+++ b/elf/dl-tunables.c
@@ -300,6 +300,9 @@  __tunables_init (char **envp)
   if (__libc_enable_secure)
     return;
 
+  enum { tunable_num_env_alias = array_length (tunable_env_alias_list) };
+  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 +314,44 @@  __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];
+	  tunable_t *cur = &tunable_list[tunable_env_alias_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++)
+    {
+      /* Skip over tunables that have either been set or already initialized.  */
+      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..99a065e8db 100644
--- a/elf/tst-tunables-enable_secure.c
+++ b/elf/tst-tunables-enable_secure.c
@@ -17,6 +17,10 @@ 
    <https://www.gnu.org/licenses/>.  */
 
 #include <array_length.h>
+/* The test uses the tunable_env_alias_list size, which is only exported for
+   ld.so.  This will result in a copy of tunable_list and
+   tunable_env_alias_list, which is ununsed by the test itself.  */
+#define TUNABLES_INTERNAL 1
 #include <dl-tunables.h>
 #include <getopt.h>
 #include <intprops.h>
@@ -34,6 +38,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 +47,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;
 }
 
@@ -106,14 +197,31 @@  do_test (int argc, char *argv[])
     spargv[i] = NULL;
   }
 
+  enum { tunable_num_env_alias = array_length (tunable_env_alias_list) };
+
   for (int i = 0; i < array_length (tests); i++)
     {
       snprintf (nteststr, sizeof nteststr, "%d", i);
 
       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..fc3b41376f 100644
--- a/scripts/gen-tunables.awk
+++ b/scripts/gen-tunables.awk
@@ -156,7 +156,7 @@  END {
   print "# define TUNABLE_ALIAS_MAX " (max_alias_len + 1)
   print "# include \"dl-tunable-types.h\""
   # Finally, the tunable list.
-  print "static tunable_t tunable_list[] attribute_relro = {"
+  print "static tunable_t tunable_list[] attribute_relro __attribute_used__ = {"
   for (tnm in types) {
     split (tnm, indices, SUBSEP);
     t = indices[1];
@@ -168,5 +168,19 @@  END {
 	    default_val[t,n,m], env_alias[t,n,m]);
   }
   print "};"
+
+  # Map of tunable with environment variables aliases used during parsing.  */
+  print "\nstatic const tunable_id_t tunable_env_alias_list[] ="
+  printf "{\n"
+  for (tnm in types) {
+    split (tnm, indices, SUBSEP);
+    t = indices[1];
+    n = indices[2];
+    m = indices[3];
+    if (env_alias[t,n,m] != "{0}") {
+      printf ("  TUNABLE_ENUM_NAME(%s, %s, %s),\n", t, n, m);
+    }
+  }
+  printf "};\n"
   print "#endif"
 }