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;
Sender: Tejun Heo <htejun@gmail.com>
Date:   Wed, 10 May 2023 08:51:16 -1000
From:   Tejun Heo <tj@kernel.org>
To:     Azeem Shaikh <azeemshaikh38@gmail.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        security@kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] kernfs: Prefer strscpy over strlcpy calls
Message-ID: <ZFvnpCZM-XbOFFVh@slm.duckdns.org>
References: <20230510011122.3040915-1-azeemshaikh38@gmail.com>
 <ZFryVUHtkN5QbKTP@slm.duckdns.org>
 <CADmuW3XiYpGK7BessXJWjGnnxZti_3mawDSX7QPawsfmATxCng@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CADmuW3XiYpGK7BessXJWjGnnxZti_3mawDSX7QPawsfmATxCng@mail.gmail.com>
Precedence: bulk

Hello,

On Wed, May 10, 2023 at 12:03:41PM -0400, Azeem Shaikh wrote:
> > >  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.

That sounds great to me. I have a hard time imagining needing the length
return for single component name.

> > >  /* 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,
...
> > 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.

Yeah, I could be misremembering but istr some place which actually uses the
length return to extend buffer allocation, so this might be challenging.

Thanks.

-- 
tejun