Date: Wed, 6 Apr 2022 23:26:31 GMT From: Mateusz Guzik <mjg@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-branches@FreeBSD.org Subject: git: fffe016c8155 - releng/13.1 - vfs: fix memory leak on lookup with fds with ioctl caps Message-ID: <202204062326.236NQVMf081246@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
The branch releng/13.1 has been updated by mjg: URL: https://cgit.FreeBSD.org/src/commit/?id=fffe016c8155805d1bd957a73a612f2c5e6ceac0 commit fffe016c8155805d1bd957a73a612f2c5e6ceac0 Author: Mateusz Guzik <mjg@FreeBSD.org> AuthorDate: 2022-03-24 20:51:03 +0000 Commit: Mateusz Guzik <mjg@FreeBSD.org> CommitDate: 2022-04-06 23:25:50 +0000 vfs: fix memory leak on lookup with fds with ioctl caps Reviewed by: markj PR: 262515 Noted by: firk@cantconnect.ru Differential Revision: https://reviews.freebsd.org/D34667 Approved by: re (gjb) (cherry picked from commit 0c805718cbd3709e3ffc1a0d41612168c8242360) (cherry picked from commit 838d8e6fb60e12e610701ae10be717309f3ea935) --- sys/kern/kern_descrip.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++- sys/kern/vfs_cache.c | 3 +- sys/kern/vfs_lookup.c | 50 ++------------------------------- sys/kern/vfs_syscalls.c | 8 +++--- sys/sys/file.h | 1 + sys/sys/namei.h | 7 ++++- 6 files changed, 89 insertions(+), 55 deletions(-) diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c index 03498d5a9ce9..88a91b9e6fcf 100644 --- a/sys/kern/kern_descrip.c +++ b/sys/kern/kern_descrip.c @@ -1799,11 +1799,19 @@ filecaps_fill(struct filecaps *fcaps) /* * Free memory allocated within filecaps structure. */ +static void +filecaps_free_ioctl(struct filecaps *fcaps) +{ + + free(fcaps->fc_ioctls, M_FILECAPS); + fcaps->fc_ioctls = NULL; +} + void filecaps_free(struct filecaps *fcaps) { - free(fcaps->fc_ioctls, M_FILECAPS); + filecaps_free_ioctl(fcaps); bzero(fcaps, sizeof(*fcaps)); } @@ -3140,6 +3148,71 @@ fgetvp_lookup_smr(int fd, struct nameidata *ndp, struct vnode **vpp, bool *fsear } #endif +int +fgetvp_lookup(int fd, struct nameidata *ndp, struct vnode **vpp) +{ + struct thread *td; + struct file *fp; + struct vnode *vp; + struct componentname *cnp; + cap_rights_t rights; + int error; + + td = curthread; + rights = *ndp->ni_rightsneeded; + cap_rights_set_one(&rights, CAP_LOOKUP); + cnp = &ndp->ni_cnd; + + error = fget_cap(td, ndp->ni_dirfd, &rights, &fp, &ndp->ni_filecaps); + if (__predict_false(error != 0)) + return (error); + if (__predict_false(fp->f_ops == &badfileops)) { + error = EBADF; + goto out_free; + } + vp = fp->f_vnode; + if (__predict_false(vp == NULL)) { + error = ENOTDIR; + goto out_free; + } + vref(vp); + /* + * XXX does not check for VDIR, handled by namei_setup + */ + if ((fp->f_flag & FSEARCH) != 0) + cnp->cn_flags |= NOEXECCHECK; + fdrop(fp, td); + +#ifdef CAPABILITIES + /* + * If file descriptor doesn't have all rights, + * all lookups relative to it must also be + * strictly relative. + */ + CAP_ALL(&rights); + if (!cap_rights_contains(&ndp->ni_filecaps.fc_rights, &rights) || + ndp->ni_filecaps.fc_fcntls != CAP_FCNTL_ALL || + ndp->ni_filecaps.fc_nioctls != -1) { + ndp->ni_lcf |= NI_LCF_STRICTRELATIVE; + ndp->ni_resflags |= NIRES_STRICTREL; + } +#endif + + /* + * TODO: avoid copying ioctl caps if it can be helped to begin with + */ + if ((cnp->cn_flags & WANTIOCTLCAPS) == 0) + filecaps_free_ioctl(&ndp->ni_filecaps); + + *vpp = vp; + return (0); + +out_free: + filecaps_free(&ndp->ni_filecaps); + fdrop(fp, td); + return (error); +} + static int fget_unlocked_seq(struct filedesc *fdp, int fd, cap_rights_t *needrightsp, struct file **fpp, seqc_t *seqp) diff --git a/sys/kern/vfs_cache.c b/sys/kern/vfs_cache.c index 52ed0756db25..1847b070c426 100644 --- a/sys/kern/vfs_cache.c +++ b/sys/kern/vfs_cache.c @@ -4178,7 +4178,8 @@ cache_fpl_terminated(struct cache_fpl *fpl) #define CACHE_FPL_SUPPORTED_CN_FLAGS \ (NC_NOMAKEENTRY | NC_KEEPPOSENTRY | LOCKLEAF | LOCKPARENT | WANTPARENT | \ FAILIFEXISTS | FOLLOW | EMPTYPATH | LOCKSHARED | SAVENAME | SAVESTART | \ - WILLBEDIR | ISOPEN | NOMACCHECK | AUDITVNODE1 | AUDITVNODE2 | NOCAPCHECK) + WILLBEDIR | ISOPEN | NOMACCHECK | AUDITVNODE1 | AUDITVNODE2 | NOCAPCHECK | \ + WANTIOCTLCAPS) #define CACHE_FPL_INTERNAL_CN_FLAGS \ (ISDOTDOT | MAKEENTRY | ISLASTCN) diff --git a/sys/kern/vfs_lookup.c b/sys/kern/vfs_lookup.c index 95c5599ab232..7fee9d2c488f 100644 --- a/sys/kern/vfs_lookup.c +++ b/sys/kern/vfs_lookup.c @@ -288,10 +288,8 @@ static int namei_setup(struct nameidata *ndp, struct vnode **dpp, struct pwd **pwdp) { struct componentname *cnp; - struct file *dfp; struct thread *td; struct pwd *pwd; - cap_rights_t rights; int error; bool startdir_used; @@ -352,56 +350,12 @@ namei_setup(struct nameidata *ndp, struct vnode **dpp, struct pwd **pwdp) *dpp = pwd->pwd_cdir; vrefact(*dpp); } else { - rights = *ndp->ni_rightsneeded; - cap_rights_set_one(&rights, CAP_LOOKUP); - if (cnp->cn_flags & AUDITVNODE1) AUDIT_ARG_ATFD1(ndp->ni_dirfd); if (cnp->cn_flags & AUDITVNODE2) AUDIT_ARG_ATFD2(ndp->ni_dirfd); - /* - * Effectively inlined fgetvp_rights, because - * we need to inspect the file as well as - * grabbing the vnode. No check for O_PATH, - * files to implement its semantic. - */ - error = fget_cap(td, ndp->ni_dirfd, &rights, - &dfp, &ndp->ni_filecaps); - if (error != 0) { - /* - * Preserve the error; it should either be EBADF - * or capability-related, both of which can be - * safely returned to the caller. - */ - } else { - if (dfp->f_ops == &badfileops) { - error = EBADF; - } else if (dfp->f_vnode == NULL) { - error = ENOTDIR; - } else { - *dpp = dfp->f_vnode; - vref(*dpp); - - if ((dfp->f_flag & FSEARCH) != 0) - cnp->cn_flags |= NOEXECCHECK; - } - fdrop(dfp, td); - } -#ifdef CAPABILITIES - /* - * If file descriptor doesn't have all rights, - * all lookups relative to it must also be - * strictly relative. - */ - CAP_ALL(&rights); - if (!cap_rights_contains(&ndp->ni_filecaps.fc_rights, - &rights) || - ndp->ni_filecaps.fc_fcntls != CAP_FCNTL_ALL || - ndp->ni_filecaps.fc_nioctls != -1) { - ndp->ni_lcf |= NI_LCF_STRICTRELATIVE; - ndp->ni_resflags |= NIRES_STRICTREL; - } -#endif + + error = fgetvp_lookup(ndp->ni_dirfd, ndp, dpp); } if (error == 0 && (*dpp)->v_type != VDIR && (cnp->cn_pnbuf[0] != '\0' || diff --git a/sys/kern/vfs_syscalls.c b/sys/kern/vfs_syscalls.c index 19a32a175895..ed75316f8add 100644 --- a/sys/kern/vfs_syscalls.c +++ b/sys/kern/vfs_syscalls.c @@ -1153,8 +1153,8 @@ kern_openat(struct thread *td, int fd, const char *path, enum uio_seg pathseg, /* Set the flags early so the finit in devfs can pick them up. */ fp->f_flag = flags & FMASK; cmode = ((mode & ~pdp->pd_cmask) & ALLPERMS) & ~S_ISTXT; - NDINIT_ATRIGHTS(&nd, LOOKUP, FOLLOW | AUDITVNODE1, pathseg, path, fd, - &rights, td); + NDINIT_ATRIGHTS(&nd, LOOKUP, FOLLOW | AUDITVNODE1 | WANTIOCTLCAPS, + pathseg, path, fd, &rights, td); td->td_dupfd = -1; /* XXX check for fdopen */ error = vn_open(&nd, &flags, cmode, fp); if (error != 0) { @@ -1236,11 +1236,10 @@ success: error = finstall_refed(td, fp, &indx, flags, fcaps); /* On success finstall_refed() consumes fcaps. */ if (error != 0) { - filecaps_free(&nd.ni_filecaps); goto bad; } } else { - filecaps_free(&nd.ni_filecaps); + NDFREE_IOCTLCAPS(&nd); falloc_abort(td, fp); } @@ -1248,6 +1247,7 @@ success: return (0); bad: KASSERT(indx == -1, ("indx=%d, should be -1", indx)); + NDFREE_IOCTLCAPS(&nd); falloc_abort(td, fp); return (error); } diff --git a/sys/sys/file.h b/sys/sys/file.h index c97841d1a108..66b50c418953 100644 --- a/sys/sys/file.h +++ b/sys/sys/file.h @@ -287,6 +287,7 @@ int fgetvp_read(struct thread *td, int fd, cap_rights_t *rightsp, int fgetvp_write(struct thread *td, int fd, cap_rights_t *rightsp, struct vnode **vpp); int fgetvp_lookup_smr(int fd, struct nameidata *ndp, struct vnode **vpp, bool *fsearch); +int fgetvp_lookup(int fd, struct nameidata *ndp, struct vnode **vpp); static __inline __result_use_check bool fhold(struct file *fp) diff --git a/sys/sys/namei.h b/sys/sys/namei.h index 9e0a82ea1659..d22864a3c2c8 100644 --- a/sys/sys/namei.h +++ b/sys/sys/namei.h @@ -185,7 +185,7 @@ int cache_fplookup(struct nameidata *ndp, enum cache_fpl_status *status, #define NOCAPCHECK 0x00100000 /* do not perform capability checks */ /* UNUSED 0x00200000 */ /* UNUSED 0x00400000 */ -/* UNUSED 0x00800000 */ +#define WANTIOCTLCAPS 0x00800000 /* leave ioctl caps for the caller */ #define HASBUF 0x01000000 /* has allocated pathname buffer */ #define NOEXECCHECK 0x02000000 /* do not perform exec check on dir */ #define MAKEENTRY 0x04000000 /* entry is to be added to name cache */ @@ -269,6 +269,7 @@ do { \ #define NDREINIT(ndp) do { \ struct nameidata *_ndp = (ndp); \ NDREINIT_DBG(_ndp); \ + filecaps_free(&_ndp->ni_filecaps); \ _ndp->ni_resflags = 0; \ _ndp->ni_startdir = NULL; \ } while (0) @@ -288,6 +289,10 @@ do { \ #define NDF_NO_FREE_PNBUF 0x00000020 #define NDF_ONLY_PNBUF (~NDF_NO_FREE_PNBUF) +#define NDFREE_IOCTLCAPS(ndp) do { \ + struct nameidata *_ndp = (ndp); \ + filecaps_free(&_ndp->ni_filecaps); \ +} while (0) void NDFREE_PNBUF(struct nameidata *); void NDFREE(struct nameidata *, const u_int); #define NDFREE(ndp, flags) do { \
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202204062326.236NQVMf081246>