git: 838d8e6fb60e - stable/13 - vfs: fix memory leak on lookup with fds with ioctl caps
- Go to: [ bottom of page ] [ top of archives ] [ this month ]
Date: Mon, 04 Apr 2022 19:19:14 UTC
The branch stable/13 has been updated by mjg:
URL: https://cgit.FreeBSD.org/src/commit/?id=838d8e6fb60e12e610701ae10be717309f3ea935
commit 838d8e6fb60e12e610701ae10be717309f3ea935
Author: Mateusz Guzik <mjg@FreeBSD.org>
AuthorDate: 2022-03-24 20:51:03 +0000
Commit: Mateusz Guzik <mjg@FreeBSD.org>
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 { \