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

From: Enji Cooper <yaneurabeya_at_gmail.com>
Date: Fri, 23 Jun 2023 15:38:44 UTC
> 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