linux-kernel - Re: [PATCH v2] kernfs: Prefer strscpy over strlcpy calls

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <ZFvnpCZM-XbOFFVh@slm.duckdns.org>
Date:   Wed, 10 May 2023 08:51:16 -1000
From:   Tejun Heo <tj@...nel.org>
To:     Azeem Shaikh <azeemshaikh38@...il.com>
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

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