[1/2] string: Add stracpy and stracpy_pad mechanisms
diff mbox series

Message ID 7ab8957eaf9b0931a59eff6e2bd8c5169f2f6c41.1563841972.git.joe@perches.com
State New
Headers show
Series
  • string: Add stracpy and stracpy_pad
Related show

Commit Message

Joe Perches July 23, 2019, 12:38 a.m. UTC
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(+)

Comments

Andrew Morton July 23, 2019, 4:35 a.m. UTC | #1
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.
Joe Perches July 23, 2019, 4:42 a.m. UTC | #2
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;
 }
Rasmus Villemoes July 23, 2019, 6:55 a.m. UTC | #3
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
David Laight July 23, 2019, 3:41 p.m. UTC | #4
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)
Joe Perches July 23, 2019, 3:50 p.m. UTC | #5
(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?
Kees Cook July 23, 2019, 9:29 p.m. UTC | #6
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 July 23, 2019, 9:34 p.m. UTC | #7
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 July 23, 2019, 9:36 p.m. UTC | #8
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
>
Joe Perches July 24, 2019, 11:40 a.m. UTC | #9
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.
Yann Droneaud July 24, 2019, 12:05 p.m. UTC | #10
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.
Rasmus Villemoes July 24, 2019, 1:09 p.m. UTC | #11
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
Linus Torvalds July 24, 2019, 5:08 p.m. UTC | #12
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
Kees Cook July 25, 2019, 8:03 p.m. UTC | #13
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
Joe Perches July 26, 2019, 2:46 a.m. UTC | #14
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
>

Patch
diff mbox series

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