* [PATCH v2 1/4] elf: Only process multiple tunable once (BZ 31686)
2024-05-02 16:35 [PATCH v2 0/4] More tunable fixes Adhemerval Zanella
@ 2024-05-02 16:35 ` Adhemerval Zanella
2024-05-02 16:57 ` Joe Simmons-Talbott
2024-05-03 14:59 ` Siddhesh Poyarekar
2024-05-02 16:35 ` [PATCH v2 2/4] elf: Remove glibc.rtld.enable_secure check from parse_tunables_string Adhemerval Zanella
` (2 subsequent siblings)
3 siblings, 2 replies; 10+ messages in thread
From: Adhemerval Zanella @ 2024-05-02 16:35 UTC (permalink / raw)
To: libc-alpha
Cc: Joe Simmons-Talbott, Siddhesh Poyarekar, Yuto Maeda, Yutaro Shimizu
The 680c597e9c3 commit made loader reject ill-formatted strings by
first tracking all set tunables and then applying them. However, it does
not take into consideration if the same tunable is set multiple times,
where parse_tunables_string appends the found tunable without checking
if it was already in the list. It leads to a stack-based buffer overflow
if the tunable is specified more than the total number of tunables. 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>
Reported-by: Yutaro Shimizu <shimizu@cyberdefense.jp>
---
elf/dl-tunables.c | 30 ++++++-----
elf/tst-tunables.c | 58 +++++++++++++++++++++-
sysdeps/aarch64/multiarch/memset_generic.S | 4 ++
sysdeps/sparc/sparc64/rtld-memset.c | 3 ++
4 files changed, 81 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..8f4ee46ad5 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
@@ -327,6 +352,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] 10+ messages in thread* Re: [PATCH v2 1/4] elf: Only process multiple tunable once (BZ 31686)
2024-05-02 16:35 ` [PATCH v2 1/4] elf: Only process multiple tunable once (BZ 31686) Adhemerval Zanella
@ 2024-05-02 16:57 ` Joe Simmons-Talbott
2024-05-03 14:59 ` Siddhesh Poyarekar
1 sibling, 0 replies; 10+ messages in thread
From: Joe Simmons-Talbott @ 2024-05-02 16:57 UTC (permalink / raw)
To: Adhemerval Zanella
Cc: libc-alpha, Siddhesh Poyarekar, Yuto Maeda, Yutaro Shimizu
On Thu, May 2, 2024 at 12:37 PM Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
>
> The 680c597e9c3 commit made loader reject ill-formatted strings by
> first tracking all set tunables and then applying them. However, it does
> not take into consideration if the same tunable is set multiple times,
> where parse_tunables_string appends the found tunable without checking
> if it was already in the list. It leads to a stack-based buffer overflow
> if the tunable is specified more than the total number of tunables. 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>
> Reported-by: Yutaro Shimizu <shimizu@cyberdefense.jp>
> ---
> elf/dl-tunables.c | 30 ++++++-----
> elf/tst-tunables.c | 58 +++++++++++++++++++++-
> sysdeps/aarch64/multiarch/memset_generic.S | 4 ++
> sysdeps/sparc/sparc64/rtld-memset.c | 3 ++
> 4 files changed, 81 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..8f4ee46ad5 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
> @@ -327,6 +352,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
value is different
> + 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
>
--
Joe Simmons-Talbott
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2 1/4] elf: Only process multiple tunable once (BZ 31686)
2024-05-02 16:35 ` [PATCH v2 1/4] elf: Only process multiple tunable once (BZ 31686) Adhemerval Zanella
2024-05-02 16:57 ` Joe Simmons-Talbott
@ 2024-05-03 14:59 ` Siddhesh Poyarekar
1 sibling, 0 replies; 10+ messages in thread
From: Siddhesh Poyarekar @ 2024-05-03 14:59 UTC (permalink / raw)
To: Adhemerval Zanella, libc-alpha
Cc: Joe Simmons-Talbott, Yuto Maeda, Yutaro Shimizu
On 2024-05-02 12:35, Adhemerval Zanella wrote:
> The 680c597e9c3 commit made loader reject ill-formatted strings by
> first tracking all set tunables and then applying them. However, it does
> not take into consideration if the same tunable is set multiple times,
> where parse_tunables_string appends the found tunable without checking
> if it was already in the list. It leads to a stack-based buffer overflow
> if the tunable is specified more than the total number of tunables. 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>
> Reported-by: Yutaro Shimizu <shimizu@cyberdefense.jp>
> ---
LGTM modulo the typo Joe pointed out.
Thanks,
Sid
> elf/dl-tunables.c | 30 ++++++-----
> elf/tst-tunables.c | 58 +++++++++++++++++++++-
> sysdeps/aarch64/multiarch/memset_generic.S | 4 ++
> sysdeps/sparc/sparc64/rtld-memset.c | 3 ++
> 4 files changed, 81 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..8f4ee46ad5 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
> @@ -327,6 +352,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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 2/4] elf: Remove glibc.rtld.enable_secure check from parse_tunables_string
2024-05-02 16:35 [PATCH v2 0/4] More tunable fixes Adhemerval Zanella
2024-05-02 16:35 ` [PATCH v2 1/4] elf: Only process multiple tunable once (BZ 31686) Adhemerval Zanella
@ 2024-05-02 16:35 ` Adhemerval Zanella
2024-05-02 16:35 ` [PATCH v2 3/4] support: Add envp argument to support_capture_subprogram Adhemerval Zanella
2024-05-02 16:35 ` [PATCH v2 4/4] elf: Make glibc.rtld.enable_secure ignore alias environment variables Adhemerval Zanella
3 siblings, 0 replies; 10+ messages in thread
From: Adhemerval Zanella @ 2024-05-02 16:35 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.
Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
---
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] 10+ messages in thread* [PATCH v2 3/4] support: Add envp argument to support_capture_subprogram
2024-05-02 16:35 [PATCH v2 0/4] More tunable fixes Adhemerval Zanella
2024-05-02 16:35 ` [PATCH v2 1/4] elf: Only process multiple tunable once (BZ 31686) Adhemerval Zanella
2024-05-02 16:35 ` [PATCH v2 2/4] elf: Remove glibc.rtld.enable_secure check from parse_tunables_string Adhemerval Zanella
@ 2024-05-02 16:35 ` Adhemerval Zanella
2024-05-03 15:06 ` Siddhesh Poyarekar
2024-05-02 16:35 ` [PATCH v2 4/4] elf: Make glibc.rtld.enable_secure ignore alias environment variables Adhemerval Zanella
3 siblings, 1 reply; 10+ messages in thread
From: Adhemerval Zanella @ 2024-05-02 16:35 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 8f4ee46ad5..a2b3a677e2 100644
--- a/elf/tst-tunables.c
+++ b/elf/tst-tunables.c
@@ -393,7 +393,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..93b7245d2a 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 used. The out.buffer and
+ err.buffer are handle by 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..8274a2b22b 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 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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2 3/4] support: Add envp argument to support_capture_subprogram
2024-05-02 16:35 ` [PATCH v2 3/4] support: Add envp argument to support_capture_subprogram Adhemerval Zanella
@ 2024-05-03 15:06 ` Siddhesh Poyarekar
0 siblings, 0 replies; 10+ messages in thread
From: Siddhesh Poyarekar @ 2024-05-03 15:06 UTC (permalink / raw)
To: Adhemerval Zanella, libc-alpha; +Cc: Joe Simmons-Talbott
On 2024-05-02 12:35, Adhemerval Zanella wrote:
> So tests can specific a list of environment variables.
> ---
The commit typo Joe pointed in v1 still exists ^^ s/specific/specify/.
LGTM otherwise, please fix up the commit message before pushing.
Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
> 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 8f4ee46ad5..a2b3a677e2 100644
> --- a/elf/tst-tunables.c
> +++ b/elf/tst-tunables.c
> @@ -393,7 +393,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..93b7245d2a 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 used. The out.buffer and
> + err.buffer are handle by 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..8274a2b22b 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 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);
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 4/4] elf: Make glibc.rtld.enable_secure ignore alias environment variables
2024-05-02 16:35 [PATCH v2 0/4] More tunable fixes Adhemerval Zanella
` (2 preceding siblings ...)
2024-05-02 16:35 ` [PATCH v2 3/4] support: Add envp argument to support_capture_subprogram Adhemerval Zanella
@ 2024-05-02 16:35 ` Adhemerval Zanella
2024-05-03 15:30 ` Siddhesh Poyarekar
3 siblings, 1 reply; 10+ messages in thread
From: Adhemerval Zanella @ 2024-05-02 16:35 UTC (permalink / raw)
To: libc-alpha; +Cc: Joe Simmons-Talbott, Siddhesh Poyarekar
Tunable with environment variables aliases are also ignored if
glibc.rtld.enable_secure is enabled. The tunable parsing is also
optimized a bit, where the loop that checks each environment variable
only checks for the tunables with aliases instead of all tables.
Checked on aarch64-linux-gnu and x86_64-linux-gnu.
---
elf/dl-tunables.c | 36 ++++++---
elf/tst-tunables-enable_secure.c | 133 +++++++++++++++++++++++++++----
scripts/gen-tunables.awk | 16 +++-
3 files changed, 161 insertions(+), 24 deletions(-)
diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
index 63cf8c7ab5..e87d0628b2 100644
--- a/elf/dl-tunables.c
+++ b/elf/dl-tunables.c
@@ -300,6 +300,9 @@ __tunables_init (char **envp)
if (__libc_enable_secure)
return;
+ enum { tunable_num_env_alias = array_length (tunable_env_alias_list) };
+ struct tunable_toset_t tunables_env_alias[tunable_num_env_alias] = { 0 };
+
while ((envp = get_next_env (envp, &envname, &envval, &prev_envp)) != NULL)
{
/* The environment variable is allocated on the stack by the kernel, so
@@ -311,29 +314,44 @@ __tunables_init (char **envp)
continue;
}
- for (int i = 0; i < tunables_list_size; i++)
+ for (int i = 0; i < tunable_num_env_alias; i++)
{
- tunable_t *cur = &tunable_list[i];
+ tunable_t *cur = &tunable_list[tunable_env_alias_list[i]];
+ const char *name = cur->env_alias;
- /* Skip over tunables that have either been set already or should be
- skipped. */
- if (cur->initialized || cur->env_alias[0] == '\0')
+ if (name[0] == '\0')
continue;
- const char *name = cur->env_alias;
-
- /* We have a match. Initialize and move on to the next line. */
if (tunable_is_name (name, envname))
{
size_t envvallen = 0;
/* The environment variable is always null-terminated. */
for (const char *p = envval; *p != '\0'; p++, envvallen++);
- tunable_initialize (cur, envval, envvallen);
+ tunables_env_alias[i] =
+ (struct tunable_toset_t) { cur, envval, envvallen };
break;
}
}
}
+
+ /* Check if glibc.rtld.enable_secure was set and skip over the environment
+ variables aliases. */
+ if (__libc_enable_secure)
+ return;
+
+ for (int i = 0; i < tunable_num_env_alias; i++)
+ {
+ /* Skip over tunables that have either been set. */
+ 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..a0da0fd179 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;
}
@@ -106,14 +194,31 @@ do_test (int argc, char *argv[])
spargv[i] = NULL;
}
+ enum { tunable_num_env_alias = array_length (tunable_env_alias_list) };
+
for (int i = 0; i < array_length (tests); i++)
{
snprintf (nteststr, sizeof nteststr, "%d", i);
printf ("[%d] Spawned test for %s\n", i, tests[i].env);
setenv ("GLIBC_TUNABLES", tests[i].env, 1);
+
+ char *envp[2 + tunable_num_env_alias + 1] =
+ {
+ (char *) tests[i].env,
+ (char *) tests[i].extraenv,
+ NULL,
+ };
+ if (tests[i].check_multiple)
+ {
+ int j;
+ for (j=0; j < tunable_num_env_alias; j++)
+ envp[j + 2] = (char *) tests[i].extraenv;
+ envp[j + 2] = NULL;
+ }
+
struct support_capture_subprocess result
- = support_capture_subprogram (spargv[0], spargv, NULL);
+ = support_capture_subprogram (spargv[0], spargv, envp);
support_capture_subprocess_check (&result, "tst-tunables-enable_secure",
0, sc_allow_stderr);
support_capture_subprocess_free (&result);
diff --git a/scripts/gen-tunables.awk b/scripts/gen-tunables.awk
index 9f5336381e..fc3b41376f 100644
--- a/scripts/gen-tunables.awk
+++ b/scripts/gen-tunables.awk
@@ -156,7 +156,7 @@ END {
print "# define TUNABLE_ALIAS_MAX " (max_alias_len + 1)
print "# include \"dl-tunable-types.h\""
# Finally, the tunable list.
- print "static tunable_t tunable_list[] attribute_relro = {"
+ print "static tunable_t tunable_list[] attribute_relro __attribute_used__ = {"
for (tnm in types) {
split (tnm, indices, SUBSEP);
t = indices[1];
@@ -168,5 +168,19 @@ END {
default_val[t,n,m], env_alias[t,n,m]);
}
print "};"
+
+ # Map of tunable with environment variables aliases used during parsing. */
+ print "\nstatic const tunable_id_t tunable_env_alias_list[] ="
+ printf "{\n"
+ for (tnm in types) {
+ split (tnm, indices, SUBSEP);
+ t = indices[1];
+ n = indices[2];
+ m = indices[3];
+ if (env_alias[t,n,m] != "{0}") {
+ printf (" TUNABLE_ENUM_NAME(%s, %s, %s),\n", t, n, m);
+ }
+ }
+ printf "};\n"
print "#endif"
}
--
2.43.0
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2 4/4] elf: Make glibc.rtld.enable_secure ignore alias environment variables
2024-05-02 16:35 ` [PATCH v2 4/4] elf: Make glibc.rtld.enable_secure ignore alias environment variables Adhemerval Zanella
@ 2024-05-03 15:30 ` Siddhesh Poyarekar
2024-05-06 14:10 ` Adhemerval Zanella Netto
0 siblings, 1 reply; 10+ messages in thread
From: Siddhesh Poyarekar @ 2024-05-03 15:30 UTC (permalink / raw)
To: Adhemerval Zanella, libc-alpha; +Cc: Joe Simmons-Talbott
On 2024-05-02 12:35, Adhemerval Zanella wrote:
> Tunable with environment variables aliases are also ignored if
> glibc.rtld.enable_secure is enabled. The tunable parsing is also
> optimized a bit, where the loop that checks each environment variable
> only checks for the tunables with aliases instead of all tables.
>
> Checked on aarch64-linux-gnu and x86_64-linux-gnu.
> ---
> elf/dl-tunables.c | 36 ++++++---
> elf/tst-tunables-enable_secure.c | 133 +++++++++++++++++++++++++++----
> scripts/gen-tunables.awk | 16 +++-
> 3 files changed, 161 insertions(+), 24 deletions(-)
>
> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
> index 63cf8c7ab5..e87d0628b2 100644
> --- a/elf/dl-tunables.c
> +++ b/elf/dl-tunables.c
> @@ -300,6 +300,9 @@ __tunables_init (char **envp)
> if (__libc_enable_secure)
> return;
>
> + enum { tunable_num_env_alias = array_length (tunable_env_alias_list) };
> + struct tunable_toset_t tunables_env_alias[tunable_num_env_alias] = { 0 };
> +
> while ((envp = get_next_env (envp, &envname, &envval, &prev_envp)) != NULL)
> {
> /* The environment variable is allocated on the stack by the kernel, so
> @@ -311,29 +314,44 @@ __tunables_init (char **envp)
> continue;
> }
>
> - for (int i = 0; i < tunables_list_size; i++)
> + for (int i = 0; i < tunable_num_env_alias; i++)
> {
> - tunable_t *cur = &tunable_list[i];
> + tunable_t *cur = &tunable_list[tunable_env_alias_list[i]];
> + const char *name = cur->env_alias;
>
> - /* Skip over tunables that have either been set already or should be
> - skipped. */
> - if (cur->initialized || cur->env_alias[0] == '\0')
> + if (name[0] == '\0')
> continue;
You don't skip over initialized ones here because you're going to have
to check for that later anyway, in case GLIBC_TUNABLES came in after any
of the aliases. OK.
>
> - const char *name = cur->env_alias;
> -
> - /* We have a match. Initialize and move on to the next line. */
> if (tunable_is_name (name, envname))
> {
> size_t envvallen = 0;
> /* The environment variable is always null-terminated. */
> for (const char *p = envval; *p != '\0'; p++, envvallen++);
>
> - tunable_initialize (cur, envval, envvallen);
> + tunables_env_alias[i] =
> + (struct tunable_toset_t) { cur, envval, envvallen };
> break;
> }
> }
> }
> +
> + /* Check if glibc.rtld.enable_secure was set and skip over the environment
> + variables aliases. */
> + if (__libc_enable_secure)
> + return;
> +
> + for (int i = 0; i < tunable_num_env_alias; i++)
> + {
> + /* Skip over tunables that have either been set. */
either been set or already initialized?
> + if (tunables_env_alias[i].t == NULL
> + || tunables_env_alias[i].t->initialized)
> + continue;
> +
> + if (!tunable_initialize (tunables_env_alias[i].t,
> + tunables_env_alias[i].value,
> + tunables_env_alias[i].len))
> + parse_tunable_print_error (&tunables_env_alias[i]);
> + }
> }
>
> void
> diff --git a/elf/tst-tunables-enable_secure.c b/elf/tst-tunables-enable_secure.c
> index f5db1c84e9..a0da0fd179 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
OK, I noticed this in patch 1 but it didn't strike me then; this macro
will result in a new array tunable_list being initialized in the binary.
The tunable references themselves should go via __tunable_get_val and
hence refer to the structure in ld.so, so this array only serves as a
copy to get the total size. I suppose it's not much for a test case,
but do you think it's worth adding a comment here, and also in
tst-tunables.c? That way it could be easier to weed out any unforeseen
problems with these tests in future.
> #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",
Same question as in v1, should we issue a warning for enable_secure=0?
We really should never be able to set it to anything other than 1.
Which also means that a bunch of these enable_secure=0 tests are
probably redundant.
> + NULL,
> + false,
> 2,
> 0,
> },
> + /* Tunables should be ignored if enable_secure is set. */
> + {
> + "GLIBC_TUNABLES=glibc.malloc.check=2:glibc.rtld.enable_secure=1",
> + "MALLOC_CHECK_=2",
> + false,
> + 0,
> + 1,
> + },
> + /* Same as before, but with enviroment alias prior GLIBC_TUNABLES. */
> + {
> + "MALLOC_CHECK_=2",
> + "GLIBC_TUNABLES=glibc.malloc.check=2:glibc.rtld.enable_secure=1",
> + false,
> + 0,
> + 1,
> + },
> + /* Tunables should be ignored if enable_secure is set. */
> + {
> + "GLIBC_TUNABLES=glibc.rtld.enable_secure=1:glibc.malloc.check=2",
> + "MALLOC_CHECK_=2",
> + false,
> + 0,
> + 1,
> + },
> + {
> + "MALLOC_CHECK_=2",
> + "GLIBC_TUNABLES=glibc.rtld.enable_secure=1:glibc.malloc.check=2",
> + false,
> + 0,
> + 1,
> + },
> + /* Tunables should be set if enable_secure is unset. */
> + {
> + "GLIBC_TUNABLES=glibc.rtld.enable_secure=0:glibc.malloc.check=2",
> + /* Tunable have precedence over the environment variable. */
> + "MALLOC_CHECK_=1",
> + false,
> + 2,
> + 0,
> + },
> + {
> + "MALLOC_CHECK_=1",
> + "GLIBC_TUNABLES=glibc.rtld.enable_secure=0:glibc.malloc.check=2",
> + /* Tunable have precedence over the environment variable. */
> + false,
> + 2,
> + 0,
> + },
> + /* Tunables should be set if enable_secure is unset. */
> + {
> + "GLIBC_TUNABLES=glibc.rtld.enable_secure=0",
> + /* Tunable have precedence over the environment variable. */
> + "MALLOC_CHECK_=1",
> + false,
> + 1,
> + 0,
> + },
> + /* Tunables should be set if enable_secure is unset. */
> + {
> + "GLIBC_TUNABLES=glibc.rtld.enable_secure=0",
> + /* Tunable have precedence over the environment variable. */
> + "MALLOC_CHECK_=1",
> + false,
> + 1,
> + 0,
> + },
> + /* Check with tunables environment variable alias set multiple times. */
> + {
> + "GLIBC_TUNABLES=glibc.rtld.enable_secure=1:glibc.malloc.check=2",
> + "MALLOC_CHECK_=2",
> + true,
> + 0,
> + 1,
> + },
> + /* Tunables should be set if enable_secure is unset. */
> + {
> + "GLIBC_TUNABLES=glibc.rtld.enable_secure=0",
> + /* Tunable have precedence over the environment variable. */
> + "MALLOC_CHECK_=1",
> + true,
> + 1,
> + 0,
> + },
> };
>
> static int
> handle_restart (int i)
> {
> if (tests[i].expected_enable_secure == 1)
> - {
> - TEST_COMPARE (1, __libc_enable_secure);
> - }
> + TEST_COMPARE (1, __libc_enable_secure);
> else
> - {
> - TEST_COMPARE (tests[i].expected_malloc_check,
> - TUNABLE_GET_FULL (glibc, malloc, check, int32_t, NULL));
> - TEST_COMPARE (tests[i].expected_enable_secure,
> - TUNABLE_GET_FULL (glibc, rtld, enable_secure, int32_t,
> - NULL));
> - }
> + TEST_COMPARE (tests[i].expected_enable_secure,
> + TUNABLE_GET_FULL (glibc, rtld, enable_secure, int32_t,
> + NULL));
> + TEST_COMPARE (tests[i].expected_malloc_check,
> + TUNABLE_GET_FULL (glibc, malloc, check, int32_t, NULL));
> return 0;
> }
>
> @@ -106,14 +194,31 @@ do_test (int argc, char *argv[])
> spargv[i] = NULL;
> }
>
> + enum { tunable_num_env_alias = array_length (tunable_env_alias_list) };
> +
> for (int i = 0; i < array_length (tests); i++)
> {
> snprintf (nteststr, sizeof nteststr, "%d", i);
>
> printf ("[%d] Spawned test for %s\n", i, tests[i].env);
> setenv ("GLIBC_TUNABLES", tests[i].env, 1);
> +
> + char *envp[2 + tunable_num_env_alias + 1] =
> + {
> + (char *) tests[i].env,
> + (char *) tests[i].extraenv,
> + NULL,
> + };
> + if (tests[i].check_multiple)
> + {
> + int j;
> + for (j=0; j < tunable_num_env_alias; j++)
> + envp[j + 2] = (char *) tests[i].extraenv;
> + envp[j + 2] = NULL;
> + }
> +
> struct support_capture_subprocess result
> - = support_capture_subprogram (spargv[0], spargv, NULL);
> + = support_capture_subprogram (spargv[0], spargv, envp);
> support_capture_subprocess_check (&result, "tst-tunables-enable_secure",
> 0, sc_allow_stderr);
> support_capture_subprocess_free (&result);
> diff --git a/scripts/gen-tunables.awk b/scripts/gen-tunables.awk
> index 9f5336381e..fc3b41376f 100644
> --- a/scripts/gen-tunables.awk
> +++ b/scripts/gen-tunables.awk
> @@ -156,7 +156,7 @@ END {
> print "# define TUNABLE_ALIAS_MAX " (max_alias_len + 1)
> print "# include \"dl-tunable-types.h\""
> # Finally, the tunable list.
> - print "static tunable_t tunable_list[] attribute_relro = {"
> + print "static tunable_t tunable_list[] attribute_relro __attribute_used__ = {"
Why is this needed? Doesn't array_length ensures at least one "use"?
> for (tnm in types) {
> split (tnm, indices, SUBSEP);
> t = indices[1];
> @@ -168,5 +168,19 @@ END {
> default_val[t,n,m], env_alias[t,n,m]);
> }
> print "};"
> +
> + # Map of tunable with environment variables aliases used during parsing. */
> + print "\nstatic const tunable_id_t tunable_env_alias_list[] ="
> + printf "{\n"
> + for (tnm in types) {
> + split (tnm, indices, SUBSEP);
> + t = indices[1];
> + n = indices[2];
> + m = indices[3];
> + if (env_alias[t,n,m] != "{0}") {
> + printf (" TUNABLE_ENUM_NAME(%s, %s, %s),\n", t, n, m);
> + }
> + }
> + printf "};\n"
> print "#endif"
> }
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2 4/4] elf: Make glibc.rtld.enable_secure ignore alias environment variables
2024-05-03 15:30 ` Siddhesh Poyarekar
@ 2024-05-06 14:10 ` Adhemerval Zanella Netto
0 siblings, 0 replies; 10+ messages in thread
From: Adhemerval Zanella Netto @ 2024-05-06 14:10 UTC (permalink / raw)
To: Siddhesh Poyarekar, libc-alpha; +Cc: Joe Simmons-Talbott
On 03/05/24 12:30, Siddhesh Poyarekar wrote:
> On 2024-05-02 12:35, Adhemerval Zanella wrote:
>> Tunable with environment variables aliases are also ignored if
>> glibc.rtld.enable_secure is enabled. The tunable parsing is also
>> optimized a bit, where the loop that checks each environment variable
>> only checks for the tunables with aliases instead of all tables.
>>
>> Checked on aarch64-linux-gnu and x86_64-linux-gnu.
>> ---
>> elf/dl-tunables.c | 36 ++++++---
>> elf/tst-tunables-enable_secure.c | 133 +++++++++++++++++++++++++++----
>> scripts/gen-tunables.awk | 16 +++-
>> 3 files changed, 161 insertions(+), 24 deletions(-)
>>
>> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
>> index 63cf8c7ab5..e87d0628b2 100644
>> --- a/elf/dl-tunables.c
>> +++ b/elf/dl-tunables.c
>> @@ -300,6 +300,9 @@ __tunables_init (char **envp)
>> if (__libc_enable_secure)
>> return;
>> + enum { tunable_num_env_alias = array_length (tunable_env_alias_list) };
>> + struct tunable_toset_t tunables_env_alias[tunable_num_env_alias] = { 0 };
>> +
>> while ((envp = get_next_env (envp, &envname, &envval, &prev_envp)) != NULL)
>> {
>> /* The environment variable is allocated on the stack by the kernel, so
>> @@ -311,29 +314,44 @@ __tunables_init (char **envp)
>> continue;
>> }
>> - for (int i = 0; i < tunables_list_size; i++)
>> + for (int i = 0; i < tunable_num_env_alias; i++)
>> {
>> - tunable_t *cur = &tunable_list[i];
>> + tunable_t *cur = &tunable_list[tunable_env_alias_list[i]];
>> + const char *name = cur->env_alias;
>> - /* Skip over tunables that have either been set already or should be
>> - skipped. */
>> - if (cur->initialized || cur->env_alias[0] == '\0')
>> + if (name[0] == '\0')
>> continue;
>
> You don't skip over initialized ones here because you're going to have to check for that later anyway, in case GLIBC_TUNABLES came in after any of the aliases. OK.
>
>> - const char *name = cur->env_alias;
>> -
>> - /* We have a match. Initialize and move on to the next line. */
>> if (tunable_is_name (name, envname))
>> {
>> size_t envvallen = 0;
>> /* The environment variable is always null-terminated. */
>> for (const char *p = envval; *p != '\0'; p++, envvallen++);
>> - tunable_initialize (cur, envval, envvallen);
>> + tunables_env_alias[i] =
>> + (struct tunable_toset_t) { cur, envval, envvallen };
>> break;
>> }
>> }
>> }
>> +
>> + /* Check if glibc.rtld.enable_secure was set and skip over the environment
>> + variables aliases. */
>> + if (__libc_enable_secure)
>> + return;
>> +
>> + for (int i = 0; i < tunable_num_env_alias; i++)
>> + {
>> + /* Skip over tunables that have either been set. */
>
> either been set or already initialized?
Ack.
>
>> + 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..a0da0fd179 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
>
> OK, I noticed this in patch 1 but it didn't strike me then; this macro will result in a new array tunable_list being initialized in the binary. The tunable references themselves should go via __tunable_get_val and hence refer to the structure in ld.so, so this array only serves as a copy to get the total size. I suppose it's not much for a test case, but do you think it's worth adding a comment here, and also in tst-tunables.c? That way it could be easier to weed out any unforeseen problems with these tests in future.
Ok, I will add a comment why this is required.
>
>> #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",
>
> Same question as in v1, should we issue a warning for enable_secure=0? We really should never be able to set it to anything other than 1. Which also means that a bunch of these enable_secure=0 tests are probably redundant.
I am not sure if adding another knob for an specific tunable, specially one that
is only aimed for debug, would yield much gain here. The enable_secure=0 is just
the default value, similar to setting any other tunable to its default value.
>
>> + NULL,
>> + false,
>> 2,
>> 0,
>> },
>> + /* Tunables should be ignored if enable_secure is set. */
>> + {
>> + "GLIBC_TUNABLES=glibc.malloc.check=2:glibc.rtld.enable_secure=1",
>> + "MALLOC_CHECK_=2",
>> + false,
>> + 0,
>> + 1,
>> + },
>> + /* Same as before, but with enviroment alias prior GLIBC_TUNABLES. */
>> + {
>> + "MALLOC_CHECK_=2",
>> + "GLIBC_TUNABLES=glibc.malloc.check=2:glibc.rtld.enable_secure=1",
>> + false,
>> + 0,
>> + 1,
>> + },
>> + /* Tunables should be ignored if enable_secure is set. */
>> + {
>> + "GLIBC_TUNABLES=glibc.rtld.enable_secure=1:glibc.malloc.check=2",
>> + "MALLOC_CHECK_=2",
>> + false,
>> + 0,
>> + 1,
>> + },
>> + {
>> + "MALLOC_CHECK_=2",
>> + "GLIBC_TUNABLES=glibc.rtld.enable_secure=1:glibc.malloc.check=2",
>> + false,
>> + 0,
>> + 1,
>> + },
>> + /* Tunables should be set if enable_secure is unset. */
>> + {
>> + "GLIBC_TUNABLES=glibc.rtld.enable_secure=0:glibc.malloc.check=2",
>> + /* Tunable have precedence over the environment variable. */
>> + "MALLOC_CHECK_=1",
>> + false,
>> + 2,
>> + 0,
>> + },
>> + {
>> + "MALLOC_CHECK_=1",
>> + "GLIBC_TUNABLES=glibc.rtld.enable_secure=0:glibc.malloc.check=2",
>> + /* Tunable have precedence over the environment variable. */
>> + false,
>> + 2,
>> + 0,
>> + },
>> + /* Tunables should be set if enable_secure is unset. */
>> + {
>> + "GLIBC_TUNABLES=glibc.rtld.enable_secure=0",
>> + /* Tunable have precedence over the environment variable. */
>> + "MALLOC_CHECK_=1",
>> + false,
>> + 1,
>> + 0,
>> + },
>> + /* Tunables should be set if enable_secure is unset. */
>> + {
>> + "GLIBC_TUNABLES=glibc.rtld.enable_secure=0",
>> + /* Tunable have precedence over the environment variable. */
>> + "MALLOC_CHECK_=1",
>> + false,
>> + 1,
>> + 0,
>> + },
>> + /* Check with tunables environment variable alias set multiple times. */
>> + {
>> + "GLIBC_TUNABLES=glibc.rtld.enable_secure=1:glibc.malloc.check=2",
>> + "MALLOC_CHECK_=2",
>> + true,
>> + 0,
>> + 1,
>> + },
>> + /* Tunables should be set if enable_secure is unset. */
>> + {
>> + "GLIBC_TUNABLES=glibc.rtld.enable_secure=0",
>> + /* Tunable have precedence over the environment variable. */
>> + "MALLOC_CHECK_=1",
>> + true,
>> + 1,
>> + 0,
>> + },
>> };
>> static int
>> handle_restart (int i)
>> {
>> if (tests[i].expected_enable_secure == 1)
>> - {
>> - TEST_COMPARE (1, __libc_enable_secure);
>> - }
>> + TEST_COMPARE (1, __libc_enable_secure);
>> else
>> - {
>> - TEST_COMPARE (tests[i].expected_malloc_check,
>> - TUNABLE_GET_FULL (glibc, malloc, check, int32_t, NULL));
>> - TEST_COMPARE (tests[i].expected_enable_secure,
>> - TUNABLE_GET_FULL (glibc, rtld, enable_secure, int32_t,
>> - NULL));
>> - }
>> + TEST_COMPARE (tests[i].expected_enable_secure,
>> + TUNABLE_GET_FULL (glibc, rtld, enable_secure, int32_t,
>> + NULL));
>> + TEST_COMPARE (tests[i].expected_malloc_check,
>> + TUNABLE_GET_FULL (glibc, malloc, check, int32_t, NULL));
>> return 0;
>> }
>> @@ -106,14 +194,31 @@ do_test (int argc, char *argv[])
>> spargv[i] = NULL;
>> }
>> + enum { tunable_num_env_alias = array_length (tunable_env_alias_list) };
>> +
>> for (int i = 0; i < array_length (tests); i++)
>> {
>> snprintf (nteststr, sizeof nteststr, "%d", i);
>> printf ("[%d] Spawned test for %s\n", i, tests[i].env);
>> setenv ("GLIBC_TUNABLES", tests[i].env, 1);
>> +
>> + char *envp[2 + tunable_num_env_alias + 1] =
>> + {
>> + (char *) tests[i].env,
>> + (char *) tests[i].extraenv,
>> + NULL,
>> + };
>> + if (tests[i].check_multiple)
>> + {
>> + int j;
>> + for (j=0; j < tunable_num_env_alias; j++)
>> + envp[j + 2] = (char *) tests[i].extraenv;
>> + envp[j + 2] = NULL;
>> + }
>> +
>> struct support_capture_subprocess result
>> - = support_capture_subprogram (spargv[0], spargv, NULL);
>> + = support_capture_subprogram (spargv[0], spargv, envp);
>> support_capture_subprocess_check (&result, "tst-tunables-enable_secure",
>> 0, sc_allow_stderr);
>> support_capture_subprocess_free (&result);
>> diff --git a/scripts/gen-tunables.awk b/scripts/gen-tunables.awk
>> index 9f5336381e..fc3b41376f 100644
>> --- a/scripts/gen-tunables.awk
>> +++ b/scripts/gen-tunables.awk
>> @@ -156,7 +156,7 @@ END {
>> print "# define TUNABLE_ALIAS_MAX " (max_alias_len + 1)
>> print "# include \"dl-tunable-types.h\""
>> # Finally, the tunable list.
>> - print "static tunable_t tunable_list[] attribute_relro = {"
>> + print "static tunable_t tunable_list[] attribute_relro __attribute_used__ = {"
>
> Why is this needed? Doesn't array_length ensures at least one "use"?
The array_length is used for only for tunable_env_alias_list on
elf/tst-tunables-enable_secure.c.
>
>> for (tnm in types) {
>> split (tnm, indices, SUBSEP);
>> t = indices[1];
>> @@ -168,5 +168,19 @@ END {
>> default_val[t,n,m], env_alias[t,n,m]);
>> }
>> print "};"
>> +
>> + # Map of tunable with environment variables aliases used during parsing. */
>> + print "\nstatic const tunable_id_t tunable_env_alias_list[] ="
>> + printf "{\n"
>> + for (tnm in types) {
>> + split (tnm, indices, SUBSEP);
>> + t = indices[1];
>> + n = indices[2];
>> + m = indices[3];
>> + if (env_alias[t,n,m] != "{0}") {
>> + printf (" TUNABLE_ENUM_NAME(%s, %s, %s),\n", t, n, m);
>> + }
>> + }
>> + printf "};\n"
>> print "#endif"
>> }
^ permalink raw reply [flat|nested] 10+ messages in thread