2024-01-18 22:33:22

by Kees Cook

[permalink] [raw]
Subject: [PATCH] string: Remove strlcpy()

With all the users of strlcpy() removed[1] from the kernel, remove the
API, self-tests, and other references. Leave mentions in Documentation
(about its deprecation), and in checkpatch.pl (to help migrate host-only
tools/ usage). Long live strscpy().

Link: https://github.com/KSPP/linux/issues/89 [1]
Cc: Andy Shevchenko <[email protected]>
Cc: Azeem Shaikh <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Andy Whitcroft <[email protected]>
Cc: Joe Perches <[email protected]>
Cc: Dwaipayan Ray <[email protected]>
Cc: Lukas Bulwahn <[email protected]>
Cc: [email protected]
Signed-off-by: Kees Cook <[email protected]>
---
This is on top of Linus's tree with:
https://lore.kernel.org/all/[email protected]/
I'll send this as a PR to Linus shortly now that the tree is strlcpy()-free...
---
include/linux/fortify-string.h | 51 -------------------
include/linux/string.h | 3 --
lib/nlattr.c | 2 +-
lib/string.c | 15 ------
lib/test_fortify/write_overflow-strlcpy-src.c | 5 --
lib/test_fortify/write_overflow-strlcpy.c | 5 --
6 files changed, 1 insertion(+), 80 deletions(-)
delete mode 100644 lib/test_fortify/write_overflow-strlcpy-src.c
delete mode 100644 lib/test_fortify/write_overflow-strlcpy.c

diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
index 79ef6ac4c021..89a6888f2f9e 100644
--- a/include/linux/fortify-string.h
+++ b/include/linux/fortify-string.h
@@ -214,51 +214,6 @@ __kernel_size_t __fortify_strlen(const char * const POS p)
return ret;
}

-/* Defined after fortified strlen() to reuse it. */
-extern size_t __real_strlcpy(char *, const char *, size_t) __RENAME(strlcpy);
-/**
- * strlcpy - Copy a string into another string buffer
- *
- * @p: pointer to destination of copy
- * @q: pointer to NUL-terminated source string to copy
- * @size: maximum number of bytes to write at @p
- *
- * If strlen(@q) >= @size, the copy of @q will be truncated at
- * @size - 1 bytes. @p will always be NUL-terminated.
- *
- * Do not use this function. While FORTIFY_SOURCE tries to avoid
- * over-reads when calculating strlen(@q), it is still possible.
- * Prefer strscpy(), though note its different return values for
- * detecting truncation.
- *
- * Returns total number of bytes written to @p, including terminating NUL.
- *
- */
-__FORTIFY_INLINE size_t strlcpy(char * const POS p, const char * const POS q, size_t size)
-{
- const size_t p_size = __member_size(p);
- const size_t q_size = __member_size(q);
- size_t q_len; /* Full count of source string length. */
- size_t len; /* Count of characters going into destination. */
-
- if (p_size == SIZE_MAX && q_size == SIZE_MAX)
- return __real_strlcpy(p, q, size);
- q_len = strlen(q);
- len = (q_len >= size) ? size - 1 : q_len;
- if (__builtin_constant_p(size) && __builtin_constant_p(q_len) && size) {
- /* Write size is always larger than destination. */
- if (len >= p_size)
- __write_overflow();
- }
- if (size) {
- if (len >= p_size)
- fortify_panic(__func__);
- __underlying_memcpy(p, q, len);
- p[len] = '\0';
- }
- return q_len;
-}
-
/* Defined after fortified strnlen() to reuse it. */
extern ssize_t __real_strscpy(char *, const char *, size_t) __RENAME(strscpy);
/**
@@ -272,12 +227,6 @@ extern ssize_t __real_strscpy(char *, const char *, size_t) __RENAME(strscpy);
* @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 strlcpy() since the API doesn't require reading memory
- * from the source @q string beyond the specified @size bytes, and since
- * the return value is easier to error-check than strlcpy()'s.
- * In addition, the implementation is robust to the string changing out
- * from underneath it, unlike the current strlcpy() implementation.
- *
* 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().
diff --git a/include/linux/string.h b/include/linux/string.h
index ce137830a0b9..ab148d8dbfc1 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -66,9 +66,6 @@ extern char * strcpy(char *,const char *);
#ifndef __HAVE_ARCH_STRNCPY
extern char * strncpy(char *,const char *, __kernel_size_t);
#endif
-#ifndef __HAVE_ARCH_STRLCPY
-size_t strlcpy(char *, const char *, size_t);
-#endif
#ifndef __HAVE_ARCH_STRSCPY
ssize_t strscpy(char *, const char *, size_t);
#endif
diff --git a/lib/nlattr.c b/lib/nlattr.c
index dc15e7888fc1..07c4cc8c8d79 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -758,7 +758,7 @@ EXPORT_SYMBOL(nla_find);
* @dstsize: Size of destination buffer.
*
* Copies at most dstsize - 1 bytes into the destination buffer.
- * Unlike strlcpy the destination buffer is always padded out.
+ * Unlike strscpy the destination buffer is always padded out.
*
* Return:
* * srclen - Returns @nla length (not including the trailing %NUL).
diff --git a/lib/string.c b/lib/string.c
index be26623953d2..6891d15ce991 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -103,21 +103,6 @@ char *strncpy(char *dest, const char *src, size_t count)
EXPORT_SYMBOL(strncpy);
#endif

-#ifndef __HAVE_ARCH_STRLCPY
-size_t strlcpy(char *dest, const char *src, size_t size)
-{
- size_t ret = strlen(src);
-
- if (size) {
- size_t len = (ret >= size) ? size - 1 : ret;
- __builtin_memcpy(dest, src, len);
- dest[len] = '\0';
- }
- return ret;
-}
-EXPORT_SYMBOL(strlcpy);
-#endif
-
#ifndef __HAVE_ARCH_STRSCPY
ssize_t strscpy(char *dest, const char *src, size_t count)
{
diff --git a/lib/test_fortify/write_overflow-strlcpy-src.c b/lib/test_fortify/write_overflow-strlcpy-src.c
deleted file mode 100644
index 91bf83ebd34a..000000000000
--- a/lib/test_fortify/write_overflow-strlcpy-src.c
+++ /dev/null
@@ -1,5 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-only
-#define TEST \
- strlcpy(small, large_src, sizeof(small) + 1)
-
-#include "test_fortify.h"
diff --git a/lib/test_fortify/write_overflow-strlcpy.c b/lib/test_fortify/write_overflow-strlcpy.c
deleted file mode 100644
index 1883db7c0cd6..000000000000
--- a/lib/test_fortify/write_overflow-strlcpy.c
+++ /dev/null
@@ -1,5 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-only
-#define TEST \
- strlcpy(instance.buf, large_src, sizeof(instance.buf) + 1)
-
-#include "test_fortify.h"
--
2.34.1



2024-01-19 16:06:07

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH] string: Remove strlcpy()

On Fri, Jan 19, 2024 at 12:33 AM Kees Cook <[email protected]> wrote:
>
> With all the users of strlcpy() removed[1] from the kernel, remove the
> API, self-tests, and other references. Leave mentions in Documentation
> (about its deprecation), and in checkpatch.pl (to help migrate host-only
> tools/ usage). Long live strscpy().

..

> * Copies at most dstsize - 1 bytes into the destination buffer.
> - * Unlike strlcpy the destination buffer is always padded out.
> + * Unlike strscpy the destination buffer is always padded out.

While at it, please use the strscpy() form to refer to the function.

With that,
Reviewed-by: Andy Shevchenko <[email protected]>

--
With Best Regards,
Andy Shevchenko

2024-01-19 19:58:46

by Kees Cook

[permalink] [raw]
Subject: Re: [PATCH] string: Remove strlcpy()

On Fri, Jan 19, 2024 at 06:05:19PM +0200, Andy Shevchenko wrote:
> On Fri, Jan 19, 2024 at 12:33 AM Kees Cook <[email protected]> wrote:
> >
> > With all the users of strlcpy() removed[1] from the kernel, remove the
> > API, self-tests, and other references. Leave mentions in Documentation
> > (about its deprecation), and in checkpatch.pl (to help migrate host-only
> > tools/ usage). Long live strscpy().
>
> ...
>
> > * Copies at most dstsize - 1 bytes into the destination buffer.
> > - * Unlike strlcpy the destination buffer is always padded out.
> > + * Unlike strscpy the destination buffer is always padded out.
>
> While at it, please use the strscpy() form to refer to the function.

Oh, with the trailing "()"? Sure! Good call.

-Kees

> Reviewed-by: Andy Shevchenko <[email protected]>

Thanks!

-Kees

--
Kees Cook