[PATCH v2 0/3] string.h: Use ARRAY_SIZE() for memtostr*()/strtomem*()

[PATCH v2 0/3] string.h: Use ARRAY_SIZE() for memtostr*()/strtomem*()

[Kees Cook]

Work around a Clang <15 bug by switching to ARRAY_SIZE() (with the
added benefit of explicitly checking for char array arguments) in
memtostr*/strtomem*().

 v2:
  - change __must_be_byte_array to __must_be_byte_array() (david)
  - add missed Suggested-by (kent)
  - clarify move log (miguel)
  - clarify cover letter Clang version (andy)
 v1: https://lore.kernel.org/lkml/20250206175216.work.225-kees@kernel.org/

-Kees

Kees Cook (3):
  compiler.h: Move C string helpers into C-only kernel section
  compiler.h: Introduce __must_be_byte_array()
  string.h: Use ARRAY_SIZE() for memtostr*()/strtomem*()

 include/linux/compiler.h | 32 +++++++++++++++++++-------------
 include/linux/string.h   | 12 ++++++++----
 2 files changed, 27 insertions(+), 17 deletions(-)

-- 
2.34.1

[PATCH v2 1/3] compiler.h: Move C string helpers into C-only kernel section

[Kees Cook]

The C kernel helpers for evaluating C Strings were positioned where they
were visible to assembly inclusion, which was not intended. Move them
into the kernel and C-only area of the header so future changes won't
confuse the assembler.

Fixes: d7a516c6eeae ("compiler.h: Fix undefined BUILD_BUG_ON_ZERO()")
Fixes: 559048d156ff ("string: Check for "nonstring" attribute on strscpy() arguments")
Reviewed-by: Miguel Ojeda <ojeda@kernel.org>
Signed-off-by: Kees Cook <kees@kernel.org>
---
 include/linux/compiler.h | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 240c632c5b95..7af999a131cb 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -214,6 +214,19 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
 	__v;								\
 })
 
+#ifdef __CHECKER__
+#define __BUILD_BUG_ON_ZERO_MSG(e, msg) (0)
+#else /* __CHECKER__ */
+#define __BUILD_BUG_ON_ZERO_MSG(e, msg) ((int)sizeof(struct {_Static_assert(!(e), msg);}))
+#endif /* __CHECKER__ */
+
+/* &a[0] degrades to a pointer: a different type from an array */
+#define __must_be_array(a)	__BUILD_BUG_ON_ZERO_MSG(__same_type((a), &(a)[0]), "must be array")
+
+/* Require C Strings (i.e. NUL-terminated) lack the "nonstring" attribute. */
+#define __must_be_cstr(p) \
+	__BUILD_BUG_ON_ZERO_MSG(__annotated(p, nonstring), "must be cstr (NUL-terminated)")
+
 #endif /* __KERNEL__ */
 
 /**
@@ -254,19 +267,6 @@ static inline void *offset_to_ptr(const int *off)
 
 #define __ADDRESSABLE_ASM_STR(sym) __stringify(__ADDRESSABLE_ASM(sym))
 
-#ifdef __CHECKER__
-#define __BUILD_BUG_ON_ZERO_MSG(e, msg) (0)
-#else /* __CHECKER__ */
-#define __BUILD_BUG_ON_ZERO_MSG(e, msg) ((int)sizeof(struct {_Static_assert(!(e), msg);}))
-#endif /* __CHECKER__ */
-
-/* &a[0] degrades to a pointer: a different type from an array */
-#define __must_be_array(a)	__BUILD_BUG_ON_ZERO_MSG(__same_type((a), &(a)[0]), "must be array")
-
-/* Require C Strings (i.e. NUL-terminated) lack the "nonstring" attribute. */
-#define __must_be_cstr(p) \
-	__BUILD_BUG_ON_ZERO_MSG(__annotated(p, nonstring), "must be cstr (NUL-terminated)")
-
 /*
  * This returns a constant expression while determining if an argument is
  * a constant expression, most importantly without evaluating the argument.
-- 
2.34.1

[PATCH v2 2/3] compiler.h: Introduce __must_be_byte_array()

[Kees Cook]

In preparation for adding stricter type checking to the str/mem*()
helpers, provide a way to check that a variable is a byte array
via __must_be_byte_array().

Suggested-by: Kent Overstreet <kent.overstreet@linux.dev>
Signed-off-by: Kees Cook <kees@kernel.org>
---
 include/linux/compiler.h | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 7af999a131cb..1c0688319435 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -221,7 +221,13 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
 #endif /* __CHECKER__ */
 
 /* &a[0] degrades to a pointer: a different type from an array */
-#define __must_be_array(a)	__BUILD_BUG_ON_ZERO_MSG(__same_type((a), &(a)[0]), "must be array")
+#define __is_array(a)		(!__same_type((a), &(a)[0]))
+#define __must_be_array(a)	__BUILD_BUG_ON_ZERO_MSG(!__is_array(a), \
+							"must be array")
+
+#define __is_byte_array(a)	(__is_array(a) && sizeof((a)[0]) == 1)
+#define __must_be_byte_array(a)	__BUILD_BUG_ON_ZERO_MSG(!__is_byte_array(a), \
+							"must be byte array")
 
 /* Require C Strings (i.e. NUL-terminated) lack the "nonstring" attribute. */
 #define __must_be_cstr(p) \
-- 
2.34.1

[PATCH v2 3/3] string.h: Use ARRAY_SIZE() for memtostr*()/strtomem*()

[Kees Cook]

The destination argument of memtostr*() and strtomem*() must be a
fixed-size char array at compile time, so there is no need to use
__builtin_object_size() (which is useful for when an argument is
either a pointer or unknown). Instead use ARRAY_SIZE(), which has the
benefit of working around a bug in Clang (fixed[1] in 15+) that got
__builtin_object_size() wrong sometimes.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202501310832.kiAeOt2z-lkp@intel.com/
Suggested-by: Kent Overstreet <kent.overstreet@linux.dev>
Link: https://github.com/llvm/llvm-project/commit/d8e0a6d5e9dd2311641f9a8a5d2bf90829951ddc [1]
Signed-off-by: Kees Cook <kees@kernel.org>
---
Cc: Suren Baghdasaryan <surenb@google.com>
Cc: nathan@kernel.org
Cc: Andy Shevchenko <andy@kernel.org>
Cc: linux-hardening@vger.kernel.org
---
 include/linux/string.h | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/include/linux/string.h b/include/linux/string.h
index 493ac4862c77..fc5ae145bd78 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -411,7 +411,8 @@ void memcpy_and_pad(void *dest, size_t dest_len, const void *src, size_t count,
  * must be discoverable by the compiler.
  */
 #define strtomem_pad(dest, src, pad)	do {				\
-	const size_t _dest_len = __builtin_object_size(dest, 1);	\
+	const size_t _dest_len = __must_be_byte_array(dest) +		\
+				 ARRAY_SIZE(dest);			\
 	const size_t _src_len = __builtin_object_size(src, 1);		\
 									\
 	BUILD_BUG_ON(!__builtin_constant_p(_dest_len) ||		\
@@ -434,7 +435,8 @@ void memcpy_and_pad(void *dest, size_t dest_len, const void *src, size_t count,
  * must be discoverable by the compiler.
  */
 #define strtomem(dest, src)	do {					\
-	const size_t _dest_len = __builtin_object_size(dest, 1);	\
+	const size_t _dest_len = __must_be_byte_array(dest) +		\
+				 ARRAY_SIZE(dest);			\
 	const size_t _src_len = __builtin_object_size(src, 1);		\
 									\
 	BUILD_BUG_ON(!__builtin_constant_p(_dest_len) ||		\
@@ -453,7 +455,8 @@ void memcpy_and_pad(void *dest, size_t dest_len, const void *src, size_t count,
  * Note that sizes of @dest and @src must be known at compile-time.
  */
 #define memtostr(dest, src)	do {					\
-	const size_t _dest_len = __builtin_object_size(dest, 1);	\
+	const size_t _dest_len = __must_be_byte_array(dest) +		\
+				 ARRAY_SIZE(dest);			\
 	const size_t _src_len = __builtin_object_size(src, 1);		\
 	const size_t _src_chars = strnlen(src, _src_len);		\
 	const size_t _copy_len = min(_dest_len - 1, _src_chars);	\
@@ -478,7 +481,8 @@ void memcpy_and_pad(void *dest, size_t dest_len, const void *src, size_t count,
  * Note that sizes of @dest and @src must be known at compile-time.
  */
 #define memtostr_pad(dest, src)		do {				\
-	const size_t _dest_len = __builtin_object_size(dest, 1);	\
+	const size_t _dest_len = __must_be_byte_array(dest) +		\
+				 ARRAY_SIZE(dest);			\
 	const size_t _src_len = __builtin_object_size(src, 1);		\
 	const size_t _src_chars = strnlen(src, _src_len);		\
 	const size_t _copy_len = min(_dest_len - 1, _src_chars);	\
-- 
2.34.1

Re: [PATCH v2 3/3] string.h: Use ARRAY_SIZE() for memtostr*()/strtomem*()

[Suren Baghdasaryan]

On Thu, Feb 6, 2025 at 1:40 PM Kees Cook <kees@kernel.org> wrote:
>
> The destination argument of memtostr*() and strtomem*() must be a
> fixed-size char array at compile time, so there is no need to use
> __builtin_object_size() (which is useful for when an argument is
> either a pointer or unknown). Instead use ARRAY_SIZE(), which has the
> benefit of working around a bug in Clang (fixed[1] in 15+) that got
> __builtin_object_size() wrong sometimes.
>
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202501310832.kiAeOt2z-lkp@intel.com/
> Suggested-by: Kent Overstreet <kent.overstreet@linux.dev>
> Link: https://github.com/llvm/llvm-project/commit/d8e0a6d5e9dd2311641f9a8a5d2bf90829951ddc [1]
> Signed-off-by: Kees Cook <kees@kernel.org>

Tested-by: Suren Baghdasaryan <surenb@google.com>

Thanks Kees!
CC'ing Andrew Morton since mm-unstable contains my workaround patch
[1] for the issue fixed by this patchset. I'm not sure which tree will
be taking these fixes but once they are in we should drop [1]. Or
maybe we just drop it now from mm-unstable? Seems like only testbot
was the only one who complained...

[1] 0e91345e75b2 ("alloc_tag: work around clang-14 build issue with
__builtin_object_size()")

> ---
> Cc: Suren Baghdasaryan <surenb@google.com>
> Cc: nathan@kernel.org
> Cc: Andy Shevchenko <andy@kernel.org>
> Cc: linux-hardening@vger.kernel.org
> ---
>  include/linux/string.h | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/string.h b/include/linux/string.h
> index 493ac4862c77..fc5ae145bd78 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -411,7 +411,8 @@ void memcpy_and_pad(void *dest, size_t dest_len, const void *src, size_t count,
>   * must be discoverable by the compiler.
>   */
>  #define strtomem_pad(dest, src, pad)   do {                            \
> -       const size_t _dest_len = __builtin_object_size(dest, 1);        \
> +       const size_t _dest_len = __must_be_byte_array(dest) +           \
> +                                ARRAY_SIZE(dest);                      \
>         const size_t _src_len = __builtin_object_size(src, 1);          \
>                                                                         \
>         BUILD_BUG_ON(!__builtin_constant_p(_dest_len) ||                \
> @@ -434,7 +435,8 @@ void memcpy_and_pad(void *dest, size_t dest_len, const void *src, size_t count,
>   * must be discoverable by the compiler.
>   */
>  #define strtomem(dest, src)    do {                                    \
> -       const size_t _dest_len = __builtin_object_size(dest, 1);        \
> +       const size_t _dest_len = __must_be_byte_array(dest) +           \
> +                                ARRAY_SIZE(dest);                      \
>         const size_t _src_len = __builtin_object_size(src, 1);          \
>                                                                         \
>         BUILD_BUG_ON(!__builtin_constant_p(_dest_len) ||                \
> @@ -453,7 +455,8 @@ void memcpy_and_pad(void *dest, size_t dest_len, const void *src, size_t count,
>   * Note that sizes of @dest and @src must be known at compile-time.
>   */
>  #define memtostr(dest, src)    do {                                    \
> -       const size_t _dest_len = __builtin_object_size(dest, 1);        \
> +       const size_t _dest_len = __must_be_byte_array(dest) +           \
> +                                ARRAY_SIZE(dest);                      \
>         const size_t _src_len = __builtin_object_size(src, 1);          \
>         const size_t _src_chars = strnlen(src, _src_len);               \
>         const size_t _copy_len = min(_dest_len - 1, _src_chars);        \
> @@ -478,7 +481,8 @@ void memcpy_and_pad(void *dest, size_t dest_len, const void *src, size_t count,
>   * Note that sizes of @dest and @src must be known at compile-time.
>   */
>  #define memtostr_pad(dest, src)                do {                            \
> -       const size_t _dest_len = __builtin_object_size(dest, 1);        \
> +       const size_t _dest_len = __must_be_byte_array(dest) +           \
> +                                ARRAY_SIZE(dest);                      \
>         const size_t _src_len = __builtin_object_size(src, 1);          \
>         const size_t _src_chars = strnlen(src, _src_len);               \
>         const size_t _copy_len = min(_dest_len - 1, _src_chars);        \
> --
> 2.34.1
>

Re: [PATCH v2 3/3] string.h: Use ARRAY_SIZE() for memtostr*()/strtomem*()

[Kees Cook]

On Thu, Feb 06, 2025 at 03:59:53PM -0800, Suren Baghdasaryan wrote:
> On Thu, Feb 6, 2025 at 1:40 PM Kees Cook <kees@kernel.org> wrote:
> >
> > The destination argument of memtostr*() and strtomem*() must be a
> > fixed-size char array at compile time, so there is no need to use
> > __builtin_object_size() (which is useful for when an argument is
> > either a pointer or unknown). Instead use ARRAY_SIZE(), which has the
> > benefit of working around a bug in Clang (fixed[1] in 15+) that got
> > __builtin_object_size() wrong sometimes.
> >
> > Reported-by: kernel test robot <lkp@intel.com>
> > Closes: https://lore.kernel.org/oe-kbuild-all/202501310832.kiAeOt2z-lkp@intel.com/
> > Suggested-by: Kent Overstreet <kent.overstreet@linux.dev>
> > Link: https://github.com/llvm/llvm-project/commit/d8e0a6d5e9dd2311641f9a8a5d2bf90829951ddc [1]
> > Signed-off-by: Kees Cook <kees@kernel.org>
> 
> Tested-by: Suren Baghdasaryan <surenb@google.com>
> 
> Thanks Kees!
> CC'ing Andrew Morton since mm-unstable contains my workaround patch
> [1] for the issue fixed by this patchset. I'm not sure which tree will
> be taking these fixes but once they are in we should drop [1]. Or
> maybe we just drop it now from mm-unstable? Seems like only testbot
> was the only one who complained...
> 
> [1] 0e91345e75b2 ("alloc_tag: work around clang-14 build issue with
> __builtin_object_size()")

Yeah, that should be dropped. I intend to send this fix series to Linus
tomorrow or Saturday before rc2.

-Kees

> 
> > ---
> > Cc: Suren Baghdasaryan <surenb@google.com>
> > Cc: nathan@kernel.org
> > Cc: Andy Shevchenko <andy@kernel.org>
> > Cc: linux-hardening@vger.kernel.org
> > ---
> >  include/linux/string.h | 12 ++++++++----
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/linux/string.h b/include/linux/string.h
> > index 493ac4862c77..fc5ae145bd78 100644
> > --- a/include/linux/string.h
> > +++ b/include/linux/string.h
> > @@ -411,7 +411,8 @@ void memcpy_and_pad(void *dest, size_t dest_len, const void *src, size_t count,
> >   * must be discoverable by the compiler.
> >   */
> >  #define strtomem_pad(dest, src, pad)   do {                            \
> > -       const size_t _dest_len = __builtin_object_size(dest, 1);        \
> > +       const size_t _dest_len = __must_be_byte_array(dest) +           \
> > +                                ARRAY_SIZE(dest);                      \
> >         const size_t _src_len = __builtin_object_size(src, 1);          \
> >                                                                         \
> >         BUILD_BUG_ON(!__builtin_constant_p(_dest_len) ||                \
> > @@ -434,7 +435,8 @@ void memcpy_and_pad(void *dest, size_t dest_len, const void *src, size_t count,
> >   * must be discoverable by the compiler.
> >   */
> >  #define strtomem(dest, src)    do {                                    \
> > -       const size_t _dest_len = __builtin_object_size(dest, 1);        \
> > +       const size_t _dest_len = __must_be_byte_array(dest) +           \
> > +                                ARRAY_SIZE(dest);                      \
> >         const size_t _src_len = __builtin_object_size(src, 1);          \
> >                                                                         \
> >         BUILD_BUG_ON(!__builtin_constant_p(_dest_len) ||                \
> > @@ -453,7 +455,8 @@ void memcpy_and_pad(void *dest, size_t dest_len, const void *src, size_t count,
> >   * Note that sizes of @dest and @src must be known at compile-time.
> >   */
> >  #define memtostr(dest, src)    do {                                    \
> > -       const size_t _dest_len = __builtin_object_size(dest, 1);        \
> > +       const size_t _dest_len = __must_be_byte_array(dest) +           \
> > +                                ARRAY_SIZE(dest);                      \
> >         const size_t _src_len = __builtin_object_size(src, 1);          \
> >         const size_t _src_chars = strnlen(src, _src_len);               \
> >         const size_t _copy_len = min(_dest_len - 1, _src_chars);        \
> > @@ -478,7 +481,8 @@ void memcpy_and_pad(void *dest, size_t dest_len, const void *src, size_t count,
> >   * Note that sizes of @dest and @src must be known at compile-time.
> >   */
> >  #define memtostr_pad(dest, src)                do {                            \
> > -       const size_t _dest_len = __builtin_object_size(dest, 1);        \
> > +       const size_t _dest_len = __must_be_byte_array(dest) +           \
> > +                                ARRAY_SIZE(dest);                      \
> >         const size_t _src_len = __builtin_object_size(src, 1);          \
> >         const size_t _src_chars = strnlen(src, _src_len);               \
> >         const size_t _copy_len = min(_dest_len - 1, _src_chars);        \
> > --
> > 2.34.1
> >

-- 
Kees Cook