2023-06-13 01:07:05

by Azeem Shaikh

Subject: [PATCH] tracing/boot: Replace strlcpy with strscpy

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 <[email protected]>
---
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

2023-06-13 19:47:14

by Kees Cook

Subject: Re: [PATCH] tracing/boot: Replace strlcpy with strscpy

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 <[email protected]>

Either way

Reviewed-by: Kees Cook <[email protected]>

-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

2023-06-14 14:27:57

by Azeem Shaikh

Subject: Re: [PATCH] tracing/boot: Replace strlcpy with strscpy

On Tue, Jun 13, 2023 at 3:27 PM Kees Cook <[email protected]> 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 <[email protected]>
>
> Either way
>
> Reviewed-by: Kees Cook <[email protected]>
>
> -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

2023-06-14 17:59:44

by Eric Biggers

Subject: Re: [PATCH] tracing/boot: Replace strlcpy with strscpy

On Wed, Jun 14, 2023 at 10:01:57AM -0400, Azeem Shaikh wrote:
> On Tue, Jun 13, 2023 at 3:27 PM Kees Cook <[email protected]> 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

2023-06-15 18:30:08

by Azeem Shaikh

Subject: Re: [PATCH] tracing/boot: Replace strlcpy with strscpy

On Wed, Jun 14, 2023 at 1:13 PM Eric Biggers <[email protected]> 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 <[email protected]> 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.