Re: [PATCH v2 00/10] Improve fortify support with clang

[Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>]

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(-)
>