Check and avoid overflow when incrementing fp->f_count in

fget_unlocked() and fhold().

On sufficiently large machine, f_count can be legitimately very large,
e.g. malicious code can dup same fd up to the per-process
filedescriptors limit, and then fork as much as it can.
On some smaller machine, I see
	kern.maxfilesperproc: 939132
	kern.maxprocperuid: 34203
which already overflows u_int.  More, the malicious code can create
transient references by sending fds over unix sockets.

I realized that this check is missed after reading
https://secfault-security.com/blog/FreeBSD-SA-1902.fd.html

Reviewed by:	markj (previous version), mjg
Tested by:	pho (previous version)
Sponsored by:	The FreeBSD Foundation
MFC after:	1 week
Differential revision:	https://reviews.freebsd.org/D20947

sys/kern/kern_descrip.c

@@ -853,6 +853,10 @@ kern_dup(struct thread *td, u_int mode, int flags, int old, int new)
 		goto unlock;
 	}
 
+	oldfde = &fdp->fd_ofiles[old];
+	if (!fhold(oldfde->fde_file))
+		goto unlock;
+
 	/*
 	 * If the caller specified a file descriptor, make sure the file
 	 * table is large enough to hold it, and grab it.  Otherwise, just
@@ -861,13 +865,17 @@ kern_dup(struct thread *td, u_int mode, int flags, int old, int new)
 	switch (mode) {
 	case FDDUP_NORMAL:
 	case FDDUP_FCNTL:
-		if ((error = fdalloc(td, new, &new)) != 0)
+		if ((error = fdalloc(td, new, &new)) != 0) {
+			fdrop(oldfde->fde_file, td);
 			goto unlock;
+		}
 		break;
 	case FDDUP_MUSTREPLACE:
 		/* Target file descriptor must exist. */
-		if (fget_locked(fdp, new) == NULL)
+		if (fget_locked(fdp, new) == NULL) {
+			fdrop(oldfde->fde_file, td);
 			goto unlock;
+		}
 		break;
 	case FDDUP_FIXED:
 		if (new >= fdp->fd_nfiles) {
@@ -884,6 +892,7 @@ kern_dup(struct thread *td, u_int mode, int flags, int old, int new)
 				error = racct_set_unlocked(p, RACCT_NOFILE, new + 1);
 				if (error != 0) {
 					error = EMFILE;
+					fdrop(oldfde->fde_file, td);
 					goto unlock;
 				}
 			}
@@ -899,8 +908,6 @@ kern_dup(struct thread *td, u_int mode, int flags, int old, int new)
 
 	KASSERT(old != new, ("new fd is same as old"));
 
-	oldfde = &fdp->fd_ofiles[old];
-	fhold(oldfde->fde_file);
 	newfde = &fdp->fd_ofiles[new];
 	delfp = newfde->fde_file;
 
@@ -1901,12 +1908,14 @@ finstall(struct thread *td, struct file *fp, int *fd, int flags,
 
 	MPASS(fd != NULL);
 
+	if (!fhold(fp))
+		return (EBADF);
 	FILEDESC_XLOCK(fdp);
 	if ((error = fdalloc(td, 0, fd))) {
 		FILEDESC_XUNLOCK(fdp);
+		fdrop(fp, td);
 		return (error);
 	}
-	fhold(fp);
 	_finstall(fdp, fp, *fd, flags, fcaps);
 	FILEDESC_XUNLOCK(fdp);
 	return (0);
@@ -2047,7 +2056,8 @@ fdcopy(struct filedesc *fdp)
 	for (i = 0; i <= fdp->fd_lastfile; ++i) {
 		ofde = &fdp->fd_ofiles[i];
 		if (ofde->fde_file == NULL ||
-		    (ofde->fde_file->f_ops->fo_flags & DFLAG_PASSABLE) == 0) {
+		    (ofde->fde_file->f_ops->fo_flags & DFLAG_PASSABLE) == 0 ||
+		    !fhold(ofde->fde_file)) {
 			if (newfdp->fd_freefile == -1)
 				newfdp->fd_freefile = i;
 			continue;
@@ -2055,7 +2065,6 @@ fdcopy(struct filedesc *fdp)
 		nfde = &newfdp->fd_ofiles[i];
 		*nfde = *ofde;
 		filecaps_copy(&ofde->fde_caps, &nfde->fde_caps, true);
-		fhold(nfde->fde_file);
 		fdused_init(newfdp, i);
 		newfdp->fd_lastfile = i;
 	}
@@ -2108,10 +2117,13 @@ fdcopy_remapped(struct filedesc *fdp, const int *fds, size_t nfds,
 			error = EINVAL;
 			goto bad;
 		}
+		if (!fhold(nfde->fde_file)) {
+			error = EBADF;
+			goto bad;
+		}
 		nfde = &newfdp->fd_ofiles[i];
 		*nfde = *ofde;
 		filecaps_copy(&ofde->fde_caps, &nfde->fde_caps, true);
-		fhold(nfde->fde_file);
 		fdused_init(newfdp, i);
 		newfdp->fd_lastfile = i;
 	}
@@ -2153,9 +2165,9 @@ fdclearlocks(struct thread *td)
 	    (p->p_leader->p_flag & P_ADVLOCK) != 0) {
 		for (i = 0; i <= fdp->fd_lastfile; i++) {
 			fp = fdp->fd_ofiles[i].fde_file;
-			if (fp == NULL || fp->f_type != DTYPE_VNODE)
+			if (fp == NULL || fp->f_type != DTYPE_VNODE ||
+			    !fhold(fp))
 				continue;
-			fhold(fp);
 			FILEDESC_XUNLOCK(fdp);
 			lf.l_whence = SEEK_SET;
 			lf.l_start = 0;
@@ -2596,8 +2608,8 @@ fget_cap(struct thread *td, int fd, cap_rights_t *needrightsp,
 get_locked:
 	FILEDESC_SLOCK(fdp);
 	error = fget_cap_locked(fdp, fd, needrightsp, fpp, havecapsp);
-	if (error == 0)
+	if (error == 0 && !fhold(*fpp))
-		fhold(*fpp);
+		error = EBADF;
 	FILEDESC_SUNLOCK(fdp);
 #endif
 	return (error);
@@ -2656,14 +2668,19 @@ fget_unlocked(struct filedesc *fdp, int fd, cap_rights_t *needrightsp,
 			 * table before this fd was closed, so it possible that
 			 * there is a stale fp pointer in cached version.
 			 */
-			fdt = *(const struct fdescenttbl * const volatile *)&(fdp->fd_files);
+			fdt = *(const struct fdescenttbl * const volatile *)
+			    &(fdp->fd_files);
 			continue;
 		}
+		if (__predict_false(count + 1 < count))
+			return (EBADF);
+
 		/*
 		 * Use an acquire barrier to force re-reading of fdt so it is
 		 * refreshed for verification.
 		 */
-		if (atomic_fcmpset_acq_int(&fp->f_count, &count, count + 1) == 0)
+		if (__predict_false(atomic_fcmpset_acq_int(&fp->f_count,
+		    &count, count + 1) == 0))
 			goto retry;
 		fdt = fdp->fd_files;
 #ifdef	CAPABILITIES
@@ -3048,7 +3065,11 @@ dupfdopen(struct thread *td, struct filedesc *fdp, int dfd, int mode,
 			FILEDESC_XUNLOCK(fdp);
 			return (EACCES);
 		}
-		fhold(fp);
+		if (!fhold(fp)) {
+			fdunused(fdp, indx);
+			FILEDESC_XUNLOCK(fdp);
+			return (EBADF);
+		}
 		newfde = &fdp->fd_ofiles[indx];
 		oldfde = &fdp->fd_ofiles[dfd];
 		ioctls = filecaps_copy_prep(&oldfde->fde_caps);

sys/kern/sys_generic.c

@@ -757,7 +757,11 @@ kern_ioctl(struct thread *td, int fd, u_long com, caddr_t data)
 		fp = NULL;	/* fhold() was not called yet */
 		goto out;
 	}
-	fhold(fp);
+	if (!fhold(fp)) {
+		error = EBADF;
+		fp = NULL;
+		goto out;
+	}
 	if (locked == LA_SLOCKED) {
 		FILEDESC_SUNLOCK(fdp);
 		locked = LA_UNLOCKED;

sys/kern/uipc_usrreq.c

@@ -2154,7 +2154,7 @@ unp_internalize(struct mbuf **controlp, struct thread *td)
 	struct timespec *ts;
 	void *data;
 	socklen_t clen, datalen;
-	int i, error, *fdp, oldfds;
+	int i, j, error, *fdp, oldfds;
 	u_int newlen;
 
 	UNP_LINK_UNLOCK_ASSERT();
@@ -2237,6 +2237,19 @@ unp_internalize(struct mbuf **controlp, struct thread *td)
 				goto out;
 			}
 			fdp = data;
+			for (i = 0; i < oldfds; i++, fdp++) {
+				if (!fhold(fdesc->fd_ofiles[*fdp].fde_file)) {
+					fdp = data;
+					for (j = 0; j < i; j++, fdp++) {
+						fdrop(fdesc->fd_ofiles[*fdp].
+						    fde_file, td);
+					}
+					FILEDESC_SUNLOCK(fdesc);
+					error = EBADF;
+					goto out;
+				}
+			}
+			fdp = data;
 			fdep = (struct filedescent **)
 			    CMSG_DATA(mtod(*controlp, struct cmsghdr *));
 			fdev = malloc(sizeof(*fdev) * oldfds, M_FILECAPS,
@@ -2440,7 +2453,6 @@ unp_internalize_fp(struct file *fp)
 		unp->unp_file = fp;
 		unp->unp_msgcount++;
 	}
-	fhold(fp);
 	unp_rights++;
 	UNP_LINK_WUNLOCK();
 }
@@ -2601,10 +2613,10 @@ unp_gc(__unused void *arg, int pending)
 			if ((unp->unp_gcflag & UNPGC_DEAD) != 0) {
 				f = unp->unp_file;
 				if (unp->unp_msgcount == 0 || f == NULL ||
-				    f->f_count != unp->unp_msgcount)
+				    f->f_count != unp->unp_msgcount ||
+				    !fhold(f))
 					continue;
 				unref[total++] = f;
-				fhold(f);
 				KASSERT(total <= unp_unreachable,
 				    ("unp_gc: incorrect unreachable count."));
 			}

sys/sys/file.h

@@ -284,8 +284,12 @@ _fnoop(void)
 	return (0);
 }
 
-#define	fhold(fp)							\
+static __inline __result_use_check bool
-	(refcount_acquire(&(fp)->f_count))
+fhold(struct file *fp)
+{
+	return (refcount_acquire_checked(&fp->f_count));
+}
+
 #define	fdrop(fp, td)							\
 	(refcount_release(&(fp)->f_count) ? _fdrop((fp), (td)) : _fnoop())
 

sys/sys/refcount.h

@@ -54,6 +54,20 @@ refcount_acquire(volatile u_int *count)
 	atomic_add_int(count, 1);
 }
 
+static __inline __result_use_check bool
+refcount_acquire_checked(volatile u_int *count)
+{
+	u_int lcount;
+
+	for (lcount = *count;;) {
+		if (__predict_false(lcount + 1 < lcount))
+			return (false);
+		if (__predict_true(atomic_fcmpset_int(count, &lcount,
+		    lcount + 1) == 1))
+			return (true);
+	}
+}
+
 static __inline int
 refcount_release(volatile u_int *count)
 {