Skip site navigation (1)Skip section navigation (2) 
Date:      Fri, 30 Jun 2017 17:05:56 -0700
From:      Mark Millard <markmi@dsl-only.net>
To:        freebsd-hackers@freebsd.org, FreeBSD PowerPC ML <freebsd-ppc@freebsd.org>
Subject:   head -r320482 (e.g.): DNINIT_ALL and SOCK_MAXADDRLEN vs. SOCK_MAXADDRLEN+1 use in char buf[<?>] declarations: is the usage all correct?
Message-ID:  <3390B896-622E-4548-B483-5772AB3ABD0C@dsl-only.net>
next in thread | raw e-mail | index | archive | help 
While trying to track down a repeatable crash
for powerpc production kernel's (but not
debug ones) I ran into the following. (I'm
not familiar with the material involved.)

NDINIT_ALL in /usr/src/sys/kern/vfs_lookup.c
records its char* namep argument in ndp->ni_dirp :

/usr/src/sys/kern/uipc_usrreq.c:
        char buf[SOCK_MAXADDRLEN];
. . .
        NDINIT_ATRIGHTS(&nd, LOOKUP, FOLLOW | LOCKSHARED | LOCKLEAF,
            UIO_SYSSPACE, buf, fd, cap_rights_init(&rights, =
CAP_CONNECTAT), td);
. . .
(elsewhere:)
void
NDINIT_ALL(struct nameidata *ndp, u_long op, u_long flags, enum uio_seg =
segflg,
    const char *namep, int dirfd, struct vnode *startdir, cap_rights_t =
*rightsp,
    struct thread *td)
{

        ndp->ni_cnd.cn_nameiop =3D op;
        ndp->ni_cnd.cn_flags =3D flags;
        ndp->ni_segflg =3D segflg;
        ndp->ni_dirp =3D namep;
        ndp->ni_dirfd =3D dirfd;
        ndp->ni_startdir =3D startdir;
        if (rightsp !=3D NULL)
                ndp->ni_rightsneeded =3D *rightsp;
        else
                cap_rights_init(&ndp->ni_rightsneeded);
        filecaps_init(&ndp->ni_filecaps);
        ndp->ni_cnd.cn_thread =3D td;
}

But the size of the buffer supplied in other calls varies
from the size used for this call:

/usr/src/sys/fs/nfsclient/nfs_clvfsops.c:                       if =
(args.addrlen > SOCK_MAXADDRLEN) {
/usr/src/sys/kern/uipc_syscalls.c:      if (len > SOCK_MAXADDRLEN)
/usr/src/sys/kern/uipc_usrreq.c:        char buf[SOCK_MAXADDRLEN];
/usr/src/sys/netgraph/ng_ksocket.c:             if (pathlen > =
SOCK_MAXADDRLEN) {
/usr/src/sys/netgraph/ng_ksocket.c:             char =
pathbuf[SOCK_MAXADDRLEN + 1];
/usr/src/sys/sys/socket.h:#define       SOCK_MAXADDRLEN 255             =
/* longest possible addresses */

Some of the other usage of SOCK_MAXADDRLEN is:

                if (vfs_getopt(mp->mnt_optnew, "addr",
                    (void **)&args.addr, &args.addrlen) =3D=3D 0) {
                        if (args.addrlen > SOCK_MAXADDRLEN) {
                                error =3D ENAMETOOLONG;
                                goto out;
                        }
                        nam =3D malloc(args.addrlen, M_SONAME, =
M_WAITOK);
                        bcopy(args.addr, nam, args.addrlen);
                        nam->sa_len =3D args.addrlen;
. . .
        if (len > SOCK_MAXADDRLEN)
                return (ENAMETOOLONG);
        if (len < offsetof(struct sockaddr, sa_data[0]))
                return (EINVAL);
        sa =3D malloc(len, M_SONAME, M_WAITOK);
. . .
                if ((path =3D ng_get_string_token(s, off, &toklen, =
NULL)) =3D=3D NULL)
                        return (EINVAL);
                pathlen =3D strlen(path);
                if (pathlen > SOCK_MAXADDRLEN) {
                        free(path, M_NETGRAPH_KSOCKET);
                        return (E2BIG);
                }
                if (*buflen < pathoff + pathlen) {
                        free(path, M_NETGRAPH_KSOCKET);
                        return (ERANGE);
                }
                *off +=3D toklen;
                bcopy(path, sun->sun_path, pathlen);

That last suggests that pathlen does not include a '\0'
termination character position but that path is supposed
to be '\0' terminated (in order to justify strlen(.) use)
yet the later bcopy avoids the '\0' position for
sun->sun_path.

This makes it unclear which of the following is
more appropriate vs. if each is correct for its
specific usage:

/usr/src/sys/kern/uipc_usrreq.c:
. . .
        char buf[SOCK_MAXADDRLEN];
. . .
        NDINIT_ATRIGHTS(&nd, LOOKUP, FOLLOW | LOCKSHARED | LOCKLEAF,
            UIO_SYSSPACE, buf, fd, cap_rights_init(&rights, =
CAP_CONNECTAT), td);

(The above buf has an odd number of bytes.)

(The above code is in the execution sequence that leads
to the powerpc fatal trap.)

/usr/src/sys/netgraph/ng_ksocket.c:
. . .
        case PF_LOCAL:
            {
                const int pathoff =3D OFFSETOF(struct sockaddr_un, =
sun_path);
                const struct sockaddr_un *sun =3D (const struct =
sockaddr_un *)sa;
                const int pathlen =3D sun->sun_len - pathoff;
                char pathbuf[SOCK_MAXADDRLEN + 1];
                char *pathtoken;

                bcopy(sun->sun_path, pathbuf, pathlen);
                if ((pathtoken =3D ng_encode_string(pathbuf, pathlen)) =
=3D=3D NULL)
                        return (ENOMEM);
                slen +=3D snprintf(cbuf, cbuflen, "local/%s", =
pathtoken);
                free(pathtoken, M_NETGRAPH_KSOCKET);
                if (slen >=3D cbuflen)
                        return (ERANGE);
                *off +=3D sun->sun_len;
                return (0);
            }


=3D=3D=3D
Mark Millard
markmi at dsl-only.net

Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?3390B896-622E-4548-B483-5772AB3ABD0C>