From: Kees Cook Date: Mon, 23 Mar 2026 01:27:25 +0000 (+0000) Subject: string: Remove strncpy() from the kernel X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=079a028d6327e68cfa5d38b36123637b321c19a7;p=thirdparty%2Flinux.git 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. Link: https://github.com/KSPP/linux/issues/90 [1] Signed-off-by: Kees Cook --- diff --git a/Documentation/process/deprecated.rst b/Documentation/process/deprecated.rst index 03de71f654c76..22a5e62c92eaf 100644 --- a/Documentation/process/deprecated.rst +++ b/Documentation/process/deprecated.rst @@ -131,27 +131,32 @@ value of strcpy() 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). -strncpy() on NUL-terminated strings ------------------------------------ -Use of strncpy() does not guarantee that the destination buffer will -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. - -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, strtomem() should be -used, and the destinations should be marked with the `__nonstring -`_ -attribute to avoid future compiler warnings. For cases still needing -NUL-padding, strtomem_pad() can be used. +strncpy() +--------- +strncpy() has been removed from the kernel. All former callers have +been migrated to safer alternatives. + +strncpy() did not guarantee NUL-termination of the destination buffer, +leading to linear read overflows and other misbehavior. It also +unconditionally NUL-padded the destination, which was a needless +performance penalty for callers using only NUL-terminated strings. Due +to its various behaviors, it was an ambiguous API for determining what +an author's true intent was for the copy. + +The replacements for strncpy() are: + +- strscpy() when the destination must be NUL-terminated. +- strscpy_pad() when the destination must be NUL-terminated and + zero-padded (e.g., structs crossing privilege boundaries). +- memtostr() for NUL-terminated destinations from non-NUL-terminated + fixed-width sources (with the `__nonstring` attribute on the source). +- memtostr_pad() for the same, but with zero-padding. +- strtomem() for non-NUL-terminated fixed-width destinations, with + the `__nonstring` attribute on the destination. +- strtomem_pad() for non-NUL-terminated destinations that also need + zero-padding. +- memcpy_and_pad() for bounded copies from potentially unterminated + sources where the destination size is a runtime value. strlcpy() --------- diff --git a/drivers/misc/lkdtm/fortify.c b/drivers/misc/lkdtm/fortify.c index 7615a02dfc477..5e66707e11804 100644 --- a/drivers/misc/lkdtm/fortify.c +++ b/drivers/misc/lkdtm/fortify.c @@ -98,7 +98,7 @@ static void lkdtm_FORTIFY_MEM_MEMBER(void) pr_info("trying to memcpy() past the end of a struct member...\n"); /* - * strncpy(target.a, src, 20); will hit a compile error because the + * memcpy(target.a, src, 20); will hit a compile error because the * compiler knows at build time that target.a < 20 bytes. Use a * volatile to force a runtime error. */ diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h index 171982e53c9aa..cf841dc71feff 100644 --- a/include/linux/fortify-string.h +++ b/include/linux/fortify-string.h @@ -26,7 +26,6 @@ #define FORTIFY_WRITE 1 #define EACH_FORTIFY_FUNC(macro) \ - macro(strncpy), \ macro(strnlen), \ macro(strlen), \ macro(strscpy), \ @@ -95,8 +94,6 @@ extern char *__underlying_strcat(char *p, const char *q) __RENAME(strcat); extern char *__underlying_strcpy(char *p, const char *q) __RENAME(strcpy); extern __kernel_size_t __underlying_strlen(const char *p) __RENAME(strlen); extern char *__underlying_strncat(char *p, const char *q, __kernel_size_t count) __RENAME(strncat); -extern char *__underlying_strncpy(char *p, const char *q, __kernel_size_t size) __RENAME(strncpy); - #else #if defined(__SANITIZE_MEMORY__) @@ -120,7 +117,6 @@ extern char *__underlying_strncpy(char *p, const char *q, __kernel_size_t size) #define __underlying_strcpy __builtin_strcpy #define __underlying_strlen __builtin_strlen #define __underlying_strncat __builtin_strncat -#define __underlying_strncpy __builtin_strncpy #endif @@ -159,50 +155,6 @@ extern char *__underlying_strncpy(char *p, const char *q, __kernel_size_t size) (bounds) < (length) \ ) -/** - * 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) -{ - const size_t p_size = __member_size(p); - - if (__compiletime_lessthan(p_size, size)) - __write_overflow(); - if (p_size < size) - fortify_panic(FORTIFY_FUNC_strncpy, FORTIFY_WRITE, p_size, size, p); - return __underlying_strncpy(p, q, size); -} - extern __kernel_size_t __real_strnlen(const char *, __kernel_size_t) __RENAME(strnlen); /** * 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) #undef __underlying_strcpy #undef __underlying_strlen #undef __underlying_strncat -#undef __underlying_strncpy #undef POS #undef POS0 diff --git a/include/linux/string.h b/include/linux/string.h index b850bd91b3d88..5702daca4326b 100644 --- a/include/linux/string.h +++ b/include/linux/string.h @@ -67,9 +67,6 @@ void *vmemdup_array_user(const void __user *src, size_t n, size_t size) #ifndef __HAVE_ARCH_STRCPY extern char * strcpy(char *,const char *); #endif -#ifndef __HAVE_ARCH_STRNCPY -extern char * strncpy(char *,const char *, __kernel_size_t); -#endif ssize_t sized_strscpy(char *, const char *, size_t); /* diff --git a/lib/string.c b/lib/string.c index b632c71df1a50..45ca1cfe01844 100644 --- a/lib/string.c +++ b/lib/string.c @@ -88,22 +88,6 @@ char *strcpy(char *dest, const char *src) EXPORT_SYMBOL(strcpy); #endif -#ifndef __HAVE_ARCH_STRNCPY -char *strncpy(char *dest, const char *src, size_t count) -{ - char *tmp = dest; - - while (count) { - if ((*tmp = *src) != 0) - src++; - tmp++; - count--; - } - return dest; -} -EXPORT_SYMBOL(strncpy); -#endif - #ifdef __BIG_ENDIAN # define ALLBUTLAST_BYTE_MASK (~255ul) #else diff --git a/lib/test_fortify/write_overflow-strncpy-src.c b/lib/test_fortify/write_overflow-strncpy-src.c deleted file mode 100644 index 8dcfb8c788dd2..0000000000000 --- a/lib/test_fortify/write_overflow-strncpy-src.c +++ /dev/null @@ -1,5 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0-only -#define TEST \ - strncpy(small, large_src, sizeof(small) + 1) - -#include "test_fortify.h" diff --git a/lib/test_fortify/write_overflow-strncpy.c b/lib/test_fortify/write_overflow-strncpy.c deleted file mode 100644 index b85f079c815dd..0000000000000 --- a/lib/test_fortify/write_overflow-strncpy.c +++ /dev/null @@ -1,5 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0-only -#define TEST \ - strncpy(instance.buf, large_src, sizeof(instance.buf) + 1) - -#include "test_fortify.h" diff --git a/lib/tests/fortify_kunit.c b/lib/tests/fortify_kunit.c index fc9c76f026d63..413cdbf3dc0da 100644 --- a/lib/tests/fortify_kunit.c +++ b/lib/tests/fortify_kunit.c @@ -458,7 +458,7 @@ static void fortify_test_strnlen(struct kunit *test) /* Make string unterminated, and recount. */ pad.buf[end] = 'A'; end = sizeof(pad.buf); - /* Reading beyond with strncpy() will fail. */ + /* Reading beyond will fail. */ KUNIT_EXPECT_EQ(test, strnlen(pad.buf, end + 1), end); KUNIT_EXPECT_EQ(test, fortify_read_overflows, 1); KUNIT_EXPECT_EQ(test, strnlen(pad.buf, end + 2), end); @@ -531,64 +531,6 @@ static void fortify_test_strcpy(struct kunit *test) KUNIT_EXPECT_EQ(test, pad.bytes_after, 0); } -static void fortify_test_strncpy(struct kunit *test) -{ - struct fortify_padding pad = { }; - char src[] = "Copy me fully into a small buffer and I will overflow!"; - size_t sizeof_buf = sizeof(pad.buf); - - OPTIMIZER_HIDE_VAR(sizeof_buf); - - /* Destination is %NUL-filled to start with. */ - KUNIT_EXPECT_EQ(test, pad.bytes_before, 0); - KUNIT_EXPECT_EQ(test, pad.buf[sizeof_buf - 1], '\0'); - KUNIT_EXPECT_EQ(test, pad.buf[sizeof_buf - 2], '\0'); - KUNIT_EXPECT_EQ(test, pad.buf[sizeof_buf - 3], '\0'); - KUNIT_EXPECT_EQ(test, pad.bytes_after, 0); - - /* Legitimate strncpy() 1 less than of max size. */ - KUNIT_ASSERT_TRUE(test, strncpy(pad.buf, src, sizeof_buf - 1) - == pad.buf); - KUNIT_EXPECT_EQ(test, fortify_write_overflows, 0); - /* Only last byte should be %NUL */ - KUNIT_EXPECT_EQ(test, pad.buf[sizeof_buf - 1], '\0'); - KUNIT_EXPECT_NE(test, pad.buf[sizeof_buf - 2], '\0'); - KUNIT_EXPECT_NE(test, pad.buf[sizeof_buf - 3], '\0'); - - /* Legitimate (though unterminated) max-size strncpy. */ - KUNIT_ASSERT_TRUE(test, strncpy(pad.buf, src, sizeof_buf) - == pad.buf); - KUNIT_EXPECT_EQ(test, fortify_write_overflows, 0); - /* No trailing %NUL -- thanks strncpy API. */ - KUNIT_EXPECT_NE(test, pad.buf[sizeof_buf - 1], '\0'); - KUNIT_EXPECT_NE(test, pad.buf[sizeof_buf - 2], '\0'); - KUNIT_EXPECT_NE(test, pad.buf[sizeof_buf - 2], '\0'); - /* But we will not have gone beyond. */ - KUNIT_EXPECT_EQ(test, pad.bytes_after, 0); - - /* Now verify that FORTIFY is working... */ - KUNIT_ASSERT_TRUE(test, strncpy(pad.buf, src, sizeof_buf + 1) - == pad.buf); - /* Should catch the overflow. */ - KUNIT_EXPECT_EQ(test, fortify_write_overflows, 1); - KUNIT_EXPECT_NE(test, pad.buf[sizeof_buf - 1], '\0'); - KUNIT_EXPECT_NE(test, pad.buf[sizeof_buf - 2], '\0'); - KUNIT_EXPECT_NE(test, pad.buf[sizeof_buf - 2], '\0'); - /* And we will not have gone beyond. */ - KUNIT_EXPECT_EQ(test, pad.bytes_after, 0); - - /* And further... */ - KUNIT_ASSERT_TRUE(test, strncpy(pad.buf, src, sizeof_buf + 2) - == pad.buf); - /* Should catch the overflow. */ - KUNIT_EXPECT_EQ(test, fortify_write_overflows, 2); - KUNIT_EXPECT_NE(test, pad.buf[sizeof_buf - 1], '\0'); - KUNIT_EXPECT_NE(test, pad.buf[sizeof_buf - 2], '\0'); - KUNIT_EXPECT_NE(test, pad.buf[sizeof_buf - 2], '\0'); - /* And we will not have gone beyond. */ - KUNIT_EXPECT_EQ(test, pad.bytes_after, 0); -} - static void fortify_test_strscpy(struct kunit *test) { struct fortify_padding pad = { }; @@ -1100,7 +1042,6 @@ static struct kunit_case fortify_test_cases[] = { KUNIT_CASE(fortify_test_strlen), KUNIT_CASE(fortify_test_strnlen), KUNIT_CASE(fortify_test_strcpy), - KUNIT_CASE(fortify_test_strncpy), KUNIT_CASE(fortify_test_strscpy), KUNIT_CASE(fortify_test_strcat), KUNIT_CASE(fortify_test_strncat),