From nobody Mon Apr 04 19:19:14 2022 X-Original-To: dev-commits-src-branches@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 8FC3B1A8310F; Mon, 4 Apr 2022 19:19:14 +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 4KXLDf3YHXz3w6x; Mon, 4 Apr 2022 19:19:14 +0000 (UTC) (envelope-from git@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1649099954; 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=oVLYcrqr0rds0ckg/rReMyKBIbr4NRDrVtDwCJSgRrU=; b=dKz8Eld5O+0Wdw6+SjnMP47rgT+aEh1kjxbeK0laKp7pTfJ+MWkdVTqYhGNprlGnqnttfH 9kZNvkVDVeSxFqjCO3y1GkixFfSVfhMS16QwFiYISgu5TGARUmwgeJSRHenYbBSYMYcEyj +zYjv8lrWrWUldHIxArj+bJdz8pZGbssYa4FVmnJcytCyOHEIbrjEn4z7xta1LFCKGbSWi dCCyfDiKfjbPNJZkZZlFXv5nogcLyD6gnpkWUbq5psMsPO7Yy5MNFXEBWSDoyZJ4TDWOiU p7JHL9acQNfUGpqaczUV0yYi0HKk/6MTk6Mpp1J0tNIbBkuUcfKH87NVCgJ4lg== 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 5547117E1B; Mon, 4 Apr 2022 19:19:14 +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 234JJEJ0053120; Mon, 4 Apr 2022 19:19:14 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.16.1/8.16.1/Submit) id 234JJE08053119; Mon, 4 Apr 2022 19:19:14 GMT (envelope-from git) Date: Mon, 4 Apr 2022 19:19:14 GMT Message-Id: <202204041919.234JJE08053119@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-branches@FreeBSD.org From: Mateusz Guzik Subject: git: 838d8e6fb60e - stable/13 - vfs: fix memory leak on lookup with fds with ioctl caps List-Id: Commits to the stable branches of the FreeBSD src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-branches List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-branches@freebsd.org X-BeenThere: dev-commits-src-branches@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/stable/13 X-Git-Reftype: branch X-Git-Commit: 838d8e6fb60e12e610701ae10be717309f3ea935 Auto-Submitted: auto-generated ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1649099954; 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=oVLYcrqr0rds0ckg/rReMyKBIbr4NRDrVtDwCJSgRrU=; b=uAJb0+2feSXwOQk6pN4J1Uf2VpSk72OkzOBJUYr+XGCRM7oSCDnqjL3z2wuvGzsdKW3Wit Z80VSzKf+XFOG4aMVhkwGFvFQdEiL1Ay9DkuY0fqQoGTruGi6jl1p+/RXaOAQwAMggaHtY ogLxhoUmXB2rjRY4nceGgNhxqH4xfx0XdUC2kJrpMxwG63NH0DocDN4D7JvTkrPMoQJfro lqNbmRYwID3Y8j5IghSr6HWqmaAAwAPvD4QnodqB48lSHQwQl3il0vPh+EOAnJi9NMiUVc UY6xrhYD2eK3FUOqjIec6ndZ9rXYH6m7G4VB+XjpgzPVkXjbqaT8K1ylT4xfeg== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1649099954; a=rsa-sha256; cv=none; b=GakZ6arDRjv7BsScCDudAhcRpJtK49JujQnwMCvgBpsa0KyGCZV0jqRNa1cqkok57bQq9S QaWW2LfcFkMhZEgehl7TW7MDWTn6uadVtzzhGiY0AhKsTk7Y41UFVmmQRhVUnimKjGV7QC 9mk4D7X+t2eA7BrFzcfi2aRhZd1xgTjdk30U3kuAQjZtsUHu2xJhO5s5xaqmzVuwT4H1de iBt4h0cnncrUs1EUw9KnP7qmZRRJein7pIiYXfpT//mUKG6/mFuB1ip0cgZ614QTfI52wP 1GdbbwBvfJ1mj/mUWVU7Dq3clIcKNoeh113ZvZlKYqcBFFK7S9e4HN9gbOxzPA== ARC-Authentication-Results: i=1; mx1.freebsd.org; none X-ThisMailContainsUnwantedMimeParts: N The branch stable/13 has been updated by mjg: URL: https://cgit.FreeBSD.org/src/commit/?id=838d8e6fb60e12e610701ae10be717309f3ea935 commit 838d8e6fb60e12e610701ae10be717309f3ea935 Author: Mateusz Guzik AuthorDate: 2022-03-24 20:51:03 +0000 Commit: Mateusz Guzik CommitDate: 2022-04-04 19:16:18 +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 (cherry picked from commit 0c805718cbd3709e3ffc1a0d41612168c8242360) --- 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 de70be1a997f..8eb83ea44159 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 3a8d32a69dbf..fdd32b90212f 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 { \