Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230510011122.3040915-1-azeemshaikh38@gmail.com> <ZFryVUHtkN5QbKTP@slm.duckdns.org>
In-Reply-To: <ZFryVUHtkN5QbKTP@slm.duckdns.org>
From:   Azeem Shaikh <azeemshaikh38@gmail.com>
Date:   Wed, 10 May 2023 12:03:41 -0400
Message-ID: <CADmuW3XiYpGK7BessXJWjGnnxZti_3mawDSX7QPawsfmATxCng@mail.gmail.com>
Subject: Re: [PATCH v2] kernfs: Prefer strscpy over strlcpy calls
To:     Tejun Heo <tj@kernel.org>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        security@kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, May 9, 2023 at 9:24=E2=80=AFPM Tejun Heo <tj@kernel.org> wrote:
>
> On Wed, May 10, 2023 at 01:11:22AM +0000, Azeem Shaikh wrote:
> ...
> > +/* strscpy_mock_strlcpy - imitates strlcpy API but uses strscpy undern=
eath. */
> > +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 : "/", buf=
len);
> >  }
>
> Can you follow the users and convert the users accordingly rather than
> masking it from here? ie. make kernfs_name() and friends return -E2BIG wh=
en
> 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 ke=
rnfs_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 =3D kernfs_root(kn_to)->kn;
> >
> >       if (kn_from =3D=3D kn_to)
> > -             return strlcpy(buf, "/", buflen);
> > +             return strscpy_mock_strlcpy(buf, "/", buflen);
> >
> >       common =3D kernfs_common_ancestor(kn_from, kn_to);
> >       if (WARN_ON(!common))
> > @@ -159,16 +166,16 @@ static int kernfs_path_from_node_locked(struct ke=
rnfs_node *kn_to,
> >       buf[0] =3D '\0';
> >
> >       for (i =3D 0; i < depth_from; i++)
> > -             len +=3D strlcpy(buf + len, parent_str,
> > +             len +=3D strscpy_mock_strlcpy(buf + len, parent_str,
> >                              len < buflen ? buflen - len : 0);
> >
> >       /* Calculate how many bytes we need for the rest */
> >       for (i =3D depth_to - 1; i >=3D 0; i--) {
> >               for (kn =3D kn_to, j =3D 0; j < i; j++)
> >                       kn =3D kn->parent;
> > -             len +=3D strlcpy(buf + len, "/",
> > +             len +=3D strscpy_mock_strlcpy(buf + len, "/",
> >                              len < buflen ? buflen - len : 0);
> > -             len +=3D strlcpy(buf + len, kn->name,
> > +             len +=3D 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 ke=
rnfs_node *parent,
> >
> >       spin_lock_irq(&kernfs_pr_cont_lock);
> >
> > -     len =3D strlcpy(kernfs_pr_cont_buf, path, sizeof(kernfs_pr_cont_b=
uf));
> > +     len =3D strscpy_mock_strlcpy(kernfs_pr_cont_buf, path, sizeof(ker=
nfs_pr_cont_buf));
> >
> >       if (len >=3D sizeof(kernfs_pr_cont_buf)) {
> >               spin_unlock_irq(&kernfs_pr_cont_lock);
>
> This is an easy conversion to strscpy().

Ack.

>
> Thanks.


>
> --
> tejun