From: Siddhesh Poyarekar <siddhesh@sourceware.org>
To: Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>,
libc-alpha@sourceware.org
Cc: Joe Simmons-Talbott <josimmon@redhat.com>,
Florian Weimer <fweimer@redhat.com>
Subject: Re: [PATCH 4/4] elf: Make glibc.rtld.enable_secure ignore alias environment variables
Date: Thu, 2 May 2024 07:03:30 -0400 [thread overview]
Message-ID: <52ab2ede-1668-4067-ac27-333d9e7b2344@sourceware.org> (raw)
In-Reply-To: <59df1d0f-098b-4299-95dd-7b9ed0cf650e@linaro.org>
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"
>>> }
>
prev parent reply other threads:[~2024-05-02 11:03 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-30 19:25 [PATCH 0/4] More tunable fixes Adhemerval Zanella
2024-04-30 19:25 ` [PATCH 1/4] elf: Only process multiple tunable once (BZ 31686) Adhemerval Zanella
2024-05-01 12:54 ` Florian Weimer
2024-05-01 14:19 ` Adhemerval Zanella Netto
2024-05-01 16:30 ` Siddhesh Poyarekar
2024-04-30 19:25 ` [PATCH 2/4] elf: Remove glibc.rtld.enable_secure check from parse_tunables_string Adhemerval Zanella
2024-05-01 17:15 ` Siddhesh Poyarekar
2024-04-30 19:25 ` [PATCH 3/4] support: Add envp argument to support_capture_subprogram Adhemerval Zanella
2024-04-30 20:06 ` Joe Simmons-Talbott
2024-04-30 19:25 ` [PATCH 4/4] elf: Make glibc.rtld.enable_secure ignore alias environment variables Adhemerval Zanella
2024-05-01 17:40 ` Siddhesh Poyarekar
2024-05-01 18:00 ` Adhemerval Zanella Netto
2024-05-02 11:03 ` Siddhesh Poyarekar [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=52ab2ede-1668-4067-ac27-333d9e7b2344@sourceware.org \
--to=siddhesh@sourceware.org \
--cc=adhemerval.zanella@linaro.org \
--cc=fweimer@redhat.com \
--cc=josimmon@redhat.com \
--cc=libc-alpha@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).