From nobody Sat Apr 2 12:09:12 2022 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 8D1E11A6153A; Sat, 2 Apr 2022 12:09:12 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4KVwnN3XJBz3nNW; Sat, 2 Apr 2022 12:09:12 +0000 (UTC) (envelope-from git@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1648901352; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=eYDZHc37NbZsO51oOn+pnpFlkAhlItclks2k1aYcXqQ=; b=mSDGaFs2o54BFW01v11asPP19rCmDKQcDB2cJM4I/a7oejPZ9t9iYQfplP7DgcgxLrz+LN l+7g3Z5qCD6V/Hbic4NYhXvysxWH5RXUkfW8B/FQ1ayIgeI6+ZA1xs0i1Lr9hYj/SDlF/r ymp8MNmh4kPjybKlPsLQwrkIRZfS/XTJYomSFHZKONiPOlHJtX0f5By1scN12V8ZBGA774 Q2auNuaA3jl6Bm4fADcXAK08Fr3PhnSSyjcrDMLNJvgiyq1hUEGfPmcW8DdOhqKcSyUc0S vMSXdE1hJTS3vn52JY+OtpDCTE2eiA+4tJ25DmNufnsYFjKJoG/6DcDvPHgPtQ== Received: from gitrepo.freebsd.org (gitrepo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 599EA734; Sat, 2 Apr 2022 12:09:12 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org ([127.0.1.44]) by gitrepo.freebsd.org (8.16.1/8.16.1) with ESMTP id 232C9Clr076510; Sat, 2 Apr 2022 12:09:12 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.16.1/8.16.1/Submit) id 232C9Caq076509; Sat, 2 Apr 2022 12:09:12 GMT (envelope-from git) Date: Sat, 2 Apr 2022 12:09:12 GMT Message-Id: <202204021209.232C9Caq076509@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org From: Mateusz Guzik Subject: git: 0c805718cbd3 - main - vfs: fix memory leak on lookup with fds with ioctl caps List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-all@freebsd.org X-BeenThere: dev-commits-src-all@freebsd.org MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Git-Committer: mjg X-Git-Repository: src X-Git-Refname: refs/heads/main X-Git-Reftype: branch X-Git-Commit: 0c805718cbd3709e3ffc1a0d41612168c8242360 Auto-Submitted: auto-generated ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1648901352; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=eYDZHc37NbZsO51oOn+pnpFlkAhlItclks2k1aYcXqQ=; b=ewhjvzN97b9oOzk7Z5wsZRCMacYQMyRZM3TdcpuEU2b/XE87xGz5j6FYxqwlDiprdVDYLi 5+lXNBlCfhpF/GyE5+vCNmxUV0VY0QuxzZ/v/EbioBxMJhdEeXRbG+uQzJT638XdtAwjDn +o8xaID8+yf4Pq6XEGOpSx92+Xhc+UIJiIycHNEOL2OwGaySoThPND7Vi3E52R8YCGaC1r 0ajoumwuVaJosEAO89ba6PDuJqt9uiDwQ3zh/F8r03utWLEN5FNty0VFdPx4mz2OTc78Z3 LiEmskNKbZ/GO4oVtnoxgrswiY01DIWHGBw2g0V9ouCFdC0iIOEnrWjUeR9/Mg== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1648901352; a=rsa-sha256; cv=none; b=oQSH48d4mFMQhl6kWOffelWHOunqf4T20knNVDBsOka3EWaNTmlv6aV4QsNM+fqbtbvKl+ 4I4TQjhpd3irLIJm9bTobx78rfpoCFoYUZLwDQb4GsScF7+1fXs9f8iOaGKkyzsLfl8VQE xe9Tv6SSPpYjDsfMIPOHSWcZyYRgb8nW6UY1VjUxiHIVYbn5zByemRFU3Oo+QQxdC5v/XM 1zLqLCWxis6YC+5BYwALEmwShKo6AOcBY6VsxIEjThw/6RDFS4aW+UoLMabyV/1rbMPj0C WGPqAfFXBIeW6+7cu4xNcqtxR8tWZx+reiwlDMi3vc2G8BOYw1dF7wG0PY1bEg== ARC-Authentication-Results: i=1; mx1.freebsd.org; none X-ThisMailContainsUnwantedMimeParts: N The branch main has been updated by mjg: URL: https://cgit.FreeBSD.org/src/commit/?id=0c805718cbd3709e3ffc1a0d41612168c8242360 commit 0c805718cbd3709e3ffc1a0d41612168c8242360 Author: Mateusz Guzik AuthorDate: 2022-03-24 20:51:03 +0000 Commit: Mateusz Guzik 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: firk@cantconnect.ru 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);