string: Remove strncpy() from the kernel

strncpy() has been a persistent source of bugs due to its ambiguous intended usage and frequently counter-intuitive semantics: it may not NUL-terminate the destination, and it unconditionally zero-pads to the full length, which isn't always needed. All former callers have been migrated[1] to: - strscpy() for NUL-terminated destinations - strscpy_pad() for NUL-terminated destinations needing zero-padding - strtomem_pad() for non-NUL-terminated fixed-width fields - memcpy_and_pad() for bounded copies with explicit padding - memcpy() for known-length copies Remove the generic implementation, its declaration, the FORTIFY_SOURCE wrapper, and associated tests.

Kees Cook committed 7 months ago

Commit 079a0281 parent 58c4ce88 files · +28 −160Browse files

Unified · Split

8 files

Documentation/process/deprecated.rst +26 −21
drivers/misc/lkdtm/fortify.c +1 −1
include/linux/fortify-string.h +0 −49
include/linux/string.h +0 −3
lib/string.c +0 −16
lib/test_fortify/write_overflow-strncpy-src.c +0 −5
lib/test_fortify/write_overflow-strncpy.c +0 −5
lib/tests/fortify_kunit.c +1 −60

Documentation/process/deprecated.rst+26−21

@@ -131,27 +131,32 @@ value of strcpy() was used, since strscpy() does not return a pointer to · 130 unmodified lines
131	131	the destination, but rather a count of non-NUL bytes copied (or negative
132	132	errno when it truncates).
133	133
134		−strncpy() on NUL-terminated strings
135		−-----------------------------------
136		−Use of strncpy() does not guarantee that the destination buffer will
137		−be NUL terminated. This can lead to various linear read overflows and
138		−other misbehavior due to the missing termination. It also NUL-pads
139		−the destination buffer if the source contents are shorter than the
140		−destination buffer size, which may be a needless performance penalty
141		−for callers using only NUL-terminated strings.
142		−
143		−When the destination is required to be NUL-terminated, the replacement is
144		−strscpy(), though care must be given to any cases where the return value
145		−of strncpy() was used, since strscpy() does not return a pointer to the
146		−destination, but rather a count of non-NUL bytes copied (or negative
147		−errno when it truncates). Any cases still needing NUL-padding should
148		−instead use strscpy_pad().
149		−
150		−If a caller is using non-NUL-terminated strings, strtomem() should be
151		−used, and the destinations should be marked with the `__nonstring
152		−<https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html>`_
153		−attribute to avoid future compiler warnings. For cases still needing
154		−NUL-padding, strtomem_pad() can be used.
	134	+strncpy()
	135	+---------
	136	+strncpy() has been removed from the kernel. All former callers have
	137	+been migrated to safer alternatives.
	138	+
	139	+strncpy() did not guarantee NUL-termination of the destination buffer,
	140	+leading to linear read overflows and other misbehavior. It also
	141	+unconditionally NUL-padded the destination, which was a needless
	142	+performance penalty for callers using only NUL-terminated strings. Due
	143	+to its various behaviors, it was an ambiguous API for determining what
	144	+an author's true intent was for the copy.
	145	+
	146	+The replacements for strncpy() are:
	147	+
	148	+- strscpy() when the destination must be NUL-terminated.
	149	+- strscpy_pad() when the destination must be NUL-terminated and
	150	+ zero-padded (e.g., structs crossing privilege boundaries).
	151	+- memtostr() for NUL-terminated destinations from non-NUL-terminated
	152	+ fixed-width sources (with the `__nonstring` attribute on the source).
	153	+- memtostr_pad() for the same, but with zero-padding.
	154	+- strtomem() for non-NUL-terminated fixed-width destinations, with
	155	+ the `__nonstring` attribute on the destination.
	156	+- strtomem_pad() for non-NUL-terminated destinations that also need
	157	+ zero-padding.
	158	+- memcpy_and_pad() for bounded copies from potentially unterminated
	159	+ sources where the destination size is a runtime value.
155	160
156	161	strlcpy()
157	162	---------

drivers/misc/lkdtm/fortify.c+1−1

@@ -98,7 +98,7 @@ static void lkdtm_FORTIFY_MEM_MEMBER(void) · 97 unmodified lines
98	98	pr_info("trying to memcpy() past the end of a struct member...\n");
99	99
100	100	/*
101		− * strncpy(target.a, src, 20); will hit a compile error because the
	101	+ * memcpy(target.a, src, 20); will hit a compile error because the
102	102	* compiler knows at build time that target.a < 20 bytes. Use a
103	103	* volatile to force a runtime error.
104	104	*/

include/linux/fortify-string.h+0−49

@@ -26,7 +26,6 @@ · 25 unmodified lines
26	26	#define FORTIFY_WRITE 1
27	27
28	28	#define EACH_FORTIFY_FUNC(macro) \
29		− macro(strncpy), \
30	29	macro(strnlen), \
31	30	macro(strlen), \
32	31	macro(strscpy), \
@@ -95,8 +94,6 @@ extern char __underlying_strcat(char p, const char *q) __RENAME(strcat); · 62 unmodified lines
95	94	extern char __underlying_strcpy(char p, const char *q) __RENAME(strcpy);
96	95	extern __kernel_size_t __underlying_strlen(const char *p) __RENAME(strlen);
97	96	extern char __underlying_strncat(char p, const char *q, __kernel_size_t count) __RENAME(strncat);
98		−extern char __underlying_strncpy(char p, const char *q, __kernel_size_t size) __RENAME(strncpy);
99		−
100	97	#else
101	98
102	99	#if defined(__SANITIZE_MEMORY__)
@@ -120,7 +117,6 @@ extern char __underlying_strncpy(char p, const char *q, __kernel_size_t size) · 17 unmodified lines
120	117	#define __underlying_strcpy __builtin_strcpy
121	118	#define __underlying_strlen __builtin_strlen
122	119	#define __underlying_strncat __builtin_strncat
123		−#define __underlying_strncpy __builtin_strncpy
124	120
125	121	#endif
126	122
@@ -159,50 +155,6 @@ extern char __underlying_strncpy(char p, const char *q, __kernel_size_t size) · 32 unmodified lines
159	155	(bounds) < (length) \
160	156	)
161	157
162		−/**
163		− * strncpy - Copy a string to memory with non-guaranteed NUL padding
164		− *
165		− * @p: pointer to destination of copy
166		− * @q: pointer to NUL-terminated source string to copy
167		− * @size: bytes to write at @p
168		− *
169		− * If strlen(@q) >= @size, the copy of @q will stop after @size bytes,
170		− * and @p will NOT be NUL-terminated
171		− *
172		− * If strlen(@q) < @size, following the copy of @q, trailing NUL bytes
173		− * will be written to @p until @size total bytes have been written.
174		− *
175		− * Do not use this function. While FORTIFY_SOURCE tries to avoid
176		− * over-reads of @q, it cannot defend against writing unterminated
177		− * results to @p. Using strncpy() remains ambiguous and fragile.
178		− * Instead, please choose an alternative, so that the expectation
179		− * of @p's contents is unambiguous:
180		− *
181		− * +--------------------+--------------------+------------+
182		− * \| p needs to be: \| padded to size \| not padded \|
183		− * +====================+====================+============+
184		− * \| NUL-terminated \| strscpy_pad() \| strscpy() \|
185		− * +--------------------+--------------------+------------+
186		− * \| not NUL-terminated \| strtomem_pad() \| strtomem() \|
187		− * +--------------------+--------------------+------------+
188		− *
189		− * Note strscpy*()'s differing return values for detecting truncation,
190		− * and strtomem*()'s expectation that the destination is marked with
191		− * __nonstring when it is a character array.
192		− *
193		− */
194		−__FORTIFY_INLINE __diagnose_as(__builtin_strncpy, 1, 2, 3)
195		−char strncpy(char const POS p, const char *q, __kernel_size_t size)
196		−{
197		− const size_t p_size = __member_size(p);
198		−
199		− if (__compiletime_lessthan(p_size, size))
200		− __write_overflow();
201		− if (p_size < size)
202		− fortify_panic(FORTIFY_FUNC_strncpy, FORTIFY_WRITE, p_size, size, p);
203		− return __underlying_strncpy(p, q, size);
204		−}
205		−
206	158	extern __kernel_size_t __real_strnlen(const char *, __kernel_size_t) __RENAME(strnlen);
207	159	/**
208	160	* strnlen - Return bounded count of characters in a NUL-terminated string
@@ -809,7 +761,6 @@ char strcpy(char const POS p, const char * const POS q) · 600 unmodified lines
809	761	#undef __underlying_strcpy
810	762	#undef __underlying_strlen
811	763	#undef __underlying_strncat
812		−#undef __underlying_strncpy
813	764
814	765	#undef POS
815	766	#undef POS0

include/linux/string.h+0−3

@@ -67,9 +67,6 @@ void vmemdup_array_user(const void __user src, size_t n, size_t size) · 66 unmodified lines
67	67	#ifndef __HAVE_ARCH_STRCPY
68	68	extern char * strcpy(char ,const char );
69	69	#endif
70		−#ifndef __HAVE_ARCH_STRNCPY
71		−extern char * strncpy(char ,const char , __kernel_size_t);
72		−#endif
73	70	ssize_t sized_strscpy(char , const char , size_t);
74	71
75	72	/*

lib/string.c+0−16

@@ -88,22 +88,6 @@ char strcpy(char dest, const char *src) · 87 unmodified lines
88	88	EXPORT_SYMBOL(strcpy);
89	89	#endif
90	90
91		−#ifndef __HAVE_ARCH_STRNCPY
92		−char strncpy(char dest, const char *src, size_t count)
93		−{
94		− char *tmp = dest;
95		−
96		− while (count) {
97		− if ((tmp = src) != 0)
98		− src++;
99		− tmp++;
100		− count--;
101		− }
102		− return dest;
103		−}
104		−EXPORT_SYMBOL(strncpy);
105		−#endif
106		−
107	91	#ifdef __BIG_ENDIAN
108	92	# define ALLBUTLAST_BYTE_MASK (~255ul)
109	93	#else

lib/test_fortify/write_overflow-strncpy-src.c+0−5

@@ -1,5 +0,0 @@
1		−// SPDX-License-Identifier: GPL-2.0-only
2		−#define TEST \
3		− strncpy(small, large_src, sizeof(small) + 1)
4		−
5		−#include "test_fortify.h"

lib/test_fortify/write_overflow-strncpy.c+0−5

@@ -1,5 +0,0 @@
1		−// SPDX-License-Identifier: GPL-2.0-only
2		−#define TEST \
3		− strncpy(instance.buf, large_src, sizeof(instance.buf) + 1)
4		−
5		−#include "test_fortify.h"

lib/tests/fortify_kunit.c+1−60

@@ -458,7 +458,7 @@ static void fortify_test_strnlen(struct kunit *test) · 457 unmodified lines
458	458	/* Make string unterminated, and recount. */
459	459	pad.buf[end] = 'A';
460	460	end = sizeof(pad.buf);
461		− /* Reading beyond with strncpy() will fail. */
	461	+ /* Reading beyond will fail. */
462	462	KUNIT_EXPECT_EQ(test, strnlen(pad.buf, end + 1), end);
463	463	KUNIT_EXPECT_EQ(test, fortify_read_overflows, 1);
464	464	KUNIT_EXPECT_EQ(test, strnlen(pad.buf, end + 2), end);
@@ -531,64 +531,6 @@ static void fortify_test_strcpy(struct kunit *test) · 66 unmodified lines
531	531	KUNIT_EXPECT_EQ(test, pad.bytes_after, 0);
532	532	}
533	533
534		−static void fortify_test_strncpy(struct kunit *test)
535		−{
536		− struct fortify_padding pad = { };
537		− char src[] = "Copy me fully into a small buffer and I will overflow!";
538		− size_t sizeof_buf = sizeof(pad.buf);
539		−
540		− OPTIMIZER_HIDE_VAR(sizeof_buf);
541		−
542		− /* Destination is %NUL-filled to start with. */
543		− KUNIT_EXPECT_EQ(test, pad.bytes_before, 0);
544		− KUNIT_EXPECT_EQ(test, pad.buf[sizeof_buf - 1], '\0');
545		− KUNIT_EXPECT_EQ(test, pad.buf[sizeof_buf - 2], '\0');
546		− KUNIT_EXPECT_EQ(test, pad.buf[sizeof_buf - 3], '\0');
547		− KUNIT_EXPECT_EQ(test, pad.bytes_after, 0);
548		−
549		− /* Legitimate strncpy() 1 less than of max size. */
550		− KUNIT_ASSERT_TRUE(test, strncpy(pad.buf, src, sizeof_buf - 1)
551		− == pad.buf);
552		− KUNIT_EXPECT_EQ(test, fortify_write_overflows, 0);
553		− /* Only last byte should be %NUL */
554		− KUNIT_EXPECT_EQ(test, pad.buf[sizeof_buf - 1], '\0');
555		− KUNIT_EXPECT_NE(test, pad.buf[sizeof_buf - 2], '\0');
556		− KUNIT_EXPECT_NE(test, pad.buf[sizeof_buf - 3], '\0');
557		−
558		− /* Legitimate (though unterminated) max-size strncpy. */
559		− KUNIT_ASSERT_TRUE(test, strncpy(pad.buf, src, sizeof_buf)
560		− == pad.buf);
561		− KUNIT_EXPECT_EQ(test, fortify_write_overflows, 0);
562		− /* No trailing %NUL -- thanks strncpy API. */
563		− KUNIT_EXPECT_NE(test, pad.buf[sizeof_buf - 1], '\0');
564		− KUNIT_EXPECT_NE(test, pad.buf[sizeof_buf - 2], '\0');
565		− KUNIT_EXPECT_NE(test, pad.buf[sizeof_buf - 2], '\0');
566		− /* But we will not have gone beyond. */
567		− KUNIT_EXPECT_EQ(test, pad.bytes_after, 0);
568		−
569		− /* Now verify that FORTIFY is working... */
570		− KUNIT_ASSERT_TRUE(test, strncpy(pad.buf, src, sizeof_buf + 1)
571		− == pad.buf);
572		− /* Should catch the overflow. */
573		− KUNIT_EXPECT_EQ(test, fortify_write_overflows, 1);
574		− KUNIT_EXPECT_NE(test, pad.buf[sizeof_buf - 1], '\0');
575		− KUNIT_EXPECT_NE(test, pad.buf[sizeof_buf - 2], '\0');
576		− KUNIT_EXPECT_NE(test, pad.buf[sizeof_buf - 2], '\0');
577		− /* And we will not have gone beyond. */
578		− KUNIT_EXPECT_EQ(test, pad.bytes_after, 0);
579		−
580		− /* And further... */
581		− KUNIT_ASSERT_TRUE(test, strncpy(pad.buf, src, sizeof_buf + 2)
582		− == pad.buf);
583		− /* Should catch the overflow. */
584		− KUNIT_EXPECT_EQ(test, fortify_write_overflows, 2);
585		− KUNIT_EXPECT_NE(test, pad.buf[sizeof_buf - 1], '\0');
586		− KUNIT_EXPECT_NE(test, pad.buf[sizeof_buf - 2], '\0');
587		− KUNIT_EXPECT_NE(test, pad.buf[sizeof_buf - 2], '\0');
588		− /* And we will not have gone beyond. */
589		− KUNIT_EXPECT_EQ(test, pad.bytes_after, 0);
590		−}
591		−
592	534	static void fortify_test_strscpy(struct kunit *test)
593	535	{
594	536	struct fortify_padding pad = { };
@@ -1100,7 +1042,6 @@ static struct kunit_case fortify_test_cases[] = { · 505 unmodified lines
1100	1042	KUNIT_CASE(fortify_test_strlen),
1101	1043	KUNIT_CASE(fortify_test_strnlen),
1102	1044	KUNIT_CASE(fortify_test_strcpy),
1103		− KUNIT_CASE(fortify_test_strncpy),
1104	1045	KUNIT_CASE(fortify_test_strscpy),
1105	1046	KUNIT_CASE(fortify_test_strcat),
1106	1047	KUNIT_CASE(fortify_test_strncat),