Re: [PATCH] tunables: Avoid getenv calls and disable glibc.malloc.check by default

[Carlos O'Donell <carlos at redhat dot com>]

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