| Message ID | 20230615180420.400769-1-azeemshaikh38@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2] tracing/boot: Replace strlcpy with strscpy | expand |
On Thu, 15 Jun 2023 18:04:20 +0000 Azeem Shaikh <azeemshaikh38@gmail.com> wrote: > strlcpy() reads the entire source buffer first. > This read may exceed the destination size limit. > This is both inefficient and can lead to linear read > overflows if a source string is not NUL-terminated [1]. > In an effort to remove strlcpy() completely [2], replace > strlcpy() here with strscpy(). > > Direct replacement is safe here since return value of -errno > is used to check for truncation instead of sizeof(dest). > > [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy > [2] https://github.com/KSPP/linux/issues/89 > Looks good to me. Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> Thanks! > Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com> > --- > v2: > * Use "< 0" instead of -E2BIG. > > v1: > * https://lore.kernel.org/all/20230613004125.3539934-1-azeemshaikh38@gmail.com/ > > kernel/trace/trace_boot.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/kernel/trace/trace_boot.c b/kernel/trace/trace_boot.c > index 778200dd8ede..7ccc7a8e155b 100644 > --- a/kernel/trace/trace_boot.c > +++ b/kernel/trace/trace_boot.c > @@ -31,7 +31,7 @@ trace_boot_set_instance_options(struct trace_array *tr, struct xbc_node *node) > > /* Common ftrace options */ > xbc_node_for_each_array_value(node, "options", anode, p) { > - if (strlcpy(buf, p, ARRAY_SIZE(buf)) >= ARRAY_SIZE(buf)) { > + if (strscpy(buf, p, ARRAY_SIZE(buf)) < 0) { > pr_err("String is too long: %s\n", p); > continue; > } > @@ -87,7 +87,7 @@ trace_boot_enable_events(struct trace_array *tr, struct xbc_node *node) > const char *p; > > xbc_node_for_each_array_value(node, "events", anode, p) { > - if (strlcpy(buf, p, ARRAY_SIZE(buf)) >= ARRAY_SIZE(buf)) { > + if (strscpy(buf, p, ARRAY_SIZE(buf)) < 0) { > pr_err("String is too long: %s\n", p); > continue; > } > @@ -486,7 +486,7 @@ trace_boot_init_one_event(struct trace_array *tr, struct xbc_node *gnode, > > p = xbc_node_find_value(enode, "filter", NULL); > if (p && *p != '\0') { > - if (strlcpy(buf, p, ARRAY_SIZE(buf)) >= ARRAY_SIZE(buf)) > + if (strscpy(buf, p, ARRAY_SIZE(buf)) < 0) > pr_err("filter string is too long: %s\n", p); > else if (apply_event_filter(file, buf) < 0) > pr_err("Failed to apply filter: %s\n", buf); > @@ -494,7 +494,7 @@ trace_boot_init_one_event(struct trace_array *tr, struct xbc_node *gnode, > > if (IS_ENABLED(CONFIG_HIST_TRIGGERS)) { > xbc_node_for_each_array_value(enode, "actions", anode, p) { > - if (strlcpy(buf, p, ARRAY_SIZE(buf)) >= ARRAY_SIZE(buf)) > + if (strscpy(buf, p, ARRAY_SIZE(buf)) < 0) > pr_err("action string is too long: %s\n", p); > else if (trigger_process_regex(file, buf) < 0) > pr_err("Failed to apply an action: %s\n", p); > -- > 2.41.0.162.gfafddb0af9-goog > >
On Thu, 15 Jun 2023 18:04:20 +0000, Azeem Shaikh wrote: > strlcpy() reads the entire source buffer first. > This read may exceed the destination size limit. > This is both inefficient and can lead to linear read > overflows if a source string is not NUL-terminated [1]. > In an effort to remove strlcpy() completely [2], replace > strlcpy() here with strscpy(). > > [...] Applied to for-next/hardening, thanks! [1/1] tracing/boot: Replace strlcpy with strscpy https://git.kernel.org/kees/c/b1c38314f756
On Tue, 20 Jun 2023 13:28:26 -0700 Kees Cook <keescook@chromium.org> wrote: > On Thu, 15 Jun 2023 18:04:20 +0000, Azeem Shaikh wrote: > > strlcpy() reads the entire source buffer first. > > This read may exceed the destination size limit. > > This is both inefficient and can lead to linear read > > overflows if a source string is not NUL-terminated [1]. > > In an effort to remove strlcpy() completely [2], replace > > strlcpy() here with strscpy(). > > > > [...] > > Applied to for-next/hardening, thanks! > > [1/1] tracing/boot: Replace strlcpy with strscpy > https://git.kernel.org/kees/c/b1c38314f756 > I was going to add this to my queue. -- Steve
On Tue, Jun 20, 2023 at 04:33:25PM -0400, Steven Rostedt wrote: > On Tue, 20 Jun 2023 13:28:26 -0700 > Kees Cook <keescook@chromium.org> wrote: > > > On Thu, 15 Jun 2023 18:04:20 +0000, Azeem Shaikh wrote: > > > strlcpy() reads the entire source buffer first. > > > This read may exceed the destination size limit. > > > This is both inefficient and can lead to linear read > > > overflows if a source string is not NUL-terminated [1]. > > > In an effort to remove strlcpy() completely [2], replace > > > strlcpy() here with strscpy(). > > > > > > [...] > > > > Applied to for-next/hardening, thanks! > > > > [1/1] tracing/boot: Replace strlcpy with strscpy > > https://git.kernel.org/kees/c/b1c38314f756 > > > > I was going to add this to my queue. Ah, okay, no worries. I will drop it from mine.
On Tue, 20 Jun 2023 13:35:14 -0700 Kees Cook <keescook@chromium.org> wrote: > > > Applied to for-next/hardening, thanks! > > > > > > [1/1] tracing/boot: Replace strlcpy with strscpy > > > https://git.kernel.org/kees/c/b1c38314f756 > > > > > > > I was going to add this to my queue. > > Ah, okay, no worries. I will drop it from mine. Maybe I should have let you take it. I had v1 in my queue, and pushed that one :-p I'll fix it later. Thanks, -- Steve
diff --git a/kernel/trace/trace_boot.c b/kernel/trace/trace_boot.c index 778200dd8ede..7ccc7a8e155b 100644 --- a/kernel/trace/trace_boot.c +++ b/kernel/trace/trace_boot.c @@ -31,7 +31,7 @@ trace_boot_set_instance_options(struct trace_array *tr, struct xbc_node *node) /* Common ftrace options */ xbc_node_for_each_array_value(node, "options", anode, p) { - if (strlcpy(buf, p, ARRAY_SIZE(buf)) >= ARRAY_SIZE(buf)) { + if (strscpy(buf, p, ARRAY_SIZE(buf)) < 0) { pr_err("String is too long: %s\n", p); continue; } @@ -87,7 +87,7 @@ trace_boot_enable_events(struct trace_array *tr, struct xbc_node *node) const char *p; xbc_node_for_each_array_value(node, "events", anode, p) { - if (strlcpy(buf, p, ARRAY_SIZE(buf)) >= ARRAY_SIZE(buf)) { + if (strscpy(buf, p, ARRAY_SIZE(buf)) < 0) { pr_err("String is too long: %s\n", p); continue; } @@ -486,7 +486,7 @@ trace_boot_init_one_event(struct trace_array *tr, struct xbc_node *gnode, p = xbc_node_find_value(enode, "filter", NULL); if (p && *p != '\0') { - if (strlcpy(buf, p, ARRAY_SIZE(buf)) >= ARRAY_SIZE(buf)) + if (strscpy(buf, p, ARRAY_SIZE(buf)) < 0) pr_err("filter string is too long: %s\n", p); else if (apply_event_filter(file, buf) < 0) pr_err("Failed to apply filter: %s\n", buf); @@ -494,7 +494,7 @@ trace_boot_init_one_event(struct trace_array *tr, struct xbc_node *gnode, if (IS_ENABLED(CONFIG_HIST_TRIGGERS)) { xbc_node_for_each_array_value(enode, "actions", anode, p) { - if (strlcpy(buf, p, ARRAY_SIZE(buf)) >= ARRAY_SIZE(buf)) + if (strscpy(buf, p, ARRAY_SIZE(buf)) < 0) pr_err("action string is too long: %s\n", p); else if (trigger_process_regex(file, buf) < 0) pr_err("Failed to apply an action: %s\n", p);
strlcpy() reads the entire source buffer first. This read may exceed the destination size limit. This is both inefficient and can lead to linear read overflows if a source string is not NUL-terminated [1]. In an effort to remove strlcpy() completely [2], replace strlcpy() here with strscpy(). Direct replacement is safe here since return value of -errno is used to check for truncation instead of sizeof(dest). [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy [2] https://github.com/KSPP/linux/issues/89 Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com> --- v2: * Use "< 0" instead of -E2BIG. v1: * https://lore.kernel.org/all/20230613004125.3539934-1-azeemshaikh38@gmail.com/ kernel/trace/trace_boot.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)