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 <[email protected]>
---
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
On Mon, 22 Jul 2019 17:38:15 -0700 Joe Perches <[email protected]> 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.
On Mon, 2019-07-22 at 21:35 -0700, Andrew Morton wrote:
> On Mon, 22 Jul 2019 17:38:15 -0700 Joe Perches <[email protected]> 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;
}
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 <[email protected]>
> ---
> 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
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)
(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?
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 <[email protected]> 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
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 <[email protected]>
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 <[email protected]>
-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
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
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 <[email protected]>
>
> 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 <[email protected]>
Thanks.
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
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
On Wed, Jul 24, 2019 at 6:09 AM Rasmus Villemoes
<[email protected]> 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
On Thu, 25 Jul 2019, Markus Elfring wrote:
> > New version. I check for non-use of the return value of strlcpy and
> > address some issues that affected the matching of the case where the first
> > argument involves a pointer dereference.
>
> I suggest to take another look at corresponding implementation details
> of the shown SmPL script.
>
>
> > \(strscpy\|strlcpy\)(e1.f, e2, i2)@p
>
> Can the data access operator “->” (arrow) matter also here?
What did my email say about isomorphisms?
>
>
> > @@
> > identifier r.i1,r.i2;
> > type T;
> > @@
> > struct i1 { ... T i1[i2]; ... }
>
> Will an additional SmPL rule name be helpful for this part?
Yes, sorry, it would seem that that is necessary. I will fix and resend
the results.
>
>
> > @@
> > (
> > -x = strlcpy
> > +stracpy
> > (e1.f, e2
> > - , i2
> > )@p;
> > ... when != x
> >
> > |
>
> I wonder about the deletion of the assignment target.
> Should the setting of such a variable be usually preserved?
If it is a local variable and never subsequently used, it doesn't seem
very useful.
julia
>>> @@
>>> (
>>> -x = strlcpy
>>> +stracpy
>>> (e1.f, e2
>>> - , i2
>>> )@p;
>>> ... when != x
>>>
>>> |
>>
>> I wonder about the deletion of the assignment target.
>> Should the setting of such a variable be usually preserved?
>
> If it is a local variable and never subsequently used, it doesn't seem
> very useful.
Such an explanation is easier to understand.
* How do you think about the possibility that it was (accidentally)
forgotten to use such a local variable?
* Your transformation can result in an intentionally unused return value.
Would you like point any more source code places out
where values are unused so far?
Regards,
Markus
On Wed, Jul 24, 2019 at 10:08:57AM -0700, Linus Torvalds wrote:
> On Wed, Jul 24, 2019 at 6:09 AM Rasmus Villemoes
> <[email protected]> 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
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
> > <[email protected]> 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
>