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: >=20 > =EF=BB=BFThe branch main has been updated by markj: >=20 > URL: https://cgit.FreeBSD.org/src/commit/?id=3Dfc915f1be145a52c53f6f1c3752= 5043216e32bb8 >=20 > 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 >=20 > pseudofs: Fix a potential out-of-bounds access in pfs_lookup() >=20 > 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 t= o > 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 cours= e > isn't true. >=20 > 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. >=20 > 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(-) >=20 > diff --git a/sys/fs/pseudofs/pseudofs_vnops.c b/sys/fs/pseudofs/pseudofs_v= nops.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 =3D pd->pn_nodes; pn !=3D NULL; pn =3D pn->pn_next) > if (pn->pn_type =3D=3D pfstype_procdir) > pdn =3D pn; > - else if (pn->pn_name[namelen] =3D=3D '\0' && > - bcmp(pname, pn->pn_name, namelen) =3D=3D 0) { > + else if (strncmp(pname, pn->pn_name, namelen) =3D=3D 0 && > + pn->pn_name[namelen] =3D=3D '\0') { > pfs_unlock(pd); > goto got_pnode; > } Naive question: should this be an && conditional or an || conditional? If th= e 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>