[PATCH v2] string: Introduce strtomem() and strtomem_pad()

[PATCH v2] string: Introduce strtomem() and strtomem_pad()

[Kees Cook]

One of the "legitimate" uses of strncpy() is copying a NUL-terminated
string into a fixed-size non-NUL-terminated character array. To avoid
the weaknesses and ambiguity of intent when using strncpy(), provide
replacement functions that explicitly distinguish between trailing
padding and not, and require the destination buffer size be discoverable
by the compiler.

For example:

struct obj {
	int foo;
	char small[4] __nonstring;
	char big[8] __nonstring;
	int bar;
};

struct obj p;

/* This will truncate to 4 chars with no trailing NUL */
strncpy(p.small, "hello", sizeof(p.small));
/* p.small contains 'h', 'e', 'l', 'l' */

/* This will NUL pad to 8 chars. */
strncpy(p.big, "hello", sizeof(p.big));
/* p.big contains 'h', 'e', 'l', 'l', 'o', '\0', '\0', '\0' */

When the "__nonstring" attributes are missing, the intent of the
programmer becomes ambiguous for whether the lack of a trailing NUL
in the p.small copy is a bug. Additionally, it's not clear whether
the trailing padding in the p.big copy is _needed_. Both cases
become unambiguous with:

strtomem(p.small, "hello");
strtomem_pad(p.big, "hello", 0);

See also https://github.com/KSPP/linux/issues/90

Expand the memcpy KUnit tests to include these functions.

Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
v2:
 - updated deprecated.rst to include strtomem*()
 - added kerndoc and replacement table to strncpy()
 - switched to ULONG_MAX in KUnit tests (Geert)
 - fix typo in commit log example (Geert)
v1: https://lore.kernel.org/lkml/20220831230006.1016236-1-keescook@chromium.org
---
 Documentation/process/deprecated.rst | 11 +++---
 include/linux/fortify-string.h       | 30 ++++++++++++++++
 include/linux/string.h               | 43 ++++++++++++++++++++++
 lib/memcpy_kunit.c                   | 53 ++++++++++++++++++++++++++++
 4 files changed, 133 insertions(+), 4 deletions(-)

diff --git a/Documentation/process/deprecated.rst b/Documentation/process/deprecated.rst
index a6e36d9c3d14..783b0488cf4d 100644
--- a/Documentation/process/deprecated.rst
+++ b/Documentation/process/deprecated.rst
@@ -138,17 +138,20 @@ be NUL terminated. This can lead to various linear read overflows and
 other misbehavior due to the missing termination. It also NUL-pads
 the destination buffer if the source contents are shorter than the
 destination buffer size, which may be a needless performance penalty
-for callers using only NUL-terminated strings. The safe replacement is
+for callers using only NUL-terminated strings.
+
+When the destination is required to be NUL-terminated, the replacement is
 strscpy(), though care must be given to any cases where the return value
 of strncpy() was used, since strscpy() does not return a pointer to the
 destination, but rather a count of non-NUL bytes copied (or negative
 errno when it truncates). Any cases still needing NUL-padding should
 instead use strscpy_pad().
 
-If a caller is using non-NUL-terminated strings, strncpy() can
-still be used, but destinations should be marked with the `__nonstring
+If a caller is using non-NUL-terminated strings, strtomem() should be
+be used, and the destinations should be marked with the `__nonstring
 <https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html>`_
-attribute to avoid future compiler warnings.
+attribute to avoid future compiler warnings. For cases still needing
+NUL-padding, strtomem_pad() can be used.
 
 strlcpy()
 ---------
diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
index 3b401fa0f374..eed2119b23c5 100644
--- a/include/linux/fortify-string.h
+++ b/include/linux/fortify-string.h
@@ -77,6 +77,36 @@ extern char *__underlying_strncpy(char *p, const char *q, __kernel_size_t size)
 #define POS	__pass_object_size(1)
 #define POS0	__pass_object_size(0)
 
+/** strncpy - Copy a string to memory with non-guaranteed NUL padding
+ *
+ * @p: pointer to destination of copy
+ * @q: pointer to NUL-terminated source string to copy
+ * @size: bytes to write at @p
+ *
+ * If strlen(@q) >= @size, the copy of @q will stop after @size bytes,
+ * and @p will NOT be NUL-terminated
+ *
+ * If strlen(@q) < @size, following the copy of @q, trailing NUL bytes
+ * will be written to @p until @size total bytes have been written.
+ *
+ * Do not use this function. While FORTIFY_SOURCE tries to avoid
+ * over-reads of @q, it cannot defend against writing unterminated
+ * results to @p. Using strncpy() remains ambiguous and fragile.
+ * Instead, please choose an alternative, so that the expectation
+ * of @p's contents is unambiguous:
+ *
+ * @p needs to be:     | padded to @size | not padded
+ * --------------------+-----------------+------------+
+ *      NUL-terminated | strscpy_pad()   | strscpy()  |
+ * --------------------+-----------------+------------+
+ *  not NUL-terminated | strtomem_pad()  | strtomem() |
+ * --------------------+-----------------+------------+
+ *
+ * Note strscpy*()'s differing return values for detecting truncation,
+ * and strtomem*()'s expectation that the destination is marked with
+ * __nonstring when it is a character array.
+ *
+ */
 __FORTIFY_INLINE __diagnose_as(__builtin_strncpy, 1, 2, 3)
 char *strncpy(char * const POS p, const char *q, __kernel_size_t size)
 {
diff --git a/include/linux/string.h b/include/linux/string.h
index 61ec7e4f6311..cf7607b32102 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -260,6 +260,49 @@ static inline const char *kbasename(const char *path)
 void memcpy_and_pad(void *dest, size_t dest_len, const void *src, size_t count,
 		    int pad);
 
+/**
+ * strtomem_pad - Copy NUL-terminated string to non-NUL-terminated buffer
+ *
+ * @dest: Pointer of destination character array (marked as __nonstring)
+ * @src: Pointer to NUL-terminated string
+ * @pad: Padding character to fill any remaining bytes of @dest after copy
+ *
+ * This is a replacement for strncpy() uses where the destination is not
+ * a NUL-terminated string, but with bounds checking on the source size, and
+ * an explicit padding character. If padding is not required, use strtomem().
+ *
+ * Note that the size of @dest is not an argument, as the length of @dest
+ * must be discoverable by the compiler.
+ */
+#define strtomem_pad(dest, src, pad)	do {				\
+	const size_t _dest_len = __builtin_object_size(dest, 1);	\
+									\
+	BUILD_BUG_ON(!__builtin_constant_p(_dest_len) ||		\
+		     _dest_len == (size_t)-1);				\
+	memcpy_and_pad(dest, _dest_len, src, strnlen(src, _dest_len), pad); \
+} while (0)
+
+/**
+ * strtomem - Copy NUL-terminated string to non-NUL-terminated buffer
+ *
+ * @dest: Pointer of destination character array (marked as __nonstring)
+ * @src: Pointer to NUL-terminated string
+ *
+ * This is a replacement for strncpy() uses where the destination is not
+ * a NUL-terminated string, but with bounds checking on the source size, and
+ * without trailing padding. If padding is required, use strtomem_pad().
+ *
+ * Note that the size of @dest is not an argument, as the length of @dest
+ * must be discoverable by the compiler.
+ */
+#define strtomem(dest, src)	do {					\
+	const size_t _dest_len = __builtin_object_size(dest, 1);	\
+									\
+	BUILD_BUG_ON(!__builtin_constant_p(_dest_len) ||		\
+		     _dest_len == (size_t)-1);				\
+	memcpy(dest, src, min(_dest_len, strnlen(src, _dest_len)));	\
+} while (0)
+
 /**
  * memset_after - Set a value after a struct member to the end of a struct
  *
diff --git a/lib/memcpy_kunit.c b/lib/memcpy_kunit.c
index 62f8ffcbbaa3..598f5f7dadf4 100644
--- a/lib/memcpy_kunit.c
+++ b/lib/memcpy_kunit.c
@@ -272,10 +272,63 @@ static void memset_test(struct kunit *test)
 #undef TEST_OP
 }
 
+static void strtomem_test(struct kunit *test)
+{
+	static const char input[] = "hi";
+	static const char truncate[] = "this is too long";
+	struct {
+		unsigned long canary1;
+		unsigned char output[sizeof(unsigned long)] __nonstring;
+		unsigned long canary2;
+	} wrap;
+
+	memset(&wrap, 0xFF, sizeof(wrap));
+	KUNIT_EXPECT_EQ_MSG(test, wrap.canary1, ULONG_MAX,
+			    "bad initial canary value");
+	KUNIT_EXPECT_EQ_MSG(test, wrap.canary2, ULONG_MAX,
+			    "bad initial canary value");
+
+	/* Check unpadded copy leaves surroundings untouched. */
+	strtomem(wrap.output, input);
+	KUNIT_EXPECT_EQ(test, wrap.canary1, ULONG_MAX);
+	KUNIT_EXPECT_EQ(test, wrap.output[0], input[0]);
+	KUNIT_EXPECT_EQ(test, wrap.output[1], input[1]);
+	for (int i = 2; i < sizeof(wrap.output); i++)
+		KUNIT_EXPECT_EQ(test, wrap.output[i], 0xFF);
+	KUNIT_EXPECT_EQ(test, wrap.canary2, ULONG_MAX);
+
+	/* Check truncated copy leaves surroundings untouched. */
+	memset(&wrap, 0xFF, sizeof(wrap));
+	strtomem(wrap.output, truncate);
+	KUNIT_EXPECT_EQ(test, wrap.canary1, ULONG_MAX);
+	for (int i = 0; i < sizeof(wrap.output); i++)
+		KUNIT_EXPECT_EQ(test, wrap.output[i], truncate[i]);
+	KUNIT_EXPECT_EQ(test, wrap.canary2, ULONG_MAX);
+
+	/* Check padded copy leaves only string padded. */
+	memset(&wrap, 0xFF, sizeof(wrap));
+	strtomem_pad(wrap.output, input, 0xAA);
+	KUNIT_EXPECT_EQ(test, wrap.canary1, ULONG_MAX);
+	KUNIT_EXPECT_EQ(test, wrap.output[0], input[0]);
+	KUNIT_EXPECT_EQ(test, wrap.output[1], input[1]);
+	for (int i = 2; i < sizeof(wrap.output); i++)
+		KUNIT_EXPECT_EQ(test, wrap.output[i], 0xAA);
+	KUNIT_EXPECT_EQ(test, wrap.canary2, ULONG_MAX);
+
+	/* Check truncated padded copy has no padding. */
+	memset(&wrap, 0xFF, sizeof(wrap));
+	strtomem(wrap.output, truncate);
+	KUNIT_EXPECT_EQ(test, wrap.canary1, ULONG_MAX);
+	for (int i = 0; i < sizeof(wrap.output); i++)
+		KUNIT_EXPECT_EQ(test, wrap.output[i], truncate[i]);
+	KUNIT_EXPECT_EQ(test, wrap.canary2, ULONG_MAX);
+}
+
 static struct kunit_case memcpy_test_cases[] = {
 	KUNIT_CASE(memset_test),
 	KUNIT_CASE(memcpy_test),
 	KUNIT_CASE(memmove_test),
+	KUNIT_CASE(strtomem_test),
 	{}
 };
 
-- 
2.34.1

Re: [PATCH v2] string: Introduce strtomem() and strtomem_pad()

[Guenter Roeck]

On 9/1/22 12:09, Kees Cook wrote:
> One of the "legitimate" uses of strncpy() is copying a NUL-terminated
> string into a fixed-size non-NUL-terminated character array. To avoid
> the weaknesses and ambiguity of intent when using strncpy(), provide
> replacement functions that explicitly distinguish between trailing
> padding and not, and require the destination buffer size be discoverable
> by the compiler.
> 
> For example:
> 
> struct obj {
> 	int foo;
> 	char small[4] __nonstring;
> 	char big[8] __nonstring;
> 	int bar;
> };
> 
> struct obj p;
> 
> /* This will truncate to 4 chars with no trailing NUL */
> strncpy(p.small, "hello", sizeof(p.small));
> /* p.small contains 'h', 'e', 'l', 'l' */
> 
> /* This will NUL pad to 8 chars. */
> strncpy(p.big, "hello", sizeof(p.big));
> /* p.big contains 'h', 'e', 'l', 'l', 'o', '\0', '\0', '\0' */
> 
> When the "__nonstring" attributes are missing, the intent of the
> programmer becomes ambiguous for whether the lack of a trailing NUL
> in the p.small copy is a bug. Additionally, it's not clear whether
> the trailing padding in the p.big copy is _needed_. Both cases
> become unambiguous with:
> 
> strtomem(p.small, "hello");
> strtomem_pad(p.big, "hello", 0);
> 
> See also https://github.com/KSPP/linux/issues/90
> 
> Expand the memcpy KUnit tests to include these functions.
> 
> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Cc: Nick Desaulniers <ndesaulniers@google.com>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> v2:
>   - updated deprecated.rst to include strtomem*()
>   - added kerndoc and replacement table to strncpy()
>   - switched to ULONG_MAX in KUnit tests (Geert)
>   - fix typo in commit log example (Geert)
> v1: https://lore.kernel.org/lkml/20220831230006.1016236-1-keescook@chromium.org
> ---
>   Documentation/process/deprecated.rst | 11 +++---
>   include/linux/fortify-string.h       | 30 ++++++++++++++++
>   include/linux/string.h               | 43 ++++++++++++++++++++++
>   lib/memcpy_kunit.c                   | 53 ++++++++++++++++++++++++++++
>   4 files changed, 133 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/process/deprecated.rst b/Documentation/process/deprecated.rst
> index a6e36d9c3d14..783b0488cf4d 100644
> --- a/Documentation/process/deprecated.rst
> +++ b/Documentation/process/deprecated.rst
> @@ -138,17 +138,20 @@ be NUL terminated. This can lead to various linear read overflows and
>   other misbehavior due to the missing termination. It also NUL-pads
>   the destination buffer if the source contents are shorter than the
>   destination buffer size, which may be a needless performance penalty
> -for callers using only NUL-terminated strings. The safe replacement is
> +for callers using only NUL-terminated strings.
> +
> +When the destination is required to be NUL-terminated, the replacement is
>   strscpy(), though care must be given to any cases where the return value
>   of strncpy() was used, since strscpy() does not return a pointer to the
>   destination, but rather a count of non-NUL bytes copied (or negative
>   errno when it truncates). Any cases still needing NUL-padding should
>   instead use strscpy_pad().
>   
> -If a caller is using non-NUL-terminated strings, strncpy() can
> -still be used, but destinations should be marked with the `__nonstring
> +If a caller is using non-NUL-terminated strings, strtomem() should be
> +be used, and the destinations should be marked with the `__nonstring

s/be //

>   <https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html>`_
> -attribute to avoid future compiler warnings.
> +attribute to avoid future compiler warnings. For cases still needing
> +NUL-padding, strtomem_pad() can be used.
>   
>   strlcpy()
>   ---------
> diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
> index 3b401fa0f374..eed2119b23c5 100644
> --- a/include/linux/fortify-string.h
> +++ b/include/linux/fortify-string.h
> @@ -77,6 +77,36 @@ extern char *__underlying_strncpy(char *p, const char *q, __kernel_size_t size)
>   #define POS	__pass_object_size(1)
>   #define POS0	__pass_object_size(0)
>   
> +/** strncpy - Copy a string to memory with non-guaranteed NUL padding

Does that need a newline before strncpy() ?

> + *
> + * @p: pointer to destination of copy
> + * @q: pointer to NUL-terminated source string to copy
> + * @size: bytes to write at @p
> + *
> + * If strlen(@q) >= @size, the copy of @q will stop after @size bytes,
> + * and @p will NOT be NUL-terminated
> + *
> + * If strlen(@q) < @size, following the copy of @q, trailing NUL bytes
> + * will be written to @p until @size total bytes have been written.
> + *
> + * Do not use this function. While FORTIFY_SOURCE tries to avoid
> + * over-reads of @q, it cannot defend against writing unterminated
> + * results to @p. Using strncpy() remains ambiguous and fragile.
> + * Instead, please choose an alternative, so that the expectation
> + * of @p's contents is unambiguous:
> + *
> + * @p needs to be:     | padded to @size | not padded
> + * --------------------+-----------------+------------+
> + *      NUL-terminated | strscpy_pad()   | strscpy()  |
> + * --------------------+-----------------+------------+
> + *  not NUL-terminated | strtomem_pad()  | strtomem() |
> + * --------------------+-----------------+------------+
> + *
> + * Note strscpy*()'s differing return values for detecting truncation,
> + * and strtomem*()'s expectation that the destination is marked with
> + * __nonstring when it is a character array.
> + *
> + */
>   __FORTIFY_INLINE __diagnose_as(__builtin_strncpy, 1, 2, 3)
>   char *strncpy(char * const POS p, const char *q, __kernel_size_t size)
>   {
> diff --git a/include/linux/string.h b/include/linux/string.h
> index 61ec7e4f6311..cf7607b32102 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -260,6 +260,49 @@ static inline const char *kbasename(const char *path)
>   void memcpy_and_pad(void *dest, size_t dest_len, const void *src, size_t count,
>   		    int pad);
>   
> +/**
> + * strtomem_pad - Copy NUL-terminated string to non-NUL-terminated buffer
> + *
> + * @dest: Pointer of destination character array (marked as __nonstring)
> + * @src: Pointer to NUL-terminated string
> + * @pad: Padding character to fill any remaining bytes of @dest after copy
> + *
> + * This is a replacement for strncpy() uses where the destination is not
> + * a NUL-terminated string, but with bounds checking on the source size, and
> + * an explicit padding character. If padding is not required, use strtomem().
> + *
> + * Note that the size of @dest is not an argument, as the length of @dest
> + * must be discoverable by the compiler.
> + */
> +#define strtomem_pad(dest, src, pad)	do {				\
> +	const size_t _dest_len = __builtin_object_size(dest, 1);	\
> +									\
> +	BUILD_BUG_ON(!__builtin_constant_p(_dest_len) ||		\
> +		     _dest_len == (size_t)-1);				\
> +	memcpy_and_pad(dest, _dest_len, src, strnlen(src, _dest_len), pad); \
> +} while (0)
> +
> +/**
> + * strtomem - Copy NUL-terminated string to non-NUL-terminated buffer
> + *
> + * @dest: Pointer of destination character array (marked as __nonstring)
> + * @src: Pointer to NUL-terminated string
> + *
> + * This is a replacement for strncpy() uses where the destination is not
> + * a NUL-terminated string, but with bounds checking on the source size, and
> + * without trailing padding. If padding is required, use strtomem_pad().
> + *
> + * Note that the size of @dest is not an argument, as the length of @dest
> + * must be discoverable by the compiler.
> + */
> +#define strtomem(dest, src)	do {					\
> +	const size_t _dest_len = __builtin_object_size(dest, 1);	\
> +									\
> +	BUILD_BUG_ON(!__builtin_constant_p(_dest_len) ||		\
> +		     _dest_len == (size_t)-1);				\
> +	memcpy(dest, src, min(_dest_len, strnlen(src, _dest_len)));	\
> +} while (0)
> +
>   /**
>    * memset_after - Set a value after a struct member to the end of a struct
>    *
> diff --git a/lib/memcpy_kunit.c b/lib/memcpy_kunit.c
> index 62f8ffcbbaa3..598f5f7dadf4 100644
> --- a/lib/memcpy_kunit.c
> +++ b/lib/memcpy_kunit.c
> @@ -272,10 +272,63 @@ static void memset_test(struct kunit *test)
>   #undef TEST_OP
>   }
>   
> +static void strtomem_test(struct kunit *test)
> +{
> +	static const char input[] = "hi";
> +	static const char truncate[] = "this is too long";
> +	struct {
> +		unsigned long canary1;
> +		unsigned char output[sizeof(unsigned long)] __nonstring;
> +		unsigned long canary2;
> +	} wrap;
> +
> +	memset(&wrap, 0xFF, sizeof(wrap));
> +	KUNIT_EXPECT_EQ_MSG(test, wrap.canary1, ULONG_MAX,
> +			    "bad initial canary value");
> +	KUNIT_EXPECT_EQ_MSG(test, wrap.canary2, ULONG_MAX,
> +			    "bad initial canary value");
> +
> +	/* Check unpadded copy leaves surroundings untouched. */
> +	strtomem(wrap.output, input);
> +	KUNIT_EXPECT_EQ(test, wrap.canary1, ULONG_MAX);
> +	KUNIT_EXPECT_EQ(test, wrap.output[0], input[0]);
> +	KUNIT_EXPECT_EQ(test, wrap.output[1], input[1]);
> +	for (int i = 2; i < sizeof(wrap.output); i++)
> +		KUNIT_EXPECT_EQ(test, wrap.output[i], 0xFF);
> +	KUNIT_EXPECT_EQ(test, wrap.canary2, ULONG_MAX);
> +
> +	/* Check truncated copy leaves surroundings untouched. */
> +	memset(&wrap, 0xFF, sizeof(wrap));
> +	strtomem(wrap.output, truncate);
> +	KUNIT_EXPECT_EQ(test, wrap.canary1, ULONG_MAX);
> +	for (int i = 0; i < sizeof(wrap.output); i++)
> +		KUNIT_EXPECT_EQ(test, wrap.output[i], truncate[i]);
> +	KUNIT_EXPECT_EQ(test, wrap.canary2, ULONG_MAX);
> +
> +	/* Check padded copy leaves only string padded. */
> +	memset(&wrap, 0xFF, sizeof(wrap));
> +	strtomem_pad(wrap.output, input, 0xAA);
> +	KUNIT_EXPECT_EQ(test, wrap.canary1, ULONG_MAX);
> +	KUNIT_EXPECT_EQ(test, wrap.output[0], input[0]);
> +	KUNIT_EXPECT_EQ(test, wrap.output[1], input[1]);
> +	for (int i = 2; i < sizeof(wrap.output); i++)
> +		KUNIT_EXPECT_EQ(test, wrap.output[i], 0xAA);
> +	KUNIT_EXPECT_EQ(test, wrap.canary2, ULONG_MAX);
> +
> +	/* Check truncated padded copy has no padding. */
> +	memset(&wrap, 0xFF, sizeof(wrap));
> +	strtomem(wrap.output, truncate);
> +	KUNIT_EXPECT_EQ(test, wrap.canary1, ULONG_MAX);
> +	for (int i = 0; i < sizeof(wrap.output); i++)
> +		KUNIT_EXPECT_EQ(test, wrap.output[i], truncate[i]);
> +	KUNIT_EXPECT_EQ(test, wrap.canary2, ULONG_MAX);
> +}
> +
>   static struct kunit_case memcpy_test_cases[] = {
>   	KUNIT_CASE(memset_test),
>   	KUNIT_CASE(memcpy_test),
>   	KUNIT_CASE(memmove_test),
> +	KUNIT_CASE(strtomem_test),
>   	{}
>   };
>   

Re: [PATCH v2] string: Introduce strtomem() and strtomem_pad()

[Bagas Sanjaya]

On 9/2/22 02:09, Kees Cook wrote:
> One of the "legitimate" uses of strncpy() is copying a NUL-terminated
> string into a fixed-size non-NUL-terminated character array. To avoid
> the weaknesses and ambiguity of intent when using strncpy(), provide
> replacement functions that explicitly distinguish between trailing
> padding and not, and require the destination buffer size be discoverable
> by the compiler.
>> For example:
> 
> struct obj {
> 	int foo;
> 	char small[4] __nonstring;
> 	char big[8] __nonstring;
> 	int bar;
> };
> 
> struct obj p;
> 
> /* This will truncate to 4 chars with no trailing NUL */
> strncpy(p.small, "hello", sizeof(p.small));
> /* p.small contains 'h', 'e', 'l', 'l' */
> 
> /* This will NUL pad to 8 chars. */
> strncpy(p.big, "hello", sizeof(p.big));
> /* p.big contains 'h', 'e', 'l', 'l', 'o', '\0', '\0', '\0' */
> 
> When the "__nonstring" attributes are missing, the intent of the
> programmer becomes ambiguous for whether the lack of a trailing NUL
> in the p.small copy is a bug. Additionally, it's not clear whether
> the trailing padding in the p.big copy is _needed_. Both cases
> become unambiguous with:
> 
> strtomem(p.small, "hello");
> strtomem_pad(p.big, "hello", 0);
> 
> See also https://github.com/KSPP/linux/issues/90
> 

Should'nt strscpy() do the job?

-- 
An old man doll... just what I always wanted! - Clara

Re: [PATCH v2] string: Introduce strtomem() and strtomem_pad()

[Bagas Sanjaya]

[-- Attachment #1: Type: text/plain, Size: 2557 bytes --]

On Thu, Sep 01, 2022 at 12:09:52PM -0700, Kees Cook wrote:
> + * Do not use this function. While FORTIFY_SOURCE tries to avoid
> + * over-reads of @q, it cannot defend against writing unterminated
> + * results to @p. Using strncpy() remains ambiguous and fragile.
> + * Instead, please choose an alternative, so that the expectation
> + * of @p's contents is unambiguous:
> + *
> + * @p needs to be:     | padded to @size | not padded
> + * --------------------+-----------------+------------+
> + *      NUL-terminated | strscpy_pad()   | strscpy()  |
> + * --------------------+-----------------+------------+
> + *  not NUL-terminated | strtomem_pad()  | strtomem() |
> + * --------------------+-----------------+------------+
> + *

My htmldocs build doesn't catch any new warnings, but I think the table
above can be fixed up:

---- >8 ----

diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
index eed2119b23c523..3413a8e561fc62 100644
--- a/include/linux/fortify-string.h
+++ b/include/linux/fortify-string.h
@@ -95,12 +95,13 @@ extern char *__underlying_strncpy(char *p, const char *q, __kernel_size_t size)
  * Instead, please choose an alternative, so that the expectation
  * of @p's contents is unambiguous:
  *
- * @p needs to be:     | padded to @size | not padded
- * --------------------+-----------------+------------+
- *      NUL-terminated | strscpy_pad()   | strscpy()  |
- * --------------------+-----------------+------------+
- *  not NUL-terminated | strtomem_pad()  | strtomem() |
- * --------------------+-----------------+------------+
+ * +--------------------+-----------------+------------+
+ * |@p needs to be:     | padded to @size | not padded |
+ * +====================+=================+============+
+ * |     NUL-terminated | strscpy_pad()   | strscpy()  |
+ * +--------------------+-----------------+------------+
+ * | not NUL-terminated | strtomem_pad()  | strtomem() |
+ * +--------------------+-----------------+------------+
  *
  * Note strscpy*()'s differing return values for detecting truncation,
  * and strtomem*()'s expectation that the destination is marked with

> + * Note strscpy*()'s differing return values for detecting truncation,
> + * and strtomem*()'s expectation that the destination is marked with
> + * __nonstring when it is a character array.
> + *

Regardless, I don't see these new table above in the output (am I missing
something?).

Thanks.

-- 
An old man doll... just what I always wanted! - Clara

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2] string: Introduce strtomem() and strtomem_pad()

[Kees Cook]

On Thu, Sep 01, 2022 at 12:34:34PM -0700, Guenter Roeck wrote:
> On 9/1/22 12:09, Kees Cook wrote:
> > [...]
> > -If a caller is using non-NUL-terminated strings, strncpy() can
> > -still be used, but destinations should be marked with the `__nonstring
> > +If a caller is using non-NUL-terminated strings, strtomem() should be
> > +be used, and the destinations should be marked with the `__nonstring
> 
> s/be //

Thanks!

> > [...]
> > +++ b/include/linux/fortify-string.h
> > @@ -77,6 +77,36 @@ extern char *__underlying_strncpy(char *p, const char *q, __kernel_size_t size)
> >   #define POS	__pass_object_size(1)
> >   #define POS0	__pass_object_size(0)
> > +/** strncpy - Copy a string to memory with non-guaranteed NUL padding
> 
> Does that need a newline before strncpy() ?

What do you mean here? I think this is valid kerndoc, but I'll
double-check. (And will continue in the neighboring htmldoc build thread.)

-Kees

-- 
Kees Cook

Re: [PATCH v2] string: Introduce strtomem() and strtomem_pad()

[Kees Cook]

On Fri, Sep 02, 2022 at 08:53:34AM +0700, Bagas Sanjaya wrote:
> On 9/2/22 02:09, Kees Cook wrote:
> > One of the "legitimate" uses of strncpy() is copying a NUL-terminated
> > string into a fixed-size non-NUL-terminated character array. To avoid
> > the weaknesses and ambiguity of intent when using strncpy(), provide
> > replacement functions that explicitly distinguish between trailing
> > padding and not, and require the destination buffer size be discoverable
> > by the compiler.
> >> For example:
> > 
> > struct obj {
> > 	int foo;
> > 	char small[4] __nonstring;
> > 	char big[8] __nonstring;
> > 	int bar;
> > };
> > 
> > struct obj p;
> > 
> > /* This will truncate to 4 chars with no trailing NUL */
> > strncpy(p.small, "hello", sizeof(p.small));
> > /* p.small contains 'h', 'e', 'l', 'l' */
> > 
> > /* This will NUL pad to 8 chars. */
> > strncpy(p.big, "hello", sizeof(p.big));
> > /* p.big contains 'h', 'e', 'l', 'l', 'o', '\0', '\0', '\0' */
> > 
> > When the "__nonstring" attributes are missing, the intent of the
> > programmer becomes ambiguous for whether the lack of a trailing NUL
> > in the p.small copy is a bug. Additionally, it's not clear whether
> > the trailing padding in the p.big copy is _needed_. Both cases
> > become unambiguous with:
> > 
> > strtomem(p.small, "hello");
> > strtomem_pad(p.big, "hello", 0);
> > 
> > See also https://github.com/KSPP/linux/issues/90
> > 
> 
> Should'nt strscpy() do the job?

strscpy() will always NUL-terminate. If someone is moving a
NUL-terminated string to a fixed-length buffer (that is _not_
NUL-terminated), using strscpy() will force the final byte to be 0x00,
which will likely be a regression. For example:

struct wifi_driver {
	...
	char essid[8];
	...
};

struct wifi_driver fw;

char *essed = "12345678";

strncpy(fw.essid, essid, sizeof(fw.essid));

	fw.essid will contain: 1 2 3 4 5 6 7 8

strscpy(fw.essid, essid, sizeof(fw.essid)):

	fw.essid will contain: 1 2 3 4 5 6 7 '\0'


-- 
Kees Cook

Re: [PATCH v2] string: Introduce strtomem() and strtomem_pad()

[Kees Cook]

On Fri, Sep 02, 2022 at 11:21:21AM +0700, Bagas Sanjaya wrote:
> On Thu, Sep 01, 2022 at 12:09:52PM -0700, Kees Cook wrote:
> > + * Do not use this function. While FORTIFY_SOURCE tries to avoid
> > + * over-reads of @q, it cannot defend against writing unterminated
> > + * results to @p. Using strncpy() remains ambiguous and fragile.
> > + * Instead, please choose an alternative, so that the expectation
> > + * of @p's contents is unambiguous:
> > + *
> > + * @p needs to be:     | padded to @size | not padded
> > + * --------------------+-----------------+------------+
> > + *      NUL-terminated | strscpy_pad()   | strscpy()  |
> > + * --------------------+-----------------+------------+
> > + *  not NUL-terminated | strtomem_pad()  | strtomem() |
> > + * --------------------+-----------------+------------+
> > + *
> 
> My htmldocs build doesn't catch any new warnings, but I think the table
> above can be fixed up:
> 
> ---- >8 ----
> 
> diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
> index eed2119b23c523..3413a8e561fc62 100644
> --- a/include/linux/fortify-string.h
> +++ b/include/linux/fortify-string.h
> @@ -95,12 +95,13 @@ extern char *__underlying_strncpy(char *p, const char *q, __kernel_size_t size)
>   * Instead, please choose an alternative, so that the expectation
>   * of @p's contents is unambiguous:
>   *
> - * @p needs to be:     | padded to @size | not padded
> - * --------------------+-----------------+------------+
> - *      NUL-terminated | strscpy_pad()   | strscpy()  |
> - * --------------------+-----------------+------------+
> - *  not NUL-terminated | strtomem_pad()  | strtomem() |
> - * --------------------+-----------------+------------+
> + * +--------------------+-----------------+------------+
> + * |@p needs to be:     | padded to @size | not padded |
> + * +====================+=================+============+
> + * |     NUL-terminated | strscpy_pad()   | strscpy()  |
> + * +--------------------+-----------------+------------+
> + * | not NUL-terminated | strtomem_pad()  | strtomem() |
> + * +--------------------+-----------------+------------+

Ah thank you!

> Regardless, I don't see these new table above in the output (am I missing
> something?).

Oh, yes, you're right. I will add an kern-doc directive to pull in the
file.

-- 
Kees Cook

Re: [PATCH v2] string: Introduce strtomem() and strtomem_pad()

[Guenter Roeck]

On Fri, Sep 02, 2022 at 01:52:35PM -0700, Kees Cook wrote:
> On Thu, Sep 01, 2022 at 12:34:34PM -0700, Guenter Roeck wrote:
> > On 9/1/22 12:09, Kees Cook wrote:
> > > [...]
> > > -If a caller is using non-NUL-terminated strings, strncpy() can
> > > -still be used, but destinations should be marked with the `__nonstring
> > > +If a caller is using non-NUL-terminated strings, strtomem() should be
> > > +be used, and the destinations should be marked with the `__nonstring
> > 
> > s/be //
> 
> Thanks!
> 
> > > [...]
> > > +++ b/include/linux/fortify-string.h
> > > @@ -77,6 +77,36 @@ extern char *__underlying_strncpy(char *p, const char *q, __kernel_size_t size)
> > >   #define POS	__pass_object_size(1)
> > >   #define POS0	__pass_object_size(0)
> > > +/** strncpy - Copy a string to memory with non-guaranteed NUL padding
> > 
> > Does that need a newline before strncpy() ?
> 
> What do you mean here? I think this is valid kerndoc, but I'll
> double-check. (And will continue in the neighboring htmldoc build thread.)
> 

Just asking. "/** strncpy - Copy a string ..." seemed unusual without
newline between "/**" and the function name.

Guenter

Re: [PATCH v2] string: Introduce strtomem() and strtomem_pad()

[Kees Cook]

On Fri, Sep 02, 2022 at 02:47:04PM -0700, Guenter Roeck wrote:
> On Fri, Sep 02, 2022 at 01:52:35PM -0700, Kees Cook wrote:
> > On Thu, Sep 01, 2022 at 12:34:34PM -0700, Guenter Roeck wrote:
> > > On 9/1/22 12:09, Kees Cook wrote:
> > > > [...]
> > > > -If a caller is using non-NUL-terminated strings, strncpy() can
> > > > -still be used, but destinations should be marked with the `__nonstring
> > > > +If a caller is using non-NUL-terminated strings, strtomem() should be
> > > > +be used, and the destinations should be marked with the `__nonstring
> > > 
> > > s/be //
> > 
> > Thanks!
> > 
> > > > [...]
> > > > +++ b/include/linux/fortify-string.h
> > > > @@ -77,6 +77,36 @@ extern char *__underlying_strncpy(char *p, const char *q, __kernel_size_t size)
> > > >   #define POS	__pass_object_size(1)
> > > >   #define POS0	__pass_object_size(0)
> > > > +/** strncpy - Copy a string to memory with non-guaranteed NUL padding
> > > 
> > > Does that need a newline before strncpy() ?
> > 
> > What do you mean here? I think this is valid kerndoc, but I'll
> > double-check. (And will continue in the neighboring htmldoc build thread.)
> > 
> 
> Just asking. "/** strncpy - Copy a string ..." seemed unusual without
> newline between "/**" and the function name.

Oops, yes. Thank you again!

-- 
Kees Cook