Date: Sun, 21 Jul 2019 15:07:12 +0000 (UTC) From: Konstantin Belousov <kib@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r350199 - in head/sys: kern sys Message-ID: <201907211507.x6LF7C7G053552@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
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) {
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201907211507.x6LF7C7G053552>