* [PATCH] tracing/boot: Replace strlcpy with strscpy
@ 2023-06-13 0:41 Azeem Shaikh
2023-06-13 19:27 ` Kees Cook
0 siblings, 1 reply; 5+ messages in thread
From: Azeem Shaikh @ 2023-06-13 0:41 UTC (permalink / raw)
To: Steven Rostedt, Masami Hiramatsu
Cc: linux-hardening, Azeem Shaikh, linux-kernel, linux-trace-kernel
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 -E2BIG
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>
---
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..5fe525f1b8cc 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)) == -E2BIG) {
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)) == -E2BIG) {
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)) == -E2BIG)
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)) == -E2BIG)
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] tracing/boot: Replace strlcpy with strscpy
2023-06-13 0:41 [PATCH] tracing/boot: Replace strlcpy with strscpy Azeem Shaikh
@ 2023-06-13 19:27 ` Kees Cook
2023-06-14 14:01 ` Azeem Shaikh
0 siblings, 1 reply; 5+ messages in thread
From: Kees Cook @ 2023-06-13 19:27 UTC (permalink / raw)
To: Azeem Shaikh
Cc: Steven Rostedt, Masami Hiramatsu, linux-hardening, linux-kernel,
linux-trace-kernel
On Tue, Jun 13, 2023 at 12:41:25AM +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().
>
> Direct replacement is safe here since return value of -E2BIG
> is used to check for truncation instead of sizeof(dest).
This looks technically correct, but I wonder if "< 0" is a better test?
>
> [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>
Either way
Reviewed-by: Kees Cook <keescook@chromium.org>
-Kees
> ---
> 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..5fe525f1b8cc 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)) == -E2BIG) {
> 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)) == -E2BIG) {
> 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)) == -E2BIG)
> 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)) == -E2BIG)
> 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
>
>
--
Kees Cook
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] tracing/boot: Replace strlcpy with strscpy
2023-06-13 19:27 ` Kees Cook
@ 2023-06-14 14:01 ` Azeem Shaikh
2023-06-14 17:13 ` Eric Biggers
0 siblings, 1 reply; 5+ messages in thread
From: Azeem Shaikh @ 2023-06-14 14:01 UTC (permalink / raw)
To: Kees Cook
Cc: Steven Rostedt, Masami Hiramatsu, linux-hardening, linux-kernel,
linux-trace-kernel
On Tue, Jun 13, 2023 at 3:27 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Tue, Jun 13, 2023 at 12:41:25AM +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().
> >
> > Direct replacement is safe here since return value of -E2BIG
> > is used to check for truncation instead of sizeof(dest).
>
> This looks technically correct, but I wonder if "< 0" is a better test?
Agreed. "< 0" might more generically represent -errno. Happy to send
over a v2 if you prefer that instead of sticking with this patch.
>
> >
> > [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>
>
> Either way
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
>
> -Kees
>
> > ---
> > 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..5fe525f1b8cc 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)) == -E2BIG) {
> > 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)) == -E2BIG) {
> > 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)) == -E2BIG)
> > 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)) == -E2BIG)
> > 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
> >
> >
>
> --
> Kees Cook
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] tracing/boot: Replace strlcpy with strscpy
2023-06-14 14:01 ` Azeem Shaikh
@ 2023-06-14 17:13 ` Eric Biggers
2023-06-15 18:08 ` Azeem Shaikh
0 siblings, 1 reply; 5+ messages in thread
From: Eric Biggers @ 2023-06-14 17:13 UTC (permalink / raw)
To: Azeem Shaikh
Cc: Kees Cook, Steven Rostedt, Masami Hiramatsu, linux-hardening,
linux-kernel, linux-trace-kernel
On Wed, Jun 14, 2023 at 10:01:57AM -0400, Azeem Shaikh wrote:
> On Tue, Jun 13, 2023 at 3:27 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Tue, Jun 13, 2023 at 12:41:25AM +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().
> > >
> > > Direct replacement is safe here since return value of -E2BIG
> > > is used to check for truncation instead of sizeof(dest).
> >
> > This looks technically correct, but I wonder if "< 0" is a better test?
>
> Agreed. "< 0" might more generically represent -errno. Happy to send
> over a v2 if you prefer that instead of sticking with this patch.
Please go with "< 0", since it's easier to read and less error-prone. (It would
be easy to mistype -E2BIG as -EFBIG, or E2BIG, for example...)
- Eric
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] tracing/boot: Replace strlcpy with strscpy
2023-06-14 17:13 ` Eric Biggers
@ 2023-06-15 18:08 ` Azeem Shaikh
0 siblings, 0 replies; 5+ messages in thread
From: Azeem Shaikh @ 2023-06-15 18:08 UTC (permalink / raw)
To: Eric Biggers
Cc: Kees Cook, Steven Rostedt, Masami Hiramatsu, linux-hardening,
linux-kernel, linux-trace-kernel
On Wed, Jun 14, 2023 at 1:13 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Wed, Jun 14, 2023 at 10:01:57AM -0400, Azeem Shaikh wrote:
> > On Tue, Jun 13, 2023 at 3:27 PM Kees Cook <keescook@chromium.org> wrote:
> > >
> > > On Tue, Jun 13, 2023 at 12:41:25AM +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().
> > > >
> > > > Direct replacement is safe here since return value of -E2BIG
> > > > is used to check for truncation instead of sizeof(dest).
> > >
> > > This looks technically correct, but I wonder if "< 0" is a better test?
> >
> > Agreed. "< 0" might more generically represent -errno. Happy to send
> > over a v2 if you prefer that instead of sticking with this patch.
>
> Please go with "< 0", since it's easier to read and less error-prone. (It would
> be easy to mistype -E2BIG as -EFBIG, or E2BIG, for example...)
>
Thanks, sent out a v2 with this change.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-06-15 18:09 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-13 0:41 [PATCH] tracing/boot: Replace strlcpy with strscpy Azeem Shaikh
2023-06-13 19:27 ` Kees Cook
2023-06-14 14:01 ` Azeem Shaikh
2023-06-14 17:13 ` Eric Biggers
2023-06-15 18:08 ` Azeem Shaikh
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.