Date: Fri, 23 Jun 2023 08:38:44 -0700 From: Enji Cooper <yaneurabeya@gmail.com> To: Mark Johnston <markj@freebsd.org> Cc: src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: git: fc915f1be145 - main - pseudofs: Fix a potential out-of-bounds access in pfs_lookup() Message-ID: <0BAC85B7-6A67-4F6E-87B8-97ABD2FF7075@gmail.com> In-Reply-To: <202306231509.35NF9sAk037726@gitrepo.freebsd.org> References: <202306231509.35NF9sAk037726@gitrepo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
> On Jun 23, 2023, at 08:09, Mark Johnston <markj@freebsd.org> wrote: > > The branch main has been updated by markj: > > URL: https://cgit.FreeBSD.org/src/commit/?id=fc915f1be145a52c53f6f1c37525043216e32bb8 > > commit fc915f1be145a52c53f6f1c37525043216e32bb8 > Author: Mark Johnston <markj@FreeBSD.org> > AuthorDate: 2023-06-23 13:54:39 +0000 > Commit: Mark Johnston <markj@FreeBSD.org> > CommitDate: 2023-06-23 13:54:39 +0000 > > pseudofs: Fix a potential out-of-bounds access in pfs_lookup() > > pseudofs nodes store their name in a flexible array member, so the node > allocation is sized using the length of the name, including a nul > terminator. pfs_lookup() scans a directory of nodes, comparing names to > find a match. The comparison was incorrect and assumed that all node > names were at least as long as the name being looked up, which of course > isn't true. > > I believe the bug is mostly harmless since it cannot result in false > positive or negative matches from the lookup, but it triggers a KASAN > check. > > Reported by: pho > Reviewed by: kib, Olivier Certner <olce.freebsd@certner.fr> > MFC after: 2 weeks > Sponsored by: The FreeBSD Foundation > Differential Revision: https://reviews.freebsd.org/D40692 > --- > sys/fs/pseudofs/pseudofs_vnops.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/sys/fs/pseudofs/pseudofs_vnops.c b/sys/fs/pseudofs/pseudofs_vnops.c > index 53e4c2b6b85c..bf423f0ad4db 100644 > --- a/sys/fs/pseudofs/pseudofs_vnops.c > +++ b/sys/fs/pseudofs/pseudofs_vnops.c > @@ -537,8 +537,8 @@ pfs_lookup(struct vop_cachedlookup_args *va) > for (pn = pd->pn_nodes; pn != NULL; pn = pn->pn_next) > if (pn->pn_type == pfstype_procdir) > pdn = pn; > - else if (pn->pn_name[namelen] == '\0' && > - bcmp(pname, pn->pn_name, namelen) == 0) { > + else if (strncmp(pname, pn->pn_name, namelen) == 0 && > + pn->pn_name[namelen] == '\0') { > pfs_unlock(pd); > goto got_pnode; > } Naive question: should this be an && conditional or an || conditional? If the former, could this be simplified by using a direct NUL char equality check instead of using strncmp? Thanks! -Enji
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?0BAC85B7-6A67-4F6E-87B8-97ABD2FF7075>