* [PATCH 1/4] elf: Only process multiple tunable once (BZ 31686)
2024-04-30 19:25 [PATCH 0/4] More tunable fixes Adhemerval Zanella
@ 2024-04-30 19:25 ` Adhemerval Zanella
2024-05-01 12:54 ` Florian Weimer
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
` (2 subsequent siblings)
3 siblings, 2 replies; 13+ messages in thread
From: Adhemerval Zanella @ 2024-04-30 19:25 UTC (permalink / raw)
To: libc-alpha; +Cc: Joe Simmons-Talbott, Siddhesh Poyarekar, Yuto Maeda
The parse_tunables_string is a tunable entry on the 'tunable_list' list
to be set later without checking if the entry is already present. If
leads to a stack overflow if the tunable is set multiple times,
for instance:
GLIBC_TUNABLES=glibc.malloc.check=2:... (repeat over the number of
total support for different tunable).
Instead, use the index of the tunable list to get the expected tunable
entry. Since now the initial list is zero-initialized, the compiler
might emit an extra memset and this requires some minor adjustment
on some ports.
Checked on x86_64-linux-gnu and aarch64-linux-gnu.
Reported-by: Yuto Maeda <maeda@cyberdefense.jp>
---
elf/dl-tunables.c | 30 ++++++-----
elf/tst-tunables.c | 59 +++++++++++++++++++++-
sysdeps/aarch64/multiarch/memset_generic.S | 4 ++
sysdeps/sparc/sparc64/rtld-memset.c | 3 ++
4 files changed, 82 insertions(+), 14 deletions(-)
diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
index d3ccd2ecd4..1db80e0f92 100644
--- a/elf/dl-tunables.c
+++ b/elf/dl-tunables.c
@@ -32,6 +32,7 @@
#include <ldsodefs.h>
#include <array_length.h>
#include <dl-minimal-malloc.h>
+#include <dl-symbol-redir-ifunc.h>
#define TUNABLES_INTERNAL 1
#include "dl-tunables.h"
@@ -221,8 +222,7 @@ parse_tunables_string (const char *valstring, struct tunable_toset_t *tunables)
if (tunable_is_name (cur->name, name))
{
- tunables[ntunables++] =
- (struct tunable_toset_t) { cur, value, p - value };
+ tunables[i] = (struct tunable_toset_t) { cur, value, p - value };
/* Ignore tunables if enable_secure is set */
if (tunable_is_name ("glibc.rtld.enable_secure", name))
@@ -245,23 +245,27 @@ parse_tunables_string (const char *valstring, struct tunable_toset_t *tunables)
static void
parse_tunables (const char *valstring)
{
- struct tunable_toset_t tunables[tunables_list_size];
- int ntunables = parse_tunables_string (valstring, tunables);
- if (ntunables == -1)
+ struct tunable_toset_t tunables[tunables_list_size] = { 0 };
+ if (parse_tunables_string (valstring, tunables) == -1)
{
_dl_error_printf (
"WARNING: ld.so: invalid GLIBC_TUNABLES `%s': ignored.\n", valstring);
return;
}
- for (int i = 0; i < ntunables; i++)
- if (!tunable_initialize (tunables[i].t, tunables[i].value,
- tunables[i].len))
- _dl_error_printf ("WARNING: ld.so: invalid GLIBC_TUNABLES value `%.*s' "
- "for option `%s': ignored.\n",
- (int) tunables[i].len,
- tunables[i].value,
- tunables[i].t->name);
+ for (int i = 0; i < tunables_list_size; i++)
+ {
+ if (tunables[i].t == NULL)
+ continue;
+
+ if (!tunable_initialize (tunables[i].t, tunables[i].value,
+ tunables[i].len))
+ _dl_error_printf ("WARNING: ld.so: invalid GLIBC_TUNABLES value `%.*s' "
+ "for option `%s': ignored.\n",
+ (int) tunables[i].len,
+ tunables[i].value,
+ tunables[i].t->name);
+ }
}
/* Initialize the tunables list from the environment. For now we only use the
diff --git a/elf/tst-tunables.c b/elf/tst-tunables.c
index 095b5c81d9..ce5f62f777 100644
--- a/elf/tst-tunables.c
+++ b/elf/tst-tunables.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>
@@ -24,12 +25,13 @@
#include <stdlib.h>
#include <support/capture_subprocess.h>
#include <support/check.h>
+#include <support/support.h>
static int restart;
#define CMDLINE_OPTIONS \
{ "restart", no_argument, &restart, 1 },
-static const struct test_t
+static struct test_t
{
const char *name;
const char *value;
@@ -284,6 +286,29 @@ static const struct test_t
0,
0,
},
+ /* Also check for repeated tunables with a count larger than the total number
+ of tunables. */
+ {
+ "GLIBC_TUNABLES",
+ NULL,
+ 2,
+ 0,
+ 0,
+ },
+ {
+ "GLIBC_TUNABLES",
+ NULL,
+ 1,
+ 0,
+ 0,
+ },
+ {
+ "GLIBC_TUNABLES",
+ NULL,
+ 0,
+ 0,
+ 0,
+ },
};
static int
@@ -316,6 +341,7 @@ do_test (int argc, char *argv[])
char nteststr[INT_BUFSIZE_BOUND (int)];
+
char *spargv[10];
{
int i = 0;
@@ -327,6 +353,37 @@ do_test (int argc, char *argv[])
spargv[i] = NULL;
}
+ /* Create a tunable line with the duplicate values with a total number
+ larger than the different number of tunables. */
+ {
+ enum { tunables_list_size = array_length (tunable_list) };
+ const char *value = "";
+ for (int i = 0; i < tunables_list_size; i++)
+ value = xasprintf ("%sglibc.malloc.check=2%c",
+ value,
+ i == (tunables_list_size - 1) ? '\0' : ':');
+ tests[33].value = value;
+ }
+ /* Same as before, but the last tunable vallues is differen than the
+ rest. */
+ {
+ enum { tunables_list_size = array_length (tunable_list) };
+ const char *value = "";
+ for (int i = 0; i < tunables_list_size - 1; i++)
+ value = xasprintf ("%sglibc.malloc.check=2:", value);
+ value = xasprintf ("%sglibc.malloc.check=1", value);
+ tests[34].value = value;
+ }
+ /* Same as before, but with an invalid last entry. */
+ {
+ enum { tunables_list_size = array_length (tunable_list) };
+ const char *value = "";
+ for (int i = 0; i < tunables_list_size - 1; i++)
+ value = xasprintf ("%sglibc.malloc.check=2:", value);
+ value = xasprintf ("%sglibc.malloc.check=1=1", value);
+ tests[35].value = value;
+ }
+
for (int i = 0; i < array_length (tests); i++)
{
snprintf (nteststr, sizeof nteststr, "%d", i);
diff --git a/sysdeps/aarch64/multiarch/memset_generic.S b/sysdeps/aarch64/multiarch/memset_generic.S
index 81748bdbce..e125a5ed85 100644
--- a/sysdeps/aarch64/multiarch/memset_generic.S
+++ b/sysdeps/aarch64/multiarch/memset_generic.S
@@ -33,3 +33,7 @@
#endif
#include <../memset.S>
+
+#if IS_IN (rtld)
+strong_alias (memset, __memset_generic)
+#endif
diff --git a/sysdeps/sparc/sparc64/rtld-memset.c b/sysdeps/sparc/sparc64/rtld-memset.c
index 55f3835790..a19202a620 100644
--- a/sysdeps/sparc/sparc64/rtld-memset.c
+++ b/sysdeps/sparc/sparc64/rtld-memset.c
@@ -1 +1,4 @@
#include <string/memset.c>
+#if IS_IN(rtld)
+strong_alias (memset, __memset_ultra1)
+#endif
--
2.43.0
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 1/4] elf: Only process multiple tunable once (BZ 31686)
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
1 sibling, 1 reply; 13+ messages in thread
From: Florian Weimer @ 2024-05-01 12:54 UTC (permalink / raw)
To: Adhemerval Zanella
Cc: libc-alpha, Joe Simmons-Talbott, Siddhesh Poyarekar, Yuto Maeda
* Adhemerval Zanella:
> The parse_tunables_string is a tunable entry on the 'tunable_list' list
> to be set later without checking if the entry is already present. If
> leads to a stack overflow if the tunable is set multiple times,
> for instance:
>
> GLIBC_TUNABLES=glibc.malloc.check=2:... (repeat over the number of
> total support for different tunable).
>
> Instead, use the index of the tunable list to get the expected tunable
> entry. Since now the initial list is zero-initialized, the compiler
> might emit an extra memset and this requires some minor adjustment
> on some ports.
>
> Checked on x86_64-linux-gnu and aarch64-linux-gnu.
>
> Reported-by: Yuto Maeda <maeda@cyberdefense.jp>
Is this fixing a particular commit? If it does, could you mention that
in the commit message?
Thanks,
Florian
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/4] elf: Only process multiple tunable once (BZ 31686)
2024-05-01 12:54 ` Florian Weimer
@ 2024-05-01 14:19 ` Adhemerval Zanella Netto
0 siblings, 0 replies; 13+ messages in thread
From: Adhemerval Zanella Netto @ 2024-05-01 14:19 UTC (permalink / raw)
To: Florian Weimer
Cc: libc-alpha, Joe Simmons-Talbott, Siddhesh Poyarekar, Yuto Maeda
On 01/05/24 09:54, Florian Weimer wrote:
> * Adhemerval Zanella:
>
>> The parse_tunables_string is a tunable entry on the 'tunable_list' list
>> to be set later without checking if the entry is already present. If
>> leads to a stack overflow if the tunable is set multiple times,
>> for instance:
>>
>> GLIBC_TUNABLES=glibc.malloc.check=2:... (repeat over the number of
>> total support for different tunable).
>>
>> Instead, use the index of the tunable list to get the expected tunable
>> entry. Since now the initial list is zero-initialized, the compiler
>> might emit an extra memset and this requires some minor adjustment
>> on some ports.
>>
>> Checked on x86_64-linux-gnu and aarch64-linux-gnu.
>>
>> Reported-by: Yuto Maeda <maeda@cyberdefense.jp>
>
> Is this fixing a particular commit? If it does, could you mention that
> in the commit message?
Yes, I added on the cover-letter (680c597e9c3). I will add it on the
comment message as well.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/4] elf: Only process multiple tunable once (BZ 31686)
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 16:30 ` Siddhesh Poyarekar
1 sibling, 0 replies; 13+ messages in thread
From: Siddhesh Poyarekar @ 2024-05-01 16:30 UTC (permalink / raw)
To: Adhemerval Zanella, libc-alpha
Cc: Joe Simmons-Talbott, Yuto Maeda, Yutaro Shimizu
On 2024-04-30 15:25, Adhemerval Zanella wrote:
> The parse_tunables_string is a tunable entry on the 'tunable_list' list
> to be set later without checking if the entry is already present. If
> leads to a stack overflow if the tunable is set multiple times,
> for instance:
>
> GLIBC_TUNABLES=glibc.malloc.check=2:... (repeat over the number of
> total support for different tunable).
>
> Instead, use the index of the tunable list to get the expected tunable
> entry. Since now the initial list is zero-initialized, the compiler
> might emit an extra memset and this requires some minor adjustment
> on some ports.
>
> Checked on x86_64-linux-gnu and aarch64-linux-gnu.
>
> Reported-by: Yuto Maeda <maeda@cyberdefense.jp>
Please also credit Yutaro Shimizu <shimizu@cyberdefense.jp> in reporters.
> ---
> elf/dl-tunables.c | 30 ++++++-----
> elf/tst-tunables.c | 59 +++++++++++++++++++++-
> sysdeps/aarch64/multiarch/memset_generic.S | 4 ++
> sysdeps/sparc/sparc64/rtld-memset.c | 3 ++
> 4 files changed, 82 insertions(+), 14 deletions(-)
>
> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
> index d3ccd2ecd4..1db80e0f92 100644
> --- a/elf/dl-tunables.c
> +++ b/elf/dl-tunables.c
> @@ -32,6 +32,7 @@
> #include <ldsodefs.h>
> #include <array_length.h>
> #include <dl-minimal-malloc.h>
> +#include <dl-symbol-redir-ifunc.h>
>
> #define TUNABLES_INTERNAL 1
> #include "dl-tunables.h"
> @@ -221,8 +222,7 @@ parse_tunables_string (const char *valstring, struct tunable_toset_t *tunables)
>
> if (tunable_is_name (cur->name, name))
> {
> - tunables[ntunables++] =
> - (struct tunable_toset_t) { cur, value, p - value };
> + tunables[i] = (struct tunable_toset_t) { cur, value, p - value };
Uninitialized tunables stay as NULL, tunables appearing later in the
string will persist in case of duplicates. OK.
>
> /* Ignore tunables if enable_secure is set */
> if (tunable_is_name ("glibc.rtld.enable_secure", name))
> @@ -245,23 +245,27 @@ parse_tunables_string (const char *valstring, struct tunable_toset_t *tunables)
> static void
> parse_tunables (const char *valstring)
> {
> - struct tunable_toset_t tunables[tunables_list_size];
> - int ntunables = parse_tunables_string (valstring, tunables);
> - if (ntunables == -1)
> + struct tunable_toset_t tunables[tunables_list_size] = { 0 };
zero-initialized, so tunables[i].t is NULL unless set above. OK.
> + if (parse_tunables_string (valstring, tunables) == -1)
> {
> _dl_error_printf (
> "WARNING: ld.so: invalid GLIBC_TUNABLES `%s': ignored.\n", valstring);
> return;
> }
>
> - for (int i = 0; i < ntunables; i++)
> - if (!tunable_initialize (tunables[i].t, tunables[i].value,
> - tunables[i].len))
> - _dl_error_printf ("WARNING: ld.so: invalid GLIBC_TUNABLES value `%.*s' "
> - "for option `%s': ignored.\n",
> - (int) tunables[i].len,
> - tunables[i].value,
> - tunables[i].t->name);
> + for (int i = 0; i < tunables_list_size; i++)
> + {
> + if (tunables[i].t == NULL)
> + continue;
Skip over NULLs, initializing only those that are set. OK.
> +
> + if (!tunable_initialize (tunables[i].t, tunables[i].value,
> + tunables[i].len))
> + _dl_error_printf ("WARNING: ld.so: invalid GLIBC_TUNABLES value `%.*s' "
> + "for option `%s': ignored.\n",
> + (int) tunables[i].len,
> + tunables[i].value,
> + tunables[i].t->name);
> + }
> }
>
> /* Initialize the tunables list from the environment. For now we only use the
> diff --git a/elf/tst-tunables.c b/elf/tst-tunables.c
> index 095b5c81d9..ce5f62f777 100644
> --- a/elf/tst-tunables.c
> +++ b/elf/tst-tunables.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>
> @@ -24,12 +25,13 @@
> #include <stdlib.h>
> #include <support/capture_subprocess.h>
> #include <support/check.h>
> +#include <support/support.h>
>
> static int restart;
> #define CMDLINE_OPTIONS \
> { "restart", no_argument, &restart, 1 },
>
> -static const struct test_t
> +static struct test_t
> {
> const char *name;
> const char *value;
> @@ -284,6 +286,29 @@ static const struct test_t
> 0,
> 0,
> },
> + /* Also check for repeated tunables with a count larger than the total number
> + of tunables. */
> + {
> + "GLIBC_TUNABLES",
> + NULL,
> + 2,
> + 0,
> + 0,
> + },
> + {
> + "GLIBC_TUNABLES",
> + NULL,
> + 1,
> + 0,
> + 0,
> + },
> + {
> + "GLIBC_TUNABLES",
> + NULL,
> + 0,
> + 0,
> + 0,
> + },
> };
>
> static int
> @@ -316,6 +341,7 @@ do_test (int argc, char *argv[])
>
> char nteststr[INT_BUFSIZE_BOUND (int)];
>
> +
Spurious newline.
> char *spargv[10];
> {
> int i = 0;
> @@ -327,6 +353,37 @@ do_test (int argc, char *argv[])
> spargv[i] = NULL;
> }
>
> + /* Create a tunable line with the duplicate values with a total number
> + larger than the different number of tunables. */
> + {
> + enum { tunables_list_size = array_length (tunable_list) };
> + const char *value = "";
> + for (int i = 0; i < tunables_list_size; i++)
> + value = xasprintf ("%sglibc.malloc.check=2%c",
> + value,
> + i == (tunables_list_size - 1) ? '\0' : ':');
> + tests[33].value = value;
> + }
> + /* Same as before, but the last tunable vallues is differen than the
> + rest. */
> + {
> + enum { tunables_list_size = array_length (tunable_list) };
> + const char *value = "";
> + for (int i = 0; i < tunables_list_size - 1; i++)
> + value = xasprintf ("%sglibc.malloc.check=2:", value);
> + value = xasprintf ("%sglibc.malloc.check=1", value);
> + tests[34].value = value;
> + }
> + /* Same as before, but with an invalid last entry. */
> + {
> + enum { tunables_list_size = array_length (tunable_list) };
> + const char *value = "";
> + for (int i = 0; i < tunables_list_size - 1; i++)
> + value = xasprintf ("%sglibc.malloc.check=2:", value);
> + value = xasprintf ("%sglibc.malloc.check=1=1", value);
> + tests[35].value = value;
> + }
> +
> for (int i = 0; i < array_length (tests); i++)
> {
> snprintf (nteststr, sizeof nteststr, "%d", i);
> diff --git a/sysdeps/aarch64/multiarch/memset_generic.S b/sysdeps/aarch64/multiarch/memset_generic.S
> index 81748bdbce..e125a5ed85 100644
> --- a/sysdeps/aarch64/multiarch/memset_generic.S
> +++ b/sysdeps/aarch64/multiarch/memset_generic.S
> @@ -33,3 +33,7 @@
> #endif
>
> #include <../memset.S>
> +
> +#if IS_IN (rtld)
> +strong_alias (memset, __memset_generic)
> +#endif
The extra memset you mentioned. OK.
> diff --git a/sysdeps/sparc/sparc64/rtld-memset.c b/sysdeps/sparc/sparc64/rtld-memset.c
> index 55f3835790..a19202a620 100644
> --- a/sysdeps/sparc/sparc64/rtld-memset.c
> +++ b/sysdeps/sparc/sparc64/rtld-memset.c
> @@ -1 +1,4 @@
> #include <string/memset.c>
> +#if IS_IN(rtld)
> +strong_alias (memset, __memset_ultra1)
> +#endif
Likewise.
Thanks,
Sid
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/4] elf: Remove glibc.rtld.enable_secure check from parse_tunables_string
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-04-30 19:25 ` 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 19:25 ` [PATCH 4/4] elf: Make glibc.rtld.enable_secure ignore alias environment variables Adhemerval Zanella
3 siblings, 1 reply; 13+ messages in thread
From: Adhemerval Zanella @ 2024-04-30 19:25 UTC (permalink / raw)
To: libc-alpha; +Cc: Joe Simmons-Talbott, Siddhesh Poyarekar
And move it to parse_tunables. It avoids a string comparison for
each tunable.
Checked on aarch64-linux-gnu and x86_64-linux-gnu.
---
elf/dl-tunables.c | 58 +++++++++++++++++++++++++++++++----------------
1 file changed, 38 insertions(+), 20 deletions(-)
diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
index 1db80e0f92..63cf8c7ab5 100644
--- a/elf/dl-tunables.c
+++ b/elf/dl-tunables.c
@@ -119,6 +119,17 @@ do_tunable_update_val (tunable_t *cur, const tunable_val_t *valp,
cur->initialized = true;
}
+static bool
+tunable_parse_num (const char *strval, size_t len, tunable_num_t *val)
+{
+ char *endptr = NULL;
+ uint64_t numval = _dl_strtoul (strval, &endptr);
+ if (endptr != strval + len)
+ return false;
+ *val = (tunable_num_t) numval;
+ return true;
+}
+
/* Validate range of the input value and initialize the tunable CUR if it looks
good. */
static bool
@@ -128,11 +139,8 @@ tunable_initialize (tunable_t *cur, const char *strval, size_t len)
if (cur->type.type_code != TUNABLE_TYPE_STRING)
{
- char *endptr = NULL;
- uint64_t numval = _dl_strtoul (strval, &endptr);
- if (endptr != strval + len)
+ if (!tunable_parse_num (strval, len, &val.numval))
return false;
- val.numval = (tunable_num_t) numval;
}
else
val.strval = (struct tunable_str_t) { strval, len };
@@ -223,17 +231,6 @@ parse_tunables_string (const char *valstring, struct tunable_toset_t *tunables)
if (tunable_is_name (cur->name, name))
{
tunables[i] = (struct tunable_toset_t) { cur, value, p - value };
-
- /* Ignore tunables if enable_secure is set */
- if (tunable_is_name ("glibc.rtld.enable_secure", name))
- {
- tunable_num_t val = (tunable_num_t) _dl_strtoul (value, NULL);
- if (val == 1)
- {
- __libc_enable_secure = 1;
- return 0;
- }
- }
break;
}
}
@@ -242,6 +239,16 @@ parse_tunables_string (const char *valstring, struct tunable_toset_t *tunables)
return ntunables;
}
+static void
+parse_tunable_print_error (const struct tunable_toset_t *toset)
+{
+ _dl_error_printf ("WARNING: ld.so: invalid GLIBC_TUNABLES value `%.*s' "
+ "for option `%s': ignored.\n",
+ (int) toset->len,
+ toset->value,
+ toset->t->name);
+}
+
static void
parse_tunables (const char *valstring)
{
@@ -253,6 +260,21 @@ parse_tunables (const char *valstring)
return;
}
+ /* Ignore tunables if enable_secure is set */
+ struct tunable_toset_t *tsec =
+ &tunables[TUNABLE_ENUM_NAME(glibc, rtld, enable_secure)];
+ if (tsec->t != NULL)
+ {
+ tunable_num_t val;
+ if (!tunable_parse_num (tsec->value, tsec->len, &val))
+ parse_tunable_print_error (tsec);
+ else if (val == 1)
+ {
+ __libc_enable_secure = 1;
+ return;
+ }
+ }
+
for (int i = 0; i < tunables_list_size; i++)
{
if (tunables[i].t == NULL)
@@ -260,11 +282,7 @@ parse_tunables (const char *valstring)
if (!tunable_initialize (tunables[i].t, tunables[i].value,
tunables[i].len))
- _dl_error_printf ("WARNING: ld.so: invalid GLIBC_TUNABLES value `%.*s' "
- "for option `%s': ignored.\n",
- (int) tunables[i].len,
- tunables[i].value,
- tunables[i].t->name);
+ parse_tunable_print_error (&tunables[i]);
}
}
--
2.43.0
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 2/4] elf: Remove glibc.rtld.enable_secure check from parse_tunables_string
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
0 siblings, 0 replies; 13+ messages in thread
From: Siddhesh Poyarekar @ 2024-05-01 17:15 UTC (permalink / raw)
To: Adhemerval Zanella, libc-alpha; +Cc: Joe Simmons-Talbott
On 2024-04-30 15:25, Adhemerval Zanella wrote:
> And move it to parse_tunables. It avoids a string comparison for
> each tunable.
>
> Checked on aarch64-linux-gnu and x86_64-linux-gnu.
> ---
> elf/dl-tunables.c | 58 +++++++++++++++++++++++++++++++----------------
> 1 file changed, 38 insertions(+), 20 deletions(-)
>
> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
> index 1db80e0f92..63cf8c7ab5 100644
> --- a/elf/dl-tunables.c
> +++ b/elf/dl-tunables.c
> @@ -119,6 +119,17 @@ do_tunable_update_val (tunable_t *cur, const tunable_val_t *valp,
> cur->initialized = true;
> }
>
> +static bool
> +tunable_parse_num (const char *strval, size_t len, tunable_num_t *val)
> +{
> + char *endptr = NULL;
> + uint64_t numval = _dl_strtoul (strval, &endptr);
> + if (endptr != strval + len)
> + return false;
> + *val = (tunable_num_t) numval;
> + return true;
> +}
> +
> /* Validate range of the input value and initialize the tunable CUR if it looks
> good. */
> static bool
> @@ -128,11 +139,8 @@ tunable_initialize (tunable_t *cur, const char *strval, size_t len)
>
> if (cur->type.type_code != TUNABLE_TYPE_STRING)
> {
> - char *endptr = NULL;
> - uint64_t numval = _dl_strtoul (strval, &endptr);
> - if (endptr != strval + len)
> + if (!tunable_parse_num (strval, len, &val.numval))
Refactoring. OK.
> return false;
> - val.numval = (tunable_num_t) numval;
> }
> else
> val.strval = (struct tunable_str_t) { strval, len };
> @@ -223,17 +231,6 @@ parse_tunables_string (const char *valstring, struct tunable_toset_t *tunables)
> if (tunable_is_name (cur->name, name))
> {
> tunables[i] = (struct tunable_toset_t) { cur, value, p - value };
> -
> - /* Ignore tunables if enable_secure is set */
> - if (tunable_is_name ("glibc.rtld.enable_secure", name))
> - {
> - tunable_num_t val = (tunable_num_t) _dl_strtoul (value, NULL);
> - if (val == 1)
> - {
> - __libc_enable_secure = 1;
> - return 0;
> - }
> - }
Drop this string comparison for a tunable comparison below. OK.
> break;
> }
> }
> @@ -242,6 +239,16 @@ parse_tunables_string (const char *valstring, struct tunable_toset_t *tunables)
> return ntunables;
> }
>
> +static void
> +parse_tunable_print_error (const struct tunable_toset_t *toset)
> +{
> + _dl_error_printf ("WARNING: ld.so: invalid GLIBC_TUNABLES value `%.*s' "
> + "for option `%s': ignored.\n",
> + (int) toset->len,
> + toset->value,
> + toset->t->name);
> +}
> +
Refactor. OK.
> static void
> parse_tunables (const char *valstring)
> {
> @@ -253,6 +260,21 @@ parse_tunables (const char *valstring)
> return;
> }
>
> + /* Ignore tunables if enable_secure is set */
> + struct tunable_toset_t *tsec =
> + &tunables[TUNABLE_ENUM_NAME(glibc, rtld, enable_secure)];
> + if (tsec->t != NULL)
> + {
> + tunable_num_t val;
> + if (!tunable_parse_num (tsec->value, tsec->len, &val))
> + parse_tunable_print_error (tsec);
> + else if (val == 1)
> + {
> + __libc_enable_secure = 1;
> + return;
> + }
> + }
> +
Directly read the enable_secure tunable when it is set. OK.
> for (int i = 0; i < tunables_list_size; i++)
> {
> if (tunables[i].t == NULL)
> @@ -260,11 +282,7 @@ parse_tunables (const char *valstring)
>
> if (!tunable_initialize (tunables[i].t, tunables[i].value,
> tunables[i].len))
> - _dl_error_printf ("WARNING: ld.so: invalid GLIBC_TUNABLES value `%.*s' "
> - "for option `%s': ignored.\n",
> - (int) tunables[i].len,
> - tunables[i].value,
> - tunables[i].t->name);
> + parse_tunable_print_error (&tunables[i]);
> }
> }
>
Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/4] support: Add envp argument to support_capture_subprogram
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-04-30 19:25 ` [PATCH 2/4] elf: Remove glibc.rtld.enable_secure check from parse_tunables_string Adhemerval Zanella
@ 2024-04-30 19:25 ` 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
3 siblings, 1 reply; 13+ messages in thread
From: Adhemerval Zanella @ 2024-04-30 19:25 UTC (permalink / raw)
To: libc-alpha; +Cc: Joe Simmons-Talbott, Siddhesh Poyarekar
So tests can specific a list of environment variables.
---
elf/tst-audit18.c | 2 +-
elf/tst-audit19b.c | 2 +-
elf/tst-audit22.c | 2 +-
elf/tst-audit23.c | 2 +-
elf/tst-audit25a.c | 4 ++--
elf/tst-audit25b.c | 4 ++--
elf/tst-glibc-hwcaps-2-cache.c | 2 +-
elf/tst-rtld-run-static.c | 4 ++--
elf/tst-tunables-enable_secure.c | 2 +-
elf/tst-tunables.c | 2 +-
support/capture_subprocess.h | 9 +++++----
support/subprocess.h | 7 ++++---
support/support_capture_subprocess.c | 5 +++--
support/support_subprocess.c | 5 +++--
support/tst-support_capture_subprocess.c | 2 +-
sysdeps/x86/tst-hwcap-tunables.c | 2 +-
16 files changed, 30 insertions(+), 26 deletions(-)
diff --git a/elf/tst-audit18.c b/elf/tst-audit18.c
index 841251dd70..cec93e269c 100644
--- a/elf/tst-audit18.c
+++ b/elf/tst-audit18.c
@@ -79,7 +79,7 @@ do_test (int argc, char *argv[])
setenv ("LD_AUDIT", "tst-auditmod18.so", 0);
struct support_capture_subprocess result
- = support_capture_subprogram (spargv[0], spargv);
+ = support_capture_subprogram (spargv[0], spargv, NULL);
support_capture_subprocess_check (&result, "tst-audit18", 0, sc_allow_stderr);
struct
diff --git a/elf/tst-audit19b.c b/elf/tst-audit19b.c
index 70bfe4eadf..88d99a416b 100644
--- a/elf/tst-audit19b.c
+++ b/elf/tst-audit19b.c
@@ -69,7 +69,7 @@ do_test (int argc, char *argv[])
setenv ("LD_AUDIT", "tst-auditmod18b.so", 0);
struct support_capture_subprocess result
- = support_capture_subprogram (spargv[0], spargv);
+ = support_capture_subprogram (spargv[0], spargv, NULL);
support_capture_subprocess_check (&result, "tst-audit18b", 0, sc_allow_stderr);
bool find_symbind = false;
diff --git a/elf/tst-audit22.c b/elf/tst-audit22.c
index 4e97be3be0..6aa18af948 100644
--- a/elf/tst-audit22.c
+++ b/elf/tst-audit22.c
@@ -83,7 +83,7 @@ do_test (int argc, char *argv[])
setenv ("LD_AUDIT", "tst-auditmod22.so", 0);
struct support_capture_subprocess result
- = support_capture_subprogram (spargv[0], spargv);
+ = support_capture_subprogram (spargv[0], spargv, NULL);
support_capture_subprocess_check (&result, "tst-audit22", 0, sc_allow_stderr);
/* The respawned process should always print the vDSO address (otherwise it
diff --git a/elf/tst-audit23.c b/elf/tst-audit23.c
index 32e7c8b2a3..d2640fe8b2 100644
--- a/elf/tst-audit23.c
+++ b/elf/tst-audit23.c
@@ -82,7 +82,7 @@ do_test (int argc, char *argv[])
setenv ("LD_AUDIT", "tst-auditmod23.so", 0);
struct support_capture_subprocess result
- = support_capture_subprogram (spargv[0], spargv);
+ = support_capture_subprogram (spargv[0], spargv, NULL);
support_capture_subprocess_check (&result, "tst-audit22", 0, sc_allow_stderr);
/* The expected la_objopen/la_objclose:
diff --git a/elf/tst-audit25a.c b/elf/tst-audit25a.c
index b209ee820f..cdd4f2ce2b 100644
--- a/elf/tst-audit25a.c
+++ b/elf/tst-audit25a.c
@@ -77,7 +77,7 @@ do_test (int argc, char *argv[])
{
struct support_capture_subprocess result
- = support_capture_subprogram (spargv[0], spargv);
+ = support_capture_subprogram (spargv[0], spargv, NULL);
support_capture_subprocess_check (&result, "tst-audit25a", 0,
sc_allow_stderr);
@@ -102,7 +102,7 @@ do_test (int argc, char *argv[])
{
setenv ("LD_BIND_NOW", "1", 0);
struct support_capture_subprocess result
- = support_capture_subprogram (spargv[0], spargv);
+ = support_capture_subprogram (spargv[0], spargv, NULL);
support_capture_subprocess_check (&result, "tst-audit25a", 0,
sc_allow_stderr);
diff --git a/elf/tst-audit25b.c b/elf/tst-audit25b.c
index 9b8665d517..939f4d6188 100644
--- a/elf/tst-audit25b.c
+++ b/elf/tst-audit25b.c
@@ -76,7 +76,7 @@ do_test (int argc, char *argv[])
{
struct support_capture_subprocess result
- = support_capture_subprogram (spargv[0], spargv);
+ = support_capture_subprogram (spargv[0], spargv, NULL);
support_capture_subprocess_check (&result, "tst-audit25a", 0,
sc_allow_stderr);
@@ -102,7 +102,7 @@ do_test (int argc, char *argv[])
{
setenv ("LD_BIND_NOW", "1", 0);
struct support_capture_subprocess result
- = support_capture_subprogram (spargv[0], spargv);
+ = support_capture_subprogram (spargv[0], spargv, NULL);
support_capture_subprocess_check (&result, "tst-audit25a", 0,
sc_allow_stderr);
diff --git a/elf/tst-glibc-hwcaps-2-cache.c b/elf/tst-glibc-hwcaps-2-cache.c
index 81ab44ff78..af91476cca 100644
--- a/elf/tst-glibc-hwcaps-2-cache.c
+++ b/elf/tst-glibc-hwcaps-2-cache.c
@@ -32,7 +32,7 @@ main (int argc, char **argv)
/* Run ldconfig to populate the cache. */
char *command = xasprintf ("%s/ldconfig", support_install_rootsbindir);
struct support_capture_subprocess result =
- support_capture_subprogram (command, &((char *) { NULL }));
+ support_capture_subprogram (command, &((char *) { NULL }), NULL);
support_capture_subprocess_check (&result, "ldconfig", 0, sc_allow_none);
free (command);
diff --git a/elf/tst-rtld-run-static.c b/elf/tst-rtld-run-static.c
index b2650e85ff..f05c00eb7b 100644
--- a/elf/tst-rtld-run-static.c
+++ b/elf/tst-rtld-run-static.c
@@ -30,7 +30,7 @@ do_test (void)
{
char *argv[] = { (char *) "ld.so", ldconfig_path, (char *) "--help", NULL };
struct support_capture_subprocess cap
- = support_capture_subprogram (support_objdir_elf_ldso, argv);
+ = support_capture_subprogram (support_objdir_elf_ldso, argv, NULL);
support_capture_subprocess_check (&cap, "no --argv0", 0, sc_allow_stdout);
puts ("info: output without --argv0:");
puts (cap.out.buffer);
@@ -46,7 +46,7 @@ do_test (void)
ldconfig_path, (char *) "--help", NULL
};
struct support_capture_subprocess cap
- = support_capture_subprogram (support_objdir_elf_ldso, argv);
+ = support_capture_subprogram (support_objdir_elf_ldso, argv, NULL);
support_capture_subprocess_check (&cap, "with --argv0", 0, sc_allow_stdout);
puts ("info: output with --argv0:");
puts (cap.out.buffer);
diff --git a/elf/tst-tunables-enable_secure.c b/elf/tst-tunables-enable_secure.c
index e31e1f7faa..f5db1c84e9 100644
--- a/elf/tst-tunables-enable_secure.c
+++ b/elf/tst-tunables-enable_secure.c
@@ -113,7 +113,7 @@ do_test (int argc, char *argv[])
printf ("[%d] Spawned test for %s\n", i, tests[i].env);
setenv ("GLIBC_TUNABLES", tests[i].env, 1);
struct support_capture_subprocess result
- = support_capture_subprogram (spargv[0], spargv);
+ = support_capture_subprogram (spargv[0], spargv, NULL);
support_capture_subprocess_check (&result, "tst-tunables-enable_secure",
0, sc_allow_stderr);
support_capture_subprocess_free (&result);
diff --git a/elf/tst-tunables.c b/elf/tst-tunables.c
index ce5f62f777..7afd76fb9b 100644
--- a/elf/tst-tunables.c
+++ b/elf/tst-tunables.c
@@ -394,7 +394,7 @@ do_test (int argc, char *argv[])
tests[i].value);
setenv (tests[i].name, tests[i].value, 1);
struct support_capture_subprocess result
- = support_capture_subprogram (spargv[0], spargv);
+ = support_capture_subprogram (spargv[0], spargv, NULL);
support_capture_subprocess_check (&result, "tst-tunables", 0,
sc_allow_stderr);
support_capture_subprocess_free (&result);
diff --git a/support/capture_subprocess.h b/support/capture_subprocess.h
index 1ecbdfe4fc..d69d1afe65 100644
--- a/support/capture_subprocess.h
+++ b/support/capture_subprocess.h
@@ -35,11 +35,12 @@ struct support_capture_subprocess
struct support_capture_subprocess support_capture_subprocess
(void (*callback) (void *), void *closure);
-/* Issue FILE with ARGV arguments by using posix_spawn and capture standard
- output, standard error, and the exit status. The out.buffer and err.buffer
- are handle as support_capture_subprocess. */
+/* Issue FILE with ARGV arguments and ENVP environments by using posix_spawn
+ and capture standard output, standard error, and the exit status. If
+ ENVP is NULL the current environment variable is usued. The out.buffer and
+ err.buffer are handle as support_capture_subprocess. */
struct support_capture_subprocess support_capture_subprogram
- (const char *file, char *const argv[]);
+ (const char *file, char *const argv[], char *const envp[]);
/* Copy the running program into a setgid binary and run it with CHILD_ID
argument. If execution is successful, return the exit status of the child
diff --git a/support/subprocess.h b/support/subprocess.h
index 8fbb895353..c7ac6d6536 100644
--- a/support/subprocess.h
+++ b/support/subprocess.h
@@ -33,10 +33,11 @@ struct support_subprocess
struct support_subprocess support_subprocess
(void (*callback) (void *), void *closure);
-/* Issue FILE with ARGV arguments by using posix_spawn and return is PID, a
- pipe redirected to STDOUT, and a pipe redirected to STDERR. */
+/* Issue FILE with ARGV arguments and ENVP environments by using posix_spawn
+ and return is PID, a pipe redirected to STDOUT, and a pipe redirected to
+ STDERR. If ENVP is NULL the current environment variable is usued. */
struct support_subprocess support_subprogram
- (const char *file, char *const argv[]);
+ (const char *file, char *const argv[], char *const envp[]);
/* Invoke program FILE with ARGV arguments by using posix_spawn and wait for it
to complete. Return program exit status. */
diff --git a/support/support_capture_subprocess.c b/support/support_capture_subprocess.c
index ffced8a89f..53847194cb 100644
--- a/support/support_capture_subprocess.c
+++ b/support/support_capture_subprocess.c
@@ -93,13 +93,14 @@ support_capture_subprocess (void (*callback) (void *), void *closure)
}
struct support_capture_subprocess
-support_capture_subprogram (const char *file, char *const argv[])
+support_capture_subprogram (const char *file, char *const argv[],
+ char *const envp[])
{
struct support_capture_subprocess result;
xopen_memstream (&result.out);
xopen_memstream (&result.err);
- struct support_subprocess proc = support_subprogram (file, argv);
+ struct support_subprocess proc = support_subprogram (file, argv, envp);
support_capture_poll (&result, &proc);
return result;
diff --git a/support/support_subprocess.c b/support/support_subprocess.c
index a2fef394d4..b692a7f8b1 100644
--- a/support/support_subprocess.c
+++ b/support/support_subprocess.c
@@ -69,7 +69,7 @@ support_subprocess (void (*callback) (void *), void *closure)
}
struct support_subprocess
-support_subprogram (const char *file, char *const argv[])
+support_subprogram (const char *file, char *const argv[], char *const envp[])
{
struct support_subprocess result = support_subprocess_init ();
@@ -84,7 +84,8 @@ support_subprogram (const char *file, char *const argv[])
xposix_spawn_file_actions_addclose (&fa, result.stdout_pipe[1]);
xposix_spawn_file_actions_addclose (&fa, result.stderr_pipe[1]);
- result.pid = xposix_spawn (file, &fa, NULL, argv, environ);
+ result.pid = xposix_spawn (file, &fa, NULL, argv,
+ envp == NULL ? environ : envp);
xclose (result.stdout_pipe[1]);
xclose (result.stderr_pipe[1]);
diff --git a/support/tst-support_capture_subprocess.c b/support/tst-support_capture_subprocess.c
index 8145548982..756fb75d19 100644
--- a/support/tst-support_capture_subprocess.c
+++ b/support/tst-support_capture_subprocess.c
@@ -238,7 +238,7 @@ do_subprogram (const struct test *test)
args[argc] = NULL;
TEST_VERIFY (argc < argv_size);
- return support_capture_subprogram (args[0], args);
+ return support_capture_subprogram (args[0], args, NULL);
}
enum test_type
diff --git a/sysdeps/x86/tst-hwcap-tunables.c b/sysdeps/x86/tst-hwcap-tunables.c
index f6a65b88de..8589a9fd66 100644
--- a/sysdeps/x86/tst-hwcap-tunables.c
+++ b/sysdeps/x86/tst-hwcap-tunables.c
@@ -133,7 +133,7 @@ do_test (int argc, char *argv[])
setenv ("GLIBC_TUNABLES", tunable, 1);
struct support_capture_subprocess result
- = support_capture_subprogram (spargv[0], spargv);
+ = support_capture_subprogram (spargv[0], spargv, NULL);
support_capture_subprocess_check (&result, "tst-tunables", 0,
sc_allow_stderr);
support_capture_subprocess_free (&result);
--
2.43.0
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 3/4] support: Add envp argument to support_capture_subprogram
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
0 siblings, 0 replies; 13+ messages in thread
From: Joe Simmons-Talbott @ 2024-04-30 20:06 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: libc-alpha, Siddhesh Poyarekar
On Tue, Apr 30, 2024 at 3:33 PM Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
A few minor typos follow.
>
> So tests can specific a list of environment variables.
s/specific/specify/
> ---
> elf/tst-audit18.c | 2 +-
> elf/tst-audit19b.c | 2 +-
> elf/tst-audit22.c | 2 +-
> elf/tst-audit23.c | 2 +-
> elf/tst-audit25a.c | 4 ++--
> elf/tst-audit25b.c | 4 ++--
> elf/tst-glibc-hwcaps-2-cache.c | 2 +-
> elf/tst-rtld-run-static.c | 4 ++--
> elf/tst-tunables-enable_secure.c | 2 +-
> elf/tst-tunables.c | 2 +-
> support/capture_subprocess.h | 9 +++++----
> support/subprocess.h | 7 ++++---
> support/support_capture_subprocess.c | 5 +++--
> support/support_subprocess.c | 5 +++--
> support/tst-support_capture_subprocess.c | 2 +-
> sysdeps/x86/tst-hwcap-tunables.c | 2 +-
> 16 files changed, 30 insertions(+), 26 deletions(-)
>
> diff --git a/elf/tst-audit18.c b/elf/tst-audit18.c
> index 841251dd70..cec93e269c 100644
> --- a/elf/tst-audit18.c
> +++ b/elf/tst-audit18.c
> @@ -79,7 +79,7 @@ do_test (int argc, char *argv[])
>
> setenv ("LD_AUDIT", "tst-auditmod18.so", 0);
> struct support_capture_subprocess result
> - = support_capture_subprogram (spargv[0], spargv);
> + = support_capture_subprogram (spargv[0], spargv, NULL);
> support_capture_subprocess_check (&result, "tst-audit18", 0, sc_allow_stderr);
>
> struct
> diff --git a/elf/tst-audit19b.c b/elf/tst-audit19b.c
> index 70bfe4eadf..88d99a416b 100644
> --- a/elf/tst-audit19b.c
> +++ b/elf/tst-audit19b.c
> @@ -69,7 +69,7 @@ do_test (int argc, char *argv[])
>
> setenv ("LD_AUDIT", "tst-auditmod18b.so", 0);
> struct support_capture_subprocess result
> - = support_capture_subprogram (spargv[0], spargv);
> + = support_capture_subprogram (spargv[0], spargv, NULL);
> support_capture_subprocess_check (&result, "tst-audit18b", 0, sc_allow_stderr);
>
> bool find_symbind = false;
> diff --git a/elf/tst-audit22.c b/elf/tst-audit22.c
> index 4e97be3be0..6aa18af948 100644
> --- a/elf/tst-audit22.c
> +++ b/elf/tst-audit22.c
> @@ -83,7 +83,7 @@ do_test (int argc, char *argv[])
>
> setenv ("LD_AUDIT", "tst-auditmod22.so", 0);
> struct support_capture_subprocess result
> - = support_capture_subprogram (spargv[0], spargv);
> + = support_capture_subprogram (spargv[0], spargv, NULL);
> support_capture_subprocess_check (&result, "tst-audit22", 0, sc_allow_stderr);
>
> /* The respawned process should always print the vDSO address (otherwise it
> diff --git a/elf/tst-audit23.c b/elf/tst-audit23.c
> index 32e7c8b2a3..d2640fe8b2 100644
> --- a/elf/tst-audit23.c
> +++ b/elf/tst-audit23.c
> @@ -82,7 +82,7 @@ do_test (int argc, char *argv[])
>
> setenv ("LD_AUDIT", "tst-auditmod23.so", 0);
> struct support_capture_subprocess result
> - = support_capture_subprogram (spargv[0], spargv);
> + = support_capture_subprogram (spargv[0], spargv, NULL);
> support_capture_subprocess_check (&result, "tst-audit22", 0, sc_allow_stderr);
>
> /* The expected la_objopen/la_objclose:
> diff --git a/elf/tst-audit25a.c b/elf/tst-audit25a.c
> index b209ee820f..cdd4f2ce2b 100644
> --- a/elf/tst-audit25a.c
> +++ b/elf/tst-audit25a.c
> @@ -77,7 +77,7 @@ do_test (int argc, char *argv[])
>
> {
> struct support_capture_subprocess result
> - = support_capture_subprogram (spargv[0], spargv);
> + = support_capture_subprogram (spargv[0], spargv, NULL);
> support_capture_subprocess_check (&result, "tst-audit25a", 0,
> sc_allow_stderr);
>
> @@ -102,7 +102,7 @@ do_test (int argc, char *argv[])
> {
> setenv ("LD_BIND_NOW", "1", 0);
> struct support_capture_subprocess result
> - = support_capture_subprogram (spargv[0], spargv);
> + = support_capture_subprogram (spargv[0], spargv, NULL);
> support_capture_subprocess_check (&result, "tst-audit25a", 0,
> sc_allow_stderr);
>
> diff --git a/elf/tst-audit25b.c b/elf/tst-audit25b.c
> index 9b8665d517..939f4d6188 100644
> --- a/elf/tst-audit25b.c
> +++ b/elf/tst-audit25b.c
> @@ -76,7 +76,7 @@ do_test (int argc, char *argv[])
>
> {
> struct support_capture_subprocess result
> - = support_capture_subprogram (spargv[0], spargv);
> + = support_capture_subprogram (spargv[0], spargv, NULL);
> support_capture_subprocess_check (&result, "tst-audit25a", 0,
> sc_allow_stderr);
>
> @@ -102,7 +102,7 @@ do_test (int argc, char *argv[])
> {
> setenv ("LD_BIND_NOW", "1", 0);
> struct support_capture_subprocess result
> - = support_capture_subprogram (spargv[0], spargv);
> + = support_capture_subprogram (spargv[0], spargv, NULL);
> support_capture_subprocess_check (&result, "tst-audit25a", 0,
> sc_allow_stderr);
>
> diff --git a/elf/tst-glibc-hwcaps-2-cache.c b/elf/tst-glibc-hwcaps-2-cache.c
> index 81ab44ff78..af91476cca 100644
> --- a/elf/tst-glibc-hwcaps-2-cache.c
> +++ b/elf/tst-glibc-hwcaps-2-cache.c
> @@ -32,7 +32,7 @@ main (int argc, char **argv)
> /* Run ldconfig to populate the cache. */
> char *command = xasprintf ("%s/ldconfig", support_install_rootsbindir);
> struct support_capture_subprocess result =
> - support_capture_subprogram (command, &((char *) { NULL }));
> + support_capture_subprogram (command, &((char *) { NULL }), NULL);
> support_capture_subprocess_check (&result, "ldconfig", 0, sc_allow_none);
> free (command);
>
> diff --git a/elf/tst-rtld-run-static.c b/elf/tst-rtld-run-static.c
> index b2650e85ff..f05c00eb7b 100644
> --- a/elf/tst-rtld-run-static.c
> +++ b/elf/tst-rtld-run-static.c
> @@ -30,7 +30,7 @@ do_test (void)
> {
> char *argv[] = { (char *) "ld.so", ldconfig_path, (char *) "--help", NULL };
> struct support_capture_subprocess cap
> - = support_capture_subprogram (support_objdir_elf_ldso, argv);
> + = support_capture_subprogram (support_objdir_elf_ldso, argv, NULL);
> support_capture_subprocess_check (&cap, "no --argv0", 0, sc_allow_stdout);
> puts ("info: output without --argv0:");
> puts (cap.out.buffer);
> @@ -46,7 +46,7 @@ do_test (void)
> ldconfig_path, (char *) "--help", NULL
> };
> struct support_capture_subprocess cap
> - = support_capture_subprogram (support_objdir_elf_ldso, argv);
> + = support_capture_subprogram (support_objdir_elf_ldso, argv, NULL);
> support_capture_subprocess_check (&cap, "with --argv0", 0, sc_allow_stdout);
> puts ("info: output with --argv0:");
> puts (cap.out.buffer);
> diff --git a/elf/tst-tunables-enable_secure.c b/elf/tst-tunables-enable_secure.c
> index e31e1f7faa..f5db1c84e9 100644
> --- a/elf/tst-tunables-enable_secure.c
> +++ b/elf/tst-tunables-enable_secure.c
> @@ -113,7 +113,7 @@ do_test (int argc, char *argv[])
> printf ("[%d] Spawned test for %s\n", i, tests[i].env);
> setenv ("GLIBC_TUNABLES", tests[i].env, 1);
> struct support_capture_subprocess result
> - = support_capture_subprogram (spargv[0], spargv);
> + = support_capture_subprogram (spargv[0], spargv, NULL);
> support_capture_subprocess_check (&result, "tst-tunables-enable_secure",
> 0, sc_allow_stderr);
> support_capture_subprocess_free (&result);
> diff --git a/elf/tst-tunables.c b/elf/tst-tunables.c
> index ce5f62f777..7afd76fb9b 100644
> --- a/elf/tst-tunables.c
> +++ b/elf/tst-tunables.c
> @@ -394,7 +394,7 @@ do_test (int argc, char *argv[])
> tests[i].value);
> setenv (tests[i].name, tests[i].value, 1);
> struct support_capture_subprocess result
> - = support_capture_subprogram (spargv[0], spargv);
> + = support_capture_subprogram (spargv[0], spargv, NULL);
> support_capture_subprocess_check (&result, "tst-tunables", 0,
> sc_allow_stderr);
> support_capture_subprocess_free (&result);
> diff --git a/support/capture_subprocess.h b/support/capture_subprocess.h
> index 1ecbdfe4fc..d69d1afe65 100644
> --- a/support/capture_subprocess.h
> +++ b/support/capture_subprocess.h
> @@ -35,11 +35,12 @@ struct support_capture_subprocess
> struct support_capture_subprocess support_capture_subprocess
> (void (*callback) (void *), void *closure);
>
> -/* Issue FILE with ARGV arguments by using posix_spawn and capture standard
> - output, standard error, and the exit status. The out.buffer and err.buffer
> - are handle as support_capture_subprocess. */
> +/* Issue FILE with ARGV arguments and ENVP environments by using posix_spawn
> + and capture standard output, standard error, and the exit status. If
> + ENVP is NULL the current environment variable is usued. The out.buffer and
s/usued/used/
> + err.buffer are handle as support_capture_subprocess. */
s/handle as/handled by/ ?
> struct support_capture_subprocess support_capture_subprogram
> - (const char *file, char *const argv[]);
> + (const char *file, char *const argv[], char *const envp[]);
>
> /* Copy the running program into a setgid binary and run it with CHILD_ID
> argument. If execution is successful, return the exit status of the child
> diff --git a/support/subprocess.h b/support/subprocess.h
> index 8fbb895353..c7ac6d6536 100644
> --- a/support/subprocess.h
> +++ b/support/subprocess.h
> @@ -33,10 +33,11 @@ struct support_subprocess
> struct support_subprocess support_subprocess
> (void (*callback) (void *), void *closure);
>
> -/* Issue FILE with ARGV arguments by using posix_spawn and return is PID, a
> - pipe redirected to STDOUT, and a pipe redirected to STDERR. */
> +/* Issue FILE with ARGV arguments and ENVP environments by using posix_spawn
> + and return is PID, a pipe redirected to STDOUT, and a pipe redirected to
> + STDERR. If ENVP is NULL the current environment variable is usued. */
s/usued/used/
> struct support_subprocess support_subprogram
> - (const char *file, char *const argv[]);
> + (const char *file, char *const argv[], char *const envp[]);
>
> /* Invoke program FILE with ARGV arguments by using posix_spawn and wait for it
> to complete. Return program exit status. */
> diff --git a/support/support_capture_subprocess.c b/support/support_capture_subprocess.c
> index ffced8a89f..53847194cb 100644
> --- a/support/support_capture_subprocess.c
> +++ b/support/support_capture_subprocess.c
> @@ -93,13 +93,14 @@ support_capture_subprocess (void (*callback) (void *), void *closure)
> }
>
> struct support_capture_subprocess
> -support_capture_subprogram (const char *file, char *const argv[])
> +support_capture_subprogram (const char *file, char *const argv[],
> + char *const envp[])
> {
> struct support_capture_subprocess result;
> xopen_memstream (&result.out);
> xopen_memstream (&result.err);
>
> - struct support_subprocess proc = support_subprogram (file, argv);
> + struct support_subprocess proc = support_subprogram (file, argv, envp);
>
> support_capture_poll (&result, &proc);
> return result;
> diff --git a/support/support_subprocess.c b/support/support_subprocess.c
> index a2fef394d4..b692a7f8b1 100644
> --- a/support/support_subprocess.c
> +++ b/support/support_subprocess.c
> @@ -69,7 +69,7 @@ support_subprocess (void (*callback) (void *), void *closure)
> }
>
> struct support_subprocess
> -support_subprogram (const char *file, char *const argv[])
> +support_subprogram (const char *file, char *const argv[], char *const envp[])
> {
> struct support_subprocess result = support_subprocess_init ();
>
> @@ -84,7 +84,8 @@ support_subprogram (const char *file, char *const argv[])
> xposix_spawn_file_actions_addclose (&fa, result.stdout_pipe[1]);
> xposix_spawn_file_actions_addclose (&fa, result.stderr_pipe[1]);
>
> - result.pid = xposix_spawn (file, &fa, NULL, argv, environ);
> + result.pid = xposix_spawn (file, &fa, NULL, argv,
> + envp == NULL ? environ : envp);
>
> xclose (result.stdout_pipe[1]);
> xclose (result.stderr_pipe[1]);
> diff --git a/support/tst-support_capture_subprocess.c b/support/tst-support_capture_subprocess.c
> index 8145548982..756fb75d19 100644
> --- a/support/tst-support_capture_subprocess.c
> +++ b/support/tst-support_capture_subprocess.c
> @@ -238,7 +238,7 @@ do_subprogram (const struct test *test)
> args[argc] = NULL;
> TEST_VERIFY (argc < argv_size);
>
> - return support_capture_subprogram (args[0], args);
> + return support_capture_subprogram (args[0], args, NULL);
> }
>
> enum test_type
> diff --git a/sysdeps/x86/tst-hwcap-tunables.c b/sysdeps/x86/tst-hwcap-tunables.c
> index f6a65b88de..8589a9fd66 100644
> --- a/sysdeps/x86/tst-hwcap-tunables.c
> +++ b/sysdeps/x86/tst-hwcap-tunables.c
> @@ -133,7 +133,7 @@ do_test (int argc, char *argv[])
> setenv ("GLIBC_TUNABLES", tunable, 1);
>
> struct support_capture_subprocess result
> - = support_capture_subprogram (spargv[0], spargv);
> + = support_capture_subprogram (spargv[0], spargv, NULL);
> support_capture_subprocess_check (&result, "tst-tunables", 0,
> sc_allow_stderr);
> support_capture_subprocess_free (&result);
> --
> 2.43.0
>
--
Joe Simmons-Talbott
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 4/4] elf: Make glibc.rtld.enable_secure ignore alias environment variables
2024-04-30 19:25 [PATCH 0/4] More tunable fixes Adhemerval Zanella
` (2 preceding siblings ...)
2024-04-30 19:25 ` [PATCH 3/4] support: Add envp argument to support_capture_subprogram Adhemerval Zanella
@ 2024-04-30 19:25 ` Adhemerval Zanella
2024-05-01 17:40 ` Siddhesh Poyarekar
3 siblings, 1 reply; 13+ messages in thread
From: Adhemerval Zanella @ 2024-04-30 19:25 UTC (permalink / raw)
To: libc-alpha; +Cc: Joe Simmons-Talbott, Siddhesh Poyarekar
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(-)
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"
}
--
2.43.0
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 4/4] elf: Make glibc.rtld.enable_secure ignore alias environment variables
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
0 siblings, 1 reply; 13+ messages in thread
From: Siddhesh Poyarekar @ 2024-05-01 17:40 UTC (permalink / raw)
To: Adhemerval Zanella, libc-alpha; +Cc: Joe Simmons-Talbott
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"
> }
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 4/4] elf: Make glibc.rtld.enable_secure ignore alias environment variables
2024-05-01 17:40 ` Siddhesh Poyarekar
@ 2024-05-01 18:00 ` Adhemerval Zanella Netto
2024-05-02 11:03 ` Siddhesh Poyarekar
0 siblings, 1 reply; 13+ messages in thread
From: Adhemerval Zanella Netto @ 2024-05-01 18:00 UTC (permalink / raw)
To: Siddhesh Poyarekar, libc-alpha; +Cc: Joe Simmons-Talbott
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"
>> }
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 4/4] elf: Make glibc.rtld.enable_secure ignore alias environment variables
2024-05-01 18:00 ` Adhemerval Zanella Netto
@ 2024-05-02 11:03 ` Siddhesh Poyarekar
0 siblings, 0 replies; 13+ messages in thread
From: Siddhesh Poyarekar @ 2024-05-02 11:03 UTC (permalink / raw)
To: Adhemerval Zanella Netto, libc-alpha; +Cc: Joe Simmons-Talbott, Florian Weimer
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"
>>> }
>
^ permalink raw reply [flat|nested] 13+ messages in thread