2024-02-05 12:36:21

by Kees Cook

[permalink] [raw]
Subject: [PATCH v2 0/4] string: Allow 2-argument strscpy()

Hi,

v2:
- add strscpy_pad() coverage
- fix up ARCH=um to handle the renaming
- use __must_be_array() to validate sizeof() usage
v1: https://lore.kernel.org/all/[email protected]/

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 | 75 +++++++++++++++++++++++-
lib/string.c | 4 +-
lib/string_helpers.c | 34 -----------
11 files changed, 87 insertions(+), 69 deletions(-)

--
2.34.1



2024-02-05 12:36:39

by Kees Cook

[permalink] [raw]
Subject: [PATCH v2 4/4] um: Convert strscpy() usage to 2-argument style

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 <[email protected]>
Cc: [email protected]
Signed-off-by: Kees Cook <[email protected]>
---
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


2024-02-05 12:36:55

by Kees Cook

[permalink] [raw]
Subject: [PATCH v2 1/4] string: Redefine strscpy_pad() as a macro

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 <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: [email protected]
Signed-off-by: Kees Cook <[email protected]>
---
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


2024-02-05 12:37:05

by Kees Cook

[permalink] [raw]
Subject: [PATCH v2 2/4] string: Allow 2-argument strscpy()

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 <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Cc: [email protected]
Signed-off-by: Kees Cook <[email protected]>
---
arch/um/include/shared/user.h | 3 ++-
include/linux/fortify-string.h | 22 ++-------------------
include/linux/string.h | 35 +++++++++++++++++++++++++++++++++-
lib/string.c | 4 ++--
4 files changed, 40 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..a21371aa2fd6 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -67,9 +67,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


2024-02-05 12:59:31

by Kees Cook

[permalink] [raw]
Subject: [PATCH v2 3/4] string: Allow 2-argument strscpy_pad()

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 <[email protected]>
Cc: [email protected]
Signed-off-by: Kees Cook <[email protected]>
---
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 a21371aa2fd6..4f0f27013418 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -78,6 +78,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
@@ -103,6 +107,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
@@ -123,17 +139,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


2024-02-05 13:01:22

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v2 2/4] string: Allow 2-argument strscpy()

On Mon, Feb 05, 2024 at 01:47:08PM +0100, Geert Uytterhoeven wrote:
> On Mon, Feb 5, 2024 at 1:37 PM Kees Cook <[email protected]> wrote:

..

> > +#define __strscpy1(dst, src, size) sized_strscpy(dst, src, size)
>
> (dst), (src), (size) etc.

No need.

..

> > +#define strscpy(dst, src, ...) \
> > + CONCATENATE(__strscpy, COUNT_ARGS(__VA_ARGS__))(dst, src, __VA_ARGS__)
>
> Likewise

Likewise

--
With Best Regards,
Andy Shevchenko



2024-02-05 13:06:46

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH v2 4/4] um: Convert strscpy() usage to 2-argument style

Hi Kees,

On Mon, Feb 5, 2024 at 1:36 PM Kees Cook <[email protected]> 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.
>
> Cc: Richard Weinberger <[email protected]>
> Cc: [email protected]
> Signed-off-by: Kees Cook <[email protected]>

Thanks for your patch!

> --- 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))

(dst), (src)

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2024-02-05 13:11:46

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH v2 2/4] string: Allow 2-argument strscpy()

Hi Kees,

On Mon, Feb 5, 2024 at 1:37 PM Kees Cook <[email protected]> wrote:
> 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 <[email protected]>
> Cc: Andy Shevchenko <[email protected]>
> Cc: [email protected]
> Signed-off-by: Kees Cook <[email protected]>

Thanks for your patch!

> --- a/include/linux/string.h
> +++ b/include/linux/string.h

> +/*
> + * 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)

(dst), (src), (size) etc.


> +
> +/**
> + * 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__)

Likewise

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2024-02-05 13:29:01

by Kees Cook

[permalink] [raw]
Subject: Re: [PATCH v2 2/4] string: Allow 2-argument strscpy()

On Mon, Feb 05, 2024 at 01:47:08PM +0100, Geert Uytterhoeven wrote:
> > +/*
> > + * 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)
>
> (dst), (src), (size) etc.

I normally don't do this when macro args are being expanded into
function arguments. I've only done it for when macro args are used in
expressions. Am I missing a side-effect here, or is this more about
stylistic consistency?

--
Kees Cook

2024-02-05 13:32:45

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH v2 2/4] string: Allow 2-argument strscpy()

Hi Kees,

On Mon, Feb 5, 2024 at 2:01 PM Kees Cook <[email protected]> wrote:
> On Mon, Feb 05, 2024 at 01:47:08PM +0100, Geert Uytterhoeven wrote:
> > > +/*
> > > + * 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)
> >
> > (dst), (src), (size) etc.
>
> I normally don't do this when macro args are being expanded into
> function arguments. I've only done it for when macro args are used in
> expressions. Am I missing a side-effect here, or is this more about
> stylistic consistency?

I'm not 100% sure it is needed, but I'm always wary when using macro
parameters without parentheses, except in the most simple use-cases.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2024-02-05 13:37:30

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v2 4/4] um: Convert strscpy() usage to 2-argument style

On Mon, Feb 05, 2024 at 01:50:14PM +0100, Geert Uytterhoeven wrote:
> On Mon, Feb 5, 2024 at 1:36 PM Kees Cook <[email protected]> wrote:

..

> > +#define strscpy(dst, src) sized_strscpy(dst, src, sizeof(dst))
>
> (dst), (src)

No need.

--
With Best Regards,
Andy Shevchenko