Message ID | 20240430192739.1032549-2-adhemerval.zanella@linaro.org |
---|---|
State | New |
Headers | show |
Series | More tunable fixes | expand |
* 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
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.
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
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