From owner-freebsd-fs@freebsd.org Mon Jul 6 03:05:42 2015 Return-Path: Delivered-To: freebsd-fs@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id B7F5A98EC26 for ; Mon, 6 Jul 2015 03:05:42 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: from mail-wi0-x236.google.com (mail-wi0-x236.google.com [IPv6:2a00:1450:400c:c05::236]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 481F01D6B; Mon, 6 Jul 2015 03:05:42 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: by wibdq8 with SMTP id dq8so140451177wib.1; Sun, 05 Jul 2015 20:05:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=JaoGmXOJ1W3fx7OyHZz+9qTJhQN+C4r5PJADw74qGJc=; b=M/W/C728dg2C4Qw4RTzGQjdkk1/4/g837ppT12jPiy8shWZKPo1FxhNTljEjKhrba4 DTjVeDLsDOkNwHCBifQHKAsdAIBWBoJE4qxzYCQoUCHu/29kEc3h+wWATBr1AAzOkHVj y71i1tJsNzsaKoIQwLo/T9Cpub514M8bgbIYysyVHcp8uIrkgGYse0NeW0pVpuC/xk1H qOIPVj9nYeA8JANwVeHDnM9/H8HqZiz4XfGJrSZSvD2oPJ7vJ4f68i0pj1KUJZ56k7/o WYzge19gk/3nqMxx3B3cE3V7b+H3y59I1ALfe4qSIOHiaimssv2Z2R9aKeG4FlyYFvTA wjQA== X-Received: by 10.194.22.105 with SMTP id c9mr45960608wjf.120.1436151940475; Sun, 05 Jul 2015 20:05:40 -0700 (PDT) Received: from localhost.localdomain (ip-89-102-11-63.net.upcbroadband.cz. [89.102.11.63]) by mx.google.com with ESMTPSA id pd7sm25571851wjb.27.2015.07.05.20.05.38 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sun, 05 Jul 2015 20:05:39 -0700 (PDT) From: Mateusz Guzik To: freebsd-fs@freebsd.org Cc: kib@freesd.org, rwatson@FreeBSD.org, Mateusz Guzik Subject: [PATCH 2/2] audit: utilize vnode pointer found by namei instead of looking it up again Date: Mon, 6 Jul 2015 05:05:32 +0200 Message-Id: <1436151932-12514-3-git-send-email-mjguzik@gmail.com> X-Mailer: git-send-email 2.4.3 In-Reply-To: <1436151932-12514-1-git-send-email-mjguzik@gmail.com> References: <1436151932-12514-1-git-send-email-mjguzik@gmail.com> X-BeenThere: freebsd-fs@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: Filesystems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 06 Jul 2015 03:05:42 -0000 From: Mateusz Guzik 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