This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.

Index Nav:	[Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav:	[Date Prev] [Date Next]	[Thread Prev] [Thread Next]
Other format:	[Raw text]

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

From: "H.J. Lu" <hjl dot tools at gmail dot com>
To: Adhemerval Zanella <adhemerval dot zanella at linaro dot org>
Cc: Florian Weimer <fw at deneb dot enyo dot de>, GNU C Library <libc-alpha at sourceware dot org>
Date: Thu, 20 Jul 2017 06:04:28 -0700
Subject: Re: [PATCH] Don't all __access_noerrno with stack protector from __tunables_init [BZ #21744]
Authentication-results: sourceware.org; auth=none
References: <20170716191924.GA7226@gmail.com> <7643d65d-839e-a85d-8a4f-30eada595887@linaro.org> <CAMe9rOrU5Wp=bZgOY=91dyEJHjW6SJ5HknOeS_92pTxukpFrhA@mail.gmail.com> <874luag5n4.fsf@mid.deneb.enyo.de> <6dc48782-60c3-cead-08c0-7e018e7eb9ef@linaro.org> <cf2078d9-665e-c77d-36ad-29285719ff86@linaro.org> <CAMe9rOpBEe7FZfnWfRCpvyJ5MYGjm8UVdNFti8S9uiFbzZkmZg@mail.gmail.com> <80b843b8-6e3c-ccbe-5a6f-b37e18dbdbae@linaro.org> <CAMe9rOqQL07x-QBAHhgiGZh=s98LJ35Mrec4QKfV18SohnRQnA@mail.gmail.com> <209c41dd-d940-0625-f29b-0001a9b7019a@linaro.org> <CAMe9rOqSyvUvpO2DdYTrwraGFsvMPRChsKhwqL_JHTNs5rM9bA@mail.gmail.com> <98fc5143-e689-9e5b-d005-c9f22eaecc68@linaro.org>

On Thu, Jul 20, 2017 at 5:46 AM, Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
>
>
> On 19/07/2017 22:11, H.J. Lu wrote:
>> On Wed, Jul 19, 2017 at 5:56 PM, Adhemerval Zanella
>> <adhemerval.zanella@linaro.org> wrote:
>>>
>>>
>>> On 19/07/2017 18:53, H.J. Lu wrote:
>>>> On Tue, Jul 18, 2017 at 6:52 AM, Adhemerval Zanella
>>>> <adhemerval.zanella@linaro.org> wrote:
>>>>>
>>>>>
>>>>> On 18/07/2017 10:44, H.J. Lu wrote:
>>>>>> This should work for Linux.
>>>>>>
>>>>>> Thanks.
>>>>>>
>>>>>
>>>>> Now with a proper patch:
>>>>>
>>>>> [PATCH] tunables: Use direct syscall for access (BZ#21744)
>>>>>
>>>>> The function maybe_enable_malloc_check, which is called by
>>>>> __tunables_init, calls __access_noerrno.  It isn't problem when
>>>>> symbol 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 how Linux defines the __access_noerrno to be an
>>>>> inline call instead and thus preventing defining different build
>>>>> rules for ld/static and shared.
>>>>>
>>>>>         H.J. Lu  <hongjiu.lu@intel.com>
>>>>>         Adhemerval Zanella  <adhemerval.zanella@linaro.org>
>>>>
>>>> Missing [BZ#21744]
>>>
>>> Ack.
>>>
>>>>
>>>>>         * elf/dl-tunables.c: Include not-errno.h header.
>>>>>         * include/unistd.h (__access_noerrno): Remove definition.
>>>>>         * sysdeps/unix/sysv/linux/access.c (__access_noerrno): Likewise.
>>>>>         * sysdeps/generic/not-errno.h: New file.
>>>>>         * sysdeps/unix/sysv/linux/not-errno.h: Likewise.
>>>>> ---
>>>>>  ChangeLog                           |  9 +++++++++
>>>>>  elf/dl-tunables.c                   |  2 ++
>>>>>  include/unistd.h                    |  7 -------
>>>>>  sysdeps/generic/not-errno.h         | 19 +++++++++++++++++++
>>>>>  sysdeps/unix/sysv/linux/access.c    | 15 ---------------
>>>>>  sysdeps/unix/sysv/linux/not-errno.h | 30 ++++++++++++++++++++++++++++++
>>>>>  6 files changed, 60 insertions(+), 22 deletions(-)
>>>>>  create mode 100644 sysdeps/generic/not-errno.h
>>>>>  create mode 100644 sysdeps/unix/sysv/linux/not-errno.h
>>>>>
>>>>> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
>>>>> index 44c160c..231fb8c 100644
>>>>> --- a/elf/dl-tunables.c
>>>>> +++ b/elf/dl-tunables.c
>>>>> @@ -29,6 +29,8 @@
>>>>>  #define TUNABLES_INTERNAL 1
>>>>>  #include "dl-tunables.h"
>>>>>
>>>>> +#include <not-errno.h>
>>>>> +
>>>>>  #if TUNABLES_FRONTEND == TUNABLES_FRONTEND_valstring
>>>>>  # define GLIBC_TUNABLES "GLIBC_TUNABLES"
>>>>>  #endif
>>>>> diff --git a/include/unistd.h b/include/unistd.h
>>>>> index 5b2a414..7f1c2cc 100644
>>>>> --- a/include/unistd.h
>>>>> +++ b/include/unistd.h
>>>>> @@ -182,12 +182,5 @@ extern int __getlogin_r_loginuid (char *name, size_t namesize)
>>>>>  #   include <dl-unistd.h>
>>>>>  #  endif
>>>>>
>>>>> -#  if IS_IN (rtld) || !defined SHARED
>>>>> -/* __access variant that does not set errno.  Used in very early initialization
>>>>> -   code in libc.a and ld.so.  It follows access return semantics (zero for
>>>>> -   sucess otherwise a value different than 0).  */
>>>>> -extern __typeof (__access) __access_noerrno attribute_hidden;
>>>>> -#  endif
>>>>> -
>>>>>  # endif
>>>>>  #endif
>>>>> diff --git a/sysdeps/generic/not-errno.h b/sysdeps/generic/not-errno.h
>>>>> new file mode 100644
>>>>> index 0000000..2aac095
>>>>> --- /dev/null
>>>>> +++ b/sysdeps/generic/not-errno.h
>>>>> @@ -0,0 +1,19 @@
>>>>> +/* Syscall wrapper that do not set errno.  Generic version.
>>>>> +   Copyright (C) 2017 Free Software Foundation, Inc.
>>>>> +   This file is part of the GNU C Library.
>>>>> +
>>>>> +   The GNU C Library is free software; you can redistribute it and/or
>>>>> +   modify it under the terms of the GNU Lesser General Public
>>>>> +   License as published by the Free Software Foundation; either
>>>>> +   version 2.1 of the License, or (at your option) any later version.
>>>>> +
>>>>> +   The GNU C Library is distributed in the hope that it will be useful,
>>>>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>>>>> +   Lesser General Public License for more details.
>>>>> +
>>>>> +   You should have received a copy of the GNU Lesser General Public
>>>>> +   License along with the GNU C Library; if not, see
>>>>> +   <http://www.gnu.org/licenses/>.  */
>>>>> +
>>>>> +extern __typeof (__access) __access_noerrno attribute_hidden;
>>>>> diff --git a/sysdeps/unix/sysv/linux/access.c b/sysdeps/unix/sysv/linux/access.c
>>>>> index 67e69bd..366b6b6 100644
>>>>> --- a/sysdeps/unix/sysv/linux/access.c
>>>>> +++ b/sysdeps/unix/sysv/linux/access.c
>>>>> @@ -21,21 +21,6 @@
>>>>>  #include <sysdep-cancel.h>
>>>>>
>>>>>  int
>>>>> -__access_noerrno (const char *file, int type)
>>>>> -{
>>>>> -  int res;
>>>>> -  INTERNAL_SYSCALL_DECL (err);
>>>>> -#ifdef __NR_access
>>>>> -  res = INTERNAL_SYSCALL_CALL (access, err, file, type);
>>>>> -#else
>>>>> -  res = INTERNAL_SYSCALL_CALL (faccessat, err, AT_FDCWD, file, type);
>>>>> -#endif
>>>>> -  if (INTERNAL_SYSCALL_ERROR_P (res, err))
>>>>> -    return INTERNAL_SYSCALL_ERRNO (res, err);
>>>>> -  return 0;
>>>>> -}
>>>>> -
>>>>> -int
>>>>>  __access (const char *file, int type)
>>>>>  {
>>>>>  #ifdef __NR_access
>>>>> diff --git a/sysdeps/unix/sysv/linux/not-errno.h b/sysdeps/unix/sysv/linux/not-errno.h
>>>>> new file mode 100644
>>>>> index 0000000..4957038
>>>>> --- /dev/null
>>>>> +++ b/sysdeps/unix/sysv/linux/not-errno.h
>>>>> @@ -0,0 +1,30 @@
>>>>> +/* Syscall wrapper that do not set errno.  Linux version.
>>>>> +   Copyright (C) 2017 Free Software Foundation, Inc.
>>>>> +   This file is part of the GNU C Library.
>>>>> +
>>>>> +   The GNU C Library is free software; you can redistribute it and/or
>>>>> +   modify it under the terms of the GNU Lesser General Public
>>>>> +   License as published by the Free Software Foundation; either
>>>>> +   version 2.1 of the License, or (at your option) any later version.
>>>>> +
>>>>> +   The GNU C Library is distributed in the hope that it will be useful,
>>>>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>>>>> +   Lesser General Public License for more details.
>>>>> +
>>>>> +   You should have received a copy of the GNU Lesser General Public
>>>>> +   License along with the GNU C Library; if not, see
>>>>> +   <http://www.gnu.org/licenses/>.  */
>>>>> +
>>>>> +/* This function is used on maybe_enable_malloc_check (elf/dl-tunables.c)
>>>>> +   and to avoid having to build/use multiple versions if stack protection
>>>>> +   in enabled it is defined as inline.  */
>>>>> +static inline int
>>>>> +__access_noerrno (const char *pathname, int mode)
>>>>> +{
>>>>> +#ifdef __NR_access
>>>>> +  return INTERNAL_SYSCALL_CALL (access, pathname, mode);
>>>>> +#else
>>>>> +  return INTERNAL_SYSCALL_CALL (faccessat, AT_FDCWD, file, type);
>>>>> +#endif
>>>>> +}
>>>>
>>>> These are wrong.  INTERNAL_SYSCALL_CALL needs  a dummy err.
>>>> This seems to work:
>>>>
>>>> diff --git a/sysdeps/unix/sysv/linux/not-errno.h
>>>> b/sysdeps/unix/sysv/linux/not-errno.h
>>>> index 4957038160..a03c399f95 100644
>>>> --- a/sysdeps/unix/sysv/linux/not-errno.h
>>>> +++ b/sysdeps/unix/sysv/linux/not-errno.h
>>>> @@ -23,8 +23,8 @@ static inline int
>>>>  __access_noerrno (const char *pathname, int mode)
>>>>  {
>>>>  #ifdef __NR_access
>>>> -  return INTERNAL_SYSCALL_CALL (access, pathname, mode);
>>>> +  return INTERNAL_SYSCALL_CALL (access, err, pathname, mode);
>>>>  #else
>>>> -  return INTERNAL_SYSCALL_CALL (faccessat, AT_FDCWD, file, type);
>>>> +  return INTERNAL_SYSCALL_CALL (faccessat, err, AT_FDCWD, file, type);
>>>>  #endif
>>>>  }
>>>>
>>>
>>> Right, so we will need the 'INTERNAL_SYSCALL_DECL (err);' before as well.
>>
>> It compiles fine without "INTERNAL_SYSCALL_DECL (err);".
>
> It will fail for architectures that actually define 'err' to something other
> than empty 'do { } while (0)' (for instance sparc and tile). Below it is an
> updated patch:
>
> --
>
>         [BZ #21744]
>         * elf/dl-tunables.c: Include not-errno.h header.
>         * include/unistd.h (__access_noerrno): Remove definition.
>         * sysdeps/unix/sysv/linux/access.c (__access_noerrno): Likewise.
>         * sysdeps/generic/not-errno.h: New file.
>         * sysdeps/unix/sysv/linux/not-errno.h: Likewise.
>
> ---
>
> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
> index 44c160c..231fb8c 100644
> --- a/elf/dl-tunables.c
> +++ b/elf/dl-tunables.c
> @@ -29,6 +29,8 @@
>  #define TUNABLES_INTERNAL 1
>  #include "dl-tunables.h"
>
> +#include <not-errno.h>
> +
>  #if TUNABLES_FRONTEND == TUNABLES_FRONTEND_valstring
>  # define GLIBC_TUNABLES "GLIBC_TUNABLES"
>  #endif
> diff --git a/include/unistd.h b/include/unistd.h
> index 5b2a414..7f1c2cc 100644
> --- a/include/unistd.h
> +++ b/include/unistd.h
> @@ -182,12 +182,5 @@ extern int __getlogin_r_loginuid (char *name, size_t namesize)
>  #   include <dl-unistd.h>
>  #  endif
>
> -#  if IS_IN (rtld) || !defined SHARED
> -/* __access variant that does not set errno.  Used in very early initialization
> -   code in libc.a and ld.so.  It follows access return semantics (zero for
> -   sucess otherwise a value different than 0).  */
> -extern __typeof (__access) __access_noerrno attribute_hidden;
> -#  endif
> -
>  # endif
>  #endif
> diff --git a/sysdeps/generic/not-errno.h b/sysdeps/generic/not-errno.h
> new file mode 100644
> index 0000000..2aac095
> --- /dev/null
> +++ b/sysdeps/generic/not-errno.h
> @@ -0,0 +1,19 @@
> +/* Syscall wrapper that do not set errno.  Generic version.
> +   Copyright (C) 2017 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +extern __typeof (__access) __access_noerrno attribute_hidden;
> diff --git a/sysdeps/unix/sysv/linux/access.c b/sysdeps/unix/sysv/linux/access.c
> index 67e69bd..366b6b6 100644
> --- a/sysdeps/unix/sysv/linux/access.c
> +++ b/sysdeps/unix/sysv/linux/access.c
> @@ -21,21 +21,6 @@
>  #include <sysdep-cancel.h>
>
>  int
> -__access_noerrno (const char *file, int type)
> -{
> -  int res;
> -  INTERNAL_SYSCALL_DECL (err);
> -#ifdef __NR_access
> -  res = INTERNAL_SYSCALL_CALL (access, err, file, type);
> -#else
> -  res = INTERNAL_SYSCALL_CALL (faccessat, err, AT_FDCWD, file, type);
> -#endif
> -  if (INTERNAL_SYSCALL_ERROR_P (res, err))
> -    return INTERNAL_SYSCALL_ERRNO (res, err);
> -  return 0;
> -}
> -
> -int
>  __access (const char *file, int type)
>  {
>  #ifdef __NR_access
> diff --git a/sysdeps/unix/sysv/linux/not-errno.h b/sysdeps/unix/sysv/linux/not-errno.h
> new file mode 100644
> index 0000000..888bde0
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/not-errno.h
> @@ -0,0 +1,31 @@
> +/* Syscall wrapper that do not set errno.  Linux version.
> +   Copyright (C) 2017 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +/* This function is used on maybe_enable_malloc_check (elf/dl-tunables.c)
> +   and to avoid having to build/use multiple versions if stack protection
> +   in enabled it is defined as inline.  */
> +static inline int
> +__access_noerrno (const char *pathname, int mode)
> +{
> +  INTERNAL_SYSCALL_DECL (err);
> +#ifdef __NR_access
> +  return INTERNAL_SYSCALL_CALL (access, err, pathname, mode);
> +#else
> +  return INTERNAL_SYSCALL_CALL (faccessat, err, AT_FDCWD, file, type);
> +#endif
> +}
> --
> 2.7.4

LGTM.

Thanks.

-- 
H.J.

Follow-Ups:
- Re: [PATCH] Don't all __access_noerrno with stack protector from __tunables_init [BZ #21744]
  - From: Adhemerval Zanella

References:
- [PATCH] Don't all __access_noerrno with stack protector from __tunables_init [BZ #21744]
  - From: H.J. Lu
- Re: [PATCH] Don't all __access_noerrno with stack protector from __tunables_init [BZ #21744]
  - From: Adhemerval Zanella
- Re: [PATCH] Don't all __access_noerrno with stack protector from __tunables_init [BZ #21744]
  - From: H.J. Lu
- Re: [PATCH] Don't all __access_noerrno with stack protector from __tunables_init [BZ #21744]
  - From: Florian Weimer
- Re: [PATCH] Don't all __access_noerrno with stack protector from __tunables_init [BZ #21744]
  - From: Adhemerval Zanella
- Re: [PATCH] Don't all __access_noerrno with stack protector from __tunables_init [BZ #21744]
  - From: Adhemerval Zanella
- Re: [PATCH] Don't all __access_noerrno with stack protector from __tunables_init [BZ #21744]
  - From: H.J. Lu
- Re: [PATCH] Don't all __access_noerrno with stack protector from __tunables_init [BZ #21744]
  - From: Adhemerval Zanella
- Re: [PATCH] Don't all __access_noerrno with stack protector from __tunables_init [BZ #21744]
  - From: H.J. Lu
- Re: [PATCH] Don't all __access_noerrno with stack protector from __tunables_init [BZ #21744]
  - From: Adhemerval Zanella
- Re: [PATCH] Don't all __access_noerrno with stack protector from __tunables_init [BZ #21744]
  - From: H.J. Lu
- Re: [PATCH] Don't all __access_noerrno with stack protector from __tunables_init [BZ #21744]
  - From: Adhemerval Zanella

Index Nav:	[Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav:	[Date Prev] [Date Next]	[Thread Prev] [Thread Next]