[PATCH] string: Check for "nonstring" attribute on strscpy() arguments

[PATCH] string: Check for "nonstring" attribute on strscpy() arguments

[Kees Cook]

GCC already checks for arguments that are marked with the "nonstring"[1]
attribute when used on standard C String API functions (e.g. strcpy). Gain
this compile-time checking also for the kernel's primary string copying
function, strscpy().

Note that Clang has neither "nonstring" nor __builtin_has_attribute().

Link: https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html#index-nonstring-variable-attribute [1]
Signed-off-by: Kees Cook <kees@kernel.org>
---
Cc: Andy Shevchenko <andy@kernel.org>
Cc: Justin Stitt <justinstitt@google.com>
Cc: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Miguel Ojeda <ojeda@kernel.org>
Cc: Marco Elver <elver@google.com>
Cc: Nathan Chancellor <nathan@kernel.org>
Cc: Hao Luo <haoluo@google.com>
Cc: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Cc: linux-sparse@vger.kernel.org
Cc: linux-hardening@vger.kernel.org
---
 include/linux/compiler.h       |  3 +++
 include/linux/compiler_types.h |  7 +++++++
 include/linux/string.h         | 12 ++++++++----
 3 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 2df665fa2964..ec55bcce4146 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -242,6 +242,9 @@ static inline void *offset_to_ptr(const int *off)
 /* &a[0] degrades to a pointer: a different type from an array */
 #define __must_be_array(a)	BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
 
+/* Require C Strings (i.e. NUL-terminated) lack the "nonstring" attribute. */
+#define __must_be_cstr(p)	BUILD_BUG_ON_ZERO(__annotated(p, nonstring))
+
 /*
  * This returns a constant expression while determining if an argument is
  * a constant expression, most importantly without evaluating the argument.
diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index f14c275950b5..1a957ea2f4fe 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -421,6 +421,13 @@ struct ftrace_likely_data {
 #define __member_size(p)	__builtin_object_size(p, 1)
 #endif
 
+/* Determine if an attribute has been applied to a variable. */
+#if __has_builtin(__builtin_has_attribute)
+#define __annotated(var, attr)	__builtin_has_attribute(var, attr)
+#else
+#define __annotated(var, attr)	(false)
+#endif
+
 /*
  * Some versions of gcc do not mark 'asm goto' volatile:
  *
diff --git a/include/linux/string.h b/include/linux/string.h
index 9edace076ddb..95b3fc308f4f 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -76,12 +76,16 @@ ssize_t sized_strscpy(char *, const char *, size_t);
  * known size.
  */
 #define __strscpy0(dst, src, ...)	\
-	sized_strscpy(dst, src, sizeof(dst) + __must_be_array(dst))
-#define __strscpy1(dst, src, size)	sized_strscpy(dst, src, size)
+	sized_strscpy(dst, src, sizeof(dst) + __must_be_array(dst) +	\
+				__must_be_cstr(dst) + __must_be_cstr(src))
+#define __strscpy1(dst, src, size)	\
+	sized_strscpy(dst, src, size + __must_be_cstr(dst) + __must_be_cstr(src))
 
 #define __strscpy_pad0(dst, src, ...)	\
-	sized_strscpy_pad(dst, src, sizeof(dst) + __must_be_array(dst))
-#define __strscpy_pad1(dst, src, size)	sized_strscpy_pad(dst, src, size)
+	sized_strscpy_pad(dst, src, sizeof(dst) + __must_be_array(dst) +	\
+				    __must_be_cstr(dst) + __must_be_cstr(src))
+#define __strscpy_pad1(dst, src, size)	\
+	sized_strscpy_pad(dst, src, size + __must_be_cstr(dst) + __must_be_cstr(src))
 
 /**
  * strscpy - Copy a C-string into a sized buffer
-- 
2.34.1

Re: [PATCH] string: Check for "nonstring" attribute on strscpy() arguments

[Przemek Kitszel]

On 8/5/24 23:43, Kees Cook wrote:
> GCC already checks for arguments that are marked with the "nonstring"[1]
> attribute when used on standard C String API functions (e.g. strcpy). Gain
> this compile-time checking also for the kernel's primary string copying
> function, strscpy().
> 
> Note that Clang has neither "nonstring" nor __builtin_has_attribute().
> 
> Link: https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html#index-nonstring-variable-attribute [1]
> Signed-off-by: Kees Cook <kees@kernel.org>



> diff --git a/include/linux/string.h b/include/linux/string.h
> index 9edace076ddb..95b3fc308f4f 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -76,12 +76,16 @@ ssize_t sized_strscpy(char *, const char *, size_t);
>    * known size.
>    */
>   #define __strscpy0(dst, src, ...)	\
> -	sized_strscpy(dst, src, sizeof(dst) + __must_be_array(dst))
> -#define __strscpy1(dst, src, size)	sized_strscpy(dst, src, size)
> +	sized_strscpy(dst, src, sizeof(dst) + __must_be_array(dst) +	\
> +				__must_be_cstr(dst) + __must_be_cstr(src))
> +#define __strscpy1(dst, src, size)	\
> +	sized_strscpy(dst, src, size + __must_be_cstr(dst) + __must_be_cstr(src))
>   
>   #define __strscpy_pad0(dst, src, ...)	\
> -	sized_strscpy_pad(dst, src, sizeof(dst) + __must_be_array(dst))
> -#define __strscpy_pad1(dst, src, size)	sized_strscpy_pad(dst, src, size)
> +	sized_strscpy_pad(dst, src, sizeof(dst) + __must_be_array(dst) +	\
> +				    __must_be_cstr(dst) + __must_be_cstr(src))
> +#define __strscpy_pad1(dst, src, size)	\
> +	sized_strscpy_pad(dst, src, size + __must_be_cstr(dst) + __must_be_cstr(src))

any way to avoid the usual caveat of repeating macro argument?

a variant of BUILD_BUG that is checking argument and otherwise pasting
it would nail it, but I didn't pondered how to implement such

Re: [PATCH] string: Check for "nonstring" attribute on strscpy() arguments

[Kees Cook]

On Tue, Aug 06, 2024 at 12:29:30PM +0200, Przemek Kitszel wrote:
> On 8/5/24 23:43, Kees Cook wrote:
> > GCC already checks for arguments that are marked with the "nonstring"[1]
> > attribute when used on standard C String API functions (e.g. strcpy). Gain
> > this compile-time checking also for the kernel's primary string copying
> > function, strscpy().
> > 
> > Note that Clang has neither "nonstring" nor __builtin_has_attribute().
> > 
> > Link: https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html#index-nonstring-variable-attribute [1]
> > Signed-off-by: Kees Cook <kees@kernel.org>
> 
> 
> 
> > diff --git a/include/linux/string.h b/include/linux/string.h
> > index 9edace076ddb..95b3fc308f4f 100644
> > --- a/include/linux/string.h
> > +++ b/include/linux/string.h
> > @@ -76,12 +76,16 @@ ssize_t sized_strscpy(char *, const char *, size_t);
> >    * known size.
> >    */
> >   #define __strscpy0(dst, src, ...)	\
> > -	sized_strscpy(dst, src, sizeof(dst) + __must_be_array(dst))
> > -#define __strscpy1(dst, src, size)	sized_strscpy(dst, src, size)
> > +	sized_strscpy(dst, src, sizeof(dst) + __must_be_array(dst) +	\
> > +				__must_be_cstr(dst) + __must_be_cstr(src))
> > +#define __strscpy1(dst, src, size)	\
> > +	sized_strscpy(dst, src, size + __must_be_cstr(dst) + __must_be_cstr(src))
> >   #define __strscpy_pad0(dst, src, ...)	\
> > -	sized_strscpy_pad(dst, src, sizeof(dst) + __must_be_array(dst))
> > -#define __strscpy_pad1(dst, src, size)	sized_strscpy_pad(dst, src, size)
> > +	sized_strscpy_pad(dst, src, sizeof(dst) + __must_be_array(dst) +	\
> > +				    __must_be_cstr(dst) + __must_be_cstr(src))
> > +#define __strscpy_pad1(dst, src, size)	\
> > +	sized_strscpy_pad(dst, src, size + __must_be_cstr(dst) + __must_be_cstr(src))
> 
> any way to avoid the usual caveat of repeating macro argument?
> 
> a variant of BUILD_BUG that is checking argument and otherwise pasting
> it would nail it, but I didn't pondered how to implement such

The use of __must_be_cstr() shouldn't cause side-effects, so from what I
can tell this is all okay. And since BUILD_BUG_ON_ZERO() resolves to a
constant expression, it shouldn't change the processing of the "size"
argument in the strscpy internals...

-- 
Kees Cook

Re: [PATCH] string: Check for "nonstring" attribute on strscpy() arguments

[Miguel Ojeda]

On Mon, Aug 5, 2024 at 11:43 PM Kees Cook <kees@kernel.org> wrote:
>
> +/* Determine if an attribute has been applied to a variable. */
> +#if __has_builtin(__builtin_has_attribute)
> +#define __annotated(var, attr) __builtin_has_attribute(var, attr)
> +#else
> +#define __annotated(var, attr) (false)
> +#endif

`__annotated` is obviously best-effort given this definition, and we
do similar things elsewhere, and it has a double-underscore. However,
I wonder if this being a "query" (vs. something like an attribute) may
imply that it has a greater risk of someone thinking it will always
reply with the right answer... (if e.g. they copy-paste another use).
Perhaps there is a more explicit name to let users recall that.

Anyway, it looks sensible to me: more compile-time checking seldomly
hurts (apart from complexity in these definitions :). So:

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

I also introduced a mistake on purpose and I got the expected build error, so:

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

Cheers,
Miguel

Re: [PATCH] string: Check for "nonstring" attribute on strscpy() arguments

[Kees Cook]

On Mon, 05 Aug 2024 14:43:44 -0700, Kees Cook wrote:
> GCC already checks for arguments that are marked with the "nonstring"[1]
> attribute when used on standard C String API functions (e.g. strcpy). Gain
> this compile-time checking also for the kernel's primary string copying
> function, strscpy().
> 
> Note that Clang has neither "nonstring" nor __builtin_has_attribute().
> 
> [...]

Applied to for-next/hardening, thanks!

[1/1] string: Check for "nonstring" attribute on strscpy() arguments
      https://git.kernel.org/kees/c/559048d156ff

Take care,

-- 
Kees Cook