| Message ID | 20240506161955.1570278-5-adhemerval.zanella@linaro.org |
|---|---|
| State | Accepted |
| Commit | eb59c7b43dd5c64c38e4c3cd21e7ad75d8d29cb0 |
| Headers | show |
| Series | More tunable fixes | expand |
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 --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" }