[PATCH v6 0/4] fortify: Add Clang support

[PATCH v6 0/4] fortify: Add Clang support

[Kees Cook]

Hi,

This has been updated from feedback on the v5 series. Builds correctly with Clang 12.0.1
too now. :)

Thanks!

v1: https://lore.kernel.org/linux-hardening/20210727205855.411487-61-keescook@chromium.org/
v2: https://lore.kernel.org/linux-hardening/20210818060533.3569517-64-keescook@chromium.org/
v3: https://lore.kernel.org/linux-hardening/20211213223331.135412-18-keescook@chromium.org/
v4: https://lore.kernel.org/linux-hardening/20220130182204.420775-1-keescook@chromium.org/
v5: https://lore.kernel.org/linux-hardening/20220202003033.704951-1-keescook@chromium.org/
v6:
 - clean up comments in attributes (ojeda)
 - moved const into pass_object_size macro
 - adjusted Clang version to 12.0.1 (ndesaulniers)
 - cleaned up Clang comments (ndesaulniers)

Kees Cook (4):
  Compiler Attributes: Add __pass_object_size for Clang
  Compiler Attributes: Add __overloadable for Clang
  Compiler Attributes: Add __diagnose_as for Clang
  fortify: Add Clang support

 include/linux/compiler_attributes.h | 39 +++++++++++++++++++
 include/linux/fortify-string.h      | 58 +++++++++++++++++++++--------
 security/Kconfig                    |  3 +-
 3 files changed, 83 insertions(+), 17 deletions(-)

-- 
2.30.2

[PATCH v6 1/4] Compiler Attributes: Add __pass_object_size for Clang

[Kees Cook]

In order to gain greater visibility to type information when using
__builtin_object_size(), Clang has a function attribute "pass_object_size"
that will make size information available for marked arguments in
a function by way of implicit additional function arguments that are
then wired up the __builtin_object_size().

This is needed to implement FORTIFY_SOURCE in Clang, as a workaround
to Clang's __builtin_object_size() having limited visibility[1] into types
across function calls (even inlines).

Since any usage must also be const, include it in the macro.

This attribute has an additional benefit that it can be used even on
non-inline functions to gain argument size information.

[1] https://github.com/llvm/llvm-project/issues/53516

Cc: Miguel Ojeda <ojeda@kernel.org>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Nathan Chancellor <nathan@kernel.org>
Cc: llvm@lists.linux.dev
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/compiler_attributes.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/include/linux/compiler_attributes.h b/include/linux/compiler_attributes.h
index 37e260020221..4ce370094e3a 100644
--- a/include/linux/compiler_attributes.h
+++ b/include/linux/compiler_attributes.h
@@ -263,6 +263,20 @@
  */
 #define __packed                        __attribute__((__packed__))
 
+/*
+ * Note: the "type" argument should match any __builtin_object_size(p, type) usage.
+ *
+ * Optional: not supported by gcc.
+ * Optional: not supported by icc.
+ *
+ * clang: https://clang.llvm.org/docs/AttributeReference.html#pass-object-size-pass-dynamic-object-size
+ */
+#if __has_attribute(__pass_object_size__)
+# define __pass_object_size(type)	const __attribute__((__pass_object_size__(type)))
+#else
+# define __pass_object_size(type)
+#endif
+
 /*
  *   gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-pure-function-attribute
  */
-- 
2.30.2

[PATCH v6 2/4] Compiler Attributes: Add __overloadable for Clang

[Kees Cook]

In order for FORTIFY_SOURCE to use __pass_object_size on an "inline
extern" function, as all the fortified string functions are, the functions
must be marked as being overloadable (i.e. different prototypes).
This allows the __pass_object_size versions to take precedence.

Cc: Miguel Ojeda <ojeda@kernel.org>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Nathan Chancellor <nathan@kernel.org>
Cc: llvm@lists.linux.dev
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/compiler_attributes.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/include/linux/compiler_attributes.h b/include/linux/compiler_attributes.h
index 4ce370094e3a..dc3bf2a6e1c9 100644
--- a/include/linux/compiler_attributes.h
+++ b/include/linux/compiler_attributes.h
@@ -257,6 +257,18 @@
  */
 #define __noreturn                      __attribute__((__noreturn__))
 
+/*
+ * Optional: not supported by gcc.
+ * Optional: not supported by icc.
+ *
+ * clang: https://clang.llvm.org/docs/AttributeReference.html#overloadable
+ */
+#if __has_attribute(__overloadable__)
+# define __overloadable			__attribute__((__overloadable__))
+#else
+# define __overloadable
+#endif
+
 /*
  *   gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Type-Attributes.html#index-packed-type-attribute
  * clang: https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html#index-packed-variable-attribute
-- 
2.30.2

[PATCH v6 3/4] Compiler Attributes: Add __diagnose_as for Clang

[Kees Cook]

Clang will perform various compile-time diagnostics on uses of various
functions (e.g. simple bounds-checking on strcpy(), etc). These
diagnostics can be assigned to other functions (for example, new
implementations of the string functions under CONFIG_FORTIFY_SOURCE)
using the "diagnose_as_builtin" attribute. This allows those functions
to retain their compile-time diagnostic warnings.

Cc: Miguel Ojeda <ojeda@kernel.org>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Nathan Chancellor <nathan@kernel.org>
Cc: llvm@lists.linux.dev
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/compiler_attributes.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/include/linux/compiler_attributes.h b/include/linux/compiler_attributes.h
index dc3bf2a6e1c9..df9c7e5e8818 100644
--- a/include/linux/compiler_attributes.h
+++ b/include/linux/compiler_attributes.h
@@ -100,6 +100,19 @@
 # define __copy(symbol)
 #endif
 
+/*
+ * Optional: not supported by gcc
+ * Optional: only supported since clang >= 14.0
+ * Optional: not supported by icc
+ *
+ * clang: https://clang.llvm.org/docs/AttributeReference.html#diagnose_as_builtin
+ */
+#if __has_attribute(__diagnose_as_builtin__)
+# define __diagnose_as(builtin...)	__attribute__((__diagnose_as_builtin__(builtin)))
+#else
+# define __diagnose_as(builtin...)
+#endif
+
 /*
  * Don't. Just don't. See commit 771c035372a0 ("deprecate the '__deprecated'
  * attribute warnings entirely and for good") for more information.
-- 
2.30.2

[PATCH v6 4/4] fortify: Add Clang support

[Kees Cook]

Enable FORTIFY_SOURCE support for Clang:

Use the new __pass_object_size and __overloadable attributes so
that Clang will have appropriate visibility into argument sizes such
that __builtin_object_size(p, 1) will behave correctly. Additional
details here:
    https://github.com/llvm/llvm-project/issues/53516
    https://github.com/ClangBuiltLinux/linux/issues/1401

When available, use the new __diagnose_as attribute to make sure no
compile-time diagnostic warnings are lost due to the effectively renamed
string functions.

Redefine strlen() as a macro that tests for being a constant expression
so that strlen() can still be used in static initializers, which was
lost when adding __pass_object_size and __overloadable.

Finally, a bug with __builtin_constant_p() of globally defined variables
was fixed in Clang 13 (and backported to 12.0.1), so FORTIFY support
must depend on that version or later. Additional details here:
    https://bugs.llvm.org/show_bug.cgi?id=41459
    commit a52f8a59aef4 ("fortify: Explicitly disable Clang support")

Cc: Miguel Ojeda <ojeda@kernel.org>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Nathan Chancellor <nathan@kernel.org>
Cc: George Burgess IV <gbiv@google.com>
Cc: llvm@lists.linux.dev
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/fortify-string.h | 58 +++++++++++++++++++++++++---------
 security/Kconfig               |  3 +-
 2 files changed, 44 insertions(+), 17 deletions(-)

diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
index c45159dbdaa1..2ffe4f2f79eb 100644
--- a/include/linux/fortify-string.h
+++ b/include/linux/fortify-string.h
@@ -2,7 +2,9 @@
 #ifndef _LINUX_FORTIFY_STRING_H_
 #define _LINUX_FORTIFY_STRING_H_
 
-#define __FORTIFY_INLINE extern __always_inline __attribute__((gnu_inline))
+#include <linux/const.h>
+
+#define __FORTIFY_INLINE extern __always_inline __attribute__((gnu_inline)) __overloadable
 #define __RENAME(x) __asm__(#x)
 
 void fortify_panic(const char *name) __noreturn __cold;
@@ -50,7 +52,17 @@ extern char *__underlying_strncpy(char *p, const char *q, __kernel_size_t size)
 #define __underlying_strncpy	__builtin_strncpy
 #endif
 
-__FORTIFY_INLINE char *strncpy(char *p, const char *q, __kernel_size_t size)
+/*
+ * Clang's use of __builtin_object_size() within inlines needs hinting via
+ * __pass_object_size(). The preference is to only ever use type 1 (member
+ * size, rather than struct size), but there remain some stragglers using
+ * type 0 that will be converted in the future.
+ */
+#define POS	__pass_object_size(1)
+#define POS0	__pass_object_size(0)
+
+__FORTIFY_INLINE __diagnose_as(__builtin_strncpy, 1, 2, 3)
+char *strncpy(char * POS p, const char *q, __kernel_size_t size)
 {
 	size_t p_size = __builtin_object_size(p, 1);
 
@@ -61,7 +73,8 @@ __FORTIFY_INLINE char *strncpy(char *p, const char *q, __kernel_size_t size)
 	return __underlying_strncpy(p, q, size);
 }
 
-__FORTIFY_INLINE char *strcat(char *p, const char *q)
+__FORTIFY_INLINE __diagnose_as(__builtin_strcat, 1, 2)
+char *strcat(char * POS p, const char *q)
 {
 	size_t p_size = __builtin_object_size(p, 1);
 
@@ -73,7 +86,7 @@ __FORTIFY_INLINE char *strcat(char *p, const char *q)
 }
 
 extern __kernel_size_t __real_strnlen(const char *, __kernel_size_t) __RENAME(strnlen);
-__FORTIFY_INLINE __kernel_size_t strnlen(const char *p, __kernel_size_t maxlen)
+__FORTIFY_INLINE __kernel_size_t strnlen(const char * POS p, __kernel_size_t maxlen)
 {
 	size_t p_size = __builtin_object_size(p, 1);
 	size_t p_len = __compiletime_strlen(p);
@@ -93,8 +106,16 @@ __FORTIFY_INLINE __kernel_size_t strnlen(const char *p, __kernel_size_t maxlen)
 	return ret;
 }
 
-/* defined after fortified strnlen to reuse it. */
-__FORTIFY_INLINE __kernel_size_t strlen(const char *p)
+/*
+ * Defined after fortified strnlen to reuse it. However, it must still be
+ * possible for strlen() to be used on compile-time strings for use in
+ * static initializers (i.e. as a constant expression).
+ */
+#define strlen(p)							\
+	__builtin_choose_expr(__is_constexpr(__builtin_strlen(p)),	\
+		__builtin_strlen(p), __fortify_strlen(p))
+__FORTIFY_INLINE __diagnose_as(__builtin_strlen, 1)
+__kernel_size_t __fortify_strlen(const char * POS p)
 {
 	__kernel_size_t ret;
 	size_t p_size = __builtin_object_size(p, 1);
@@ -110,7 +131,7 @@ __FORTIFY_INLINE __kernel_size_t strlen(const char *p)
 
 /* defined after fortified strlen to reuse it */
 extern size_t __real_strlcpy(char *, const char *, size_t) __RENAME(strlcpy);
-__FORTIFY_INLINE size_t strlcpy(char *p, const char *q, size_t size)
+__FORTIFY_INLINE size_t strlcpy(char * POS p, const char * POS q, size_t size)
 {
 	size_t p_size = __builtin_object_size(p, 1);
 	size_t q_size = __builtin_object_size(q, 1);
@@ -137,7 +158,7 @@ __FORTIFY_INLINE size_t strlcpy(char *p, const char *q, size_t size)
 
 /* defined after fortified strnlen to reuse it */
 extern ssize_t __real_strscpy(char *, const char *, size_t) __RENAME(strscpy);
-__FORTIFY_INLINE ssize_t strscpy(char *p, const char *q, size_t size)
+__FORTIFY_INLINE ssize_t strscpy(char * POS p, const char * POS q, size_t size)
 {
 	size_t len;
 	/* Use string size rather than possible enclosing struct size. */
@@ -183,7 +204,8 @@ __FORTIFY_INLINE ssize_t strscpy(char *p, const char *q, size_t size)
 }
 
 /* defined after fortified strlen and strnlen to reuse them */
-__FORTIFY_INLINE char *strncat(char *p, const char *q, __kernel_size_t count)
+__FORTIFY_INLINE __diagnose_as(__builtin_strncat, 1, 2, 3)
+char *strncat(char * POS p, const char * POS q, __kernel_size_t count)
 {
 	size_t p_len, copy_len;
 	size_t p_size = __builtin_object_size(p, 1);
@@ -354,7 +376,7 @@ __FORTIFY_INLINE void fortify_memcpy_chk(__kernel_size_t size,
 		memmove)
 
 extern void *__real_memscan(void *, int, __kernel_size_t) __RENAME(memscan);
-__FORTIFY_INLINE void *memscan(void *p, int c, __kernel_size_t size)
+__FORTIFY_INLINE void *memscan(void * POS0 p, int c, __kernel_size_t size)
 {
 	size_t p_size = __builtin_object_size(p, 0);
 
@@ -365,7 +387,8 @@ __FORTIFY_INLINE void *memscan(void *p, int c, __kernel_size_t size)
 	return __real_memscan(p, c, size);
 }
 
-__FORTIFY_INLINE int memcmp(const void *p, const void *q, __kernel_size_t size)
+__FORTIFY_INLINE __diagnose_as(__builtin_memcmp, 1, 2, 3)
+int memcmp(const void * POS0 p, const void * POS0 q, __kernel_size_t size)
 {
 	size_t p_size = __builtin_object_size(p, 0);
 	size_t q_size = __builtin_object_size(q, 0);
@@ -381,7 +404,8 @@ __FORTIFY_INLINE int memcmp(const void *p, const void *q, __kernel_size_t size)
 	return __underlying_memcmp(p, q, size);
 }
 
-__FORTIFY_INLINE void *memchr(const void *p, int c, __kernel_size_t size)
+__FORTIFY_INLINE __diagnose_as(__builtin_memchr, 1, 2, 3)
+void *memchr(const void * POS0 p, int c, __kernel_size_t size)
 {
 	size_t p_size = __builtin_object_size(p, 0);
 
@@ -393,7 +417,7 @@ __FORTIFY_INLINE void *memchr(const void *p, int c, __kernel_size_t size)
 }
 
 void *__real_memchr_inv(const void *s, int c, size_t n) __RENAME(memchr_inv);
-__FORTIFY_INLINE void *memchr_inv(const void *p, int c, size_t size)
+__FORTIFY_INLINE void *memchr_inv(const void * POS0 p, int c, size_t size)
 {
 	size_t p_size = __builtin_object_size(p, 0);
 
@@ -405,7 +429,7 @@ __FORTIFY_INLINE void *memchr_inv(const void *p, int c, size_t size)
 }
 
 extern void *__real_kmemdup(const void *src, size_t len, gfp_t gfp) __RENAME(kmemdup);
-__FORTIFY_INLINE void *kmemdup(const void *p, size_t size, gfp_t gfp)
+__FORTIFY_INLINE void *kmemdup(const void * POS0 p, size_t size, gfp_t gfp)
 {
 	size_t p_size = __builtin_object_size(p, 0);
 
@@ -417,7 +441,8 @@ __FORTIFY_INLINE void *kmemdup(const void *p, size_t size, gfp_t gfp)
 }
 
 /* Defined after fortified strlen to reuse it. */
-__FORTIFY_INLINE char *strcpy(char *p, const char *q)
+__FORTIFY_INLINE __diagnose_as(__builtin_strcpy, 1, 2)
+char *strcpy(char * POS p, const char * POS q)
 {
 	size_t p_size = __builtin_object_size(p, 1);
 	size_t q_size = __builtin_object_size(q, 1);
@@ -446,4 +471,7 @@ __FORTIFY_INLINE char *strcpy(char *p, const char *q)
 #undef __underlying_strncat
 #undef __underlying_strncpy
 
+#undef POS0
+#undef POS
+
 #endif /* _LINUX_FORTIFY_STRING_H_ */
diff --git a/security/Kconfig b/security/Kconfig
index 0b847f435beb..c125026ed088 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -177,9 +177,8 @@ config HARDENED_USERCOPY_PAGESPAN
 config FORTIFY_SOURCE
 	bool "Harden common str/mem functions against buffer overflows"
 	depends on ARCH_HAS_FORTIFY_SOURCE
-	# https://bugs.llvm.org/show_bug.cgi?id=50322
 	# https://bugs.llvm.org/show_bug.cgi?id=41459
-	depends on !CC_IS_CLANG
+	depends on !CC_IS_CLANG || CLANG_VERSION >= 120001
 	help
 	  Detect overflows of buffers in common string and memory functions
 	  where the compiler can determine and validate the buffer sizes.
-- 
2.30.2

Re: [PATCH v6 0/4] fortify: Add Clang support

[Miguel Ojeda]

On Thu, Feb 3, 2022 at 6:33 PM Kees Cook <keescook@chromium.org> wrote:
>
> This has been updated from feedback on the v5 series. Builds correctly with Clang 12.0.1
> too now. :)

Looks fine to me! Thanks for the changes. I assume you are taking these, so:

Reviewed-by: Miguel Ojeda <ojeda@kernel.org>

Cheers,
Miguel

Re: [PATCH v6 0/4] fortify: Add Clang support

[Kees Cook]

On Thu, Feb 03, 2022 at 06:47:01PM +0100, Miguel Ojeda wrote:
> On Thu, Feb 3, 2022 at 6:33 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > This has been updated from feedback on the v5 series. Builds correctly with Clang 12.0.1
> > too now. :)
> 
> Looks fine to me! Thanks for the changes. I assume you are taking these, so:
> 
> Reviewed-by: Miguel Ojeda <ojeda@kernel.org>

Thanks! Does this tag apply to the fortify-string.h patch as well?

-Kees

-- 
Kees Cook

Re: [PATCH v6 1/4] Compiler Attributes: Add __pass_object_size for Clang

[Nick Desaulniers]

On Thu, Feb 3, 2022 at 9:33 AM Kees Cook <keescook@chromium.org> wrote:
>
> In order to gain greater visibility to type information when using
> __builtin_object_size(), Clang has a function attribute "pass_object_size"
> that will make size information available for marked arguments in
> a function by way of implicit additional function arguments that are
> then wired up the __builtin_object_size().
>
> This is needed to implement FORTIFY_SOURCE in Clang, as a workaround
> to Clang's __builtin_object_size() having limited visibility[1] into types
> across function calls (even inlines).
>
> Since any usage must also be const, include it in the macro.

I really don't like hiding the qualifier in the argument-attribute
macro like that.  I'd rather we be more explicit about changing the
function interfaces in include/linux/fortify-string.h.

For instance, in patch 4/4, let's take a look at this hunk:

-__FORTIFY_INLINE char *strncpy(char *p, const char *q, __kernel_size_t size)
...
+__FORTIFY_INLINE __diagnose_as(__builtin_strncpy, 1, 2, 3)
+char *strncpy(char * POS p, const char *q, __kernel_size_t size)

manually expanded, this has changed the qualifiers on the type of the
first parameter from `char *` to `char * const`.

I think it might be helpful to quote the docs
(https://clang.llvm.org/docs/AttributeReference.html#pass-object-size-pass-dynamic-object-size)

>> Additionally, any parameter that pass_object_size is applied to must be marked const at its function’s definition.

One thing that's concerning to me is though:

>> It is an error to take the address of a function with pass_object_size on any of its parameters.

Surely the kernel has indirect calls to some of these functions
somewhere? Is that just an issue for C++ name-mangling perhaps?

>
> This attribute has an additional benefit that it can be used even on
> non-inline functions to gain argument size information.
>
> [1] https://github.com/llvm/llvm-project/issues/53516
>
> Cc: Miguel Ojeda <ojeda@kernel.org>
> Cc: Nick Desaulniers <ndesaulniers@google.com>
> Cc: Nathan Chancellor <nathan@kernel.org>
> Cc: llvm@lists.linux.dev
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  include/linux/compiler_attributes.h | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/include/linux/compiler_attributes.h b/include/linux/compiler_attributes.h
> index 37e260020221..4ce370094e3a 100644
> --- a/include/linux/compiler_attributes.h
> +++ b/include/linux/compiler_attributes.h
> @@ -263,6 +263,20 @@
>   */
>  #define __packed                        __attribute__((__packed__))
>
> +/*
> + * Note: the "type" argument should match any __builtin_object_size(p, type) usage.
> + *
> + * Optional: not supported by gcc.
> + * Optional: not supported by icc.
> + *
> + * clang: https://clang.llvm.org/docs/AttributeReference.html#pass-object-size-pass-dynamic-object-size
> + */
> +#if __has_attribute(__pass_object_size__)
> +# define __pass_object_size(type)      const __attribute__((__pass_object_size__(type)))
> +#else
> +# define __pass_object_size(type)
> +#endif
> +
>  /*
>   *   gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-pure-function-attribute
>   */
> --
> 2.30.2
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v6 2/4] Compiler Attributes: Add __overloadable for Clang

[Nick Desaulniers]

On Thu, Feb 3, 2022 at 9:33 AM Kees Cook <keescook@chromium.org> wrote:
>
> In order for FORTIFY_SOURCE to use __pass_object_size on an "inline
> extern" function, as all the fortified string functions are, the functions

s/inline extern/extern inline/

Yes, they're the same thing, but it sounds backwards when I read it.

> must be marked as being overloadable (i.e. different prototypes).
> This allows the __pass_object_size versions to take precedence.

Is this because of the `const` additions to the function signatures?

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v6 3/4] Compiler Attributes: Add __diagnose_as for Clang

[Nick Desaulniers]

On Thu, Feb 3, 2022 at 9:33 AM Kees Cook <keescook@chromium.org> wrote:
>
> Clang will perform various compile-time diagnostics on uses of various
> functions (e.g. simple bounds-checking on strcpy(), etc). These
> diagnostics can be assigned to other functions (for example, new
> implementations of the string functions under CONFIG_FORTIFY_SOURCE)
> using the "diagnose_as_builtin" attribute. This allows those functions
> to retain their compile-time diagnostic warnings.
>
> Cc: Miguel Ojeda <ojeda@kernel.org>
> Cc: Nick Desaulniers <ndesaulniers@google.com>
> Cc: Nathan Chancellor <nathan@kernel.org>
> Cc: llvm@lists.linux.dev
> Signed-off-by: Kees Cook <keescook@chromium.org>

Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

> ---
>  include/linux/compiler_attributes.h | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/include/linux/compiler_attributes.h b/include/linux/compiler_attributes.h
> index dc3bf2a6e1c9..df9c7e5e8818 100644
> --- a/include/linux/compiler_attributes.h
> +++ b/include/linux/compiler_attributes.h
> @@ -100,6 +100,19 @@
>  # define __copy(symbol)
>  #endif
>
> +/*
> + * Optional: not supported by gcc
> + * Optional: only supported since clang >= 14.0
> + * Optional: not supported by icc
> + *
> + * clang: https://clang.llvm.org/docs/AttributeReference.html#diagnose_as_builtin
> + */
> +#if __has_attribute(__diagnose_as_builtin__)
> +# define __diagnose_as(builtin...)     __attribute__((__diagnose_as_builtin__(builtin)))
> +#else
> +# define __diagnose_as(builtin...)
> +#endif
> +
>  /*
>   * Don't. Just don't. See commit 771c035372a0 ("deprecate the '__deprecated'
>   * attribute warnings entirely and for good") for more information.
> --
> 2.30.2
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v6 4/4] fortify: Add Clang support

[Nick Desaulniers]

On Thu, Feb 3, 2022 at 9:33 AM Kees Cook <keescook@chromium.org> wrote:
>
> Enable FORTIFY_SOURCE support for Clang:
>
> Use the new __pass_object_size and __overloadable attributes so
> that Clang will have appropriate visibility into argument sizes such
> that __builtin_object_size(p, 1) will behave correctly. Additional
> details here:
>     https://github.com/llvm/llvm-project/issues/53516
>     https://github.com/ClangBuiltLinux/linux/issues/1401
>
> When available, use the new __diagnose_as attribute to make sure no
> compile-time diagnostic warnings are lost due to the effectively renamed
> string functions.

Consider adding something along the lines of the following to the
above paragraph:
Without diagnose_as, compile time error messages won't be as precise
as they could be, but at least users of older toolchains will have
fortified routines. That is more valuable, but certainly a tradeoff.

>
> Redefine strlen() as a macro that tests for being a constant expression
> so that strlen() can still be used in static initializers, which was
> lost when adding __pass_object_size and __overloadable.

I'd like to see `const` changes explicit in 4/4; I suspect that's
_why_ __overloadable is even needed? If so, then a comment here about
that wouldn't hurt.

Having const be more explicit in the signature will make it more
obvious why the definition cannot modify the parameter.


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v6 1/4] Compiler Attributes: Add __pass_object_size for Clang

[Kees Cook]

On Thu, Feb 03, 2022 at 12:18:24PM -0800, Nick Desaulniers wrote:
> On Thu, Feb 3, 2022 at 9:33 AM Kees Cook <keescook@chromium.org> wrote:
> >
> > In order to gain greater visibility to type information when using
> > __builtin_object_size(), Clang has a function attribute "pass_object_size"
> > that will make size information available for marked arguments in
> > a function by way of implicit additional function arguments that are
> > then wired up the __builtin_object_size().
> >
> > This is needed to implement FORTIFY_SOURCE in Clang, as a workaround
> > to Clang's __builtin_object_size() having limited visibility[1] into types
> > across function calls (even inlines).
> >
> > Since any usage must also be const, include it in the macro.
> 
> I really don't like hiding the qualifier in the argument-attribute
> macro like that.  I'd rather we be more explicit about changing the
> function interfaces in include/linux/fortify-string.h.

It seemed redundant to have it separate when it would always be needed.
In v5 I had the const in the BOS (now POS) macro, but realized that this
was silly since it would always need to be that way for __pos.

> For instance, in patch 4/4, let's take a look at this hunk:
> 
> -__FORTIFY_INLINE char *strncpy(char *p, const char *q, __kernel_size_t size)
> ...
> +__FORTIFY_INLINE __diagnose_as(__builtin_strncpy, 1, 2, 3)
> +char *strncpy(char * POS p, const char *q, __kernel_size_t size)
> 
> manually expanded, this has changed the qualifiers on the type of the
> first parameter from `char *` to `char * const`.

Yup, that's expected, and I wanted to tie it strictly to the expansion
of __pass_object_size since otherwise GCC would gain the "const" bit
(which technically shouldn't matter, but this whole series has been
nothing but corner case after corner case, and I didn't want to risk
another one).

> I think it might be helpful to quote the docs
> (https://clang.llvm.org/docs/AttributeReference.html#pass-object-size-pass-dynamic-object-size)
> 
> >> Additionally, any parameter that pass_object_size is applied to must be marked const at its function’s definition.
> 
> One thing that's concerning to me is though:
> 
> >> It is an error to take the address of a function with pass_object_size on any of its parameters.
> 
> Surely the kernel has indirect calls to some of these functions
> somewhere? Is that just an issue for C++ name-mangling perhaps?

AFAIU, this shouldn't be a problem for any of these. Nothing is trying
to take memcpy, memset, etc by address. The macro-ified version of this
change proved that out. :)

-- 
Kees Cook

Re: [PATCH v6 2/4] Compiler Attributes: Add __overloadable for Clang

[Kees Cook]

On Thu, Feb 03, 2022 at 12:26:15PM -0800, Nick Desaulniers wrote:
> On Thu, Feb 3, 2022 at 9:33 AM Kees Cook <keescook@chromium.org> wrote:
> >
> > In order for FORTIFY_SOURCE to use __pass_object_size on an "inline
> > extern" function, as all the fortified string functions are, the functions
> 
> s/inline extern/extern inline/

I will update that. :)

> 
> Yes, they're the same thing, but it sounds backwards when I read it.
> 
> > must be marked as being overloadable (i.e. different prototypes).
> > This allows the __pass_object_size versions to take precedence.
> 
> Is this because of the `const` additions to the function signatures?

That might be an issue, but the *real* issue is the implicit mutation of
the function into an inline with _additional_ arguments. i.e.

char *strcpy(char * POS p, const char * POS q)

is really

char *strcpy(char * const p, const char * const q, size_t __size_of_p, size_t __size_of_q)

(i.e. what I was doing with macros, but all internally and still an
extern inline)

-- 
Kees Cook

Re: [PATCH v6 0/4] fortify: Add Clang support

[Miguel Ojeda]

On Thu, Feb 3, 2022 at 8:57 PM Kees Cook <keescook@chromium.org> wrote:
>
> Thanks! Does this tag apply to the fortify-string.h patch as well?

No, I would have to take a better look at the new attributes. If you
need it, I can try to find a bit of time.

Cheers,
Miguel

Re: [PATCH v6 4/4] fortify: Add Clang support

[Kees Cook]

On Thu, Feb 03, 2022 at 12:37:41PM -0800, Nick Desaulniers wrote:
> On Thu, Feb 3, 2022 at 9:33 AM Kees Cook <keescook@chromium.org> wrote:
> >
> > Enable FORTIFY_SOURCE support for Clang:
> >
> > Use the new __pass_object_size and __overloadable attributes so
> > that Clang will have appropriate visibility into argument sizes such
> > that __builtin_object_size(p, 1) will behave correctly. Additional
> > details here:
> >     https://github.com/llvm/llvm-project/issues/53516
> >     https://github.com/ClangBuiltLinux/linux/issues/1401
> >
> > When available, use the new __diagnose_as attribute to make sure no
> > compile-time diagnostic warnings are lost due to the effectively renamed
> > string functions.
> 
> Consider adding something along the lines of the following to the
> above paragraph:
> Without diagnose_as, compile time error messages won't be as precise
> as they could be, but at least users of older toolchains will have
> fortified routines. That is more valuable, but certainly a tradeoff.

Sure, I've changed it to:

When available, use the new __diagnose_as attribute to make sure no
compile-time diagnostic warnings are lost due to the effectively renamed
string functions. Without __diagnose_as, Clang's compile time diagnostic
messages won't be as precise as they could be, but at least users of
older toolchains will have fortified routines.

how's that read for you?

> > Redefine strlen() as a macro that tests for being a constant expression
> > so that strlen() can still be used in static initializers, which was
> > lost when adding __pass_object_size and __overloadable.
> 
> I'd like to see `const` changes explicit in 4/4; I suspect that's
> _why_ __overloadable is even needed? If so, then a comment here about
> that wouldn't hurt.
> 
> Having const be more explicit in the signature will make it more
> obvious why the definition cannot modify the parameter.

Mostly I wanted to minimize further changes to this area when building
with GCC because of all the corner cases that keep popping up, and avoid
tweaking the prototypes any harder. :)

-- 
Kees Cook

Re: [PATCH v6 1/4] Compiler Attributes: Add __pass_object_size for Clang

[Nick Desaulniers]

On Thu, Feb 3, 2022 at 12:58 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Thu, Feb 03, 2022 at 12:18:24PM -0800, Nick Desaulniers wrote:
> > On Thu, Feb 3, 2022 at 9:33 AM Kees Cook <keescook@chromium.org> wrote:
> > >
> > > In order to gain greater visibility to type information when using
> > > __builtin_object_size(), Clang has a function attribute "pass_object_size"
> > > that will make size information available for marked arguments in
> > > a function by way of implicit additional function arguments that are
> > > then wired up the __builtin_object_size().
> > >
> > > This is needed to implement FORTIFY_SOURCE in Clang, as a workaround
> > > to Clang's __builtin_object_size() having limited visibility[1] into types
> > > across function calls (even inlines).
> > >
> > > Since any usage must also be const, include it in the macro.
> >
> > I think it might be helpful to quote the docs
> > (https://clang.llvm.org/docs/AttributeReference.html#pass-object-size-pass-dynamic-object-size)
> >
> > >> Additionally, any parameter that pass_object_size is applied to must be marked const at its function’s definition.
> >
> > One thing that's concerning to me is though:
> >
> > >> It is an error to take the address of a function with pass_object_size on any of its parameters.
> >
> > Surely the kernel has indirect calls to some of these functions
> > somewhere? Is that just an issue for C++ name-mangling perhaps?
>
> AFAIU, this shouldn't be a problem for any of these. Nothing is trying
> to take memcpy, memset, etc by address. The macro-ified version of this
> change proved that out. :)

I thought Sami had found a location where memcpy was invoked
indirectly as part of his kcfi work? Maybe I'm misremembering.

https://github.com/samitolvanen/linux/commit/46a777fb35784a8c6daf13d67de8bfb5148adc2a#diff-a27660992abdf360d01deac6364db31836d0d98b5d9573b7fc10a6785a669975R16
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v6 2/4] Compiler Attributes: Add __overloadable for Clang

[Nick Desaulniers]

On Thu, Feb 3, 2022 at 1:04 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Thu, Feb 03, 2022 at 12:26:15PM -0800, Nick Desaulniers wrote:
> > On Thu, Feb 3, 2022 at 9:33 AM Kees Cook <keescook@chromium.org> wrote:
> > >
> > > must be marked as being overloadable (i.e. different prototypes).
> > > This allows the __pass_object_size versions to take precedence.
> >
> > Is this because of the `const` additions to the function signatures?
>
> That might be an issue, but the *real* issue is the implicit mutation of
> the function into an inline with _additional_ arguments. i.e.
>
> char *strcpy(char * POS p, const char * POS q)
>
> is really
>
> char *strcpy(char * const p, const char * const q, size_t __size_of_p, size_t __size_of_q)
>
> (i.e. what I was doing with macros, but all internally and still an
> extern inline)

What do you mean "is really"? 4/4 doesn't change the number of
parameters in strcpy explicitly in the definition AFAICT.
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v6 2/4] Compiler Attributes: Add __overloadable for Clang

[Kees Cook]

On Thu, Feb 03, 2022 at 02:11:48PM -0800, Nick Desaulniers wrote:
> On Thu, Feb 3, 2022 at 1:04 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Thu, Feb 03, 2022 at 12:26:15PM -0800, Nick Desaulniers wrote:
> > > On Thu, Feb 3, 2022 at 9:33 AM Kees Cook <keescook@chromium.org> wrote:
> > > >
> > > > must be marked as being overloadable (i.e. different prototypes).
> > > > This allows the __pass_object_size versions to take precedence.
> > >
> > > Is this because of the `const` additions to the function signatures?
> >
> > That might be an issue, but the *real* issue is the implicit mutation of
> > the function into an inline with _additional_ arguments. i.e.
> >
> > char *strcpy(char * POS p, const char * POS q)
> >
> > is really
> >
> > char *strcpy(char * const p, const char * const q, size_t __size_of_p, size_t __size_of_q)
> >
> > (i.e. what I was doing with macros, but all internally and still an
> > extern inline)
> 
> What do you mean "is really"? 4/4 doesn't change the number of
> parameters in strcpy explicitly in the definition AFAICT.

It really does change the number of parameters. See the IR difference:

$ cat example.c
#ifdef USE_POS
# define POS __attribute__((pass_object_size(1)))
#else
# define POS
#endif

int func(void * const POS);

struct foo
{
        int a;
        char *b;
};

void usage(struct foo *example)
{
        func(example);
}

$ IR="-O2 -Xclang -disable-llvm-passes -emit-llvm -S"
$ clang           example.c $IR -o normal.ll
$ clang -DUSE_POS example.c $IR -o pos.ll
$ diff -u normal.ll pos.ll
--- normal.ll   2022-02-03 16:23:39.734065036 -0800
+++ pos.ll      2022-02-03 16:23:49.518083451 -0800
@@ -11,14 +11,19 @@
   store %struct.foo* %0, %struct.foo** %2, align 8, !tbaa !3
   %3 = load %struct.foo*, %struct.foo** %2, align 8, !tbaa !3
   %4 = bitcast %struct.foo* %3 to i8*
-  %5 = call i32 @func(i8* noundef %4)
+  %5 = call i64 @llvm.objectsize.i64.p0i8(i8* %4, i1 false, i1 true, i1 false)
+  %6 = call i32 @func(i8* noundef %4, i64 noundef %5)
   ret void
 }
...

This is basically doing internally exactly what I was doing in v4 and
earlier with macros (passing in the caller's view of __bos(arg, 1)).

-- 
Kees Cook

Re: [PATCH v6 1/4] Compiler Attributes: Add __pass_object_size for Clang

[Kees Cook]

On Thu, Feb 03, 2022 at 02:01:06PM -0800, Nick Desaulniers wrote:
> On Thu, Feb 3, 2022 at 12:58 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Thu, Feb 03, 2022 at 12:18:24PM -0800, Nick Desaulniers wrote:
> > > On Thu, Feb 3, 2022 at 9:33 AM Kees Cook <keescook@chromium.org> wrote:
> > > >
> > > > In order to gain greater visibility to type information when using
> > > > __builtin_object_size(), Clang has a function attribute "pass_object_size"
> > > > that will make size information available for marked arguments in
> > > > a function by way of implicit additional function arguments that are
> > > > then wired up the __builtin_object_size().
> > > >
> > > > This is needed to implement FORTIFY_SOURCE in Clang, as a workaround
> > > > to Clang's __builtin_object_size() having limited visibility[1] into types
> > > > across function calls (even inlines).
> > > >
> > > > Since any usage must also be const, include it in the macro.
> > >
> > > I think it might be helpful to quote the docs
> > > (https://clang.llvm.org/docs/AttributeReference.html#pass-object-size-pass-dynamic-object-size)
> > >
> > > >> Additionally, any parameter that pass_object_size is applied to must be marked const at its function’s definition.
> > >
> > > One thing that's concerning to me is though:
> > >
> > > >> It is an error to take the address of a function with pass_object_size on any of its parameters.
> > >
> > > Surely the kernel has indirect calls to some of these functions
> > > somewhere? Is that just an issue for C++ name-mangling perhaps?
> >
> > AFAIU, this shouldn't be a problem for any of these. Nothing is trying
> > to take memcpy, memset, etc by address. The macro-ified version of this
> > change proved that out. :)
> 
> I thought Sami had found a location where memcpy was invoked
> indirectly as part of his kcfi work? Maybe I'm misremembering.
> 
> https://github.com/samitolvanen/linux/commit/46a777fb35784a8c6daf13d67de8bfb5148adc2a#diff-a27660992abdf360d01deac6364db31836d0d98b5d9573b7fc10a6785a669975R16

Hm, I've had memcpy as a macro for a while now, so dunno! That's not a
sensible thing to call indirectly. :)

-- 
Kees Cook

Re: [PATCH v6 2/4] Compiler Attributes: Add __overloadable for Clang

[Nick Desaulniers]

On Thu, Feb 3, 2022 at 4:26 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Thu, Feb 03, 2022 at 02:11:48PM -0800, Nick Desaulniers wrote:
> > On Thu, Feb 3, 2022 at 1:04 PM Kees Cook <keescook@chromium.org> wrote:
> > >
> > > On Thu, Feb 03, 2022 at 12:26:15PM -0800, Nick Desaulniers wrote:
> > > > On Thu, Feb 3, 2022 at 9:33 AM Kees Cook <keescook@chromium.org> wrote:
> > > > >
> > > > > must be marked as being overloadable (i.e. different prototypes).
> > > > > This allows the __pass_object_size versions to take precedence.
> > > >
> > > > Is this because of the `const` additions to the function signatures?
> > >
> > > That might be an issue, but the *real* issue is the implicit mutation of
> > > the function into an inline with _additional_ arguments. i.e.
> > >
> > > char *strcpy(char * POS p, const char * POS q)
> > >
> > > is really
> > >
> > > char *strcpy(char * const p, const char * const q, size_t __size_of_p, size_t __size_of_q)
> > >
> > > (i.e. what I was doing with macros, but all internally and still an
> > > extern inline)
> >
> > What do you mean "is really"? 4/4 doesn't change the number of
> > parameters in strcpy explicitly in the definition AFAICT.
>
> It really does change the number of parameters. See the IR difference:
>
> $ cat example.c
> #ifdef USE_POS
> # define POS __attribute__((pass_object_size(1)))
> #else
> # define POS
> #endif
>
> int func(void * const POS);
>
> struct foo
> {
>         int a;
>         char *b;
> };
>
> void usage(struct foo *example)
> {
>         func(example);
> }
>
> $ IR="-O2 -Xclang -disable-llvm-passes -emit-llvm -S"
> $ clang           example.c $IR -o normal.ll
> $ clang -DUSE_POS example.c $IR -o pos.ll
> $ diff -u normal.ll pos.ll
> --- normal.ll   2022-02-03 16:23:39.734065036 -0800
> +++ pos.ll      2022-02-03 16:23:49.518083451 -0800
> @@ -11,14 +11,19 @@
>    store %struct.foo* %0, %struct.foo** %2, align 8, !tbaa !3
>    %3 = load %struct.foo*, %struct.foo** %2, align 8, !tbaa !3
>    %4 = bitcast %struct.foo* %3 to i8*
> -  %5 = call i32 @func(i8* noundef %4)
> +  %5 = call i64 @llvm.objectsize.i64.p0i8(i8* %4, i1 false, i1 true, i1 false)
> +  %6 = call i32 @func(i8* noundef %4, i64 noundef %5)
>    ret void
>  }
> ...

Woah, that's fancy.

>
> This is basically doing internally exactly what I was doing in v4 and
> earlier with macros (passing in the caller's view of __bos(arg, 1)).

Yeah. Ok now it all makes sense to me.

With the s/inline extern/extern inline/:
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v6 2/4] Compiler Attributes: Add __overloadable for Clang

[Miguel Ojeda]

On Thu, Feb 3, 2022 at 10:04 PM Kees Cook <keescook@chromium.org> wrote:
>
> That might be an issue, but the *real* issue is the implicit mutation of
> the function into an inline with _additional_ arguments. i.e.
>
> char *strcpy(char * POS p, const char * POS q)
>
> is really
>
> char *strcpy(char * const p, const char * const q, size_t __size_of_p, size_t __size_of_q)

Shouldn't that be

  char *strcpy(char * const p, size_t __size_of_p, const char * const
q, size_t __size_of_q)

? i.e. the docs point at this but say:

  "...and implicitly pass the result of this call in as an invisible
argument of type `size_t` directly after the parameter annotated with
`pass_object_size`."

Cheers,
Miguel