Date:   Wed, 10 May 2023 12:03:41 -0400
From:   Azeem Shaikh <azeemshaikh38@...il.com>
To:     Tejun Heo <tj@...nel.org>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        security@...nel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] kernfs: Prefer strscpy over strlcpy calls

On Tue, May 9, 2023 at 9:24 PM Tejun Heo <tj@...nel.org> wrote:
>
> On Wed, May 10, 2023 at 01:11:22AM +0000, Azeem Shaikh wrote:
> ...
> > +/* strscpy_mock_strlcpy - imitates strlcpy API but uses strscpy underneath. */
> > +static size_t strscpy_mock_strlcpy(char *dest, const char *src, size_t count)
> > +{
> > +     strscpy(dest, src, count);
> > +     return strlen(src);
> > +}
>
> I'm not sure this is a meaningful conversion. One benefit of strscpy() is
> that it's less error-prone to check for overflows. What's the point of
> removing strlcpy() if we end up sprinkling lesser duplicates in the tree?
>

Thanks for your patience and helpful feedback on this Tejun, very much
appreciated.
I've responded to your comments below and added my notes on the
transitive usage of the functions.
Happy to send a smaller patch which directly replaces strlcpy->strscpy.

> >  static int kernfs_name_locked(struct kernfs_node *kn, char *buf, size_t buflen)
> >  {
> >       if (!kn)
> > -             return strlcpy(buf, "(null)", buflen);
> > +             return strscpy_mock_strlcpy(buf, "(null)", buflen);
> >
> > -     return strlcpy(buf, kn->parent ? kn->name : "/", buflen);
> > +     return strscpy_mock_strlcpy(buf, kn->parent ? kn->name : "/", buflen);
> >  }
>
> Can you follow the users and convert the users accordingly rather than
> masking it from here? ie. make kernfs_name() and friends return -E2BIG when
> source is too long like strscpy(). I don't think anybody cares, actually.
>

I found 4 transitive callers of kernfs_name across the kernel, all of
whom eventually ignored the return value:

1. fs/kernfs/dir.c: calls kernfs_name. Ignores return value.
2. include/linux/cgroup.h: calls kernfs_name from cgroup_name. returns
the value of kernfs_name.
3. kernel/cgroup/debug.c: calls cgroup_name. Ignores return value.
4.mm/page_owner.c: calls cgroup_name. Ignores return value.

So replacing directly with strscpy here should be safe. Let me know
what you think.

> >  /* kernfs_node_depth - compute depth from @from to @to */
> > @@ -141,13 +148,13 @@ static int kernfs_path_from_node_locked(struct kernfs_node *kn_to,
> >       int i, j;
> >
> >       if (!kn_to)
> > -             return strlcpy(buf, "(null)", buflen);
> > +             return strscpy_mock_strlcpy(buf, "(null)", buflen);
> >
> >       if (!kn_from)
> >               kn_from = kernfs_root(kn_to)->kn;
> >
> >       if (kn_from == kn_to)
> > -             return strlcpy(buf, "/", buflen);
> > +             return strscpy_mock_strlcpy(buf, "/", buflen);
> >
> >       common = kernfs_common_ancestor(kn_from, kn_to);
> >       if (WARN_ON(!common))
> > @@ -159,16 +166,16 @@ static int kernfs_path_from_node_locked(struct kernfs_node *kn_to,
> >       buf[0] = '\0';
> >
> >       for (i = 0; i < depth_from; i++)
> > -             len += strlcpy(buf + len, parent_str,
> > +             len += strscpy_mock_strlcpy(buf + len, parent_str,
> >                              len < buflen ? buflen - len : 0);
> >
> >       /* Calculate how many bytes we need for the rest */
> >       for (i = depth_to - 1; i >= 0; i--) {
> >               for (kn = kn_to, j = 0; j < i; j++)
> >                       kn = kn->parent;
> > -             len += strlcpy(buf + len, "/",
> > +             len += strscpy_mock_strlcpy(buf + len, "/",
> >                              len < buflen ? buflen - len : 0);
> > -             len += strlcpy(buf + len, kn->name,
> > +             len += strscpy_mock_strlcpy(buf + len, kn->name,
> >                              len < buflen ? buflen - len : 0);
> >       }
>
> Ditto, please convert all the users accordingly. If that's not feasible, I
> think it'd be better to leave it as-is. I don't see how the new code is
> better.
>

kernfs_path_from_node has quite a few transitive callers. I'll leave
this as-is for now and consider tackling this separately.

> > @@ -851,7 +858,7 @@ static struct kernfs_node *kernfs_walk_ns(struct kernfs_node *parent,
> >
> >       spin_lock_irq(&kernfs_pr_cont_lock);
> >
> > -     len = strlcpy(kernfs_pr_cont_buf, path, sizeof(kernfs_pr_cont_buf));
> > +     len = strscpy_mock_strlcpy(kernfs_pr_cont_buf, path, sizeof(kernfs_pr_cont_buf));
> >
> >       if (len >= sizeof(kernfs_pr_cont_buf)) {
> >               spin_unlock_irq(&kernfs_pr_cont_lock);
>
> This is an easy conversion to strscpy().

Ack.

>
> Thanks.


>
> --
> tejun

Powered by blists - more mailing lists