tarfs: Fix issues revealed by static analysis and testing.

* tarfs_alloc_mount(): Remove an unnecessary null check (CID 1504505) and an unused variable.

* tarfs_alloc_one(): Verify that the file size is not negative (CID 1504506).  While there, also validate the mode, owner and group.

* tarfs_vget(), tarfs_zio_init(): Explicitly ignore return value from getnewvnode(), which cannot fail (CID 1504508)

* tarfs_lookup_path(): Fix a case where a specially-crafted tarball could trigger a null pointer dereference by first descending into, and then backing out of, a previously unknown directory. (CID 1504515)

* mktar: Construct a tarball that triggers the aforementioned null pointer dereference.

Reported by:	Coverity
Sponsored by:	Juniper Networks, Inc.
Sponsored by:	Klara, Inc.
Reviewed by:	imp, kib
Differential Revision:	https://reviews.freebsd.org/D38463

sys/fs/tarfs/tarfs_io.c

@@ -630,7 +630,7 @@ tarfs_zio_init(struct tarfs_mount *tmp, off_t i, off_t o)
 	zio->idx[zio->curidx].o = zio->opos = o;
 	tmp->zio = zio;
 	TARFS_DPF(ALLOC, "%s: allocated zio index\n", __func__);
-	getnewvnode("tarfsz", tmp->vfs, &tarfs_znodeops, &zvp);
+	(void)getnewvnode("tarfsz", tmp->vfs, &tarfs_znodeops, &zvp);
 	zvp->v_data = zio;
 	zvp->v_type = VREG;
 	zvp->v_mount = tmp->vfs;

sys/fs/tarfs/tarfs_vfsops.c

@@ -289,7 +289,7 @@ tarfs_lookup_path(struct tarfs_mount *tmp, char *name, size_t namelen,
     char **endp, char **sepp, struct tarfs_node **retparent,
     struct tarfs_node **retnode, boolean_t create_dirs)
 {
-	struct componentname cn;
+	struct componentname cn = { };
 	struct tarfs_node *parent, *tnp;
 	char *sep;
 	size_t len;
@@ -304,8 +304,6 @@ tarfs_lookup_path(struct tarfs_mount *tmp, char *name, size_t namelen,
 	if (tnp == NULL)
 		panic("%s: root node not yet created", __func__);
 
-	bzero(&cn, sizeof(cn));
-
 	TARFS_DPF(LOOKUP, "%s: Full path: %.*s\n", __func__, (int)namelen,
 	    name);
 
@@ -329,24 +327,21 @@ tarfs_lookup_path(struct tarfs_mount *tmp, char *name, size_t namelen,
 			/* nothing */ ;
 
 		/* check for . and .. */
-		if (name[0] == '.' && len <= 2) {
-			if (len == 1) {
-				/* . */
-				name += len;
-				namelen -= len;
-				continue;
-			} else if (name[1] == '.') {
-				/* .. */
-				if (tnp == tmp->root) {
-					error = EINVAL;
-					break;
-				}
-				tnp = tnp->parent;
-				parent = tnp->parent;
-				name += len;
-				namelen -= len;
-				continue;
+		if (name[0] == '.' && len == 1) {
+			name += len;
+			namelen -= len;
+			continue;
+		}
+		if (name[0] == '.' && name[1] == '.' && len == 2) {
+			if (tnp == tmp->root) {
+				error = EINVAL;
+				break;
 			}
+			tnp = parent;
+			parent = tnp->parent;
+			name += len;
+			namelen -= len;
+			continue;
 		}
 
 		/* create parent if necessary */
@@ -441,6 +436,7 @@ tarfs_alloc_one(struct tarfs_mount *tmp, off_t *blknump)
 	struct sbuf *namebuf = NULL;
 	char *exthdr = NULL, *name = NULL, *link = NULL;
 	off_t blknum = *blknump;
+	int64_t num;
 	int endmarker = 0;
 	char *namep, *sep;
 	struct tarfs_node *parent, *tnp;
@@ -511,10 +507,41 @@ again:
 	}
 
 	/* get standard attributes */
-	mode = tarfs_str2int64(hdrp->mode, sizeof(hdrp->mode));
-	uid = tarfs_str2int64(hdrp->uid, sizeof(hdrp->uid));
-	gid = tarfs_str2int64(hdrp->gid, sizeof(hdrp->gid));
-	sz = tarfs_str2int64(hdrp->size, sizeof(hdrp->size));
+	num = tarfs_str2int64(hdrp->mode, sizeof(hdrp->mode));
+	if (num < 0 || num > ALLPERMS) {
+		TARFS_DPF(ALLOC, "%s: invalid file mode at %zu\n",
+		    __func__, TARFS_BLOCKSIZE * (blknum - 1));
+		mode = S_IRUSR;
+	} else {
+		mode = num;
+	}
+	num = tarfs_str2int64(hdrp->uid, sizeof(hdrp->uid));
+	if (num < 0 || num > UID_MAX) {
+		TARFS_DPF(ALLOC, "%s: UID out of range at %zu\n",
+		    __func__, TARFS_BLOCKSIZE * (blknum - 1));
+		uid = tmp->root->uid;
+		mode &= ~S_ISUID;
+	} else {
+		uid = num;
+	}
+	num = tarfs_str2int64(hdrp->gid, sizeof(hdrp->gid));
+	if (num < 0 || num > GID_MAX) {
+		TARFS_DPF(ALLOC, "%s: GID out of range at %zu\n",
+		    __func__, TARFS_BLOCKSIZE * (blknum - 1));
+		gid = tmp->root->gid;
+		mode &= ~S_ISGID;
+	} else {
+		gid = num;
+	}
+	num = tarfs_str2int64(hdrp->size, sizeof(hdrp->size));
+	if (num < 0) {
+		TARFS_DPF(ALLOC, "%s: negative size at %zu\n",
+		    __func__, TARFS_BLOCKSIZE * (blknum - 1));
+		error = EINVAL;
+		goto bad;
+	} else {
+		sz = num;
+	}
 	mtime = tarfs_str2int64(hdrp->mtime, sizeof(hdrp->mtime));
 	rdev = NODEV;
 	TARFS_DPF(ALLOC, "%s: [%c] %zu @%jd %o %d:%d\n", __func__,
@@ -777,7 +804,6 @@ tarfs_alloc_mount(struct mount *mp, struct vnode *vp,
 {
 	struct vattr va;
 	struct thread *td = curthread;
-	char *fullpath;
 	struct tarfs_mount *tmp;
 	struct tarfs_node *root;
 	off_t blknum;
@@ -788,7 +814,6 @@ tarfs_alloc_mount(struct mount *mp, struct vnode *vp,
 	ASSERT_VOP_LOCKED(vp, __func__);
 
 	tmp = NULL;
-	fullpath = NULL;
 
 	TARFS_DPF(ALLOC, "%s: Allocating tarfs mount structure for vp %p\n",
 	    __func__, vp);
@@ -802,8 +827,7 @@ tarfs_alloc_mount(struct mount *mp, struct vnode *vp,
 	mtime = va.va_mtime.tv_sec;
 
 	/* Allocate and initialize tarfs mount structure */
-	tmp = (struct tarfs_mount *)malloc(sizeof(struct tarfs_mount),
-	    M_TARFSMNT, M_WAITOK | M_ZERO);
+	tmp = malloc(sizeof(*tmp), M_TARFSMNT, M_WAITOK | M_ZERO);
 	TARFS_DPF(ALLOC, "%s: Allocated mount structure\n", __func__);
 	mp->mnt_data = tmp;
 
@@ -848,9 +872,7 @@ tarfs_alloc_mount(struct mount *mp, struct vnode *vp,
 	return (0);
 
 bad:
-	if (tmp != NULL)
-		tarfs_free_mount(tmp);
-	free(fullpath, M_TEMP);
+	tarfs_free_mount(tmp);
 	return (error);
 }
 
@@ -1104,9 +1126,7 @@ tarfs_vget(struct mount *mp, ino_t ino, int lkflags, struct vnode **vpp)
 	if (tnp == NULL)
 		return (ENOENT);
 
-	error = getnewvnode("tarfs", mp, &tarfs_vnodeops, &vp);
-	if (error != 0)
-		goto bad;
+	(void)getnewvnode("tarfs", mp, &tarfs_vnodeops, &vp);
 	TARFS_DPF(FS, "%s: allocated vnode\n", __func__);
 	vp->v_data = tnp;
 	vp->v_type = tnp->type;

sys/fs/tarfs/tarfs_vnops.c

@@ -250,7 +250,7 @@ tarfs_lookup(struct vop_cachedlookup_args *ap)
 static int
 tarfs_readdir(struct vop_readdir_args *ap)
 {
-	struct dirent cde;
+	struct dirent cde = { };
 	struct tarfs_node *current, *tnp;
 	struct vnode *vp;
 	struct uio *uio;

tests/sys/fs/tarfs/mktar.c

@@ -41,6 +41,8 @@
 #define PROGNAME	"mktar"
 
 #define SUBDIRNAME	"directory"
+#define EMPTYDIRNAME	"empty"
+#define NORMALFILENAME	"file"
 #define SPARSEFILENAME	"sparse_file"
 #define HARDLINKNAME	"hard_link"
 #define SHORTLINKNAME	"short_link"
@@ -61,6 +63,25 @@ static void verbose(const char *fmt, ...)
 	fprintf(stderr, "\n");
 }
 
+static void
+mknormalfile(const char *filename, mode_t mode)
+{
+	char buf[512];
+	ssize_t res;
+	int fd;
+
+	if ((fd = open(filename, O_RDWR|O_CREAT|O_EXCL, mode)) < 0)
+		err(1, "%s", filename);
+	for (unsigned int i = 0; i < sizeof(buf); i++)
+		buf[i] = 32 + i % 64;
+	res = write(fd, buf, sizeof(buf));
+	if (res < 0)
+		err(1, "%s", filename);
+	if (res != sizeof(buf))
+		errx(1, "%s: short write", filename);
+	close(fd);
+}
+
 static void
 mksparsefile(const char *filename, mode_t mode)
 {
@@ -68,7 +89,7 @@ mksparsefile(const char *filename, mode_t mode)
 	ssize_t res;
 	int fd;
 
-	if ((fd = open(filename, O_RDWR|O_CREAT|O_TRUNC, mode)) < 0)
+	if ((fd = open(filename, O_RDWR|O_CREAT|O_EXCL, mode)) < 0)
 		err(1, "%s", filename);
 	for (unsigned int i = 33; i <= 126; i++) {
 		memset(buf, i, sizeof(buf));
@@ -106,6 +127,15 @@ mktar(void)
 	if (mkdir(SUBDIRNAME, 0755) != 0)
 		err(1, "%s", SUBDIRNAME);
 
+	/* create a second subdirectory which will remain empty */
+	verbose("mkdir %s", EMPTYDIRNAME);
+	if (mkdir(EMPTYDIRNAME, 0755) != 0)
+		err(1, "%s", EMPTYDIRNAME);
+
+	/* create a normal file */
+	verbose("creating %s", NORMALFILENAME);
+	mknormalfile(NORMALFILENAME, 0644);
+
 	/* create a sparse file */
 	verbose("creating %s", SPARSEFILENAME);
 	mksparsefile(SPARSEFILENAME, 0644);
@@ -198,7 +228,12 @@ main(int argc, char *argv[])
 #if 0
 		    "--options", "zstd:frame-per-file",
 #endif
-		    ".",
+		    "./" EMPTYDIRNAME "/../" NORMALFILENAME,
+		    "./" SPARSEFILENAME,
+		    "./" HARDLINKNAME,
+		    "./" SHORTLINKNAME,
+		    "./" SUBDIRNAME,
+		    "./" LONGLINKNAME,
 		    NULL);
 		err(1, "execlp()");
 	}
@@ -222,7 +257,9 @@ main(int argc, char *argv[])
 		(void)unlink(HARDLINKNAME);
 		verbose("rm %s", SPARSEFILENAME);
 		(void)unlink(SPARSEFILENAME);
-		verbose("rm %s", SUBDIRNAME);
+		verbose("rmdir %s", EMPTYDIRNAME);
+		(void)rmdir(EMPTYDIRNAME);
+		verbose("rmdir %s", SUBDIRNAME);
 		(void)rmdir(SUBDIRNAME);
 		verbose("cd -");
 		exit(0);