Date: Mon, 6 Jul 2015 05:07:15 +0200 From: Mateusz Guzik <mjguzik@gmail.com> To: freebsd-fs@freebsd.org Cc: kib@freebsd.org, rwatson@FreeBSD.org, Mateusz Guzik <mjg@freebsd.org> Subject: [PATCH 2/2] audit: utilize vnode pointer found by namei instead of looking it up again Message-ID: <1436152035-12564-3-git-send-email-mjguzik@gmail.com> In-Reply-To: <1436152035-12564-1-git-send-email-mjguzik@gmail.com> References: <1436152035-12564-1-git-send-email-mjguzik@gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
From: Mateusz Guzik <mjg@freebsd.org>
With the file descriptor translated only once, the code no longer
imposes the need to hold filedesc lock, previously needed to make sure
namei and audit translation return the same vnode.
---
sys/kern/vfs_lookup.c | 41 +++++++++++--------
sys/security/audit/audit.h | 14 +++++++
sys/security/audit/audit_arg.c | 36 ++++++++++++++++
sys/security/audit/audit_bsm_klib.c | 82 ++++++++++++++++++++++++-------------
sys/security/audit/audit_private.h | 2 +
5 files changed, 129 insertions(+), 46 deletions(-)
diff --git a/sys/kern/vfs_lookup.c b/sys/kern/vfs_lookup.c
index c5218ec..4fbe18b 100644
--- a/sys/kern/vfs_lookup.c
+++ b/sys/kern/vfs_lookup.c
@@ -158,7 +158,7 @@ namei(struct nameidata *ndp)
struct vnode *dp; /* the directory we are searching */
struct iovec aiov; /* uio for reading symbolic links */
struct uio auio;
- int error, linklen;
+ int error, needcapcheck, linklen;
struct componentname *cnp = &ndp->ni_cnd;
struct thread *td = cnp->cn_thread;
struct proc *p = td->td_proc;
@@ -235,14 +235,7 @@ namei(struct nameidata *ndp)
ndp->ni_rootdir = fdp->fd_rdir;
ndp->ni_topdir = fdp->fd_jdir;
- /*
- * If we are auditing the kernel pathname, save the user pathname.
- */
- if (cnp->cn_flags & AUDITVNODE1)
- AUDIT_ARG_UPATH1(td, ndp->ni_dirfd, cnp->cn_pnbuf);
- if (cnp->cn_flags & AUDITVNODE2)
- AUDIT_ARG_UPATH2(td, ndp->ni_dirfd, cnp->cn_pnbuf);
-
+ needcapcheck = 0;
cnp->cn_nameptr = cnp->cn_pnbuf;
if (cnp->cn_pnbuf[0] == '/') {
error = namei_handle_root(ndp, &dp);
@@ -253,17 +246,33 @@ namei(struct nameidata *ndp)
dp = fdp->fd_cdir;
VREF(dp);
} else {
- cap_rights_t rights;
-
- rights = ndp->ni_rightsneeded;
- cap_rights_set(&rights, CAP_LOOKUP);
+ needcapcheck = 1;
if (cnp->cn_flags & AUDITVNODE1)
AUDIT_ARG_ATFD1(ndp->ni_dirfd);
if (cnp->cn_flags & AUDITVNODE2)
AUDIT_ARG_ATFD2(ndp->ni_dirfd);
error = fgetvp_rights(td, ndp->ni_dirfd,
- &rights, &ndp->ni_filecaps, &dp);
+ NULL, &ndp->ni_filecaps, &dp);
+ if (error == 0 && dp->v_type != VDIR)
+ error = ENOTDIR;
+ }
+ }
+ /*
+ * If we are auditing the kernel pathname, save the user pathname.
+ */
+ if (cnp->cn_flags & AUDITVNODE1)
+ AUDIT_ARG_UPATH1_VP(td, dp, cnp->cn_pnbuf);
+ if (cnp->cn_flags & AUDITVNODE2)
+ AUDIT_ARG_UPATH2_VP(td, dp, cnp->cn_pnbuf);
+ FILEDESC_SUNLOCK(fdp);
+ if (error == 0 && needcapcheck) {
+ cap_rights_t rights;
+
+ rights = ndp->ni_rightsneeded;
+ cap_rights_set(&rights, CAP_LOOKUP);
+
+ error = cap_check(&ndp->ni_filecaps.fc_rights, &rights);
#ifdef CAPABILITIES
/*
* If file descriptor doesn't have all rights,
@@ -278,11 +287,7 @@ namei(struct nameidata *ndp)
ndp->ni_strictrelative = 1;
}
#endif
- if (error == 0 && dp->v_type != VDIR)
- error = ENOTDIR;
- }
}
- FILEDESC_SUNLOCK(fdp);
if (error != 0) {
if (dp != NULL)
vrele(dp);
diff --git a/sys/security/audit/audit.h b/sys/security/audit/audit.h
index 559d571..2f6420f 100644
--- a/sys/security/audit/audit.h
+++ b/sys/security/audit/audit.h
@@ -101,6 +101,10 @@ void audit_arg_auditinfo(struct auditinfo *au_info);
void audit_arg_auditinfo_addr(struct auditinfo_addr *au_info);
void audit_arg_upath1(struct thread *td, int dirfd, char *upath);
void audit_arg_upath2(struct thread *td, int dirfd, char *upath);
+void audit_arg_upath1_vp(struct thread *td, struct vnode *dirvp,
+ char *upath);
+void audit_arg_upath2_vp(struct thread *td, struct vnode *dirvp,
+ char *upath);
void audit_arg_vnode1(struct vnode *vp);
void audit_arg_vnode2(struct vnode *vp);
void audit_arg_text(char *text);
@@ -297,6 +301,16 @@ void audit_thread_free(struct thread *td);
audit_arg_upath2((td), (dirfd), (upath)); \
} while (0)
+#define AUDIT_ARG_UPATH1_VP(td, dirvp, upath) do { \
+ if (AUDITING_TD(curthread)) \
+ audit_arg_upath1_vp((td), (dirvp), (upath)); \
+} while (0)
+
+#define AUDIT_ARG_UPATH2_VP(td, dirvp, upath) do { \
+ if (AUDITING_TD(curthread)) \
+ audit_arg_upath2_vp((td), (dirvp), (upath)); \
+} while (0)
+
#define AUDIT_ARG_VALUE(value) do { \
if (AUDITING_TD(curthread)) \
audit_arg_value((value)); \
diff --git a/sys/security/audit/audit_arg.c b/sys/security/audit/audit_arg.c
index c006b90..c019bad 100644
--- a/sys/security/audit/audit_arg.c
+++ b/sys/security/audit/audit_arg.c
@@ -719,6 +719,16 @@ audit_arg_upath(struct thread *td, int dirfd, char *upath, char **pathp)
audit_canon_path(td, dirfd, upath, *pathp);
}
+static void
+audit_arg_upath_vp(struct thread *td, struct vnode *dirvp, char *upath,
+ char **pathp)
+{
+
+ if (*pathp == NULL)
+ *pathp = malloc(MAXPATHLEN, M_AUDITPATH, M_WAITOK);
+ audit_canon_path_vp(td, dirvp, upath, *pathp);
+}
+
void
audit_arg_upath1(struct thread *td, int dirfd, char *upath)
{
@@ -745,6 +755,32 @@ audit_arg_upath2(struct thread *td, int dirfd, char *upath)
ARG_SET_VALID(ar, ARG_UPATH2);
}
+void
+audit_arg_upath1_vp(struct thread *td, struct vnode *dirvp, char *upath)
+{
+ struct kaudit_record *ar;
+
+ ar = currecord();
+ if (ar == NULL)
+ return;
+
+ audit_arg_upath_vp(td, dirvp, upath, &ar->k_ar.ar_arg_upath1);
+ ARG_SET_VALID(ar, ARG_UPATH1);
+}
+
+void
+audit_arg_upath2_vp(struct thread *td, struct vnode *dirvp, char *upath)
+{
+ struct kaudit_record *ar;
+
+ ar = currecord();
+ if (ar == NULL)
+ return;
+
+ audit_arg_upath_vp(td, dirvp, upath, &ar->k_ar.ar_arg_upath2);
+ ARG_SET_VALID(ar, ARG_UPATH2);
+}
+
/*
* Function to save the path and vnode attr information into the audit
* record.
diff --git a/sys/security/audit/audit_bsm_klib.c b/sys/security/audit/audit_bsm_klib.c
index b687a15..7e8dac5 100644
--- a/sys/security/audit/audit_bsm_klib.c
+++ b/sys/security/audit/audit_bsm_klib.c
@@ -461,23 +461,19 @@ auditon_command_event(int cmd)
* but this results in a volfs name written to the audit log. So we will
* leave the filename starting with '/' in the audit log in this case.
*/
-void
-audit_canon_path(struct thread *td, int dirfd, char *path, char *cpath)
+static void
+audit_canon_path_common(struct thread *td, struct vnode *dirvp,
+ char *path, char *cpath)
{
struct vnode *cvnp, *rvnp;
- char *rbuf, *fbuf, *copy;
struct filedesc *fdp;
+ char *rbuf, *fbuf, *copy;
struct sbuf sbf;
- cap_rights_t rights;
int error, needslash;
- WITNESS_WARN(WARN_GIANTOK | WARN_SLEEPOK, NULL, "%s: at %s:%d",
- __func__, __FILE__, __LINE__);
-
copy = path;
- rvnp = cvnp = NULL;
+ cvnp = rvnp = NULL;
fdp = td->td_proc->p_fd;
- FILEDESC_SLOCK(fdp);
/*
* Make sure that we handle the chroot(2) case. If there is an
* alternate root directory, prepend it to the audited pathname.
@@ -492,22 +488,7 @@ audit_canon_path(struct thread *td, int dirfd, char *path, char *cpath)
* path.
*/
if (*path != '/') {
- if (dirfd == AT_FDCWD) {
- cvnp = fdp->fd_cdir;
- vhold(cvnp);
- } else {
- /* XXX: fgetvp() that vhold()s vnode instead of vref()ing it would be better */
- error = fgetvp(td, dirfd, cap_rights_init(&rights), &cvnp);
- if (error) {
- FILEDESC_SUNLOCK(fdp);
- cpath[0] = '\0';
- if (rvnp != NULL)
- vdrop(rvnp);
- return;
- }
- vhold(cvnp);
- vrele(cvnp);
- }
+ cvnp = dirvp;
needslash = (fdp->fd_rdir != cvnp);
} else {
needslash = 1;
@@ -536,8 +517,6 @@ audit_canon_path(struct thread *td, int dirfd, char *path, char *cpath)
vdrop(rvnp);
if (error) {
cpath[0] = '\0';
- if (cvnp != NULL)
- vdrop(cvnp);
return;
}
(void) sbuf_cat(&sbf, rbuf);
@@ -545,7 +524,6 @@ audit_canon_path(struct thread *td, int dirfd, char *path, char *cpath)
}
if (cvnp != NULL) {
error = vn_fullpath(td, cvnp, &rbuf, &fbuf);
- vdrop(cvnp);
if (error) {
cpath[0] = '\0';
return;
@@ -571,3 +549,51 @@ audit_canon_path(struct thread *td, int dirfd, char *path, char *cpath)
}
sbuf_finish(&sbf);
}
+
+void
+audit_canon_path(struct thread *td, int dirfd, char *path, char *cpath)
+{
+ struct vnode *dirvp;
+ struct filedesc *fdp;
+ cap_rights_t rights;
+
+ WITNESS_WARN(WARN_GIANTOK | WARN_SLEEPOK, NULL, "%s: at %s:%d",
+ __func__, __FILE__, __LINE__);
+
+ dirvp = NULL;
+ fdp = td->td_proc->p_fd;
+ FILEDESC_SLOCK(fdp);
+ if (*path != '/') {
+ if (dirfd == AT_FDCWD) {
+ dirvp = fdp->fd_cdir;
+ vhold(dirvp);
+ } else {
+ /* XXX: fgetvp() that vhold()s vnode instead of vref()ing it would be better */
+ if (fgetvp(td, dirfd, cap_rights_init(&rights), &dirvp) != 0) {
+ FILEDESC_SUNLOCK(fdp);
+ cpath[0] = '\0';
+ return;
+ }
+ vhold(dirvp);
+ vrele(dirvp);
+ }
+ }
+
+ audit_canon_path_common(td, dirvp, path, cpath);
+ if (dirvp != NULL)
+ vdrop(dirvp);
+}
+
+void
+audit_canon_path_vp(struct thread *td, struct vnode *dirvp, char *path, char *cpath)
+{
+
+ WITNESS_WARN(WARN_GIANTOK | WARN_SLEEPOK, NULL, "%s: at %s:%d",
+ __func__, __FILE__, __LINE__);
+
+ if (dirvp == NULL)
+ return;
+
+ FILEDESC_SLOCK(td->td_proc->p_fd);
+ audit_canon_path_common(td, dirvp, path, cpath);
+}
diff --git a/sys/security/audit/audit_private.h b/sys/security/audit/audit_private.h
index b5c373a..7ecf3a6 100644
--- a/sys/security/audit/audit_private.h
+++ b/sys/security/audit/audit_private.h
@@ -394,6 +394,8 @@ au_event_t audit_msgctl_to_event(int cmd);
au_event_t audit_semctl_to_event(int cmr);
void audit_canon_path(struct thread *td, int dirfd, char *path,
char *cpath);
+void audit_canon_path_vp(struct thread *td, struct vnode *dirvp,
+ char *path, char *cpath);
au_event_t auditon_command_event(int cmd);
/*
--
2.4.5
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1436152035-12564-3-git-send-email-mjguzik>