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

[Azeem Shaikh <azeemshaikh38@gmail.com>]

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