diff mbox series

[v5,2/5] elf: Do not set invalid tunables values

Message ID 20231122204325.4058222-3-adhemerval.zanella@linaro.org
State Superseded
Headers
Series Improve loader environment variable handling |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch success Patch applied to master at the time it was sent
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_glibc_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_glibc_check--master-arm success Testing passed

Commit Message

Adhemerval Zanella Netto Nov. 22, 2023, 8:43 p.m. UTC 
  The loader now warns for invalid and out-of-range tunable values. The
patch also fixes the parsing of size_t maximum values, where
_dl_strtoul was failing for large values close to SIZE_MAX.

Checked on x86_64-linux-gnu.
---
 elf/dl-misc.c      |  5 ++++-
 elf/dl-tunables.c  | 35 ++++++++++++++++++++++++++++++-----
 elf/tst-tunables.c | 30 ++++++++++++++++++++++++++++++
 3 files changed, 64 insertions(+), 6 deletions(-)

Comments

Siddhesh Poyarekar Dec. 1, 2023, 3:32 p.m. UTC | #1 
On 2023-11-22 15:43, Adhemerval Zanella wrote:
> The loader now warns for invalid and out-of-range tunable values. The
> patch also fixes the parsing of size_t maximum values, where
> _dl_strtoul was failing for large values close to SIZE_MAX.
> 
> Checked on x86_64-linux-gnu.
> ---
>   elf/dl-misc.c      |  5 ++++-
>   elf/dl-tunables.c  | 35 ++++++++++++++++++++++++++++++-----
>   elf/tst-tunables.c | 30 ++++++++++++++++++++++++++++++
>   3 files changed, 64 insertions(+), 6 deletions(-)
> 
> diff --git a/elf/dl-misc.c b/elf/dl-misc.c
> index 5b84adc2f4..037cbb3650 100644
> --- a/elf/dl-misc.c
> +++ b/elf/dl-misc.c
> @@ -190,6 +190,9 @@ _dl_strtoul (const char *nptr, char **endptr)
>   	}
>       }
>   
> +  const uint64_t cutoff = (UINT64_MAX * 2UL + 1UL) / 10;
> +  const uint64_t cutlim = (UINT64_MAX * 2UL + 1UL) % 10;
> +
>     while (1)
>       {
>         int digval;
> @@ -207,7 +210,7 @@ _dl_strtoul (const char *nptr, char **endptr)
>         else
>           break;
>   
> -      if (result >= (UINT64_MAX - digval) / base)
> +      if (result > cutoff || (result == cutoff && digval > cutlim))

I don't understand this change; how does this work with octal or 
hexadecimal inputs?

>   	{
>   	  if (endptr != NULL)
>   	    *endptr = (char *) nptr;
> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
> index 26161c68e7..67a37ff704 100644
> --- a/elf/dl-tunables.c
> +++ b/elf/dl-tunables.c
> @@ -77,16 +77,27 @@ do_tunable_update_val (tunable_t *cur, const tunable_val_t *valp,
>   {
>     tunable_num_t val, min, max;
>   
> -  if (cur->type.type_code == TUNABLE_TYPE_STRING)
> +  switch (cur->type.type_code)
>       {
> +    case TUNABLE_TYPE_STRING:
>         cur->val.strval = valp->strval;
>         cur->initialized = true;
>         return;
> +    case TUNABLE_TYPE_INT_32:
> +      val = (int32_t) valp->numval;
> +      break;
> +    case TUNABLE_TYPE_UINT_64:
> +      val = (int64_t) valp->numval;
> +      break;
> +    case TUNABLE_TYPE_SIZE_T:
> +      val = (size_t) valp->numval;
> +      break;
> +    default:
> +      __builtin_unreachable ();
>       }
>   
>     bool unsigned_cmp = unsigned_tunable_type (cur->type.type_code);
>   
> -  val = valp->numval;
>     min = minp != NULL ? *minp : cur->type.min;
>     max = maxp != NULL ? *maxp : cur->type.max;
>   
> @@ -117,16 +128,24 @@ do_tunable_update_val (tunable_t *cur, const tunable_val_t *valp,
>   
>   /* Validate range of the input value and initialize the tunable CUR if it looks
>      good.  */
> -static void
> +static bool
>   tunable_initialize (tunable_t *cur, const char *strval, size_t len)
>   {
>     tunable_val_t val = { 0 };
>   
>     if (cur->type.type_code != TUNABLE_TYPE_STRING)
> -    val.numval = (tunable_num_t) _dl_strtoul (strval, NULL);
> +    {
> +      char *endptr = NULL;
> +      uint64_t numval = _dl_strtoul (strval, &endptr);
> +      if (endptr != strval + len)
> +	return false;
> +      val.numval = (tunable_num_t) numval;
> +    }
>     else
>       val.strval = (struct tunable_str_t) { strval, len };
>     do_tunable_update_val (cur, &val, NULL, NULL);
> +
> +  return true;
>   }
>   
>   void
> @@ -226,7 +245,13 @@ parse_tunables (const char *valstring)
>       }
>   
>     for (int i = 0; i < ntunables; i++)
> -    tunable_initialize (tunables[i].t, tunables[i].value, tunables[i].len);
> +    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 188345b070..d6a1e1b3ac 100644
> --- a/elf/tst-tunables.c
> +++ b/elf/tst-tunables.c
> @@ -53,6 +53,13 @@ static const struct test_t
>       4096,
>       0,
>     },
> +  {
> +    "GLIBC_TUNABLES",
> +    "glibc.malloc.mmap_threshold=-1",
> +    0,
> +    SIZE_MAX,
> +    0,
> +  },
>     /* Empty tunable are ignored.  */
>     {
>       "GLIBC_TUNABLES",
> @@ -224,6 +231,29 @@ static const struct test_t
>       0,
>       0,
>     },
> +  /* Invalid numbers are ignored.  */
> +  {
> +    "GLIBC_TUNABLES",
> +    "glibc.malloc.check=abc:glibc.malloc.mmap_threshold=4096",
> +    0,
> +    4096,
> +    0,
> +  },
> +  {
> +    "GLIBC_TUNABLES",
> +    "glibc.malloc.check=2:glibc.malloc.mmap_threshold=abc",
> +    2,
> +    0,
> +    0,
> +  },
> +  {
> +    "GLIBC_TUNABLES",
> +    /* SIZE_MAX + 1 */
> +    "glibc.malloc.mmap_threshold=18446744073709551616",
> +    0,
> +    0,
> +    0,
> +  },
>     /* Also check some tunable aliases.  */
>     {
>       "MALLOC_CHECK_",
Adhemerval Zanella Netto Dec. 6, 2023, 1:06 p.m. UTC | #2 
On 01/12/23 12:32, Siddhesh Poyarekar wrote:
> 
> 
> On 2023-11-22 15:43, Adhemerval Zanella wrote:
>> The loader now warns for invalid and out-of-range tunable values. The
>> patch also fixes the parsing of size_t maximum values, where
>> _dl_strtoul was failing for large values close to SIZE_MAX.
>>
>> Checked on x86_64-linux-gnu.
>> ---
>>   elf/dl-misc.c      |  5 ++++-
>>   elf/dl-tunables.c  | 35 ++++++++++++++++++++++++++++++-----
>>   elf/tst-tunables.c | 30 ++++++++++++++++++++++++++++++
>>   3 files changed, 64 insertions(+), 6 deletions(-)
>>
>> diff --git a/elf/dl-misc.c b/elf/dl-misc.c
>> index 5b84adc2f4..037cbb3650 100644
>> --- a/elf/dl-misc.c
>> +++ b/elf/dl-misc.c
>> @@ -190,6 +190,9 @@ _dl_strtoul (const char *nptr, char **endptr)
>>       }
>>       }
>>   +  const uint64_t cutoff = (UINT64_MAX * 2UL + 1UL) / 10;
>> +  const uint64_t cutlim = (UINT64_MAX * 2UL + 1UL) % 10;
>> +
>>     while (1)
>>       {
>>         int digval;
>> @@ -207,7 +210,7 @@ _dl_strtoul (const char *nptr, char **endptr)
>>         else
>>           break;
>>   -      if (result >= (UINT64_MAX - digval) / base)
>> +      if (result > cutoff || (result == cutoff && digval > cutlim))
> 
> I don't understand this change; how does this work with octal or hexadecimal inputs?

In fact the cutoff/cutlim should be adjusted when a different base is used,
I will fix it.  The logic here is similar to the stdlib/strtol_l.c:486.

> 
>>       {
>>         if (endptr != NULL)
>>           *endptr = (char *) nptr;
>> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
>> index 26161c68e7..67a37ff704 100644
>> --- a/elf/dl-tunables.c
>> +++ b/elf/dl-tunables.c
>> @@ -77,16 +77,27 @@ do_tunable_update_val (tunable_t *cur, const tunable_val_t *valp,
>>   {
>>     tunable_num_t val, min, max;
>>   -  if (cur->type.type_code == TUNABLE_TYPE_STRING)
>> +  switch (cur->type.type_code)
>>       {
>> +    case TUNABLE_TYPE_STRING:
>>         cur->val.strval = valp->strval;
>>         cur->initialized = true;
>>         return;
>> +    case TUNABLE_TYPE_INT_32:
>> +      val = (int32_t) valp->numval;
>> +      break;
>> +    case TUNABLE_TYPE_UINT_64:
>> +      val = (int64_t) valp->numval;
>> +      break;
>> +    case TUNABLE_TYPE_SIZE_T:
>> +      val = (size_t) valp->numval;
>> +      break;
>> +    default:
>> +      __builtin_unreachable ();
>>       }
>>       bool unsigned_cmp = unsigned_tunable_type (cur->type.type_code);
>>   -  val = valp->numval;
>>     min = minp != NULL ? *minp : cur->type.min;
>>     max = maxp != NULL ? *maxp : cur->type.max;
>>   @@ -117,16 +128,24 @@ do_tunable_update_val (tunable_t *cur, const tunable_val_t *valp,
>>     /* Validate range of the input value and initialize the tunable CUR if it looks
>>      good.  */
>> -static void
>> +static bool
>>   tunable_initialize (tunable_t *cur, const char *strval, size_t len)
>>   {
>>     tunable_val_t val = { 0 };
>>       if (cur->type.type_code != TUNABLE_TYPE_STRING)
>> -    val.numval = (tunable_num_t) _dl_strtoul (strval, NULL);
>> +    {
>> +      char *endptr = NULL;
>> +      uint64_t numval = _dl_strtoul (strval, &endptr);
>> +      if (endptr != strval + len)
>> +    return false;
>> +      val.numval = (tunable_num_t) numval;
>> +    }
>>     else
>>       val.strval = (struct tunable_str_t) { strval, len };
>>     do_tunable_update_val (cur, &val, NULL, NULL);
>> +
>> +  return true;
>>   }
>>     void
>> @@ -226,7 +245,13 @@ parse_tunables (const char *valstring)
>>       }
>>       for (int i = 0; i < ntunables; i++)
>> -    tunable_initialize (tunables[i].t, tunables[i].value, tunables[i].len);
>> +    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 188345b070..d6a1e1b3ac 100644
>> --- a/elf/tst-tunables.c
>> +++ b/elf/tst-tunables.c
>> @@ -53,6 +53,13 @@ static const struct test_t
>>       4096,
>>       0,
>>     },
>> +  {
>> +    "GLIBC_TUNABLES",
>> +    "glibc.malloc.mmap_threshold=-1",
>> +    0,
>> +    SIZE_MAX,
>> +    0,
>> +  },
>>     /* Empty tunable are ignored.  */
>>     {
>>       "GLIBC_TUNABLES",
>> @@ -224,6 +231,29 @@ static const struct test_t
>>       0,
>>       0,
>>     },
>> +  /* Invalid numbers are ignored.  */
>> +  {
>> +    "GLIBC_TUNABLES",
>> +    "glibc.malloc.check=abc:glibc.malloc.mmap_threshold=4096",
>> +    0,
>> +    4096,
>> +    0,
>> +  },
>> +  {
>> +    "GLIBC_TUNABLES",
>> +    "glibc.malloc.check=2:glibc.malloc.mmap_threshold=abc",
>> +    2,
>> +    0,
>> +    0,
>> +  },
>> +  {
>> +    "GLIBC_TUNABLES",
>> +    /* SIZE_MAX + 1 */
>> +    "glibc.malloc.mmap_threshold=18446744073709551616",
>> +    0,
>> +    0,
>> +    0,
>> +  },
>>     /* Also check some tunable aliases.  */
>>     {
>>       "MALLOC_CHECK_",
diff mbox series

Patch

diff --git a/elf/dl-misc.c b/elf/dl-misc.c
index 5b84adc2f4..037cbb3650 100644
--- a/elf/dl-misc.c
+++ b/elf/dl-misc.c
@@ -190,6 +190,9 @@  _dl_strtoul (const char *nptr, char **endptr)
 	}
     }
 
+  const uint64_t cutoff = (UINT64_MAX * 2UL + 1UL) / 10;
+  const uint64_t cutlim = (UINT64_MAX * 2UL + 1UL) % 10;
+
   while (1)
     {
       int digval;
@@ -207,7 +210,7 @@  _dl_strtoul (const char *nptr, char **endptr)
       else
         break;
 
-      if (result >= (UINT64_MAX - digval) / base)
+      if (result > cutoff || (result == cutoff && digval > cutlim))
 	{
 	  if (endptr != NULL)
 	    *endptr = (char *) nptr;
diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
index 26161c68e7..67a37ff704 100644
--- a/elf/dl-tunables.c
+++ b/elf/dl-tunables.c
@@ -77,16 +77,27 @@  do_tunable_update_val (tunable_t *cur, const tunable_val_t *valp,
 {
   tunable_num_t val, min, max;
 
-  if (cur->type.type_code == TUNABLE_TYPE_STRING)
+  switch (cur->type.type_code)
     {
+    case TUNABLE_TYPE_STRING:
       cur->val.strval = valp->strval;
       cur->initialized = true;
       return;
+    case TUNABLE_TYPE_INT_32:
+      val = (int32_t) valp->numval;
+      break;
+    case TUNABLE_TYPE_UINT_64:
+      val = (int64_t) valp->numval;
+      break;
+    case TUNABLE_TYPE_SIZE_T:
+      val = (size_t) valp->numval;
+      break;
+    default:
+      __builtin_unreachable ();
     }
 
   bool unsigned_cmp = unsigned_tunable_type (cur->type.type_code);
 
-  val = valp->numval;
   min = minp != NULL ? *minp : cur->type.min;
   max = maxp != NULL ? *maxp : cur->type.max;
 
@@ -117,16 +128,24 @@  do_tunable_update_val (tunable_t *cur, const tunable_val_t *valp,
 
 /* Validate range of the input value and initialize the tunable CUR if it looks
    good.  */
-static void
+static bool
 tunable_initialize (tunable_t *cur, const char *strval, size_t len)
 {
   tunable_val_t val = { 0 };
 
   if (cur->type.type_code != TUNABLE_TYPE_STRING)
-    val.numval = (tunable_num_t) _dl_strtoul (strval, NULL);
+    {
+      char *endptr = NULL;
+      uint64_t numval = _dl_strtoul (strval, &endptr);
+      if (endptr != strval + len)
+	return false;
+      val.numval = (tunable_num_t) numval;
+    }
   else
     val.strval = (struct tunable_str_t) { strval, len };
   do_tunable_update_val (cur, &val, NULL, NULL);
+
+  return true;
 }
 
 void
@@ -226,7 +245,13 @@  parse_tunables (const char *valstring)
     }
 
   for (int i = 0; i < ntunables; i++)
-    tunable_initialize (tunables[i].t, tunables[i].value, tunables[i].len);
+    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 188345b070..d6a1e1b3ac 100644
--- a/elf/tst-tunables.c
+++ b/elf/tst-tunables.c
@@ -53,6 +53,13 @@  static const struct test_t
     4096,
     0,
   },
+  {
+    "GLIBC_TUNABLES",
+    "glibc.malloc.mmap_threshold=-1",
+    0,
+    SIZE_MAX,
+    0,
+  },
   /* Empty tunable are ignored.  */
   {
     "GLIBC_TUNABLES",
@@ -224,6 +231,29 @@  static const struct test_t
     0,
     0,
   },
+  /* Invalid numbers are ignored.  */
+  {
+    "GLIBC_TUNABLES",
+    "glibc.malloc.check=abc:glibc.malloc.mmap_threshold=4096",
+    0,
+    4096,
+    0,
+  },
+  {
+    "GLIBC_TUNABLES",
+    "glibc.malloc.check=2:glibc.malloc.mmap_threshold=abc",
+    2,
+    0,
+    0,
+  },
+  {
+    "GLIBC_TUNABLES",
+    /* SIZE_MAX + 1 */
+    "glibc.malloc.mmap_threshold=18446744073709551616",
+    0,
+    0,
+    0,
+  },
   /* Also check some tunable aliases.  */
   {
     "MALLOC_CHECK_",