Re: git: fc915f1be145 - main - pseudofs: Fix a potential out-of-bounds access in pfs_lookup()

On Fri, Jun 23, 2023 at 08:38:44AM -0700, Enji Cooper wrote:
> 
> > 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?

It should be &&.  Using || here would reintroduce the original bug.
If strncmp(pname, pn->pn_name, namelen) == 0, then
strlen(pn->pn_name) >= namelen, and pn->pn_name is nul-terminated, so it
is safe to check pn->pn_name[namelen] == '\0'.

> If the former, could this be simplified by using a direct NUL char equality check instead of using strncmp?

I'm not sure what you mean by this.  This code is simply checking
whether pname and pn->pn_name are the same string, without assuming that
pname is nul-terminated.