The branch main has been updated by mjg: URL: https://cgit.FreeBSD.org/src/commit/?id=0c805718cbd3709e3ffc1a0d41612168c8242360
commit 0c805718cbd3709e3ffc1a0d41612168c8242360 Author: Mateusz Guzik <[email protected]> AuthorDate: 2022-03-24 20:51:03 +0000 Commit: Mateusz Guzik <[email protected]> CommitDate: 2022-04-02 12:09:07 +0000 vfs: fix memory leak on lookup with fds with ioctl caps Reviewed by: markj PR: 262515 Noted by: [email protected] Differential Revision: https://reviews.freebsd.org/D34667 --- sys/kern/kern_descrip.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++- sys/kern/vfs_cache.c | 2 +- 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, 88 insertions(+), 55 deletions(-) diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c index b13fc719c2b0..dd510cfd23f9 100644 --- a/sys/kern/kern_descrip.c +++ b/sys/kern/kern_descrip.c @@ -1805,11 +1805,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)); } @@ -3047,6 +3055,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); +} + /* * Fetch the descriptor locklessly. * diff --git a/sys/kern/vfs_cache.c b/sys/kern/vfs_cache.c index 556c5e459a55..b3e450001e61 100644 --- a/sys/kern/vfs_cache.c +++ b/sys/kern/vfs_cache.c @@ -4189,7 +4189,7 @@ cache_fpl_terminated(struct cache_fpl *fpl) (NC_NOMAKEENTRY | NC_KEEPPOSENTRY | LOCKLEAF | LOCKPARENT | WANTPARENT | \ FAILIFEXISTS | FOLLOW | EMPTYPATH | LOCKSHARED | SAVENAME | SAVESTART | \ WILLBEDIR | ISOPEN | NOMACCHECK | AUDITVNODE1 | AUDITVNODE2 | NOCAPCHECK | \ - OPENREAD | OPENWRITE) + OPENREAD | OPENWRITE | 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 34ecd91a064d..729c1922ee8c 100644 --- a/sys/kern/vfs_lookup.c +++ b/sys/kern/vfs_lookup.c @@ -286,10 +286,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; @@ -350,56 +348,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 af08d493f68c..dec7fdca92f2 100644 --- a/sys/kern/vfs_syscalls.c +++ b/sys/kern/vfs_syscalls.c @@ -1164,8 +1164,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); + NDINIT_ATRIGHTS(&nd, LOOKUP, FOLLOW | AUDITVNODE1 | WANTIOCTLCAPS, + pathseg, path, fd, &rights); td->td_dupfd = -1; /* XXX check for fdopen */ error = vn_open(&nd, &flags, cmode, fp); if (error != 0) { @@ -1247,11 +1247,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); } @@ -1259,6 +1258,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 8c4a2394770d..7d2a4885e415 100644 --- a/sys/sys/file.h +++ b/sys/sys/file.h @@ -291,6 +291,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 cc93bf7a5ca2..0b293d1d1abf 100644 --- a/sys/sys/namei.h +++ b/sys/sys/namei.h @@ -184,7 +184,7 @@ int cache_fplookup(struct nameidata *ndp, enum cache_fpl_status *status, #define NOCAPCHECK 0x00100000 /* do not perform capability checks */ #define OPENREAD 0x00200000 /* open for reading */ #define OPENWRITE 0x00400000 /* open for writing */ -/* 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 */ @@ -270,6 +270,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_STARTDIR_RELE 0x00000010 #define NDF_NO_FREE_PNBUF 0x00000020 +#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);
