[PATCH v3 0/4] string: Allow 2-argument strscpy()

[PATCH v3 0/4] string: Allow 2-argument strscpy()

[Kees Cook]

v3:
 - add missed args.h include (andy)
v2: https://lore.kernel.org/all/20240205122916.it.909-kees@kernel.org/
v1: https://lore.kernel.org/all/20240131055340.work.279-kees@kernel.org/

Hi,

Make it possible for strscpy() and strscpy_pad() to use 2 arguments,
making "sizeof(dst)" be the the default 3rd argument for the destination
size. This can make future usage much easier to read. Additionally allows
treewide changes to save a bunch of lines:
 1177 files changed, 2455 insertions(+), 3026 deletions(-)

-Kees

Kees Cook (4):
  string: Redefine strscpy_pad() as a macro
  string: Allow 2-argument strscpy()
  string: Allow 2-argument strscpy_pad()
  um: Convert strscpy() usage to 2-argument style

 arch/um/drivers/net_kern.c               |  2 +-
 arch/um/drivers/vector_kern.c            |  2 +-
 arch/um/drivers/vector_user.c            |  4 +-
 arch/um/include/shared/user.h            |  3 +-
 arch/um/os-Linux/drivers/ethertap_user.c |  2 +-
 arch/um/os-Linux/drivers/tuntap_user.c   |  2 +-
 arch/um/os-Linux/umid.c                  |  6 +-
 include/linux/fortify-string.h           | 22 +------
 include/linux/string.h                   | 76 +++++++++++++++++++++++-
 lib/string.c                             |  4 +-
 lib/string_helpers.c                     | 34 -----------
 11 files changed, 88 insertions(+), 69 deletions(-)

-- 
2.34.1

[PATCH v3 1/4] string: Redefine strscpy_pad() as a macro

[Kees Cook]

In preparation for making strscpy_pad()'s 3rd argument optional, redefine
it as a macro. This also has the benefit of allowing greater FORITFY
introspection, as it couldn't see into the strscpy() nor the memset()
within strscpy_pad().

Cc: Andy Shevchenko <andy@kernel.org>
Cc: linux-hardening@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/string.h | 33 +++++++++++++++++++++++++++++++--
 lib/string_helpers.c   | 34 ----------------------------------
 2 files changed, 31 insertions(+), 36 deletions(-)

diff --git a/include/linux/string.h b/include/linux/string.h
index ab148d8dbfc1..03f59cf7fe72 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -70,8 +70,37 @@ extern char * strncpy(char *,const char *, __kernel_size_t);
 ssize_t strscpy(char *, const char *, size_t);
 #endif
 
-/* Wraps calls to strscpy()/memset(), no arch specific code required */
-ssize_t strscpy_pad(char *dest, const char *src, size_t count);
+/**
+ * strscpy_pad() - Copy a C-string into a sized buffer
+ * @dest: Where to copy the string to
+ * @src: Where to copy the string from
+ * @count: Size of destination buffer
+ *
+ * Copy the string, or as much of it as fits, into the dest buffer. The
+ * behavior is undefined if the string buffers overlap. The destination
+ * buffer is always %NUL terminated, unless it's zero-sized.
+ *
+ * If the source string is shorter than the destination buffer, zeros
+ * the tail of the destination buffer.
+ *
+ * For full explanation of why you may want to consider using the
+ * 'strscpy' functions please see the function docstring for strscpy().
+ *
+ * Returns:
+ * * The number of characters copied (not including the trailing %NULs)
+ * * -E2BIG if count is 0 or @src was truncated.
+ */
+#define strscpy_pad(dest, src, count)	({			\
+	char *__dst = (dest);						\
+	const char *__src = (src);					\
+	const size_t __count = (count);					\
+	ssize_t __wrote;						\
+									\
+	__wrote = strscpy(__dst, __src, __count);			\
+	if (__wrote >= 0 && __wrote < __count)				\
+		memset(__dst + __wrote + 1, 0, __count - __wrote - 1);	\
+	__wrote;							\
+})
 
 #ifndef __HAVE_ARCH_STRCAT
 extern char * strcat(char *, const char *);
diff --git a/lib/string_helpers.c b/lib/string_helpers.c
index 7713f73e66b0..606c3099013f 100644
--- a/lib/string_helpers.c
+++ b/lib/string_helpers.c
@@ -825,40 +825,6 @@ char **devm_kasprintf_strarray(struct device *dev, const char *prefix, size_t n)
 }
 EXPORT_SYMBOL_GPL(devm_kasprintf_strarray);
 
-/**
- * strscpy_pad() - Copy a C-string into a sized buffer
- * @dest: Where to copy the string to
- * @src: Where to copy the string from
- * @count: Size of destination buffer
- *
- * Copy the string, or as much of it as fits, into the dest buffer.  The
- * behavior is undefined if the string buffers overlap.  The destination
- * buffer is always %NUL terminated, unless it's zero-sized.
- *
- * If the source string is shorter than the destination buffer, zeros
- * the tail of the destination buffer.
- *
- * For full explanation of why you may want to consider using the
- * 'strscpy' functions please see the function docstring for strscpy().
- *
- * Returns:
- * * The number of characters copied (not including the trailing %NUL)
- * * -E2BIG if count is 0 or @src was truncated.
- */
-ssize_t strscpy_pad(char *dest, const char *src, size_t count)
-{
-	ssize_t written;
-
-	written = strscpy(dest, src, count);
-	if (written < 0 || written == count - 1)
-		return written;
-
-	memset(dest + written + 1, 0, count - written - 1);
-
-	return written;
-}
-EXPORT_SYMBOL(strscpy_pad);
-
 /**
  * skip_spaces - Removes leading whitespace from @str.
  * @str: The string to be stripped.
-- 
2.34.1

Re: [PATCH v3 1/4] string: Redefine strscpy_pad() as a macro

[Justin Stitt]

Hi,

On Tue, Feb 06, 2024 at 06:22:16AM -0800, Kees Cook wrote:
> In preparation for making strscpy_pad()'s 3rd argument optional, redefine
> it as a macro. This also has the benefit of allowing greater FORITFY
> introspection, as it couldn't see into the strscpy() nor the memset()
> within strscpy_pad().
>
> Cc: Andy Shevchenko <andy@kernel.org>
> Cc: linux-hardening@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  include/linux/string.h | 33 +++++++++++++++++++++++++++++++--
>  lib/string_helpers.c   | 34 ----------------------------------
>  2 files changed, 31 insertions(+), 36 deletions(-)
>
> diff --git a/include/linux/string.h b/include/linux/string.h
> index ab148d8dbfc1..03f59cf7fe72 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -70,8 +70,37 @@ extern char * strncpy(char *,const char *, __kernel_size_t);
>  ssize_t strscpy(char *, const char *, size_t);
>  #endif
>
> -/* Wraps calls to strscpy()/memset(), no arch specific code required */
> -ssize_t strscpy_pad(char *dest, const char *src, size_t count);
> +/**
> + * strscpy_pad() - Copy a C-string into a sized buffer
> + * @dest: Where to copy the string to
> + * @src: Where to copy the string from
> + * @count: Size of destination buffer
> + *
> + * Copy the string, or as much of it as fits, into the dest buffer. The
> + * behavior is undefined if the string buffers overlap. The destination
> + * buffer is always %NUL terminated, unless it's zero-sized.
> + *
> + * If the source string is shorter than the destination buffer, zeros
> + * the tail of the destination buffer.
> + *
> + * For full explanation of why you may want to consider using the
> + * 'strscpy' functions please see the function docstring for strscpy().
> + *
> + * Returns:
> + * * The number of characters copied (not including the trailing %NULs)
> + * * -E2BIG if count is 0 or @src was truncated.
> + */
> +#define strscpy_pad(dest, src, count)	({			\
> +	char *__dst = (dest);						\
> +	const char *__src = (src);					\
> +	const size_t __count = (count);					\
> +	ssize_t __wrote;						\
> +									\
> +	__wrote = strscpy(__dst, __src, __count);			\
> +	if (__wrote >= 0 && __wrote < __count)				\
> +		memset(__dst + __wrote + 1, 0, __count - __wrote - 1);	\
> +	__wrote;							\
> +})
>
>  #ifndef __HAVE_ARCH_STRCAT
>  extern char * strcat(char *, const char *);
> diff --git a/lib/string_helpers.c b/lib/string_helpers.c
> index 7713f73e66b0..606c3099013f 100644
> --- a/lib/string_helpers.c
> +++ b/lib/string_helpers.c
> @@ -825,40 +825,6 @@ char **devm_kasprintf_strarray(struct device *dev, const char *prefix, size_t n)
>  }
>  EXPORT_SYMBOL_GPL(devm_kasprintf_strarray);
>
> -/**
> - * strscpy_pad() - Copy a C-string into a sized buffer
> - * @dest: Where to copy the string to
> - * @src: Where to copy the string from
> - * @count: Size of destination buffer
> - *
> - * Copy the string, or as much of it as fits, into the dest buffer.  The
> - * behavior is undefined if the string buffers overlap.  The destination
> - * buffer is always %NUL terminated, unless it's zero-sized.
> - *
> - * If the source string is shorter than the destination buffer, zeros
> - * the tail of the destination buffer.
> - *
> - * For full explanation of why you may want to consider using the
> - * 'strscpy' functions please see the function docstring for strscpy().
> - *
> - * Returns:
> - * * The number of characters copied (not including the trailing %NUL)
> - * * -E2BIG if count is 0 or @src was truncated.
> - */
> -ssize_t strscpy_pad(char *dest, const char *src, size_t count)
> -{
> -	ssize_t written;
> -
> -	written = strscpy(dest, src, count);
> -	if (written < 0 || written == count - 1)
> -		return written;
> -
> -	memset(dest + written + 1, 0, count - written - 1);
> -
> -	return written;
> -}
> -EXPORT_SYMBOL(strscpy_pad);

Yep, looks good. This is reminiscent of strtomem and strtomem_pad.

Reviewed-by: Justin Stitt <justinstitt@google.com>

> -
>  /**
>   * skip_spaces - Removes leading whitespace from @str.
>   * @str: The string to be stripped.
> --
> 2.34.1
>

Thanks
Justin

[PATCH v3 2/4] string: Allow 2-argument strscpy()

[Kees Cook]

Using sizeof(dst) for the "size" argument in strscpy() is the
overwhelmingly common case. Instead of requiring this everywhere, allow a
2-argument version to be used that will use the sizeof() internally. There
are other functions in the kernel with optional arguments[1], so this
isn't unprecedented, and improves readability. Update and relocate the
kern-doc for strscpy() too.

Adjust ARCH=um build to notice the changed export name, as it doesn't
do full header includes for the string helpers.

This could additionally let us save a few hundred lines of code:
 1177 files changed, 2455 insertions(+), 3026 deletions(-)
with a treewide cleanup using Coccinelle:

@needless_arg@
expression DST, SRC;
@@

        strscpy(DST, SRC
-, sizeof(DST)
        )

Link: https://elixir.bootlin.com/linux/v6.7/source/include/linux/pci.h#L1517 [1]
Reviewed-by: Justin Stitt <justinstitt@google.com>
Cc: Andy Shevchenko <andy@kernel.org>
Cc: linux-hardening@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 arch/um/include/shared/user.h  |  3 ++-
 include/linux/fortify-string.h | 22 ++-------------------
 include/linux/string.h         | 36 +++++++++++++++++++++++++++++++++-
 lib/string.c                   |  4 ++--
 4 files changed, 41 insertions(+), 24 deletions(-)

diff --git a/arch/um/include/shared/user.h b/arch/um/include/shared/user.h
index 981e11d8e025..9568cc04cbb7 100644
--- a/arch/um/include/shared/user.h
+++ b/arch/um/include/shared/user.h
@@ -51,7 +51,8 @@ static inline int printk(const char *fmt, ...)
 
 extern int in_aton(char *str);
 extern size_t strlcat(char *, const char *, size_t);
-extern size_t strscpy(char *, const char *, size_t);
+extern size_t sized_strscpy(char *, const char *, size_t);
+#define strscpy(dst, src, size)	sized_strscpy(dst, src, size)
 
 /* Copied from linux/compiler-gcc.h since we can't include it directly */
 #define barrier() __asm__ __volatile__("": : :"memory")
diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
index 89a6888f2f9e..06b3aaa63724 100644
--- a/include/linux/fortify-string.h
+++ b/include/linux/fortify-string.h
@@ -215,26 +215,8 @@ __kernel_size_t __fortify_strlen(const char * const POS p)
 }
 
 /* Defined after fortified strnlen() to reuse it. */
-extern ssize_t __real_strscpy(char *, const char *, size_t) __RENAME(strscpy);
-/**
- * strscpy - Copy a C-string into a sized buffer
- *
- * @p: Where to copy the string to
- * @q: Where to copy the string from
- * @size: Size of destination buffer
- *
- * Copy the source string @q, or as much of it as fits, into the destination
- * @p buffer. The behavior is undefined if the string buffers overlap. The
- * destination @p buffer is always NUL terminated, unless it's zero-sized.
- *
- * Preferred to strncpy() since it always returns a valid string, and
- * doesn't unnecessarily force the tail of the destination buffer to be
- * zero padded. If padding is desired please use strscpy_pad().
- *
- * Returns the number of characters copied in @p (not including the
- * trailing %NUL) or -E2BIG if @size is 0 or the copy of @q was truncated.
- */
-__FORTIFY_INLINE ssize_t strscpy(char * const POS p, const char * const POS q, size_t size)
+extern ssize_t __real_strscpy(char *, const char *, size_t) __RENAME(sized_strscpy);
+__FORTIFY_INLINE ssize_t sized_strscpy(char * const POS p, const char * const POS q, size_t size)
 {
 	/* Use string size rather than possible enclosing struct size. */
 	const size_t p_size = __member_size(p);
diff --git a/include/linux/string.h b/include/linux/string.h
index 03f59cf7fe72..79b875de615e 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -2,6 +2,7 @@
 #ifndef _LINUX_STRING_H_
 #define _LINUX_STRING_H_
 
+#include <linux/args.h>
 #include <linux/array_size.h>
 #include <linux/compiler.h>	/* for inline */
 #include <linux/types.h>	/* for size_t */
@@ -67,9 +68,42 @@ extern char * strcpy(char *,const char *);
 extern char * strncpy(char *,const char *, __kernel_size_t);
 #endif
 #ifndef __HAVE_ARCH_STRSCPY
-ssize_t strscpy(char *, const char *, size_t);
+ssize_t sized_strscpy(char *, const char *, size_t);
 #endif
 
+/*
+ * The 2 argument style can only be used when dst is an array with a
+ * 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)
+
+/**
+ * strscpy - Copy a C-string into a sized buffer
+ * @dst: Where to copy the string to
+ * @src: Where to copy the string from
+ * @...: Size of destination buffer (optional)
+ *
+ * Copy the source string @src, or as much of it as fits, into the
+ * destination @dst buffer. The behavior is undefined if the string
+ * buffers overlap. The destination @dst buffer is always NUL terminated,
+ * unless it's zero-sized.
+ *
+ * The size argument @... is only required when @dst is not an array, or
+ * when the copy needs to be smaller than sizeof(@dst).
+ *
+ * Preferred to strncpy() since it always returns a valid string, and
+ * doesn't unnecessarily force the tail of the destination buffer to be
+ * zero padded. If padding is desired please use strscpy_pad().
+ *
+ * Returns the number of characters copied in @dst (not including the
+ * trailing %NUL) or -E2BIG if @size is 0 or the copy from @src was
+ * truncated.
+ */
+#define strscpy(dst, src, ...)	\
+	CONCATENATE(__strscpy, COUNT_ARGS(__VA_ARGS__))(dst, src, __VA_ARGS__)
+
 /**
  * strscpy_pad() - Copy a C-string into a sized buffer
  * @dest: Where to copy the string to
diff --git a/lib/string.c b/lib/string.c
index 6891d15ce991..2869895a1180 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -104,7 +104,7 @@ EXPORT_SYMBOL(strncpy);
 #endif
 
 #ifndef __HAVE_ARCH_STRSCPY
-ssize_t strscpy(char *dest, const char *src, size_t count)
+ssize_t sized_strscpy(char *dest, const char *src, size_t count)
 {
 	const struct word_at_a_time constants = WORD_AT_A_TIME_CONSTANTS;
 	size_t max = count;
@@ -170,7 +170,7 @@ ssize_t strscpy(char *dest, const char *src, size_t count)
 
 	return -E2BIG;
 }
-EXPORT_SYMBOL(strscpy);
+EXPORT_SYMBOL(sized_strscpy);
 #endif
 
 /**
-- 
2.34.1

[PATCH v3 3/4] string: Allow 2-argument strscpy_pad()

[Kees Cook]

Similar to strscpy(), update strscpy_pad()'s 3rd argument to be
optional when the destination is a compile-time known size array.

Cc: Andy Shevchenko <andy@kernel.org>
Cc: linux-hardening@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/string.h | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/include/linux/string.h b/include/linux/string.h
index 79b875de615e..9bd421ad92a4 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -79,6 +79,10 @@ ssize_t sized_strscpy(char *, const char *, size_t);
 	sized_strscpy(dst, src, sizeof(dst) + __must_be_array(dst))
 #define __strscpy1(dst, src, size)	sized_strscpy(dst, src, size)
 
+#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)
+
 /**
  * strscpy - Copy a C-string into a sized buffer
  * @dst: Where to copy the string to
@@ -104,6 +108,18 @@ ssize_t sized_strscpy(char *, const char *, size_t);
 #define strscpy(dst, src, ...)	\
 	CONCATENATE(__strscpy, COUNT_ARGS(__VA_ARGS__))(dst, src, __VA_ARGS__)
 
+#define sized_strscpy_pad(dest, src, count)	({			\
+	char *__dst = (dest);						\
+	const char *__src = (src);					\
+	const size_t __count = (count);					\
+	ssize_t __wrote;						\
+									\
+	__wrote = sized_strscpy(__dst, __src, __count);			\
+	if (__wrote >= 0 && __wrote < __count)				\
+		memset(__dst + __wrote + 1, 0, __count - __wrote - 1);	\
+	__wrote;							\
+})
+
 /**
  * strscpy_pad() - Copy a C-string into a sized buffer
  * @dest: Where to copy the string to
@@ -124,17 +140,8 @@ ssize_t sized_strscpy(char *, const char *, size_t);
  * * The number of characters copied (not including the trailing %NULs)
  * * -E2BIG if count is 0 or @src was truncated.
  */
-#define strscpy_pad(dest, src, count)	({			\
-	char *__dst = (dest);						\
-	const char *__src = (src);					\
-	const size_t __count = (count);					\
-	ssize_t __wrote;						\
-									\
-	__wrote = strscpy(__dst, __src, __count);			\
-	if (__wrote >= 0 && __wrote < __count)				\
-		memset(__dst + __wrote + 1, 0, __count - __wrote - 1);	\
-	__wrote;							\
-})
+#define strscpy_pad(dst, src, ...)	\
+	CONCATENATE(__strscpy_pad, COUNT_ARGS(__VA_ARGS__))(dst, src, __VA_ARGS__)
 
 #ifndef __HAVE_ARCH_STRCAT
 extern char * strcat(char *, const char *);
-- 
2.34.1

Re: [PATCH v3 3/4] string: Allow 2-argument strscpy_pad()

[Justin Stitt]

Hi,

On Tue, Feb 06, 2024 at 06:22:18AM -0800, Kees Cook wrote:
> Similar to strscpy(), update strscpy_pad()'s 3rd argument to be
> optional when the destination is a compile-time known size array.

This patch is diff'd against Patch 1/4 in this series, right? I wonder
why you split them up. If I hadn't literally just read that patch I
would be mildly confused.

I suppose one reason may be that 1/4 is a standalone change with a high
percentage chance of landing whilst this overloading magic may not land
as easily?

At any rate,
Reviewed-by: Justin Stitt <justinstitt@google.com>

>
> Cc: Andy Shevchenko <andy@kernel.org>
> Cc: linux-hardening@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  include/linux/string.h | 29 ++++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
>
> diff --git a/include/linux/string.h b/include/linux/string.h
> index 79b875de615e..9bd421ad92a4 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -79,6 +79,10 @@ ssize_t sized_strscpy(char *, const char *, size_t);
>  	sized_strscpy(dst, src, sizeof(dst) + __must_be_array(dst))
>  #define __strscpy1(dst, src, size)	sized_strscpy(dst, src, size)
>
> +#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)
> +
>  /**
>   * strscpy - Copy a C-string into a sized buffer
>   * @dst: Where to copy the string to
> @@ -104,6 +108,18 @@ ssize_t sized_strscpy(char *, const char *, size_t);
>  #define strscpy(dst, src, ...)	\
>  	CONCATENATE(__strscpy, COUNT_ARGS(__VA_ARGS__))(dst, src, __VA_ARGS__)
>
> +#define sized_strscpy_pad(dest, src, count)	({			\
> +	char *__dst = (dest);						\
> +	const char *__src = (src);					\
> +	const size_t __count = (count);					\
> +	ssize_t __wrote;						\
> +									\
> +	__wrote = sized_strscpy(__dst, __src, __count);			\
> +	if (__wrote >= 0 && __wrote < __count)				\
> +		memset(__dst + __wrote + 1, 0, __count - __wrote - 1);	\
> +	__wrote;							\
> +})
> +
>  /**
>   * strscpy_pad() - Copy a C-string into a sized buffer
>   * @dest: Where to copy the string to
> @@ -124,17 +140,8 @@ ssize_t sized_strscpy(char *, const char *, size_t);
>   * * The number of characters copied (not including the trailing %NULs)
>   * * -E2BIG if count is 0 or @src was truncated.
>   */
> -#define strscpy_pad(dest, src, count)	({			\
> -	char *__dst = (dest);						\
> -	const char *__src = (src);					\
> -	const size_t __count = (count);					\
> -	ssize_t __wrote;						\
> -									\
> -	__wrote = strscpy(__dst, __src, __count);			\
> -	if (__wrote >= 0 && __wrote < __count)				\
> -		memset(__dst + __wrote + 1, 0, __count - __wrote - 1);	\
> -	__wrote;							\
> -})
> +#define strscpy_pad(dst, src, ...)	\
> +	CONCATENATE(__strscpy_pad, COUNT_ARGS(__VA_ARGS__))(dst, src, __VA_ARGS__)
>
>  #ifndef __HAVE_ARCH_STRCAT
>  extern char * strcat(char *, const char *);
> --
> 2.34.1
>

Thanks
Justin

Re: [PATCH v3 3/4] string: Allow 2-argument strscpy_pad()

[Kees Cook]

On Wed, Feb 07, 2024 at 12:51:51AM +0000, Justin Stitt wrote:
> Hi,
> 
> On Tue, Feb 06, 2024 at 06:22:18AM -0800, Kees Cook wrote:
> > Similar to strscpy(), update strscpy_pad()'s 3rd argument to be
> > optional when the destination is a compile-time known size array.
> 
> This patch is diff'd against Patch 1/4 in this series, right? I wonder
> why you split them up. If I hadn't literally just read that patch I
> would be mildly confused.
> 
> I suppose one reason may be that 1/4 is a standalone change with a high
> percentage chance of landing whilst this overloading magic may not land
> as easily?

I viewed it as a distinct logical change. I could certainly combine
them, but I think it's easier to review the conversion from function to
macro without needing to consider anything else. No behavioral changes
are expected, etc.

But if they were together, there's a little more cognitive load to keep
the func/macro conversion in mind while looking at the optional arg
magic, etc.

I don't think it's a strict rule or anything; it just felt like the
right thing to do to split them up.

> At any rate,
> Reviewed-by: Justin Stitt <justinstitt@google.com>

Thanks!

-Kees

-- 
Kees Cook

RE: [PATCH v3 3/4] string: Allow 2-argument strscpy_pad()

[David Laight]

From: Kees Cook
> Sent: 07 February 2024 09:19
> 
> On Wed, Feb 07, 2024 at 12:51:51AM +0000, Justin Stitt wrote:
> > Hi,
> >
> > On Tue, Feb 06, 2024 at 06:22:18AM -0800, Kees Cook wrote:
> > > Similar to strscpy(), update strscpy_pad()'s 3rd argument to be
> > > optional when the destination is a compile-time known size array.
> >
> > This patch is diff'd against Patch 1/4 in this series, right? I wonder
> > why you split them up. If I hadn't literally just read that patch I
> > would be mildly confused.
> >
> > I suppose one reason may be that 1/4 is a standalone change with a high
> > percentage chance of landing whilst this overloading magic may not land
> > as easily?
> 
> I viewed it as a distinct logical change. I could certainly combine
> them, but I think it's easier to review the conversion from function to
> macro without needing to consider anything else. No behavioral changes
> are expected, etc.

I wonder about the code-bloat from inlining strscpy_pad()?
Especially given the code that gcc is likely to generate
for string ops.

I strongly suspect that the end of strscpy() knows exactly
you many bytes weren't written (in the non-truncate path).
So maybe implement both strscpy() and strscp_pad() in terms
of an inline function that has a parameter that 'turns on'
padding.

That way you get a simple call site and still only one
implementation.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

[PATCH v3 4/4] um: Convert strscpy() usage to 2-argument style

[Kees Cook]

The ARCH=um build has its own idea about strscpy()'s definition. Adjust
the callers to remove the redundant sizeof() arguments ahead of treewide
changes, since it needs a manual adjustment for the newly named
sized_strscpy() export.

Cc: Richard Weinberger <richard@nod.at>
Cc: linux-um@lists.infradead.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 arch/um/drivers/net_kern.c               | 2 +-
 arch/um/drivers/vector_kern.c            | 2 +-
 arch/um/drivers/vector_user.c            | 4 ++--
 arch/um/include/shared/user.h            | 2 +-
 arch/um/os-Linux/drivers/ethertap_user.c | 2 +-
 arch/um/os-Linux/drivers/tuntap_user.c   | 2 +-
 arch/um/os-Linux/umid.c                  | 6 +++---
 7 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/arch/um/drivers/net_kern.c b/arch/um/drivers/net_kern.c
index cabcc501b448..77c4afb8ab90 100644
--- a/arch/um/drivers/net_kern.c
+++ b/arch/um/drivers/net_kern.c
@@ -265,7 +265,7 @@ static void uml_net_poll_controller(struct net_device *dev)
 static void uml_net_get_drvinfo(struct net_device *dev,
 				struct ethtool_drvinfo *info)
 {
-	strscpy(info->driver, DRIVER_NAME, sizeof(info->driver));
+	strscpy(info->driver, DRIVER_NAME);
 }
 
 static const struct ethtool_ops uml_net_ethtool_ops = {
diff --git a/arch/um/drivers/vector_kern.c b/arch/um/drivers/vector_kern.c
index 131b7cb29576..dc2feae789cb 100644
--- a/arch/um/drivers/vector_kern.c
+++ b/arch/um/drivers/vector_kern.c
@@ -1373,7 +1373,7 @@ static void vector_net_poll_controller(struct net_device *dev)
 static void vector_net_get_drvinfo(struct net_device *dev,
 				struct ethtool_drvinfo *info)
 {
-	strscpy(info->driver, DRIVER_NAME, sizeof(info->driver));
+	strscpy(info->driver, DRIVER_NAME);
 }
 
 static int vector_net_load_bpf_flash(struct net_device *dev,
diff --git a/arch/um/drivers/vector_user.c b/arch/um/drivers/vector_user.c
index c719e1ec4645..b16a5e5619d3 100644
--- a/arch/um/drivers/vector_user.c
+++ b/arch/um/drivers/vector_user.c
@@ -141,7 +141,7 @@ static int create_tap_fd(char *iface)
 	}
 	memset(&ifr, 0, sizeof(ifr));
 	ifr.ifr_flags = IFF_TAP | IFF_NO_PI | IFF_VNET_HDR;
-	strscpy(ifr.ifr_name, iface, sizeof(ifr.ifr_name));
+	strscpy(ifr.ifr_name, iface);
 
 	err = ioctl(fd, TUNSETIFF, (void *) &ifr);
 	if (err != 0) {
@@ -171,7 +171,7 @@ static int create_raw_fd(char *iface, int flags, int proto)
 		goto raw_fd_cleanup;
 	}
 	memset(&ifr, 0, sizeof(ifr));
-	strscpy(ifr.ifr_name, iface, sizeof(ifr.ifr_name));
+	strscpy(ifr.ifr_name, iface);
 	if (ioctl(fd, SIOCGIFINDEX, (void *) &ifr) < 0) {
 		err = -errno;
 		goto raw_fd_cleanup;
diff --git a/arch/um/include/shared/user.h b/arch/um/include/shared/user.h
index 9568cc04cbb7..326e52450e41 100644
--- a/arch/um/include/shared/user.h
+++ b/arch/um/include/shared/user.h
@@ -52,7 +52,7 @@ static inline int printk(const char *fmt, ...)
 extern int in_aton(char *str);
 extern size_t strlcat(char *, const char *, size_t);
 extern size_t sized_strscpy(char *, const char *, size_t);
-#define strscpy(dst, src, size)	sized_strscpy(dst, src, size)
+#define strscpy(dst, src)	sized_strscpy(dst, src, sizeof(dst))
 
 /* Copied from linux/compiler-gcc.h since we can't include it directly */
 #define barrier() __asm__ __volatile__("": : :"memory")
diff --git a/arch/um/os-Linux/drivers/ethertap_user.c b/arch/um/os-Linux/drivers/ethertap_user.c
index 3363851a4ae8..bdf215c0eca7 100644
--- a/arch/um/os-Linux/drivers/ethertap_user.c
+++ b/arch/um/os-Linux/drivers/ethertap_user.c
@@ -105,7 +105,7 @@ static int etap_tramp(char *dev, char *gate, int control_me,
 	sprintf(data_fd_buf, "%d", data_remote);
 	sprintf(version_buf, "%d", UML_NET_VERSION);
 	if (gate != NULL) {
-		strscpy(gate_buf, gate, sizeof(gate_buf));
+		strscpy(gate_buf, gate);
 		args = setup_args;
 	}
 	else args = nosetup_args;
diff --git a/arch/um/os-Linux/drivers/tuntap_user.c b/arch/um/os-Linux/drivers/tuntap_user.c
index 2284e9c1cbbb..91f0e27ca3a6 100644
--- a/arch/um/os-Linux/drivers/tuntap_user.c
+++ b/arch/um/os-Linux/drivers/tuntap_user.c
@@ -146,7 +146,7 @@ static int tuntap_open(void *data)
 		}
 		memset(&ifr, 0, sizeof(ifr));
 		ifr.ifr_flags = IFF_TAP | IFF_NO_PI;
-		strscpy(ifr.ifr_name, pri->dev_name, sizeof(ifr.ifr_name));
+		strscpy(ifr.ifr_name, pri->dev_name);
 		if (ioctl(pri->fd, TUNSETIFF, &ifr) < 0) {
 			err = -errno;
 			printk(UM_KERN_ERR "TUNSETIFF failed, errno = %d\n",
diff --git a/arch/um/os-Linux/umid.c b/arch/um/os-Linux/umid.c
index 288c422bfa96..e09d65b05d1c 100644
--- a/arch/um/os-Linux/umid.c
+++ b/arch/um/os-Linux/umid.c
@@ -40,7 +40,7 @@ static int __init make_uml_dir(void)
 				__func__);
 			goto err;
 		}
-		strscpy(dir, home, sizeof(dir));
+		strscpy(dir, home);
 		uml_dir++;
 	}
 	strlcat(dir, uml_dir, sizeof(dir));
@@ -243,7 +243,7 @@ int __init set_umid(char *name)
 	if (strlen(name) > UMID_LEN - 1)
 		return -E2BIG;
 
-	strscpy(umid, name, sizeof(umid));
+	strscpy(umid, name);
 
 	return 0;
 }
@@ -262,7 +262,7 @@ static int __init make_umid(void)
 	make_uml_dir();
 
 	if (*umid == '\0') {
-		strscpy(tmp, uml_dir, sizeof(tmp));
+		strscpy(tmp, uml_dir);
 		strlcat(tmp, "XXXXXX", sizeof(tmp));
 		fd = mkstemp(tmp);
 		if (fd < 0) {
-- 
2.34.1

Re: [PATCH v3 4/4] um: Convert strscpy() usage to 2-argument style

[Andy Shevchenko]

On Tue, Feb 6, 2024 at 4:22 PM Kees Cook <keescook@chromium.org> wrote:
>
> The ARCH=um build has its own idea about strscpy()'s definition. Adjust
> the callers to remove the redundant sizeof() arguments ahead of treewide
> changes, since it needs a manual adjustment for the newly named
> sized_strscpy() export.

...

> -               strscpy(dir, home, sizeof(dir));
> +               strscpy(dir, home);
>                 uml_dir++;
>         }
>         strlcat(dir, uml_dir, sizeof(dir));

An (unrelated) side note: are we going to get rid of strlcat() as well
(after strlcpy() is gone)?

...

>         if (*umid == '\0') {
> -               strscpy(tmp, uml_dir, sizeof(tmp));
> +               strscpy(tmp, uml_dir);
>                 strlcat(tmp, "XXXXXX", sizeof(tmp));

This code is interesting... (Esp. taking into account making a
temporary folder out of this...)

-- 
With Best Regards,
Andy Shevchenko

Removing more str APIs (was Re: [PATCH v3 4/4] um: Convert strscpy() usage to 2-argument style)

[Kees Cook]

On Tue, Feb 06, 2024 at 05:02:16PM +0200, Andy Shevchenko wrote:
> On Tue, Feb 6, 2024 at 4:22 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > The ARCH=um build has its own idea about strscpy()'s definition. Adjust
> > the callers to remove the redundant sizeof() arguments ahead of treewide
> > changes, since it needs a manual adjustment for the newly named
> > sized_strscpy() export.
> 
> ...
> 
> > -               strscpy(dir, home, sizeof(dir));
> > +               strscpy(dir, home);
> >                 uml_dir++;
> >         }
> >         strlcat(dir, uml_dir, sizeof(dir));
> 
> An (unrelated) side note: are we going to get rid of strlcat() as well
> (after strlcpy() is gone)?

I think it would be worthwhile to remove it, yes. Switching to seq_buf
in many cases seemed to be the clear solution, which is what triggered
my trying to improve the allocation ergonomics for seq_buf recently:
https://lore.kernel.org/linux-hardening/20231027155634.make.260-kees@kernel.org/

Its not in super common usage, so I think we can start chipping away at
it:

$ git grep '\bstrlcat(' | wc -l
480

It's more risky cases (using the return value) is relatively rare,
though, so I hadn't been prioritizing its removal:

$ git grep ' = strlcat(\b' | wc -l
13

(And almost all of it is in security/selinux/ima.c)


As a comparison, strncpy has even fewer currently, with Justin making a
dent on it recently:

$ git grep '\bstrncpy(' | wc -l
311


> 
> ...
> 
> >         if (*umid == '\0') {
> > -               strscpy(tmp, uml_dir, sizeof(tmp));
> > +               strscpy(tmp, uml_dir);
> >                 strlcat(tmp, "XXXXXX", sizeof(tmp));
> 
> This code is interesting... (Esp. taking into account making a
> temporary folder out of this...)

I have tried to avoid reading UML code too closely. ;)

-- 
Kees Cook