| Message ID | 7ab8957eaf9b0931a59eff6e2bd8c5169f2f6c41.1563841972.git.joe@perches.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | string: Add stracpy and stracpy_pad | expand |
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.
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; }
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
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 <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; > } > >
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 *.)
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 >
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.
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.
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 <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
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
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 >
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
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(+)