[glibc/release/2.39/master] elf: Only process multiple tunable once (BZ 31686)

[glibc/release/2.39/master] elf: Only process multiple tunable once (BZ 31686)

[Adhemerval Zanella]

https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=71149c2a2e85a8233631cc816030d449f021bb2a

commit 71149c2a2e85a8233631cc816030d449f021bb2a
Author: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Date:   Mon May 6 13:18:45 2024 -0300

    elf: Only process multiple tunable once (BZ 31686)
    
    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>
    Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
    (cherry picked from commit bcae44ea8536b30a7119c0986ff5692bddacb672)

Diff:
---
 elf/dl-tunables.c                          | 28 ++++++++------
 elf/tst-tunables.c                         | 61 +++++++++++++++++++++++++++++-
 sysdeps/aarch64/multiarch/memset_generic.S |  4 ++
 sysdeps/sparc/sparc64/rtld-memset.c        |  3 ++
 4 files changed, 84 insertions(+), 12 deletions(-)

diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
index 03e1a68675..614ac9c047 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"
@@ -223,6 +224,7 @@ parse_tunables_string (const char *valstring, struct tunable_toset_t *tunables)
 	    {
 	      tunables[ntunables++] =
 		(struct tunable_toset_t) { cur, value, p - value };
+
 	      break;
 	    }
 	}
@@ -234,23 +236,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..dff34ed748 100644
--- a/elf/tst-tunables.c
+++ b/elf/tst-tunables.c
@@ -17,6 +17,10 @@
    <https://www.gnu.org/licenses/>.  */
 
 #include <array_length.h>
+/* The test uses the tunable_list size, which is only exported for
+   ld.so.  This will result in a copy of tunable_list, which is ununsed by
+   the test itself.  */
+#define TUNABLES_INTERNAL 1
 #include <dl-tunables.h>
 #include <getopt.h>
 #include <intprops.h>
@@ -24,12 +28,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 +289,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 +355,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 values 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