Re: [PATCH] Don't all __access_noerrno with stack protector from __tunables_init [BZ #21744]

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 16/07/2017 16:19, H.J. Lu wrote:
> maybe_enable_malloc_check, which is called by __tunables_init, call
> __access_noerrno.  It isn't problem when maybe_enable_malloc_check is
> is in ld.so, which has a special version of __access_noerrno without
> stack protector.  But when glibc is built with stack protector,
> maybe_enable_malloc_check in libc.a can't call the regular version of
> __access_noerrno with stack protector.
> 
> This patch changes maybe_enable_malloc_check to call _dl_access_noerrno
> instead.  For ld.so or glibc built without stack protector, it is defined
> to __access_noerrno.  Otherwise a special version of __access_noerrno
> without stack protector is used by maybe_enable_malloc_check in libc.a.
> 
> Tested on x86-64 with and without --enable-stack-protector=all.
> 

I think a much more simpler solution would be just to inline the access
call on 'maybe_enable_malloc_check':

diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
index 44c160c..e6db258 100644
--- a/elf/dl-tunables.c
+++ b/elf/dl-tunables.c
@@ -281,7 +281,8 @@ __always_inline
 maybe_enable_malloc_check (void)
 {
   tunable_id_t id = TUNABLE_ENUM_NAME (glibc, malloc, check);
-  if (__libc_enable_secure && __access_noerrno ("/etc/suid-debug", F_OK) == 0)
+  if (__libc_enable_secure
+      && INTERNAL_SYSCALL_CALL (access, "/etc/suid-debug", F_OK) == 0)
     tunable_list[id].security_level = TUNABLE_SECLEVEL_NONE;
 }
 
We can cleanup the non required access_noerro later.

> OK for master?
> 
> H.J.
> ---
> 	[BZ #21744]
> 	* config.h.in (HAVE_STACK_PROTECTOR): New.
> 	* configure.ac (HAVE_STACK_PROTECTOR): AC_DEFINE if configured
> 	with stack protector.
> 	* configure: Regenerated.
> 	* elf/dl-support.c [HAVE_TUNABLES != 0]: Include <access.c> if
> 	__access_noerrno is defined.
> 	* elf/dl-tunables.c (maybe_enable_malloc_check): Replace
> 	__access_noerrno with _dl_access_noerrno.
> 	* elf/dl-tunables.h [SHARED != 0 || HAVE_STACK_PROTECTOR == 0]
> 	(_dl_access_noerrno): New macro.
> 	[SHARED == 0 && HAVE_STACK_PROTECTOR != 0] (__access_noerrno):
> 	New macro.
> 	[SHARED == 0 && HAVE_STACK_PROTECTOR != 0] (_dl_access_noerrno):
> 	New prototype.
> 	* io/access.c (__access): Don't define if __access_noerrno is
> 	defined.
> 	* sysdeps/mach/hurd/access.c (__access): Likewise.
> 	* sysdeps/unix/sysv/linux/access.c (__access): Likewise.
> ---
>  config.h.in                      |  4 ++++
>  configure                        |  6 ++++++
>  configure.ac                     |  3 +++
>  elf/dl-support.c                 |  8 +++++++-
>  elf/dl-tunables.c                |  3 ++-
>  elf/dl-tunables.h                | 14 ++++++++++++++
>  io/access.c                      |  2 ++
>  sysdeps/mach/hurd/access.c       |  2 ++
>  sysdeps/unix/sysv/linux/access.c |  2 ++
>  9 files changed, 42 insertions(+), 2 deletions(-)
> 
> diff --git a/config.h.in b/config.h.in
> index 22418576a0..01b1a5374b 100644
> --- a/config.h.in
> +++ b/config.h.in
> @@ -250,4 +250,8 @@
>     in i386 6 argument syscall issue).  */
>  #define CAN_USE_REGISTER_ASM_EBP 0
>  
> +/* Build glibc with -fstack-protector, -fstack-protector-all, or
> +   -fstack-protector-strong.  */
> +#define HAVE_STACK_PROTECTOR 0
> +
>  #endif
> diff --git a/configure b/configure
> index d8e1c50e11..5cc8b2f979 100755
> --- a/configure
> +++ b/configure
> @@ -4051,14 +4051,20 @@ if test "$enable_stack_protector" = yes && test "$libc_cv_ssp" = yes; then
>    stack_protector="-fstack-protector"
>    $as_echo "#define STACK_PROTECTOR_LEVEL 1" >>confdefs.h
>  
> +  $as_echo "#define HAVE_STACK_PROTECTOR 1" >>confdefs.h
> +
>  elif test "$enable_stack_protector" = all && test "$libc_cv_ssp_all" = yes; then
>    stack_protector="-fstack-protector-all"
>    $as_echo "#define STACK_PROTECTOR_LEVEL 2" >>confdefs.h
>  
> +  $as_echo "#define HAVE_STACK_PROTECTOR 1" >>confdefs.h
> +
>  elif test "$enable_stack_protector" = strong && test "$libc_cv_ssp_strong" = yes; then
>    stack_protector="-fstack-protector-strong"
>    $as_echo "#define STACK_PROTECTOR_LEVEL 3" >>confdefs.h
>  
> +  $as_echo "#define HAVE_STACK_PROTECTOR 1" >>confdefs.h
> +
>  else
>    stack_protector="-fno-stack-protector"
>    $as_echo "#define STACK_PROTECTOR_LEVEL 0" >>confdefs.h
> diff --git a/configure.ac b/configure.ac
> index 77456aa8d9..05a0182792 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -687,12 +687,15 @@ fi
>  if test "$enable_stack_protector" = yes && test "$libc_cv_ssp" = yes; then
>    stack_protector="-fstack-protector"
>    AC_DEFINE(STACK_PROTECTOR_LEVEL, 1)
> +  AC_DEFINE(HAVE_STACK_PROTECTOR)
>  elif test "$enable_stack_protector" = all && test "$libc_cv_ssp_all" = yes; then
>    stack_protector="-fstack-protector-all"
>    AC_DEFINE(STACK_PROTECTOR_LEVEL, 2)
> +  AC_DEFINE(HAVE_STACK_PROTECTOR)
>  elif test "$enable_stack_protector" = strong && test "$libc_cv_ssp_strong" = yes; then
>    stack_protector="-fstack-protector-strong"
>    AC_DEFINE(STACK_PROTECTOR_LEVEL, 3)
> +  AC_DEFINE(HAVE_STACK_PROTECTOR)
>  else
>    stack_protector="-fno-stack-protector"
>    AC_DEFINE(STACK_PROTECTOR_LEVEL, 0)
> diff --git a/elf/dl-support.c b/elf/dl-support.c
> index c22be854f4..3228cf4020 100644
> --- a/elf/dl-support.c
> +++ b/elf/dl-support.c
> @@ -164,7 +164,13 @@ uint64_t _dl_hwcap2 __attribute__ ((nocommon));
>  /* The value of the FPU control word the kernel will preset in hardware.  */
>  fpu_control_t _dl_fpu_control = _FPU_DEFAULT;
>  
> -#if !HAVE_TUNABLES
> +#if HAVE_TUNABLES
> +# ifdef __access_noerrno
> +/* If __access_noerrno is defined, include <access.c> to define the
> +   special version of __access_noerrno without stack protector.  */
> +#  include <access.c>
> +# endif
> +#else
>  /* This is not initialized to HWCAP_IMPORTANT, matching the definition
>     of _dl_important_hwcaps, below, where no hwcap strings are ever
>     used.  This mask is still used to mediate the lookups in the cache
> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
> index 44c160cac5..3fef367d72 100644
> --- a/elf/dl-tunables.c
> +++ b/elf/dl-tunables.c
> @@ -281,7 +281,8 @@ __always_inline
>  maybe_enable_malloc_check (void)
>  {
>    tunable_id_t id = TUNABLE_ENUM_NAME (glibc, malloc, check);
> -  if (__libc_enable_secure && __access_noerrno ("/etc/suid-debug", F_OK) == 0)
> +  if (__libc_enable_secure
> +      && _dl_access_noerrno ("/etc/suid-debug", F_OK) == 0)
>      tunable_list[id].security_level = TUNABLE_SECLEVEL_NONE;
>  }
>  
> diff --git a/elf/dl-tunables.h b/elf/dl-tunables.h
> index c92882acba..62d5ef841f 100644
> --- a/elf/dl-tunables.h
> +++ b/elf/dl-tunables.h
> @@ -30,6 +30,20 @@ __tunables_init (char **unused __attribute__ ((unused)))
>  }
>  #else
>  
> +# if defined SHARED || !HAVE_STACK_PROTECTOR
> +/* maybe_enable_malloc_check in ld.so uses the special version of
> +   __access_noerrno without stack protector.  When glibc is built
> +   without stack protector, maybe_enable_malloc_check in libc.a uses
> +   the regular version of __access_noerrno, which is compiled without
> +   stack protector.  */
> +#  define _dl_access_noerrno __access_noerrno
> +# else
> +/* When glibc is built with stack protector, maybe_enable_malloc_check
> +   in libc.a must use the special version of __access_noerrno without
> +   stack protector.  */
> +#  define __access_noerrno _dl_access_noerrno
> +extern int _dl_access_noerrno (const char *, int);
> +# endif
>  # include <stddef.h>
>  # include "dl-tunable-types.h"
>  
> diff --git a/io/access.c b/io/access.c
> index 72eba36e3f..d27e789923 100644
> --- a/io/access.c
> +++ b/io/access.c
> @@ -26,6 +26,7 @@ __access_noerrno (const char *file, int type)
>    return -1;
>  }
>  
> +#ifndef __access_noerrno
>  /* Test for access to FILE.  */
>  int
>  __access (const char *file, int type)
> @@ -42,3 +43,4 @@ __access (const char *file, int type)
>  stub_warning (access)
>  
>  weak_alias (__access, access)
> +#endif
> diff --git a/sysdeps/mach/hurd/access.c b/sysdeps/mach/hurd/access.c
> index 185e000c9d..329a9fc70a 100644
> --- a/sysdeps/mach/hurd/access.c
> +++ b/sysdeps/mach/hurd/access.c
> @@ -164,6 +164,7 @@ __access_noerrno (const char *file, int type)
>    return access_common (file, type, hurd_fail_noerrno);
>  }
>  
> +#ifndef __access_noerrno
>  /* Test for access to FILE by our real user and group IDs.  */
>  int
>  __access (const char *file, int type)
> @@ -171,3 +172,4 @@ __access (const char *file, int type)
>    return access_common (file, type, hurd_fail_seterrno);
>  }
>  weak_alias (__access, access)
> +#endif
> diff --git a/sysdeps/unix/sysv/linux/access.c b/sysdeps/unix/sysv/linux/access.c
> index 67e69bd163..f0e7dd2e83 100644
> --- a/sysdeps/unix/sysv/linux/access.c
> +++ b/sysdeps/unix/sysv/linux/access.c
> @@ -35,6 +35,7 @@ __access_noerrno (const char *file, int type)
>    return 0;
>  }
>  
> +#ifndef __access_noerrno
>  int
>  __access (const char *file, int type)
>  {
> @@ -45,3 +46,4 @@ __access (const char *file, int type)
>  #endif
>  }
>  weak_alias (__access, access)
> +#endif
>