[v2,00/10] Improve fortify support with clang

  When using clang, the fortify wrappers show less coverage on both
compile and runtime. For instance, a snippet from tst-fortify.c:

  $ cat t.c
  #include <stdio.h>
  #include <string.h>
  
  const char *str1 = "JIHGFEDCBA";
  
  int main (int argc, char *argv[])
  {
  #define O 0
    struct A { char buf1[9]; char buf2[1]; } a;
    strcpy (a.buf1 + (O + 4), str1 + 5);
  
    return 0;
  }
  $ gcc -O2 t.c  -Wp,-U_FORTIFY_SOURCE,-D_FORTIFY_SOURCE=2 -o t
  $ ./t
  *** buffer overflow detected ***: terminated
  Aborted (core dumped)
  $ clang  -O2 t.c  -Wp,-U_FORTIFY_SOURCE,-D_FORTIFY_SOURCE=2 -o t
  $ ./t
  $

Although clang does support __builtin___strcpy_chk (which correctly
lowers to __strcpy_chk), the __builtin_object_size passed as third
the argument is not fully correct and thus limits the possible runtime
checks.

This limitation was already being raised some years ago [1], but the
work has been staled.  However, a similar patch has been used for
ChromeOS for some time [2].  Bionic libc also uses a similar approach to
enable fortified wrappers.

Improve its support with clang, requires defining the fortified wrapper
differently. For instance, the read wrapper is currently expanded as:

  extern __inline
  __attribute__((__always_inline__))
  __attribute__((__artificial__))
  __attribute__((__warn_unused_result__))
  ssize_t read (int __fd, void *__buf, size_t __nbytes)
  {
     return __glibc_safe_or_unknown_len (__nbytes,
                                         sizeof (char),
                                         __glibc_objsize0 (__buf))
            ? __read_alias (__fd, __buf, __nbytes)
            : __glibc_unsafe_len (__nbytes,
                                  sizeof (char),
                                  __glibc_objsize0 (__buf))
              ? __read_chk_warn (__fd,
                                 __buf,
                                 __nbytes,
                                 __builtin_object_size (__buf, 0))
              : __read_chk (__fd,
                            __buf,
                            __nbytes,
                            __builtin_object_size (__buf, 0));
  }

The wrapper relies on __builtin_object_size call lowers to a constant at
Compile time and many other operations in the wrapper depend on
having a single, known value for parameters.   Because this is
impossible to have for function parameters, the wrapper depends heavily
on inlining to work and While this is an entirely viable approach on
GCC is not fully reliable on clang.  This is because by the time llvm
gets to inlining and optimizing, there is a minimal reliable source and
type-level information available (more information on a more deep
explanation on how to fortify wrapper works on clang [4]).

To allow the wrapper to work reliably and with the same functionality as
with GCC, clang requires a different approach:

  * __attribute__((diagnose_if(c, “str”, “warning”))) which is a
  * function
    level attribute; if the compiler can determine that 'c' is true at
    compile-time, it will emit a warning with the text 'str1'.  If it
    would be better to emit an error, the wrapper can use "error"
    instead of "warning".

  * __attribute__((overloadable)) which is also a function-level
    attribute; and it allows C++-style overloading to occur on C
functions.

  * __attribute__((pass_object_size(n))) which is a parameter-level
    attribute; and it makes the compiler evaluate
    __builtin_object_size(param, n) at each call site of the function
    that has the parameter and passes it in as a hidden parameter.

    This attribute has two side effects that are key to how FORTIFY
    works:

    1. It can overload solely on pass_object_size (e.g. there are two
       overloads of foo in

         void foo(char * __attribute__((pass_object_size(0))) c);
         void foo(char *);

      (The one with pass_object_size attribute has preceded the
      default one).

    2. A function with at least one pass_object_size parameter can never
       have its address taken (and overload resolution respects this).

Thus the read wrapper can be implemented as follows, without
hindering any fortify coverage compile and runtime: 

Thus the read wrapper can be implemented as follows, without
hindering any fortify coverage compile and runtime:

  extern __inline
  __attribute__((__always_inline__))
  __attribute__((__artificial__))
  __attribute__((__overloadable__))
  __attribute__((__warn_unused_result__))
  ssize_t read (int __fd,
                void *const __attribute__((pass_object_size (0))) __buf,
                size_t __nbytes)
     __attribute__((__diagnose_if__ ((((__builtin_object_size (__buf,
0)) != -1ULL
                                        && (__nbytes) >
(__builtin_object_size (__buf, 0)) / (1))),
                                     "read called with bigger length
than the size of the destination buffer",
                                     "warning")))
  {
    return (__builtin_object_size (__buf, 0) == (size_t) -1)
      ? __read_alias (__fd,
                      __buf,
                      __nbytes)
      : __read_chk (__fd,
                    __buf,
                    __nbytes,
                    __builtin_object_size (__buf, 0));
  }

To avoid changing the current semantic for GCC, a set of macros is
defined to enable the clang required attributes, along with some changes
on internal macros to avoid the need to issue the symbol_chk symbols
(which are done through the __diagnose_if__ attribute for clang).
The read wrapper can be simplified as:

  __fortify_function __attribute_overloadable__ __wur
  ssize_t read (int __fd,
                __fortify_clang_overload_arg0 (void *, ,__buf),
                size_t __nbytes)
       __fortify_clang_warning_only_if_bos0_lt (__nbytes, __buf,
                                                "read called with bigger
length than "
                                                "size of the destination
buffer")

  {
    return __glibc_fortify (read, __nbytes, sizeof (char),
                            __glibc_objsize0 (__buf),
                            __fd, __buf, __nbytes);
  }

There is no expected semantic or code change when using GCC.

Also, clang does not support __va_arg_pack, so variadic functions are
expanded to call va_arg implementations.  The error function must not
have bodies (address takes are expanded to nonfortified calls), and
with the __fortify_function compiler might still create a body with the
C++ mangling name (due to the overload attribute).  In this case, the
function is defined with __fortify_function_error_function macro
instead.

To fully test it, I used my clang branch [4] which allowed me to fully
build all fortify tests with clang. With this patchset, there is no
regressions anymore.

[1] https://sourceware.org/legacy-ml/libc-alpha/2017-09/msg00434.html
[2] https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/refs/heads/main/sys-libs/glibc/files/local/glibc-2.35/0006-glibc-add-clang-style-FORTIFY.patch
[3] https://docs.google.com/document/d/1DFfZDICTbL7RqS74wJVIJ-YnjQOj1SaoqfhbgddFYSM/edit
[4] https://sourceware.org/git/?p=glibc.git;a=shortlog;h=refs/heads/azanella/clang

Changes from v1:
- Use implementation-namespace identifiers pass_object_size and
  pass_dynamic_object_size.
- Simplify the clang macros and enable it iff for clang 5.0
  (that supports __diagnose_if__).

Adhemerval Zanella (10):
  cdefs.h: Add clang fortify directives
  libio: Improve fortify with clang
  string: Improve fortify with clang
  stdlib: Improve fortify with clang
  unistd: Improve fortify with clang
  socket: Improve fortify with clang
  syslog: Improve fortify with clang
  wcsmbs: Improve fortify with clang
  debug: Improve fcntl.h fortify warnings with clang
  debug: Improve mqueue.h fortify warnings with clang

 io/bits/fcntl2.h               |  92 ++++++++++++++++++
 io/bits/poll2.h                |  29 ++++--
 io/fcntl.h                     |   3 +-
 libio/bits/stdio2.h            | 173 +++++++++++++++++++++++++++++----
 misc/bits/syslog.h             |  14 ++-
 misc/sys/cdefs.h               | 158 +++++++++++++++++++++++++++++-
 posix/bits/unistd.h            | 110 +++++++++++++++------
 rt/bits/mqueue2.h              |  29 ++++++
 rt/mqueue.h                    |   3 +-
 socket/bits/socket2.h          |  20 +++-
 stdlib/bits/stdlib.h           |  40 +++++---
 string/bits/string_fortified.h |  57 ++++++-----
 wcsmbs/bits/wchar2.h           | 167 ++++++++++++++++++++++---------
 13 files changed, 746 insertions(+), 149 deletions(-)
  

Am Montag, 8. Januar 2024, 21:21:39 CET schrieb Adhemerval Zanella:
> When using clang, the fortify wrappers show less coverage on both
> compile and runtime. For instance, a snippet from tst-fortify.c:
> 

This looks like post-release material to me.
  

Ping.

On 08/01/24 17:21, Adhemerval Zanella wrote:
> When using clang, the fortify wrappers show less coverage on both
> compile and runtime. For instance, a snippet from tst-fortify.c:
> 
>   $ cat t.c
>   #include <stdio.h>
>   #include <string.h>
>   
>   const char *str1 = "JIHGFEDCBA";
>   
>   int main (int argc, char *argv[])
>   {
>   #define O 0
>     struct A { char buf1[9]; char buf2[1]; } a;
>     strcpy (a.buf1 + (O + 4), str1 + 5);
>   
>     return 0;
>   }
>   $ gcc -O2 t.c  -Wp,-U_FORTIFY_SOURCE,-D_FORTIFY_SOURCE=2 -o t
>   $ ./t
>   *** buffer overflow detected ***: terminated
>   Aborted (core dumped)
>   $ clang  -O2 t.c  -Wp,-U_FORTIFY_SOURCE,-D_FORTIFY_SOURCE=2 -o t
>   $ ./t
>   $
> 
> Although clang does support __builtin___strcpy_chk (which correctly
> lowers to __strcpy_chk), the __builtin_object_size passed as third
> the argument is not fully correct and thus limits the possible runtime
> checks.
> 
> This limitation was already being raised some years ago [1], but the
> work has been staled.  However, a similar patch has been used for
> ChromeOS for some time [2].  Bionic libc also uses a similar approach to
> enable fortified wrappers.
> 
> Improve its support with clang, requires defining the fortified wrapper
> differently. For instance, the read wrapper is currently expanded as:
> 
>   extern __inline
>   __attribute__((__always_inline__))
>   __attribute__((__artificial__))
>   __attribute__((__warn_unused_result__))
>   ssize_t read (int __fd, void *__buf, size_t __nbytes)
>   {
>      return __glibc_safe_or_unknown_len (__nbytes,
>                                          sizeof (char),
>                                          __glibc_objsize0 (__buf))
>             ? __read_alias (__fd, __buf, __nbytes)
>             : __glibc_unsafe_len (__nbytes,
>                                   sizeof (char),
>                                   __glibc_objsize0 (__buf))
>               ? __read_chk_warn (__fd,
>                                  __buf,
>                                  __nbytes,
>                                  __builtin_object_size (__buf, 0))
>               : __read_chk (__fd,
>                             __buf,
>                             __nbytes,
>                             __builtin_object_size (__buf, 0));
>   }
> 
> The wrapper relies on __builtin_object_size call lowers to a constant at
> Compile time and many other operations in the wrapper depend on
> having a single, known value for parameters.   Because this is
> impossible to have for function parameters, the wrapper depends heavily
> on inlining to work and While this is an entirely viable approach on
> GCC is not fully reliable on clang.  This is because by the time llvm
> gets to inlining and optimizing, there is a minimal reliable source and
> type-level information available (more information on a more deep
> explanation on how to fortify wrapper works on clang [4]).
> 
> To allow the wrapper to work reliably and with the same functionality as
> with GCC, clang requires a different approach:
> 
>   * __attribute__((diagnose_if(c, “str”, “warning”))) which is a
>   * function
>     level attribute; if the compiler can determine that 'c' is true at
>     compile-time, it will emit a warning with the text 'str1'.  If it
>     would be better to emit an error, the wrapper can use "error"
>     instead of "warning".
> 
>   * __attribute__((overloadable)) which is also a function-level
>     attribute; and it allows C++-style overloading to occur on C
> functions.
> 
>   * __attribute__((pass_object_size(n))) which is a parameter-level
>     attribute; and it makes the compiler evaluate
>     __builtin_object_size(param, n) at each call site of the function
>     that has the parameter and passes it in as a hidden parameter.
> 
>     This attribute has two side effects that are key to how FORTIFY
>     works:
> 
>     1. It can overload solely on pass_object_size (e.g. there are two
>        overloads of foo in
> 
>          void foo(char * __attribute__((pass_object_size(0))) c);
>          void foo(char *);
> 
>       (The one with pass_object_size attribute has preceded the
>       default one).
> 
>     2. A function with at least one pass_object_size parameter can never
>        have its address taken (and overload resolution respects this).
> 
> Thus the read wrapper can be implemented as follows, without
> hindering any fortify coverage compile and runtime: 
> 
> Thus the read wrapper can be implemented as follows, without
> hindering any fortify coverage compile and runtime:
> 
>   extern __inline
>   __attribute__((__always_inline__))
>   __attribute__((__artificial__))
>   __attribute__((__overloadable__))
>   __attribute__((__warn_unused_result__))
>   ssize_t read (int __fd,
>                 void *const __attribute__((pass_object_size (0))) __buf,
>                 size_t __nbytes)
>      __attribute__((__diagnose_if__ ((((__builtin_object_size (__buf,
> 0)) != -1ULL
>                                         && (__nbytes) >
> (__builtin_object_size (__buf, 0)) / (1))),
>                                      "read called with bigger length
> than the size of the destination buffer",
>                                      "warning")))
>   {
>     return (__builtin_object_size (__buf, 0) == (size_t) -1)
>       ? __read_alias (__fd,
>                       __buf,
>                       __nbytes)
>       : __read_chk (__fd,
>                     __buf,
>                     __nbytes,
>                     __builtin_object_size (__buf, 0));
>   }
> 
> To avoid changing the current semantic for GCC, a set of macros is
> defined to enable the clang required attributes, along with some changes
> on internal macros to avoid the need to issue the symbol_chk symbols
> (which are done through the __diagnose_if__ attribute for clang).
> The read wrapper can be simplified as:
> 
>   __fortify_function __attribute_overloadable__ __wur
>   ssize_t read (int __fd,
>                 __fortify_clang_overload_arg0 (void *, ,__buf),
>                 size_t __nbytes)
>        __fortify_clang_warning_only_if_bos0_lt (__nbytes, __buf,
>                                                 "read called with bigger
> length than "
>                                                 "size of the destination
> buffer")
> 
>   {
>     return __glibc_fortify (read, __nbytes, sizeof (char),
>                             __glibc_objsize0 (__buf),
>                             __fd, __buf, __nbytes);
>   }
> 
> There is no expected semantic or code change when using GCC.
> 
> Also, clang does not support __va_arg_pack, so variadic functions are
> expanded to call va_arg implementations.  The error function must not
> have bodies (address takes are expanded to nonfortified calls), and
> with the __fortify_function compiler might still create a body with the
> C++ mangling name (due to the overload attribute).  In this case, the
> function is defined with __fortify_function_error_function macro
> instead.
> 
> To fully test it, I used my clang branch [4] which allowed me to fully
> build all fortify tests with clang. With this patchset, there is no
> regressions anymore.
> 
> [1] https://sourceware.org/legacy-ml/libc-alpha/2017-09/msg00434.html
> [2] https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/refs/heads/main/sys-libs/glibc/files/local/glibc-2.35/0006-glibc-add-clang-style-FORTIFY.patch
> [3] https://docs.google.com/document/d/1DFfZDICTbL7RqS74wJVIJ-YnjQOj1SaoqfhbgddFYSM/edit
> [4] https://sourceware.org/git/?p=glibc.git;a=shortlog;h=refs/heads/azanella/clang
> 
> Changes from v1:
> - Use implementation-namespace identifiers pass_object_size and
>   pass_dynamic_object_size.
> - Simplify the clang macros and enable it iff for clang 5.0
>   (that supports __diagnose_if__).
> 
> Adhemerval Zanella (10):
>   cdefs.h: Add clang fortify directives
>   libio: Improve fortify with clang
>   string: Improve fortify with clang
>   stdlib: Improve fortify with clang
>   unistd: Improve fortify with clang
>   socket: Improve fortify with clang
>   syslog: Improve fortify with clang
>   wcsmbs: Improve fortify with clang
>   debug: Improve fcntl.h fortify warnings with clang
>   debug: Improve mqueue.h fortify warnings with clang
> 
>  io/bits/fcntl2.h               |  92 ++++++++++++++++++
>  io/bits/poll2.h                |  29 ++++--
>  io/fcntl.h                     |   3 +-
>  libio/bits/stdio2.h            | 173 +++++++++++++++++++++++++++++----
>  misc/bits/syslog.h             |  14 ++-
>  misc/sys/cdefs.h               | 158 +++++++++++++++++++++++++++++-
>  posix/bits/unistd.h            | 110 +++++++++++++++------
>  rt/bits/mqueue2.h              |  29 ++++++
>  rt/mqueue.h                    |   3 +-
>  socket/bits/socket2.h          |  20 +++-
>  stdlib/bits/stdlib.h           |  40 +++++---
>  string/bits/string_fortified.h |  57 ++++++-----
>  wcsmbs/bits/wchar2.h           | 167 ++++++++++++++++++++++---------
>  13 files changed, 746 insertions(+), 149 deletions(-)
>