Re: git: fc915f1be145 - main - pseudofs: Fix a potential out-of-bounds access in pfs_lookup()
- Reply: Mark Johnston : "Re: git: fc915f1be145 - main - pseudofs: Fix a potential out-of-bounds access in pfs_lookup()"
- In reply to: Mark Johnston : "git: fc915f1be145 - main - pseudofs: Fix a potential out-of-bounds access in pfs_lookup()"
- Go to: [ bottom of page ] [ top of archives ] [ this month ]
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