Re: [PATCH v3 01/10] cdefs.h: Add clang fortify directives

[Sam James <sam@gentoo.org>]

[-- Attachment #1: Type: text/plain, Size: 17390 bytes --]


Siddhesh Poyarekar <siddhesh@gotplt.org> writes:

> Apologies for taking so long to get to this.  Overall I've been
> struggling to understand the design, mainly because of the amount of
> macro wizardry that's become necessary to implement this.
> [...]

I'm glad it's not just me. I wanted to at least partly review this but I
found it hard to see through the macros.

>
> On 2024-02-08 13:46, Adhemerval Zanella wrote:
>> 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));
>>    }
>
> It should be __glibc_objsz0 for all occurrences of
> __builtin_object_size you've mentioned above.  That is, it will pass
> __builtin_dynamic_object_size for _FORTIFY_SOURCE=3.
>
>> The wrapper relies on __builtin_object_size call lowers to a
>> constant at
>> compile-time and many other operations in the wrapper depends on
>> having a single, known value for parameters.   Because this is
>
> Actually, it doesn't.  It relies on the *comparison* of
> __builtin[_dynamic]_object_size with __nbytes having a compile-time
> constant value.  gcc can use ranges of __nbytes and __bos to arrive at
> a constant true/false value for the comparison even if those values
> themselves are not constant.
>
>> impossible to have for function parameters, the wrapper depends heavily
>> on inlining to work and While this is an entirely viable approach on
>> GCC, it 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 [1]).
>> 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 precende over
>> 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).
>
> ... and there's a pass_dynamic_object_size too, as you noted in the
> actual patch.
>
>> 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 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));
>>    }
>
> This would be sufficient for _FORTIFY_SOURCE=2, but not level 3, but I
> reckon this is just a problem with the description; the patch itself
> seems to cater for dynamic sizes.
>
>> 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 is 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.
>> [1]
>> https://docs.google.com/document/d/1DFfZDICTbL7RqS74wJVIJ-YnjQOj1SaoqfhbgddFYSM/edit
>> Checked on aarch64, armhf, x86_64, and i686.
>> ---
>>   misc/sys/cdefs.h | 151 ++++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 149 insertions(+), 2 deletions(-)
>> diff --git a/misc/sys/cdefs.h b/misc/sys/cdefs.h
>> index 520231dbea..62507044c8 100644
>> --- a/misc/sys/cdefs.h
>> +++ b/misc/sys/cdefs.h
>> @@ -145,6 +145,14 @@
>>   #endif
>>     +/* The overloadable attribute was added on clang 2.6. */
>> +#if defined __clang_major__ \
>> +    && (__clang_major__ + (__clang_minor__ >= 6) > 2)
>> +# define __attribute_overloadable__ __attribute__((__overloadable__))
>> +#else
>> +# define __attribute_overloadable__
>> +#endif
>> +
>>   /* Fortify support.  */
>>   #define __bos(ptr) __builtin_object_size (ptr, __USE_FORTIFY_LEVEL > 1)
>>   #define __bos0(ptr) __builtin_object_size (ptr, 0)
>> @@ -187,27 +195,166 @@
>>   						   __s, __osz))		      \
>>      && !__glibc_safe_len_cond ((__SIZE_TYPE__) (__l), __s, __osz))
>>   +/* To correctly instrument the fortify wrapper clang requires the
>> +   pass_object_size attribute, and the attribute has the restriction that the
>> +   argument needs to be 'const'.  Furthermore, to make it usable with C
>> +   interfaces, clang provides the overload attribute, which provides a C++
>> +   like function overload support.  The overloaded fortify wrapper with the
>> +   pass_object_size attribute has precedence over the default symbol.
>> +
>> +   Also, clang does not support __va_arg_pack, so variadic functions are
>> +   expanded to issue va_arg implementations. The error function must not have
>> +   bodies (address takes are expanded to nonfortified calls), and with
>> +   __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.
>> +
>> +   The argument size check is also done with a clang-only attribute,
>> +   __attribute__ ((__diagnose_if__ (...))), different than gcc which calls
>> +   symbol_chk_warn alias with uses __warnattr attribute.
>> +
>> +   The pass_object_size was added on clang 4.0, __diagnose_if__ on 5.0,
>> +   and pass_dynamic_object_size on 9.0.  */
>> +#if defined __clang_major__ && __clang_major__ >= 5
>> +# define __fortify_use_clang 1
>> +
>> +# define __fortify_function_error_function static __attribute__((__unused__))
>> +
>> +# define __fortify_clang_pass_object_size_n(n) \
>> +  __attribute__ ((__pass_object_size__ (n)))
>> +# define __fortify_clang_pass_object_size0 \
>> +  __fortify_clang_pass_object_size_n (0)
>> +# define __fortify_clang_pass_object_size \
>> +  __fortify_clang_pass_object_size_n (__USE_FORTIFY_LEVEL > 1)
>> +
>> +# if __clang_major__ >= 9
>> +#  define __fortify_clang_pass_dynamic_object_size_n(n) \
>> +  __attribute__ ((__pass_dynamic_object_size__ (n)))
>> +#  define __fortify_clang_pass_dynamic_object_size0 \
>> +  __fortify_clang_pass_dynamic_object_size_n (0)
>> +#  define __fortify_clang_pass_dynamic_object_size \
>> +  __fortify_clang_pass_dynamic_object_size_n (1)
>> +# else
>> +#  define __fortify_clang_pass_dynamic_object_size_n(n)
>> +#  define __fortify_clang_pass_dynamic_object_size0
>> +#  define __fortify_clang_pass_dynamic_object_size
>> +# endif
>> +
>> +# define __fortify_clang_bos_static_lt_impl(bos_val, n, s) \
>> +  ((bos_val) != -1ULL && (n) > (bos_val) / (s))
>> +# define __fortify_clang_bos_static_lt2(__n, __e, __s) \
>> +  __fortify_clang_bos_static_lt_impl (__bos (__e), __n, __s)
>> +# define __fortify_clang_bos_static_lt(__n, __e) \
>> +  __fortify_clang_bos_static_lt2 (__n, __e, 1)
>> +# define __fortify_clang_bos0_static_lt2(__n, __e, __s) \
>> +  __fortify_clang_bos_static_lt_impl (__bos0 (__e), __n, __s)
>> +# define __fortify_clang_bos0_static_lt(__n, __e) \
>> +  __fortify_clang_bos0_static_lt2 (__n, __e, 1)
>> +
>> +# define __fortify_clang_bosn_args(bos_fn, n, buf, div, complaint) \
>> +  (__fortify_clang_bos_static_lt_impl (bos_fn (buf), n, div)), (complaint), \
>> +  "warning"
>> +
>> +# define __fortify_clang_warning(__c, __msg) \
>> +  __attribute__ ((__diagnose_if__ ((__c), (__msg), "warning")))
>> +# define __fortify_clang_warning_only_if_bos0_lt(n, buf, complaint) \
>> +  __attribute__ ((__diagnose_if__ \
>> +		  (__fortify_clang_bosn_args (__bos0, n, buf, 1, complaint))))
>> +# define __fortify_clang_warning_only_if_bos0_lt2(n, buf, div, complaint) \
>> +  __attribute__ ((__diagnose_if__ \
>> +		  (__fortify_clang_bosn_args (__bos0, n, buf, div, complaint))))
>> +# define __fortify_clang_warning_only_if_bos_lt(n, buf, complaint) \
>> +  __attribute__ ((__diagnose_if__ \
>> +		  (__fortify_clang_bosn_args (__bos, n, buf, 1, complaint))))
>> +# define __fortify_clang_warning_only_if_bos_lt2(n, buf, div, complaint) \
>> +  __attribute__ ((__diagnose_if__ \
>> +		  (__fortify_clang_bosn_args (__bos, n, buf, div, complaint))))
>
> Do we need so many abstractions here?  ISTM that simply a
> __glibc_clang_warn() and then __fortify_warn_bos0 and
> __fortify_warn_bos ought to be sufficient, reusing/repurposing
> __glibc_unsafe_len instead of reimplementing the check in
> __fortify_clang_bos_static_lt_impl.  The multilevel bos_fn macros seem
> like overkill.
>
> Also given the amount of fortification related code here, I wonder if
> we should split out a separate misc/sys/cdefs-fortify.h.
>
>> +
>> +# if __USE_FORTIFY_LEVEL == 3
>> +#  define __fortify_clang_overload_arg(__type, __attr, __name) \
>> +  __type __attr const __fortify_clang_pass_dynamic_object_size __name
>> +#  define __fortify_clang_overload_arg0(__type, __attr, __name) \
>> +  __type __attr const __fortify_clang_pass_dynamic_object_size0 __name
>> +# else
>> +#  define __fortify_clang_overload_arg(__type, __attr, __name) \
>> +  __type __attr const __fortify_clang_pass_object_size __name
>> +#  define __fortify_clang_overload_arg0(__type, __attr, __name) \
>> +  __type __attr const __fortify_clang_pass_object_size0 __name
>> +# endif
>> +
>> +# define __fortify_clang_mul_may_overflow(size, n) \
>> +  ((size | n) >= (((size_t)1) << (8 * sizeof (size_t) / 2)))
>> +
>> +# define __fortify_clang_size_too_small(__bos, __dest, __len) \
>> +  (__bos (__dest) != (size_t) -1 && __bos (__dest) < __len)
>> +# define __fortify_clang_warn_if_src_too_large(__dest, __src) \
>> +  __fortify_clang_warning (__fortify_clang_size_too_small (__glibc_objsize, \
>> +							   __dest, \
>> +							   __builtin_strlen (__src) + 1), \
>> +			   "destination buffer will always be overflown by source")
>> +# define __fortify_clang_warn_if_dest_too_small(__dest, __len) \
>> +  __fortify_clang_warning (__fortify_clang_size_too_small (__glibc_objsize, \
>> +                                                           __dest, \
>> +                                                           __len), \
>> +                           "function called with bigger length than the destination buffer")
>> +# define __fortify_clang_warn_if_dest_too_small0(__dest, __len) \
>> +  __fortify_clang_warning (__fortify_clang_size_too_small (__glibc_objsize0, \
>> +                                                           __dest, \
>> +                                                           __len), \
>> +                           "function called with bigger length than the destination buffer")
>> +#else
>> +# define __fortify_use_clang 0
>> +# define __fortify_clang_warning(__c, __msg)
>> +# define __fortify_clang_warning_only_if_bos0_lt(__n, __buf, __complaint)
>> +# define __fortify_clang_warning_only_if_bos0_lt2(__n, __buf, __div, complaint)
>> +# define __fortify_clang_warning_only_if_bos_lt(__n, __buf, __complaint)
>> +# define __fortify_clang_warning_only_if_bos_lt2(__n, __buf, div, __complaint)
>> +# define __fortify_clang_overload_arg(__type, __attr, __name) \
>> + __type __attr __name
>> +# define __fortify_clang_overload_arg0(__type, __attr, __name) \
>> +  __fortify_clang_overload_arg (__type, __attr, __name)
>> +# define __fortify_clang_warn_if_src_too_large(__dest, __src)
>> +# define __fortify_clang_warn_if_dest_too_small(__dest, __len)
>> +# define __fortify_clang_warn_if_dest_too_small0(__dest, __len)
>> +#endif
>> +
>> +
>>   /* Fortify function f.  __f_alias, __f_chk and __f_chk_warn must be
>>      declared.  */
>>   -#define __glibc_fortify(f, __l, __s, __osz, ...) \
>> +#if !__fortify_use_clang
>> +# define __glibc_fortify(f, __l, __s, __osz, ...) \
>>     (__glibc_safe_or_unknown_len (__l, __s, __osz)			      \
>>      ? __ ## f ## _alias (__VA_ARGS__)					      \
>>      : (__glibc_unsafe_len (__l, __s, __osz)				      \
>>         ? __ ## f ## _chk_warn (__VA_ARGS__, __osz)			      \
>>         : __ ## f ## _chk (__VA_ARGS__, __osz)))
>> +#else
>> +# define __glibc_fortify(f, __l, __s, __osz, ...) \
>> +  (__osz == (__SIZE_TYPE__) -1)						      \
>> +   ? __ ## f ## _alias (__VA_ARGS__)					      \
>> +   : __ ## f ## _chk (__VA_ARGS__, __osz)
>> +#endif
>>     /* Fortify function f, where object size argument passed to f is
>> the number of
>>      elements and not total size.  */
>>   -#define __glibc_fortify_n(f, __l, __s, __osz, ...) \
>> +#if !__fortify_use_clang
>> +# define __glibc_fortify_n(f, __l, __s, __osz, ...) \
>>     (__glibc_safe_or_unknown_len (__l, __s, __osz)			      \
>>      ? __ ## f ## _alias (__VA_ARGS__)					      \
>>      : (__glibc_unsafe_len (__l, __s, __osz)				      \
>>         ? __ ## f ## _chk_warn (__VA_ARGS__, (__osz) / (__s))		      \
>>         : __ ## f ## _chk (__VA_ARGS__, (__osz) / (__s))))
>> +# else
>> +# define __glibc_fortify_n(f, __l, __s, __osz, ...) \
>> +  (__osz == (__SIZE_TYPE__) -1)						      \
>> +   ? __ ## f ## _alias (__VA_ARGS__)					      \
>> +   : __ ## f ## _chk (__VA_ARGS__, (__osz) / (__s))
>>   #endif
>>   +#endif /* __USE_FORTIFY_LEVEL > 0 */
>> +
>>   #if __GNUC_PREREQ (4,3)
>>   # define __warnattr(msg) __attribute__((__warning__ (msg)))
>>   # define __errordecl(name, msg) \


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 377 bytes --]