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

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

[Kees Cook]

Hi,

So, after looking at v4 at little longer I decided that it is just too
invasive. After spending time researching the primary issue that needed to
be worked around (__builtin_object_size(p, 1) not working from inlines),
I got some help from gbiv to use some Clang-specific attributes to get the
same effect.

I think the result is much less invasive, and it even lets us easily
expand size verification coverage into non-inlines if we ever want to.

Please take a look. :)

-Kees

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: - rewritten to use Clang attributes

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

 include/linux/compiler_attributes.h | 29 ++++++++++++++++
 include/linux/fortify-string.h      | 52 ++++++++++++++++++++---------
 security/Kconfig                    |  2 +-
 3 files changed, 67 insertions(+), 16 deletions(-)

-- 
2.30.2

[PATCH 1/4] Compiler Attributes: Add Clang's __pass_object_size

[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).

This 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 | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/linux/compiler_attributes.h b/include/linux/compiler_attributes.h
index 37e260020221..cc751e0770f5 100644
--- a/include/linux/compiler_attributes.h
+++ b/include/linux/compiler_attributes.h
@@ -263,6 +263,17 @@
  */
 #define __packed                        __attribute__((__packed__))
 
+/*
+ * clang: https://clang.llvm.org/docs/AttributeReference.html#pass-object-size-pass-dynamic-object-size
+ *
+ * The "type" argument should match the __builtin_object_size(p, type) usage.
+ */
+#if __has_attribute(__pass_object_size__)
+# define __pass_object_size(type)	__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 2/4] Compiler Attributes: Add __overloadable

[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 | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/linux/compiler_attributes.h b/include/linux/compiler_attributes.h
index cc751e0770f5..51e063347fd6 100644
--- a/include/linux/compiler_attributes.h
+++ b/include/linux/compiler_attributes.h
@@ -257,6 +257,15 @@
  */
 #define __noreturn                      __attribute__((__noreturn__))
 
+/*
+ * 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 3/4] Compiler Attributes: Add __diagnose_as

[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 | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/linux/compiler_attributes.h b/include/linux/compiler_attributes.h
index 51e063347fd6..b4ae0d771302 100644
--- a/include/linux/compiler_attributes.h
+++ b/include/linux/compiler_attributes.h
@@ -100,6 +100,15 @@
 # define __copy(symbol)
 #endif
 
+/*
+ * 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 4/4 v5] 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 types 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

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, 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 | 52 ++++++++++++++++++++++++----------
 security/Kconfig               |  2 +-
 2 files changed, 38 insertions(+), 16 deletions(-)

diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
index c45159dbdaa1..5482536d3197 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,11 @@ 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)
+#define BOS	const __pass_object_size(1)
+#define BOS0	const __pass_object_size(0)
+
+__FORTIFY_INLINE __diagnose_as(__builtin_strncpy, 1, 2, 3)
+char *strncpy(char * BOS p, const char *q, __kernel_size_t size)
 {
 	size_t p_size = __builtin_object_size(p, 1);
 
@@ -61,7 +67,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 * BOS p, const char *q)
 {
 	size_t p_size = __builtin_object_size(p, 1);
 
@@ -73,7 +80,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 * BOS p, __kernel_size_t maxlen)
 {
 	size_t p_size = __builtin_object_size(p, 1);
 	size_t p_len = __compiletime_strlen(p);
@@ -93,8 +100,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 * BOS p)
 {
 	__kernel_size_t ret;
 	size_t p_size = __builtin_object_size(p, 1);
@@ -110,7 +125,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 * BOS p, const char * BOS q, size_t size)
 {
 	size_t p_size = __builtin_object_size(p, 1);
 	size_t q_size = __builtin_object_size(q, 1);
@@ -137,7 +152,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 * BOS p, const char * BOS q, size_t size)
 {
 	size_t len;
 	/* Use string size rather than possible enclosing struct size. */
@@ -183,7 +198,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 * BOS p, const char * BOS q, __kernel_size_t count)
 {
 	size_t p_len, copy_len;
 	size_t p_size = __builtin_object_size(p, 1);
@@ -354,7 +370,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 * BOS0 p, int c, __kernel_size_t size)
 {
 	size_t p_size = __builtin_object_size(p, 0);
 
@@ -365,7 +381,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 * BOS0 p, const void * BOS0 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 +398,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 * BOS0 p, int c, __kernel_size_t size)
 {
 	size_t p_size = __builtin_object_size(p, 0);
 
@@ -393,7 +411,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 * BOS0 p, int c, size_t size)
 {
 	size_t p_size = __builtin_object_size(p, 0);
 
@@ -405,7 +423,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 * BOS0 p, size_t size, gfp_t gfp)
 {
 	size_t p_size = __builtin_object_size(p, 0);
 
@@ -417,7 +435,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 * BOS p, const char * BOS q)
 {
 	size_t p_size = __builtin_object_size(p, 1);
 	size_t q_size = __builtin_object_size(q, 1);
@@ -446,4 +465,7 @@ __FORTIFY_INLINE char *strcpy(char *p, const char *q)
 #undef __underlying_strncat
 #undef __underlying_strncpy
 
+#undef BOS0
+#undef BOS
+
 #endif /* _LINUX_FORTIFY_STRING_H_ */
diff --git a/security/Kconfig b/security/Kconfig
index 0b847f435beb..1a25a567965f 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -179,7 +179,7 @@ config FORTIFY_SOURCE
 	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 >= 130000
 	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 1/4] Compiler Attributes: Add Clang's __pass_object_size

[Miguel Ojeda]

On Wed, Feb 2, 2022 at 1:30 AM Kees Cook <keescook@chromium.org> wrote:
>
> +/*
> + * clang: https://clang.llvm.org/docs/AttributeReference.html#pass-object-size-pass-dynamic-object-size

For attributes that are not supported under all compilers, we have the
"Optional" lines in the comment. From a quick look in Godbolt,
`__pass_object_size__` and `__overloadable__` are supported for all
Clang >= 11 but not GCC/ICC. Thus, could you please add to the
comment:

    * Optional: not supported by gcc.
    * Optional: not supported by icc.

to those two patches?

For `__diagnose_as_builtin__`, I only see it on Clang trunk, so I
assume >= 14, thus could you please add:

    * Optional: only supported since clang >= 14.

?

Thanks!

> + * The "type" argument should match the __builtin_object_size(p, type) usage.

This should go above on top of the comment (it is true there is one
case that does not follow it, but that one has to be cleaned up).

Also, this bit seems to be explained in the Clang documentation (i.e.
not kernel-specific). Do you think we need it here?

Cheers,
Miguel

Re: [PATCH 1/4] Compiler Attributes: Add Clang's __pass_object_size

[Miguel Ojeda]

On Wed, Feb 2, 2022 at 2:11 AM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> For `__diagnose_as_builtin__`, I only see it on Clang trunk, so I
> assume >= 14, thus could you please add:
>
>     * Optional: only supported since clang >= 14.

...and the other two too:

>     * Optional: not supported by gcc.
>     * Optional: not supported by icc.

Cheers,
Miguel

Re: [PATCH 1/4] Compiler Attributes: Add Clang's __pass_object_size

[Kees Cook]

On Wed, Feb 02, 2022 at 02:11:51AM +0100, Miguel Ojeda wrote:
> On Wed, Feb 2, 2022 at 1:30 AM Kees Cook <keescook@chromium.org> wrote:
> >
> > +/*
> > + * clang: https://clang.llvm.org/docs/AttributeReference.html#pass-object-size-pass-dynamic-object-size
> 
> For attributes that are not supported under all compilers, we have the
> "Optional" lines in the comment. From a quick look in Godbolt,
> `__pass_object_size__` and `__overloadable__` are supported for all
> Clang >= 11 but not GCC/ICC. Thus, could you please add to the
> comment:
> 
>     * Optional: not supported by gcc.
>     * Optional: not supported by icc.
> 
> to those two patches?
> 
> For `__diagnose_as_builtin__`, I only see it on Clang trunk, so I
> assume >= 14, thus could you please add:
> 
>     * Optional: only supported since clang >= 14.
> 
> ?
> 
> Thanks!

Gotcha, I will adjust these.

> > + * The "type" argument should match the __builtin_object_size(p, type) usage.
> 
> This should go above on top of the comment (it is true there is one
> case that does not follow it, but that one has to be cleaned up).
> 
> Also, this bit seems to be explained in the Clang documentation (i.e.
> not kernel-specific). Do you think we need it here?

It's the kind of detail I feel like I might forget a month from now, so
I added it as a bit of a hint. If you think it's too redundant, I can
leave it off.

-- 
Kees Cook

Re: [PATCH 1/4] Compiler Attributes: Add Clang's __pass_object_size

[Miguel Ojeda]

On Wed, Feb 2, 2022 at 10:09 PM Kees Cook <keescook@chromium.org> wrote:
>
> It's the kind of detail I feel like I might forget a month from now, so
> I added it as a bit of a hint. If you think it's too redundant, I can
> leave it off.

If you think it is valuable having it nearby (e.g. when inspecting the
definition of the attribute in a call site), then it is fine having
them -- I am all for documentation (and we could consider having a
small summary/one-liner for all attributes). But, in general, I would
avoid repeating all the compiler docs (or, if needed, we could fix
those upstream if they are not clear etc.).

Thanks!

Cheers,
Miguel

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

[Nick Desaulniers]

On Tue, Feb 1, 2022 at 4:30 PM Kees Cook <keescook@chromium.org> wrote:
>
> --- a/security/Kconfig
> +++ b/security/Kconfig
> @@ -179,7 +179,7 @@ config FORTIFY_SOURCE
>         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 >= 130000

Are these comments still relevant, and is the clang version still correct?

In https://lore.kernel.org/llvm/CANiq72n1d7ouKNi+pbsy7chsg0DfCXxez27qqtS9XE1n3m5=8Q@mail.gmail.com/
Miguel notes that diagnose_as only exists in clang-14+.  If this
series relies on diagnose_as, then should this version check be for
clang-14+ rather than clang-13+?

https://bugs.llvm.org/show_bug.cgi?id=50322 is still open, but doesn't
signify why there's a version check. It makes sense if there's no
version check, but I'm not sure it's still relevant to this Kconfig
option after your series.

https://bugs.llvm.org/show_bug.cgi?id=41459 was fixed in clang-13, but
it was also backported to the clang 12.0.1 release.  Is it still
relevant if we're gated on diagnose_as from clang-14?

Perhaps a single comment, about the diagnose_as attribute or a link to
https://reviews.llvm.org/rGbc5f2d12cadce765620efc56a1ca815221db47af or
whatever, and updating the version check to be against clang-14 would
be more precise?
-- 
Thanks,
~Nick Desaulniers

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

[Nick Desaulniers]

On Tue, Feb 1, 2022 at 4:30 PM Kees Cook <keescook@chromium.org> wrote:
>
> +#define BOS    const __pass_object_size(1)
> +#define BOS0   const __pass_object_size(0)

A dumb bikeshed, but would you mind naming these BOS1 and BOS0, and
perhaps consider adding a comment or pointer or link to something that
describes why we use the two different modes?  I recognize that the
code already uses the two different modes already without comments,
but this might be a nice place to point folks like myself to so that
in a month or so when I forget what the difference is between modes
(again), we have a shorter trail of breadcrumbs.
-- 
Thanks,
~Nick Desaulniers

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

[Kees Cook]

On Wed, Feb 02, 2022 at 01:22:09PM -0800, Nick Desaulniers wrote:
> On Tue, Feb 1, 2022 at 4:30 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > --- a/security/Kconfig
> > +++ b/security/Kconfig
> > @@ -179,7 +179,7 @@ config FORTIFY_SOURCE
> >         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 >= 130000
> 
> Are these comments still relevant, and is the clang version still correct?

Oh, good call. I thought the version was still correct (more below),
but yes, the comments need adjusting.

> In https://lore.kernel.org/llvm/CANiq72n1d7ouKNi+pbsy7chsg0DfCXxez27qqtS9XE1n3m5=8Q@mail.gmail.com/
> Miguel notes that diagnose_as only exists in clang-14+.  If this
> series relies on diagnose_as, then should this version check be for
> clang-14+ rather than clang-13+?

It doesn't rely on it; this is just taking advantage of an improvement.

> https://bugs.llvm.org/show_bug.cgi?id=50322 is still open, but doesn't
> signify why there's a version check. It makes sense if there's no
> version check, but I'm not sure it's still relevant to this Kconfig
> option after your series.

With __overloadable, this probably ended up going away.

> https://bugs.llvm.org/show_bug.cgi?id=41459 was fixed in clang-13, but
> it was also backported to the clang 12.0.1 release.  Is it still
> relevant if we're gated on diagnose_as from clang-14?

Ah-ha! I missed that this got backported. Looks like 12.0.1 and later
have this fixed. That's excellent!

> Perhaps a single comment, about the diagnose_as attribute or a link to
> https://reviews.llvm.org/rGbc5f2d12cadce765620efc56a1ca815221db47af or
> whatever, and updating the version check to be against clang-14 would
> be more precise?

Yup, I will rework this after double-checking 12.0.1 builds.

Thanks!

-- 
Kees Cook

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

[Kees Cook]

On Wed, Feb 02, 2022 at 01:27:11PM -0800, Nick Desaulniers wrote:
> On Tue, Feb 1, 2022 at 4:30 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > +#define BOS    const __pass_object_size(1)
> > +#define BOS0   const __pass_object_size(0)
> 
> A dumb bikeshed, but would you mind naming these BOS1 and BOS0, and
> perhaps consider adding a comment or pointer or link to something that
> describes why we use the two different modes?  I recognize that the
> code already uses the two different modes already without comments,
> but this might be a nice place to point folks like myself to so that
> in a month or so when I forget what the difference is between modes
> (again), we have a shorter trail of breadcrumbs.

Sure, I can do that. My expectation was to entirely eliminate mode 0
usage in the future.

Though now that things are so close, I'll just do some builds with the
last few users switched over. But maybe memcmp() was a pain? I'll go
check...

-- 
Kees Cook

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

[Nick Desaulniers]

On Tue, Feb 1, 2022 at 4:30 PM Kees Cook <keescook@chromium.org> wrote:
>
> diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
> index c45159dbdaa1..5482536d3197 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

Sorry, I just noticed this line (already) uses a mix of open coding
__attribute__ and not. Would you mind also please changing the open
coded gnu_inline to simply __gnu_inline to make the entire line
consistent?
-- 
Thanks,
~Nick Desaulniers

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

[Miguel Ojeda]

On Thu, Feb 3, 2022 at 11:13 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> Sorry, I just noticed this line (already) uses a mix of open coding
> __attribute__ and not. Would you mind also please changing the open
> coded gnu_inline to simply __gnu_inline to make the entire line
> consistent?

+1

Cheers,
Miguel

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

[Kees Cook]

On Thu, Feb 03, 2022 at 02:13:33PM -0800, Nick Desaulniers wrote:
> On Tue, Feb 1, 2022 at 4:30 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
> > index c45159dbdaa1..5482536d3197 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
> 
> Sorry, I just noticed this line (already) uses a mix of open coding
> __attribute__ and not. Would you mind also please changing the open
> coded gnu_inline to simply __gnu_inline to make the entire line
> consistent?

Ah yeah, thanks. Fixed.

-- 
Kees Cook