[PATCH 0/2] string: Add stracpy and stracpy_pad

[PATCH 0/2] string: Add stracpy and stracpy_pad

[Joe Perches]

Add more string copy mechanisms to help avoid defects

Joe Perches (2):
  string: Add stracpy and stracpy_pad mechanisms
  kernel-doc: core-api: Include string.h into core-api

 Documentation/core-api/kernel-api.rst |  3 +++
 include/linux/string.h                | 46 +++++++++++++++++++++++++++++++++--
 lib/string.c                          | 10 +++++---
 3 files changed, 53 insertions(+), 6 deletions(-)

-- 
2.15.0

[PATCH 1/2] string: Add stracpy and stracpy_pad mechanisms

[Joe Perches]

Several uses of strlcpy and strscpy have had defects because the
last argument of each function is misused or typoed.

Add macro mechanisms to avoid this defect.

stracpy (copy a string to a string array) must have a string
array as the first argument (to) and uses sizeof(to) as the
size.

These mechanisms verify that the to argument is an array of
char or other compatible types like u8 or unsigned char.

A BUILD_BUG is emitted when the type of to is not compatible.

Signed-off-by: Joe Perches <joe@perches.com>
---
 include/linux/string.h | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/include/linux/string.h b/include/linux/string.h
index 4deb11f7976b..f80b0973f0e5 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -35,6 +35,47 @@ ssize_t strscpy(char *, const char *, size_t);
 /* Wraps calls to strscpy()/memset(), no arch specific code required */
 ssize_t strscpy_pad(char *dest, const char *src, size_t count);
 
+/**
+ * stracpy - Copy a C-string into an array of char
+ * @to: Where to copy the string, must be an array of char and not a pointer
+ * @from: String to copy, may be a pointer or const char array
+ *
+ * Helper for strscpy.
+ * Copies a maximum of sizeof(@to) bytes of @from with %NUL termination.
+ *
+ * Returns:
+ * * The number of characters copied (not including the trailing %NUL)
+ * * -E2BIG if @to is a zero size array.
+ */
+#define stracpy(to, from)					\
+({								\
+	size_t size = ARRAY_SIZE(to);				\
+	BUILD_BUG_ON(!__same_type(typeof(*to), char));		\
+								\
+	strscpy(to, from, size);				\
+})
+
+/**
+ * stracpy_pad - Copy a C-string into an array of char with %NUL padding
+ * @to: Where to copy the string, must be an array of char and not a pointer
+ * @from: String to copy, may be a pointer or const char array
+ *
+ * Helper for strscpy_pad.
+ * Copies a maximum of sizeof(@to) bytes of @from with %NUL termination
+ * and zero-pads the remaining size of @to
+ *
+ * Returns:
+ * * The number of characters copied (not including the trailing %NUL)
+ * * -E2BIG if @to is a zero size array.
+ */
+#define stracpy_pad(to, from)					\
+({								\
+	size_t size = ARRAY_SIZE(to);				\
+	BUILD_BUG_ON(!__same_type(typeof(*to), char));		\
+								\
+	strscpy_pad(to, from, size);				\
+})
+
 #ifndef __HAVE_ARCH_STRCAT
 extern char * strcat(char *, const char *);
 #endif
-- 
2.15.0

Re: [PATCH 1/2] string: Add stracpy and stracpy_pad mechanisms

[Andrew Morton]

On Mon, 22 Jul 2019 17:38:15 -0700 Joe Perches <joe@perches.com> wrote:

> Several uses of strlcpy and strscpy have had defects because the
> last argument of each function is misused or typoed.
> 
> Add macro mechanisms to avoid this defect.
> 
> stracpy (copy a string to a string array) must have a string
> array as the first argument (to) and uses sizeof(to) as the
> size.
> 
> These mechanisms verify that the to argument is an array of
> char or other compatible types like u8 or unsigned char.
> 
> A BUILD_BUG is emitted when the type of to is not compatible.
> 

It would be nice to include some conversions.  To demonstrate the need,
to test the code, etc.

Re: [PATCH 1/2] string: Add stracpy and stracpy_pad mechanisms

[Joe Perches]

On Mon, 2019-07-22 at 21:35 -0700, Andrew Morton wrote:
> On Mon, 22 Jul 2019 17:38:15 -0700 Joe Perches <joe@perches.com> wrote:
> 
> > Several uses of strlcpy and strscpy have had defects because the
> > last argument of each function is misused or typoed.
> > 
> > Add macro mechanisms to avoid this defect.
> > 
> > stracpy (copy a string to a string array) must have a string
> > array as the first argument (to) and uses sizeof(to) as the
> > size.
> > 
> > These mechanisms verify that the to argument is an array of
> > char or other compatible types like u8 or unsigned char.
> > 
> > A BUILD_BUG is emitted when the type of to is not compatible.
> > 
> 
> It would be nice to include some conversions.  To demonstrate the need,
> to test the code, etc.

How about all the kernel/ ?
---
 kernel/acct.c                  | 2 +-
 kernel/cgroup/cgroup-v1.c      | 3 +--
 kernel/debug/gdbstub.c         | 4 ++--
 kernel/debug/kdb/kdb_support.c | 2 +-
 kernel/events/core.c           | 4 ++--
 kernel/module.c                | 2 +-
 kernel/printk/printk.c         | 2 +-
 kernel/time/clocksource.c      | 2 +-
 8 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/kernel/acct.c b/kernel/acct.c
index 81f9831a7859..5ad29248b654 100644
--- a/kernel/acct.c
+++ b/kernel/acct.c
@@ -425,7 +425,7 @@ static void fill_ac(acct_t *ac)
 	memset(ac, 0, sizeof(acct_t));
 
 	ac->ac_version = ACCT_VERSION | ACCT_BYTEORDER;
-	strlcpy(ac->ac_comm, current->comm, sizeof(ac->ac_comm));
+	stracpy(ac->ac_comm, current->comm);
 
 	/* calculate run_time in nsec*/
 	run_time = ktime_get_ns();
diff --git a/kernel/cgroup/cgroup-v1.c b/kernel/cgroup/cgroup-v1.c
index 88006be40ea3..dd4f041e4179 100644
--- a/kernel/cgroup/cgroup-v1.c
+++ b/kernel/cgroup/cgroup-v1.c
@@ -571,8 +571,7 @@ static ssize_t cgroup_release_agent_write(struct kernfs_open_file *of,
 	if (!cgrp)
 		return -ENODEV;
 	spin_lock(&release_agent_path_lock);
-	strlcpy(cgrp->root->release_agent_path, strstrip(buf),
-		sizeof(cgrp->root->release_agent_path));
+	stracpy(cgrp->root->release_agent_path, strstrip(buf));
 	spin_unlock(&release_agent_path_lock);
 	cgroup_kn_unlock(of->kn);
 	return nbytes;
diff --git a/kernel/debug/gdbstub.c b/kernel/debug/gdbstub.c
index 4b280fc7dd67..a263f27f51ad 100644
--- a/kernel/debug/gdbstub.c
+++ b/kernel/debug/gdbstub.c
@@ -1095,10 +1095,10 @@ int gdbstub_state(struct kgdb_state *ks, char *cmd)
 		return error;
 	case 's':
 	case 'c':
-		strscpy(remcom_in_buffer, cmd, sizeof(remcom_in_buffer));
+		stracpy(remcom_in_buffer, cmd);
 		return 0;
 	case '$':
-		strscpy(remcom_in_buffer, cmd, sizeof(remcom_in_buffer));
+		stracpy(remcom_in_buffer, cmd);
 		gdbstub_use_prev_in_buf = strlen(remcom_in_buffer);
 		gdbstub_prev_in_buf_pos = 0;
 		return 0;
diff --git a/kernel/debug/kdb/kdb_support.c b/kernel/debug/kdb/kdb_support.c
index b8e6306e7e13..b49b6c3976c7 100644
--- a/kernel/debug/kdb/kdb_support.c
+++ b/kernel/debug/kdb/kdb_support.c
@@ -192,7 +192,7 @@ int kallsyms_symbol_complete(char *prefix_name, int max_len)
 
 	while ((name = kdb_walk_kallsyms(&pos))) {
 		if (strncmp(name, prefix_name, prefix_len) == 0) {
-			strscpy(ks_namebuf, name, sizeof(ks_namebuf));
+			stracpy(ks_namebuf, name);
 			/* Work out the longest name that matches the prefix */
 			if (++number == 1) {
 				prev_len = min_t(int, max_len-1,
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 026a14541a38..25bd8c777270 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -7049,7 +7049,7 @@ static void perf_event_comm_event(struct perf_comm_event *comm_event)
 	unsigned int size;
 
 	memset(comm, 0, sizeof(comm));
-	strlcpy(comm, comm_event->task->comm, sizeof(comm));
+	stracpy(comm, comm_event->task->comm);
 	size = ALIGN(strlen(comm)+1, sizeof(u64));
 
 	comm_event->comm = comm;
@@ -7394,7 +7394,7 @@ static void perf_event_mmap_event(struct perf_mmap_event *mmap_event)
 	}
 
 cpy_name:
-	strlcpy(tmp, name, sizeof(tmp));
+	stracpy(tmp, name);
 	name = tmp;
 got_name:
 	/*
diff --git a/kernel/module.c b/kernel/module.c
index 5933395af9a0..39384b0c90b8 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1021,7 +1021,7 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
 	async_synchronize_full();
 
 	/* Store the name of the last unloaded module for diagnostic purposes */
-	strlcpy(last_unloaded_module, mod->name, sizeof(last_unloaded_module));
+	stracpy(last_unloaded_module, mod->name);
 
 	free_module(mod);
 	return 0;
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 424abf802f02..029633052be4 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2127,7 +2127,7 @@ static int __add_preferred_console(char *name, int idx, char *options,
 		return -E2BIG;
 	if (!brl_options)
 		preferred_console = i;
-	strlcpy(c->name, name, sizeof(c->name));
+	stracpy(c->name, name);
 	c->options = options;
 	braille_set_options(c, brl_options);
 
diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
index fff5f64981c6..f0c833d89ace 100644
--- a/kernel/time/clocksource.c
+++ b/kernel/time/clocksource.c
@@ -1203,7 +1203,7 @@ static int __init boot_override_clocksource(char* str)
 {
 	mutex_lock(&clocksource_mutex);
 	if (str)
-		strlcpy(override_name, str, sizeof(override_name));
+		stracpy(override_name, str);
 	mutex_unlock(&clocksource_mutex);
 	return 1;
 }

Re: [PATCH 1/2] string: Add stracpy and stracpy_pad mechanisms

[Kees Cook]

On Mon, Jul 22, 2019 at 09:42:51PM -0700, Joe Perches wrote:
> On Mon, 2019-07-22 at 21:35 -0700, Andrew Morton wrote:
> > On Mon, 22 Jul 2019 17:38:15 -0700 Joe Perches <joe@perches.com> wrote:
> > 
> > > Several uses of strlcpy and strscpy have had defects because the
> > > last argument of each function is misused or typoed.
> > > 
> > > Add macro mechanisms to avoid this defect.
> > > 
> > > stracpy (copy a string to a string array) must have a string
> > > array as the first argument (to) and uses sizeof(to) as the
> > > size.
> > > 
> > > These mechanisms verify that the to argument is an array of
> > > char or other compatible types like u8 or unsigned char.
> > > 
> > > A BUILD_BUG is emitted when the type of to is not compatible.
> > > 
> > 
> > It would be nice to include some conversions.  To demonstrate the need,
> > to test the code, etc.
> 
> How about all the kernel/ ?
> ---
>  kernel/acct.c                  | 2 +-
>  kernel/cgroup/cgroup-v1.c      | 3 +--
>  kernel/debug/gdbstub.c         | 4 ++--
>  kernel/debug/kdb/kdb_support.c | 2 +-
>  kernel/events/core.c           | 4 ++--
>  kernel/module.c                | 2 +-
>  kernel/printk/printk.c         | 2 +-
>  kernel/time/clocksource.c      | 2 +-
>  8 files changed, 10 insertions(+), 11 deletions(-)

I think that's a good start. I still think we could just give Linus a
Coccinelle script too, for the next merge window...

-Kees

> 
> diff --git a/kernel/acct.c b/kernel/acct.c
> index 81f9831a7859..5ad29248b654 100644
> --- a/kernel/acct.c
> +++ b/kernel/acct.c
> @@ -425,7 +425,7 @@ static void fill_ac(acct_t *ac)
>  	memset(ac, 0, sizeof(acct_t));
>  
>  	ac->ac_version = ACCT_VERSION | ACCT_BYTEORDER;
> -	strlcpy(ac->ac_comm, current->comm, sizeof(ac->ac_comm));
> +	stracpy(ac->ac_comm, current->comm);
>  
>  	/* calculate run_time in nsec*/
>  	run_time = ktime_get_ns();
> diff --git a/kernel/cgroup/cgroup-v1.c b/kernel/cgroup/cgroup-v1.c
> index 88006be40ea3..dd4f041e4179 100644
> --- a/kernel/cgroup/cgroup-v1.c
> +++ b/kernel/cgroup/cgroup-v1.c
> @@ -571,8 +571,7 @@ static ssize_t cgroup_release_agent_write(struct kernfs_open_file *of,
>  	if (!cgrp)
>  		return -ENODEV;
>  	spin_lock(&release_agent_path_lock);
> -	strlcpy(cgrp->root->release_agent_path, strstrip(buf),
> -		sizeof(cgrp->root->release_agent_path));
> +	stracpy(cgrp->root->release_agent_path, strstrip(buf));
>  	spin_unlock(&release_agent_path_lock);
>  	cgroup_kn_unlock(of->kn);
>  	return nbytes;
> diff --git a/kernel/debug/gdbstub.c b/kernel/debug/gdbstub.c
> index 4b280fc7dd67..a263f27f51ad 100644
> --- a/kernel/debug/gdbstub.c
> +++ b/kernel/debug/gdbstub.c
> @@ -1095,10 +1095,10 @@ int gdbstub_state(struct kgdb_state *ks, char *cmd)
>  		return error;
>  	case 's':
>  	case 'c':
> -		strscpy(remcom_in_buffer, cmd, sizeof(remcom_in_buffer));
> +		stracpy(remcom_in_buffer, cmd);
>  		return 0;
>  	case '$':
> -		strscpy(remcom_in_buffer, cmd, sizeof(remcom_in_buffer));
> +		stracpy(remcom_in_buffer, cmd);
>  		gdbstub_use_prev_in_buf = strlen(remcom_in_buffer);
>  		gdbstub_prev_in_buf_pos = 0;
>  		return 0;
> diff --git a/kernel/debug/kdb/kdb_support.c b/kernel/debug/kdb/kdb_support.c
> index b8e6306e7e13..b49b6c3976c7 100644
> --- a/kernel/debug/kdb/kdb_support.c
> +++ b/kernel/debug/kdb/kdb_support.c
> @@ -192,7 +192,7 @@ int kallsyms_symbol_complete(char *prefix_name, int max_len)
>  
>  	while ((name = kdb_walk_kallsyms(&pos))) {
>  		if (strncmp(name, prefix_name, prefix_len) == 0) {
> -			strscpy(ks_namebuf, name, sizeof(ks_namebuf));
> +			stracpy(ks_namebuf, name);
>  			/* Work out the longest name that matches the prefix */
>  			if (++number == 1) {
>  				prev_len = min_t(int, max_len-1,
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 026a14541a38..25bd8c777270 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -7049,7 +7049,7 @@ static void perf_event_comm_event(struct perf_comm_event *comm_event)
>  	unsigned int size;
>  
>  	memset(comm, 0, sizeof(comm));
> -	strlcpy(comm, comm_event->task->comm, sizeof(comm));
> +	stracpy(comm, comm_event->task->comm);
>  	size = ALIGN(strlen(comm)+1, sizeof(u64));
>  
>  	comm_event->comm = comm;
> @@ -7394,7 +7394,7 @@ static void perf_event_mmap_event(struct perf_mmap_event *mmap_event)
>  	}
>  
>  cpy_name:
> -	strlcpy(tmp, name, sizeof(tmp));
> +	stracpy(tmp, name);
>  	name = tmp;
>  got_name:
>  	/*
> diff --git a/kernel/module.c b/kernel/module.c
> index 5933395af9a0..39384b0c90b8 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -1021,7 +1021,7 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
>  	async_synchronize_full();
>  
>  	/* Store the name of the last unloaded module for diagnostic purposes */
> -	strlcpy(last_unloaded_module, mod->name, sizeof(last_unloaded_module));
> +	stracpy(last_unloaded_module, mod->name);
>  
>  	free_module(mod);
>  	return 0;
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 424abf802f02..029633052be4 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2127,7 +2127,7 @@ static int __add_preferred_console(char *name, int idx, char *options,
>  		return -E2BIG;
>  	if (!brl_options)
>  		preferred_console = i;
> -	strlcpy(c->name, name, sizeof(c->name));
> +	stracpy(c->name, name);
>  	c->options = options;
>  	braille_set_options(c, brl_options);
>  
> diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
> index fff5f64981c6..f0c833d89ace 100644
> --- a/kernel/time/clocksource.c
> +++ b/kernel/time/clocksource.c
> @@ -1203,7 +1203,7 @@ static int __init boot_override_clocksource(char* str)
>  {
>  	mutex_lock(&clocksource_mutex);
>  	if (str)
> -		strlcpy(override_name, str, sizeof(override_name));
> +		stracpy(override_name, str);
>  	mutex_unlock(&clocksource_mutex);
>  	return 1;
>  }
> 
> 

-- 
Kees Cook

Re: [PATCH 1/2] string: Add stracpy and stracpy_pad mechanisms

[Rasmus Villemoes]

On 23/07/2019 02.38, Joe Perches wrote:
> Several uses of strlcpy and strscpy have had defects because the
> last argument of each function is misused or typoed.
> 
> Add macro mechanisms to avoid this defect.
> 
> stracpy (copy a string to a string array) must have a string
> array as the first argument (to) and uses sizeof(to) as the
> size.
> 
> These mechanisms verify that the to argument is an array of
> char or other compatible types

yes

like u8 or unsigned char.

no. "unsigned char" aka u8, "signed char" aka s8 and plain char are not
__builtin_types_compatible_p to one another.

> A BUILD_BUG is emitted when the type of to is not compatible.
> 
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  include/linux/string.h | 41 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 
> diff --git a/include/linux/string.h b/include/linux/string.h
> index 4deb11f7976b..f80b0973f0e5 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -35,6 +35,47 @@ ssize_t strscpy(char *, const char *, size_t);
>  /* Wraps calls to strscpy()/memset(), no arch specific code required */
>  ssize_t strscpy_pad(char *dest, const char *src, size_t count);
>  
> +/**
> + * stracpy - Copy a C-string into an array of char
> + * @to: Where to copy the string, must be an array of char and not a pointer
> + * @from: String to copy, may be a pointer or const char array
> + *
> + * Helper for strscpy.
> + * Copies a maximum of sizeof(@to) bytes of @from with %NUL termination.
> + *
> + * Returns:
> + * * The number of characters copied (not including the trailing %NUL)
> + * * -E2BIG if @to is a zero size array.

Well, yes, but more importantly and generally: -E2BIG if the copy
including %NUL didn't fit. [The zero size array thing could be made into
a build bug for these stra* variants if one thinks that might actually
occur in real code.]

> + */
> +#define stracpy(to, from)					\
> +({								\
> +	size_t size = ARRAY_SIZE(to);				\
> +	BUILD_BUG_ON(!__same_type(typeof(*to), char));		\
> +								\

ARRAY_SIZE should ensure to is indeed an array, but it doesn't hurt to
spell the second condition

  BUILD_BUG_ON(!__same_type(typeof(to), char[]))

(the gcc docs explicitly mention that "The type 'int[]' and 'int[5]' are
compatible.) - just in case that line gets copy-pasted somewhere that
doesn't have another must-be-array check nearby.

You should use a more "unique" identifier than "size". from could be
some expression that refers to such a variable from the surrounding scope.

Rasmus

RE: [PATCH 1/2] string: Add stracpy and stracpy_pad mechanisms

[David Laight]

From: Rasmus Villemoes
> Sent: 23 July 2019 07:56
...
> > +/**
> > + * stracpy - Copy a C-string into an array of char
> > + * @to: Where to copy the string, must be an array of char and not a pointer
> > + * @from: String to copy, may be a pointer or const char array
> > + *
> > + * Helper for strscpy.
> > + * Copies a maximum of sizeof(@to) bytes of @from with %NUL termination.
> > + *
> > + * Returns:
> > + * * The number of characters copied (not including the trailing %NUL)
> > + * * -E2BIG if @to is a zero size array.
> 
> Well, yes, but more importantly and generally: -E2BIG if the copy
> including %NUL didn't fit. [The zero size array thing could be made into
> a build bug for these stra* variants if one thinks that might actually
> occur in real code.]

Probably better is to return the size of the destination if the copy didn't fit
(zero if the buffer is zero length).
This allows code to do repeated:
	offset += str*cpy(buf + offset, src, sizeof buf - offset);
and do a final check for overflow after all the copies.

The same is true for a snprintf()like function

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH 1/2] string: Add stracpy and stracpy_pad mechanisms

[Joe Perches]

(adding Chris Metcalf)

On Tue, 2019-07-23 at 15:41 +0000, David Laight wrote:
> From: Rasmus Villemoes
> > Sent: 23 July 2019 07:56
> ...
> > > +/**
> > > + * stracpy - Copy a C-string into an array of char
> > > + * @to: Where to copy the string, must be an array of char and not a pointer
> > > + * @from: String to copy, may be a pointer or const char array
> > > + *
> > > + * Helper for strscpy.
> > > + * Copies a maximum of sizeof(@to) bytes of @from with %NUL termination.
> > > + *
> > > + * Returns:
> > > + * * The number of characters copied (not including the trailing %NUL)
> > > + * * -E2BIG if @to is a zero size array.
> > 
> > Well, yes, but more importantly and generally: -E2BIG if the copy
> > including %NUL didn't fit. [The zero size array thing could be made into
> > a build bug for these stra* variants if one thinks that might actually
> > occur in real code.]
> 
> Probably better is to return the size of the destination if the copy didn't fit
> (zero if the buffer is zero length).
> This allows code to do repeated:
> 	offset += str*cpy(buf + offset, src, sizeof buf - offset);


> and do a final check for overflow after all the copies.
> 
> The same is true for a snprintf()like function

Chris?  You added this function.
Do you have an opinion here?

Re: [PATCH 1/2] string: Add stracpy and stracpy_pad mechanisms

[Kees Cook]

On Tue, Jul 23, 2019 at 03:41:27PM +0000, David Laight wrote:
> From: Rasmus Villemoes
> > Sent: 23 July 2019 07:56
> ...
> > > +/**
> > > + * stracpy - Copy a C-string into an array of char
> > > + * @to: Where to copy the string, must be an array of char and not a pointer
> > > + * @from: String to copy, may be a pointer or const char array
> > > + *
> > > + * Helper for strscpy.
> > > + * Copies a maximum of sizeof(@to) bytes of @from with %NUL termination.
> > > + *
> > > + * Returns:
> > > + * * The number of characters copied (not including the trailing %NUL)
> > > + * * -E2BIG if @to is a zero size array.
> > 
> > Well, yes, but more importantly and generally: -E2BIG if the copy
> > including %NUL didn't fit. [The zero size array thing could be made into
> > a build bug for these stra* variants if one thinks that might actually
> > occur in real code.]
> 
> Probably better is to return the size of the destination if the copy didn't fit
> (zero if the buffer is zero length).
> This allows code to do repeated:
> 	offset += str*cpy(buf + offset, src, sizeof buf - offset);
> and do a final check for overflow after all the copies.
> 
> The same is true for a snprintf()like function

Please no; I understand the utility of the "max on error" condition for
chaining, but chaining is less common than standard operations. And it
requires that the size of the destination be known in multiple places,
which isn't robust either.

The very point of stracpy() is to not need to know the size of the
destination (i.e. it's handled by the compiler). (And it can't be
chained since it requires the base address of the array, not a char *.)

-- 
Kees Cook

Re: [PATCH 1/2] string: Add stracpy and stracpy_pad mechanisms

[Yann Droneaud]

Hi,

Le mardi 23 juillet 2019 à 15:41 +0000, David Laight a écrit :
> From: Rasmus Villemoes
> > Sent: 23 July 2019 07:56
> ...
> > > +/**
> > > + * stracpy - Copy a C-string into an array of char
> > > + * @to: Where to copy the string, must be an array of char and
> > > not a pointer
> > > + * @from: String to copy, may be a pointer or const char array
> > > + *
> > > + * Helper for strscpy.
> > > + * Copies a maximum of sizeof(@to) bytes of @from with %NUL
> > > termination.
> > > + *
> > > + * Returns:
> > > + * * The number of characters copied (not including the trailing
> > > %NUL)
> > > + * * -E2BIG if @to is a zero size array.
> > 
> > Well, yes, but more importantly and generally: -E2BIG if the copy
> > including %NUL didn't fit. [The zero size array thing could be made
> > into
> > a build bug for these stra* variants if one thinks that might
> > actually
> > occur in real code.]
> 
> Probably better is to return the size of the destination if the copy
> didn't fit
> (zero if the buffer is zero length).
> This allows code to do repeated:
> 	offset += str*cpy(buf + offset, src, sizeof buf - offset);
> and do a final check for overflow after all the copies.
> 
> The same is true for a snprintf()like function
> 

Beware that snprintf(), per C standard, is supposed to return the
length of the formatted string, regarless of the size of the
destination buffer.

So encouraging developper to write something like code below because
snprintf() in kernel behave in a non-standard way, will likely create
some issues in the near future.

  for(;...;)
    offset += snprintf(buf + offset, size - offset, "......", ....);

(Reminder: the code below is not safe and shouldn't be used)

Regards.

-- 
Yann Droneaud
OPTEYA

Re: [PATCH 1/2] string: Add stracpy and stracpy_pad mechanisms

[Rasmus Villemoes]

On 24/07/2019 14.05, Yann Droneaud wrote:
> Hi,
> 

> Beware that snprintf(), per C standard, is supposed to return the
> length of the formatted string, regarless of the size of the
> destination buffer.
> 
> So encouraging developper to write something like code below because
> snprintf() in kernel behave in a non-standard way,

The kernel's snprintf() does not behave in a non-standard way, at least
not with respect to its return value. It doesn't support %n or floating
point, of course, and there are some quirks regarding precision (see
lib/test_printf.c for details).

There's the non-standard scnprintf() for getting the length of the
formatted string, which can safely be used in an append loop. Or one can
use the seq_buf API.

Rasmus

Re: [PATCH 1/2] string: Add stracpy and stracpy_pad mechanisms

[Linus Torvalds]

On Wed, Jul 24, 2019 at 6:09 AM Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
>
> The kernel's snprintf() does not behave in a non-standard way, at least
> not with respect to its return value.

Note that the kernels snprintf() *does* very much protect against the
overflow case - not by changing the return value, but simply by having

        /* Reject out-of-range values early.  Large positive sizes are
           used for unknown buffer sizes. */
        if (WARN_ON_ONCE(size > INT_MAX))
                return 0;

at the very top.

So you can't actually overflow in the kernel by using the repeated

        offset += vsnprintf( .. size - offset ..);

model.

Yes, it's the wrong thing to do, but it is still _safe_.

              Linus

Re: [PATCH 1/2] string: Add stracpy and stracpy_pad mechanisms

[Kees Cook]

On Wed, Jul 24, 2019 at 10:08:57AM -0700, Linus Torvalds wrote:
> On Wed, Jul 24, 2019 at 6:09 AM Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
> >
> > The kernel's snprintf() does not behave in a non-standard way, at least
> > not with respect to its return value.
> 
> Note that the kernels snprintf() *does* very much protect against the
> overflow case - not by changing the return value, but simply by having
> 
>         /* Reject out-of-range values early.  Large positive sizes are
>            used for unknown buffer sizes. */
>         if (WARN_ON_ONCE(size > INT_MAX))
>                 return 0;
> 
> at the very top.
> 
> So you can't actually overflow in the kernel by using the repeated
> 
>         offset += vsnprintf( .. size - offset ..);
> 
> model.
> 
> Yes, it's the wrong thing to do, but it is still _safe_.

Actually, perhaps we should add this test to strscpy() too?

diff --git a/lib/string.c b/lib/string.c
index 461fb620f85f..0e0d7628ddc4 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -182,7 +182,7 @@ ssize_t strscpy(char *dest, const char *src, size_t count)
 	size_t max = count;
 	long res = 0;
 
-	if (count == 0)
+	if (count == 0 || count > INT_MAX)
 		return -E2BIG;
 
 #ifdef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS

-- 
Kees Cook

Re: [PATCH 1/2] string: Add stracpy and stracpy_pad mechanisms

[Joe Perches]

On Thu, 2019-07-25 at 13:03 -0700, Kees Cook wrote:
> On Wed, Jul 24, 2019 at 10:08:57AM -0700, Linus Torvalds wrote:
> > On Wed, Jul 24, 2019 at 6:09 AM Rasmus Villemoes
> > <linux@rasmusvillemoes.dk> wrote:
> > > The kernel's snprintf() does not behave in a non-standard way, at least
> > > not with respect to its return value.
> > 
> > Note that the kernels snprintf() *does* very much protect against the
> > overflow case - not by changing the return value, but simply by having
> > 
> >         /* Reject out-of-range values early.  Large positive sizes are
> >            used for unknown buffer sizes. */
> >         if (WARN_ON_ONCE(size > INT_MAX))
> >                 return 0;
> > 
> > at the very top.
> > 
> > So you can't actually overflow in the kernel by using the repeated
> > 
> >         offset += vsnprintf( .. size - offset ..);
> > 
> > model.
> > 
> > Yes, it's the wrong thing to do, but it is still _safe_.
> 
> Actually, perhaps we should add this test to strscpy() too?

Doesn't seem to have a reason not to be added
but maybe it's better to add another WARN_ON_ONCE.

> diff --git a/lib/string.c b/lib/string.c
[]
> @@ -182,7 +182,7 @@ ssize_t strscpy(char *dest, const char *src, size_t count)
>  	size_t max = count;
>  	long res = 0;
>  
> -	if (count == 0)
> +	if (count == 0 || count > INT_MAX)
>  		return -E2BIG;
>  
>  #ifdef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
> 

Re: [PATCH 1/2] string: Add stracpy and stracpy_pad mechanisms

[Kees Cook]

On Mon, Jul 22, 2019 at 05:38:15PM -0700, Joe Perches wrote:
> Several uses of strlcpy and strscpy have had defects because the
> last argument of each function is misused or typoed.
> 
> Add macro mechanisms to avoid this defect.
> 
> stracpy (copy a string to a string array) must have a string
> array as the first argument (to) and uses sizeof(to) as the
> size.
> 
> These mechanisms verify that the to argument is an array of
> char or other compatible types like u8 or unsigned char.
> 
> A BUILD_BUG is emitted when the type of to is not compatible.
> 
> Signed-off-by: Joe Perches <joe@perches.com>

I think Rasmus's suggestion would make sense:

	BUILD_BUG_ON(!__same_type(typeof(to), char[]))

Either way, I think it should be fine:

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
>  include/linux/string.h | 41 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 
> diff --git a/include/linux/string.h b/include/linux/string.h
> index 4deb11f7976b..f80b0973f0e5 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -35,6 +35,47 @@ ssize_t strscpy(char *, const char *, size_t);
>  /* Wraps calls to strscpy()/memset(), no arch specific code required */
>  ssize_t strscpy_pad(char *dest, const char *src, size_t count);
>  
> +/**
> + * stracpy - Copy a C-string into an array of char
> + * @to: Where to copy the string, must be an array of char and not a pointer
> + * @from: String to copy, may be a pointer or const char array
> + *
> + * Helper for strscpy.
> + * Copies a maximum of sizeof(@to) bytes of @from with %NUL termination.
> + *
> + * Returns:
> + * * The number of characters copied (not including the trailing %NUL)
> + * * -E2BIG if @to is a zero size array.
> + */
> +#define stracpy(to, from)					\
> +({								\
> +	size_t size = ARRAY_SIZE(to);				\
> +	BUILD_BUG_ON(!__same_type(typeof(*to), char));		\
> +								\
> +	strscpy(to, from, size);				\
> +})
> +
> +/**
> + * stracpy_pad - Copy a C-string into an array of char with %NUL padding
> + * @to: Where to copy the string, must be an array of char and not a pointer
> + * @from: String to copy, may be a pointer or const char array
> + *
> + * Helper for strscpy_pad.
> + * Copies a maximum of sizeof(@to) bytes of @from with %NUL termination
> + * and zero-pads the remaining size of @to
> + *
> + * Returns:
> + * * The number of characters copied (not including the trailing %NUL)
> + * * -E2BIG if @to is a zero size array.
> + */
> +#define stracpy_pad(to, from)					\
> +({								\
> +	size_t size = ARRAY_SIZE(to);				\
> +	BUILD_BUG_ON(!__same_type(typeof(*to), char));		\
> +								\
> +	strscpy_pad(to, from, size);				\
> +})
> +
>  #ifndef __HAVE_ARCH_STRCAT
>  extern char * strcat(char *, const char *);
>  #endif
> -- 
> 2.15.0
> 

-- 
Kees Cook

Re: [PATCH 1/2] string: Add stracpy and stracpy_pad mechanisms

[Joe Perches]

On Tue, 2019-07-23 at 14:36 -0700, Kees Cook wrote:
> On Mon, Jul 22, 2019 at 05:38:15PM -0700, Joe Perches wrote:
> > Several uses of strlcpy and strscpy have had defects because the
> > last argument of each function is misused or typoed.
> > 
> > Add macro mechanisms to avoid this defect.
> > 
> > stracpy (copy a string to a string array) must have a string
> > array as the first argument (to) and uses sizeof(to) as the
> > size.
> > 
> > These mechanisms verify that the to argument is an array of
> > char or other compatible types like u8 or unsigned char.
> > 
> > A BUILD_BUG is emitted when the type of to is not compatible.
> > 
> > Signed-off-by: Joe Perches <joe@perches.com>
> 
> I think Rasmus's suggestion would make sense:
> 
> 	BUILD_BUG_ON(!__same_type(typeof(to), char[]))

I think Rasmus had an excellent suggestion.
I liked it and submitted it as V2.

> Reviewed-by: Kees Cook <keescook@chromium.org>

Thanks.

[PATCH 2/2] kernel-doc: core-api: Include string.h into core-api

[Joe Perches]

core-api should show all the various string functions including the
newly added stracpy and stracpy_pad.

Miscellanea:

o Update the Returns: value for strscpy
o fix a defect with %NUL)

Signed-off-by: Joe Perches <joe@perches.com>
---
 Documentation/core-api/kernel-api.rst |  3 +++
 include/linux/string.h                |  5 +++--
 lib/string.c                          | 10 ++++++----
 3 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/Documentation/core-api/kernel-api.rst b/Documentation/core-api/kernel-api.rst
index 08af5caf036d..f77de49b1d51 100644
--- a/Documentation/core-api/kernel-api.rst
+++ b/Documentation/core-api/kernel-api.rst
@@ -42,6 +42,9 @@ String Manipulation
 .. kernel-doc:: lib/string.c
    :export:
 
+.. kernel-doc:: include/linux/string.h
+   :internal:
+
 .. kernel-doc:: mm/util.c
    :functions: kstrdup kstrdup_const kstrndup kmemdup kmemdup_nul memdup_user
                vmemdup_user strndup_user memdup_user_nul
diff --git a/include/linux/string.h b/include/linux/string.h
index f80b0973f0e5..329188fffc11 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -515,8 +515,9 @@ static inline void memcpy_and_pad(void *dest, size_t dest_len,
  * But this can lead to bugs due to typos, or if prefix is a pointer
  * and not a constant. Instead use str_has_prefix().
  *
- * Returns: 0 if @str does not start with @prefix
-         strlen(@prefix) if @str does start with @prefix
+ * Returns:
+ * * strlen(@prefix) if @str starts with @prefix
+ * * 0 if @str does not start with @prefix
  */
 static __always_inline size_t str_has_prefix(const char *str, const char *prefix)
 {
diff --git a/lib/string.c b/lib/string.c
index 461fb620f85f..53582b6dce2a 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -173,8 +173,9 @@ EXPORT_SYMBOL(strlcpy);
  * doesn't unnecessarily force the tail of the destination buffer to be
  * zeroed.  If zeroing is desired please use strscpy_pad().
  *
- * Return: The number of characters copied (not including the trailing
- *         %NUL) or -E2BIG if the destination buffer wasn't big enough.
+ * Returns:
+ * * The number of characters copied (not including the trailing %NUL)
+ * * -E2BIG if count is 0.
  */
 ssize_t strscpy(char *dest, const char *src, size_t count)
 {
@@ -253,8 +254,9 @@ EXPORT_SYMBOL(strscpy);
  * For full explanation of why you may want to consider using the
  * 'strscpy' functions please see the function docstring for strscpy().
  *
- * Return: The number of characters copied (not including the trailing
- *         %NUL) or -E2BIG if the destination buffer wasn't big enough.
+ * Returns:
+ * * The number of characters copied (not including the trailing %NUL)
+ * * -E2BIG if count is 0.
  */
 ssize_t strscpy_pad(char *dest, const char *src, size_t count)
 {
-- 
2.15.0

Re: [PATCH 2/2] kernel-doc: core-api: Include string.h into core-api

[Kees Cook]

On Mon, Jul 22, 2019 at 05:38:16PM -0700, Joe Perches wrote:
> core-api should show all the various string functions including the
> newly added stracpy and stracpy_pad.
> 
> Miscellanea:
> 
> o Update the Returns: value for strscpy
> o fix a defect with %NUL)
> 
> Signed-off-by: Joe Perches <joe@perches.com>

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
>  Documentation/core-api/kernel-api.rst |  3 +++
>  include/linux/string.h                |  5 +++--
>  lib/string.c                          | 10 ++++++----
>  3 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/core-api/kernel-api.rst b/Documentation/core-api/kernel-api.rst
> index 08af5caf036d..f77de49b1d51 100644
> --- a/Documentation/core-api/kernel-api.rst
> +++ b/Documentation/core-api/kernel-api.rst
> @@ -42,6 +42,9 @@ String Manipulation
>  .. kernel-doc:: lib/string.c
>     :export:
>  
> +.. kernel-doc:: include/linux/string.h
> +   :internal:
> +
>  .. kernel-doc:: mm/util.c
>     :functions: kstrdup kstrdup_const kstrndup kmemdup kmemdup_nul memdup_user
>                 vmemdup_user strndup_user memdup_user_nul
> diff --git a/include/linux/string.h b/include/linux/string.h
> index f80b0973f0e5..329188fffc11 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -515,8 +515,9 @@ static inline void memcpy_and_pad(void *dest, size_t dest_len,
>   * But this can lead to bugs due to typos, or if prefix is a pointer
>   * and not a constant. Instead use str_has_prefix().
>   *
> - * Returns: 0 if @str does not start with @prefix
> -         strlen(@prefix) if @str does start with @prefix
> + * Returns:
> + * * strlen(@prefix) if @str starts with @prefix
> + * * 0 if @str does not start with @prefix
>   */
>  static __always_inline size_t str_has_prefix(const char *str, const char *prefix)
>  {
> diff --git a/lib/string.c b/lib/string.c
> index 461fb620f85f..53582b6dce2a 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -173,8 +173,9 @@ EXPORT_SYMBOL(strlcpy);
>   * doesn't unnecessarily force the tail of the destination buffer to be
>   * zeroed.  If zeroing is desired please use strscpy_pad().
>   *
> - * Return: The number of characters copied (not including the trailing
> - *         %NUL) or -E2BIG if the destination buffer wasn't big enough.
> + * Returns:
> + * * The number of characters copied (not including the trailing %NUL)
> + * * -E2BIG if count is 0.
>   */
>  ssize_t strscpy(char *dest, const char *src, size_t count)
>  {
> @@ -253,8 +254,9 @@ EXPORT_SYMBOL(strscpy);
>   * For full explanation of why you may want to consider using the
>   * 'strscpy' functions please see the function docstring for strscpy().
>   *
> - * Return: The number of characters copied (not including the trailing
> - *         %NUL) or -E2BIG if the destination buffer wasn't big enough.
> + * Returns:
> + * * The number of characters copied (not including the trailing %NUL)
> + * * -E2BIG if count is 0.
>   */
>  ssize_t strscpy_pad(char *dest, const char *src, size_t count)
>  {
> -- 
> 2.15.0
> 

-- 
Kees Cook