1. Patchew
  2. linux
  3. View series

[PATCH] tracing/boot: Replace strlcpy with strscpy

Azeem Shaikh posted 1 patch 2 years, 8 months ago
There is a newer version of this series
kernel/trace/trace_boot.c |    8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
[PATCH] tracing/boot: Replace strlcpy with strscpy
Posted by Azeem Shaikh 2 years, 8 months ago 
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
Re: [PATCH] tracing/boot: Replace strlcpy with strscpy
Posted by Kees Cook 2 years, 7 months ago 
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
Re: [PATCH] tracing/boot: Replace strlcpy with strscpy
Posted by Azeem Shaikh 2 years, 7 months ago 
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
Re: [PATCH] tracing/boot: Replace strlcpy with strscpy
Posted by Eric Biggers 2 years, 7 months ago 
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
Re: [PATCH] tracing/boot: Replace strlcpy with strscpy
Posted by Azeem Shaikh 2 years, 7 months ago 
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.

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.