Author: kib
Date: Sun Jul 21 15:07:12 2019
New Revision: 350199
URL: https://svnweb.freebsd.org/changeset/base/350199
Log:
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
Modified:
head/sys/kern/kern_descrip.c
head/sys/kern/sys_generic.c
head/sys/kern/uipc_usrreq.c
head/sys/sys/file.h
head/sys/sys/refcount.h
Modified: head/sys/kern/kern_descrip.c
==============================================================================
--- head/sys/kern/kern_descrip.c Sun Jul 21 11:40:00 2019
(r350198)
+++ head/sys/kern/kern_descrip.c Sun Jul 21 15:07:12 2019
(r350199)
@@ -853,6 +853,10 @@ kern_dup(struct thread *td, u_int mode, int flags, int
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
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
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
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,
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,
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 *need
get_locked:
FILEDESC_SLOCK(fdp);
error = fget_cap_locked(fdp, fd, needrightsp, fpp, havecapsp);
- if (error == 0)
- fhold(*fpp);
+ if (error == 0 && !fhold(*fpp))
+ error = EBADF;
FILEDESC_SUNLOCK(fdp);
#endif
return (error);
@@ -2656,14 +2668,19 @@ fget_unlocked(struct filedesc *fdp, int fd, cap_rights
* 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
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);
Modified: head/sys/kern/sys_generic.c
==============================================================================
--- head/sys/kern/sys_generic.c Sun Jul 21 11:40:00 2019 (r350198)
+++ head/sys/kern/sys_generic.c Sun Jul 21 15:07:12 2019 (r350199)
@@ -757,7 +757,11 @@ kern_ioctl(struct thread *td, int fd, u_long com, cadd
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;
Modified: head/sys/kern/uipc_usrreq.c
==============================================================================
--- head/sys/kern/uipc_usrreq.c Sun Jul 21 11:40:00 2019 (r350198)
+++ head/sys/kern/uipc_usrreq.c Sun Jul 21 15:07:12 2019 (r350199)
@@ -2154,7 +2154,7 @@ unp_internalize(struct mbuf **controlp, struct thread
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
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."));
}
Modified: head/sys/sys/file.h
==============================================================================
--- head/sys/sys/file.h Sun Jul 21 11:40:00 2019 (r350198)
+++ head/sys/sys/file.h Sun Jul 21 15:07:12 2019 (r350199)
@@ -284,8 +284,12 @@ _fnoop(void)
return (0);
}
-#define fhold(fp)
\
- (refcount_acquire(&(fp)->f_count))
+static __inline __result_use_check bool
+fhold(struct file *fp)
+{
+ return (refcount_acquire_checked(&fp->f_count));
+}
+
#define fdrop(fp, td)
\
(refcount_release(&(fp)->f_count) ? _fdrop((fp), (td)) : _fnoop())
Modified: head/sys/sys/refcount.h
==============================================================================
--- head/sys/sys/refcount.h Sun Jul 21 11:40:00 2019 (r350198)
+++ head/sys/sys/refcount.h Sun Jul 21 15:07:12 2019 (r350199)
@@ -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)
{
_______________________________________________
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
