diff mbox

tunables: Avoid getenv calls and disable glibc.malloc.check by default

Message ID 9a4f7ecd-e929-496f-8242-0793a74bc15a@sourceware.org
State New
Headers show

Commit Message

Siddhesh Poyarekar Jan. 2, 2017, 7:37 p.m. UTC
Builds with --enable-tunables failed on i686 because a call to getenv
got snuck into tunables, which pulled in strncmp.  This patch fixes
this build failure by making the glibc.malloc.check check even
simpler.  The previous approach was convoluted where the tunable was
disabled using an unsetenv and overwriting the tunable value with
colons.  The easier way is to simply mark the tunable as insecure by
default (i.e. won't be read for AT_SECURE programs) and then enabled
only when the /etc/suid-debug file is found.

This also ends up removing a bunch of functions that were specially
reimplemented (strlen, unsetenv) to avoid calling into string
routines.

Tested on x86_64 and i686.

        * elf/dl-tunables.c (tunables_unsetenv): Remove function.
        (min_strlen): Likewise.
        (disable_tunable): Likewise.
        (maybe_disable_malloc_check): Rename to
        maybe_enable_malloc_check.
        (maybe_enable_malloc_check): Enable glibc.malloc.check tunable
        if /etc/suid-debug file exists.
        (__tunables_init): Update caller.
        * elf/dl-tunables.list (glibc.malloc.check): Don't mark as
        secure.

Comments

Carlos O'Donell Jan. 10, 2017, 6:52 a.m. UTC | #1
On 01/02/2017 02:37 PM, Siddhesh Poyarekar wrote:
> Builds with --enable-tunables failed on i686 because a call to getenv
> got snuck into tunables, which pulled in strncmp.  This patch fixes
> this build failure by making the glibc.malloc.check check even
> simpler.  The previous approach was convoluted where the tunable was
> disabled using an unsetenv and overwriting the tunable value with
> colons.  The easier way is to simply mark the tunable as insecure by
> default (i.e. won't be read for AT_SECURE programs) and then enabled
> only when the /etc/suid-debug file is found.
> 
> This also ends up removing a bunch of functions that were specially
> reimplemented (strlen, unsetenv) to avoid calling into string
> routines.
> 
> Tested on x86_64 and i686.
> 
>         * elf/dl-tunables.c (tunables_unsetenv): Remove function.
>         (min_strlen): Likewise.
>         (disable_tunable): Likewise.
>         (maybe_disable_malloc_check): Rename to
>         maybe_enable_malloc_check.
>         (maybe_enable_malloc_check): Enable glibc.malloc.check tunable
>         if /etc/suid-debug file exists.
>         (__tunables_init): Update caller.
>         * elf/dl-tunables.list (glibc.malloc.check): Don't mark as
>         secure.

LGTM.

Fixes the build failures on Fedora Rawhide i686.

Passes on all the other Fedora targets e.g. x86_64, ppc64, ppc64le, s390x.

> 0001-tunables-Avoid-getenv-calls-and-disable-glibc.malloc.patch
> 
> 
> ---
>  elf/dl-tunables.c    | 87 ++++++----------------------------------------------
>  elf/dl-tunables.list |  1 -
>  2 files changed, 10 insertions(+), 78 deletions(-)
> 
> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
> index c20a477..e0119d1 100644
> --- a/elf/dl-tunables.c
> +++ b/elf/dl-tunables.c
> @@ -100,34 +100,6 @@ get_next_env (char **envp, char **name, size_t *namelen, char **val)
>    return NULL;
>  }
>  
> -static int
> -tunables_unsetenv (char **ep, const char *name)
> -{
> -  while (*ep != NULL)
> -    {
> -      size_t cnt = 0;
> -
> -      while ((*ep)[cnt] == name[cnt] && name[cnt] != '\0')
> -	++cnt;
> -
> -      if (name[cnt] == '\0' && (*ep)[cnt] == '=')
> -	{
> -	  /* Found it.  Remove this pointer by moving later ones to
> -	     the front.  */
> -	  char **dp = ep;
> -
> -	  do
> -	    dp[0] = dp[1];
> -	  while (*dp++);
> -	  /* Continue the loop in case NAME appears again.  */
> -	}
> -      else
> -	++ep;
> -    }
> -
> -  return 0;
> -}
> -
>  /* A stripped down strtoul-like implementation for very early use.  It does not
>     set errno if the result is outside bounds because it gets called before
>     errno may have been set up.  */
> @@ -316,57 +288,18 @@ parse_tunables (char *tunestr)
>  }
>  #endif
>  
> -static size_t
> -min_strlen (const char *s)
> -{
> -  size_t i = 0;
> -  while (*s++ != '\0')
> -    i++;
> -
> -  return i;
> -}
> -
> -/* Disable a tunable if it is set.  */
> -static void
> -disable_tunable (tunable_id_t id, char **envp)
> -{
> -  const char *env_alias = tunable_list[id].env_alias;
> -
> -  if (env_alias != NULL)
> -    tunables_unsetenv (envp, tunable_list[id].env_alias);
> -
> -#if TUNABLES_FRONTEND == TUNABLES_FRONTEND_valstring
> -  char *tunable = getenv (GLIBC_TUNABLES);
> -  const char *cmp = tunable_list[id].name;
> -  const size_t len = min_strlen (cmp);
> -
> -  while (tunable && *tunable != '\0' && *tunable != ':')
> -    {
> -      if (is_name (tunable, cmp))
> -	{
> -	  tunable += len;
> -	  /* Overwrite the = and the value with colons.  */
> -	  while (*tunable != '\0' && *tunable != ':')
> -	    *tunable++ = ':';
> -	  break;
> -	}
> -      tunable++;
> -    }
> -#endif
> -}
> -
> -/* Disable the glibc.malloc.check tunable in SETUID/SETGID programs unless
> -   the system administrator overrides it by creating the /etc/suid-debug
> -   file.  This is a special case where we want to conditionally enable/disable
> -   a tunable even for setuid binaries.  We use the special version of access()
> -   to avoid setting ERRNO, which is a TLS variable since TLS has not yet been
> -   set up.  */
> +/* Enable the glibc.malloc.check tunable in SETUID/SETGID programs only when
> +   the system administrator has created the /etc/suid-debug file.  This is a
> +   special case where we want to conditionally enable/disable a tunable even
> +   for setuid binaries.  We use the special version of access() to avoid
> +   setting ERRNO, which is a TLS variable since TLS has not yet been set
> +   up.  */
>  static inline void
>  __always_inline
> -maybe_disable_malloc_check (void)
> +maybe_enable_malloc_check (void)
>  {
> -  if (__libc_enable_secure && __access_noerrno ("/etc/suid-debug", F_OK) != 0)
> -    disable_tunable (TUNABLE_ENUM_NAME(glibc, malloc, check), __environ);
> +  if (__access_noerrno ("/etc/suid-debug", F_OK) == 0)
> +    tunable_list[TUNABLE_ENUM_NAME(glibc, malloc, check)].is_secure = true;
>  }
>  
>  /* Initialize the tunables list from the environment.  For now we only use the
> @@ -379,7 +312,7 @@ __tunables_init (char **envp)
>    char *envval = NULL;
>    size_t len = 0;
>  
> -  maybe_disable_malloc_check ();
> +  maybe_enable_malloc_check ();
>  
>    while ((envp = get_next_env (envp, &envname, &len, &envval)) != NULL)
>      {
> diff --git a/elf/dl-tunables.list b/elf/dl-tunables.list
> index cbd1f4f..d8cd912 100644
> --- a/elf/dl-tunables.list
> +++ b/elf/dl-tunables.list
> @@ -31,7 +31,6 @@ glibc {
>        minval: 0
>        maxval: 3
>        env_alias: MALLOC_CHECK_
> -      is_secure: true
>      }
>      top_pad {
>        type: SIZE_T
> -- 2.7.4
diff mbox

Patch

---
 elf/dl-tunables.c    | 87 ++++++----------------------------------------------
 elf/dl-tunables.list |  1 -
 2 files changed, 10 insertions(+), 78 deletions(-)

diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
index c20a477..e0119d1 100644
--- a/elf/dl-tunables.c
+++ b/elf/dl-tunables.c
@@ -100,34 +100,6 @@  get_next_env (char **envp, char **name, size_t *namelen, char **val)
   return NULL;
 }
 
-static int
-tunables_unsetenv (char **ep, const char *name)
-{
-  while (*ep != NULL)
-    {
-      size_t cnt = 0;
-
-      while ((*ep)[cnt] == name[cnt] && name[cnt] != '\0')
-	++cnt;
-
-      if (name[cnt] == '\0' && (*ep)[cnt] == '=')
-	{
-	  /* Found it.  Remove this pointer by moving later ones to
-	     the front.  */
-	  char **dp = ep;
-
-	  do
-	    dp[0] = dp[1];
-	  while (*dp++);
-	  /* Continue the loop in case NAME appears again.  */
-	}
-      else
-	++ep;
-    }
-
-  return 0;
-}
-
 /* A stripped down strtoul-like implementation for very early use.  It does not
    set errno if the result is outside bounds because it gets called before
    errno may have been set up.  */
@@ -316,57 +288,18 @@  parse_tunables (char *tunestr)
 }
 #endif
 
-static size_t
-min_strlen (const char *s)
-{
-  size_t i = 0;
-  while (*s++ != '\0')
-    i++;
-
-  return i;
-}
-
-/* Disable a tunable if it is set.  */
-static void
-disable_tunable (tunable_id_t id, char **envp)
-{
-  const char *env_alias = tunable_list[id].env_alias;
-
-  if (env_alias != NULL)
-    tunables_unsetenv (envp, tunable_list[id].env_alias);
-
-#if TUNABLES_FRONTEND == TUNABLES_FRONTEND_valstring
-  char *tunable = getenv (GLIBC_TUNABLES);
-  const char *cmp = tunable_list[id].name;
-  const size_t len = min_strlen (cmp);
-
-  while (tunable && *tunable != '\0' && *tunable != ':')
-    {
-      if (is_name (tunable, cmp))
-	{
-	  tunable += len;
-	  /* Overwrite the = and the value with colons.  */
-	  while (*tunable != '\0' && *tunable != ':')
-	    *tunable++ = ':';
-	  break;
-	}
-      tunable++;
-    }
-#endif
-}
-
-/* Disable the glibc.malloc.check tunable in SETUID/SETGID programs unless
-   the system administrator overrides it by creating the /etc/suid-debug
-   file.  This is a special case where we want to conditionally enable/disable
-   a tunable even for setuid binaries.  We use the special version of access()
-   to avoid setting ERRNO, which is a TLS variable since TLS has not yet been
-   set up.  */
+/* Enable the glibc.malloc.check tunable in SETUID/SETGID programs only when
+   the system administrator has created the /etc/suid-debug file.  This is a
+   special case where we want to conditionally enable/disable a tunable even
+   for setuid binaries.  We use the special version of access() to avoid
+   setting ERRNO, which is a TLS variable since TLS has not yet been set
+   up.  */
 static inline void
 __always_inline
-maybe_disable_malloc_check (void)
+maybe_enable_malloc_check (void)
 {
-  if (__libc_enable_secure && __access_noerrno ("/etc/suid-debug", F_OK) != 0)
-    disable_tunable (TUNABLE_ENUM_NAME(glibc, malloc, check), __environ);
+  if (__access_noerrno ("/etc/suid-debug", F_OK) == 0)
+    tunable_list[TUNABLE_ENUM_NAME(glibc, malloc, check)].is_secure = true;
 }
 
 /* Initialize the tunables list from the environment.  For now we only use the
@@ -379,7 +312,7 @@  __tunables_init (char **envp)
   char *envval = NULL;
   size_t len = 0;
 
-  maybe_disable_malloc_check ();
+  maybe_enable_malloc_check ();
 
   while ((envp = get_next_env (envp, &envname, &len, &envval)) != NULL)
     {
diff --git a/elf/dl-tunables.list b/elf/dl-tunables.list
index cbd1f4f..d8cd912 100644
--- a/elf/dl-tunables.list
+++ b/elf/dl-tunables.list
@@ -31,7 +31,6 @@  glibc {
       minval: 0
       maxval: 3
       env_alias: MALLOC_CHECK_
-      is_secure: true
     }
     top_pad {
       type: SIZE_T
-- 
2.7.4