[PATCH v3 0/7] Inode security label invalidation

[PATCH v3 0/7] Inode security label invalidation

[Andreas Gruenbacher]

Here is another version of the patch queue to make gfs2 and similar file
systems work with SELinux.  As suggested by Stephen Smalley [*], the relevant
uses of inode->security are wrapped in function calls that try to revalidate
invalid labels.

  [*] http://marc.info/?l=linux-kernel&m=144416710207686&w=2

The patches are looking good from my point of view; is there anything else that
needs addressing?

Does SELinux have test suites that these patches could be tested agains?

Thanks,
Andreas

Andreas Gruenbacher (7):
  selinux: Remove unused variable in selinux_inode_init_security
  selinux: Add accessor functions for inode->i_security
  selinux: Get rid of file_path_has_perm
  selinux: Push dentry down from {dentry,path,file}_has_perm
  security: Add hook to invalidate inode security labels
  selinux: Revalidate invalid inode security labels
  gfs2: Invalide security labels of inodes when they go invalid

 fs/gfs2/glops.c                   |   2 +
 include/linux/lsm_hooks.h         |   6 ++
 include/linux/security.h          |   5 +
 security/security.c               |   8 ++
 security/selinux/hooks.c          | 213 ++++++++++++++++++++++----------------
 security/selinux/include/objsec.h |   6 ++
 6 files changed, 152 insertions(+), 88 deletions(-)

-- 
2.5.0

[PATCH v3 1/7] selinux: Remove unused variable in selinux_inode_init_security

[Andreas Gruenbacher]

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 security/selinux/hooks.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index e4369d8..fc8f626 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2756,13 +2756,11 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
 				       void **value, size_t *len)
 {
 	const struct task_security_struct *tsec = current_security();
-	struct inode_security_struct *dsec;
 	struct superblock_security_struct *sbsec;
 	u32 sid, newsid, clen;
 	int rc;
 	char *context;
 
-	dsec = dir->i_security;
 	sbsec = dir->i_sb->s_security;
 
 	sid = tsec->sid;
-- 
2.5.0

[PATCH v3 2/7] selinux: Add accessor functions for inode->i_security

[Andreas Gruenbacher]

Add functions dentry_security and inode_security for accessing
inode->i_security.  These functions initially don't do much, but they
will later be used to revalidate the security labels when necessary.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 security/selinux/hooks.c | 101 ++++++++++++++++++++++++++---------------------
 1 file changed, 57 insertions(+), 44 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index fc8f626..65e8689 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -241,6 +241,24 @@ static int inode_alloc_security(struct inode *inode)
 	return 0;
 }
 
+/*
+ * Get the security label of a dentry's inode.
+ */
+static struct inode_security_struct *dentry_security(struct dentry *dentry)
+{
+	struct inode *inode = d_backing_inode(dentry);
+
+	return inode->i_security;
+}
+
+/*
+ * Get the security label of an inode.
+ */
+static struct inode_security_struct *inode_security(struct inode *inode)
+{
+	return inode->i_security;
+}
+
 static void inode_free_rcu(struct rcu_head *head)
 {
 	struct inode_security_struct *isec;
@@ -564,8 +582,8 @@ static int selinux_get_mnt_opts(const struct super_block *sb,
 		opts->mnt_opts_flags[i++] = DEFCONTEXT_MNT;
 	}
 	if (sbsec->flags & ROOTCONTEXT_MNT) {
-		struct inode *root = d_backing_inode(sbsec->sb->s_root);
-		struct inode_security_struct *isec = root->i_security;
+		struct dentry *root = sbsec->sb->s_root;
+		struct inode_security_struct *isec = dentry_security(root);
 
 		rc = security_sid_to_context(isec->sid, &context, &len);
 		if (rc)
@@ -620,8 +638,8 @@ static int selinux_set_mnt_opts(struct super_block *sb,
 	int rc = 0, i;
 	struct superblock_security_struct *sbsec = sb->s_security;
 	const char *name = sb->s_type->name;
-	struct inode *inode = d_backing_inode(sbsec->sb->s_root);
-	struct inode_security_struct *root_isec = inode->i_security;
+	struct dentry *root = sbsec->sb->s_root;
+	struct inode_security_struct *root_isec = dentry_security(root);
 	u32 fscontext_sid = 0, context_sid = 0, rootcontext_sid = 0;
 	u32 defcontext_sid = 0;
 	char **mount_options = opts->mnt_opts;
@@ -852,8 +870,8 @@ static int selinux_cmp_sb_context(const struct super_block *oldsb,
 	if ((oldflags & DEFCONTEXT_MNT) && old->def_sid != new->def_sid)
 		goto mismatch;
 	if (oldflags & ROOTCONTEXT_MNT) {
-		struct inode_security_struct *oldroot = d_backing_inode(oldsb->s_root)->i_security;
-		struct inode_security_struct *newroot = d_backing_inode(newsb->s_root)->i_security;
+		struct inode_security_struct *oldroot = dentry_security(oldsb->s_root);
+		struct inode_security_struct *newroot = dentry_security(newsb->s_root);
 		if (oldroot->sid != newroot->sid)
 			goto mismatch;
 	}
@@ -903,17 +921,14 @@ static int selinux_sb_clone_mnt_opts(const struct super_block *oldsb,
 		if (!set_fscontext)
 			newsbsec->sid = sid;
 		if (!set_rootcontext) {
-			struct inode *newinode = d_backing_inode(newsb->s_root);
-			struct inode_security_struct *newisec = newinode->i_security;
+			struct inode_security_struct *newisec = dentry_security(newsb->s_root);
 			newisec->sid = sid;
 		}
 		newsbsec->mntpoint_sid = sid;
 	}
 	if (set_rootcontext) {
-		const struct inode *oldinode = d_backing_inode(oldsb->s_root);
-		const struct inode_security_struct *oldisec = oldinode->i_security;
-		struct inode *newinode = d_backing_inode(newsb->s_root);
-		struct inode_security_struct *newisec = newinode->i_security;
+		const struct inode_security_struct *oldisec = dentry_security(oldsb->s_root);
+		struct inode_security_struct *newisec = dentry_security(newsb->s_root);
 
 		newisec->sid = oldisec->sid;
 	}
@@ -1623,7 +1638,7 @@ static int inode_has_perm(const struct cred *cred,
 		return 0;
 
 	sid = cred_sid(cred);
-	isec = inode->i_security;
+	isec = inode_security(inode);
 
 	return avc_has_perm(sid, isec->sid, isec->sclass, perms, adp);
 }
@@ -1712,13 +1727,13 @@ out:
 /*
  * Determine the label for an inode that might be unioned.
  */
-static int selinux_determine_inode_label(const struct inode *dir,
+static int selinux_determine_inode_label(struct inode *dir,
 					 const struct qstr *name,
 					 u16 tclass,
 					 u32 *_new_isid)
 {
 	const struct superblock_security_struct *sbsec = dir->i_sb->s_security;
-	const struct inode_security_struct *dsec = dir->i_security;
+	const struct inode_security_struct *dsec = inode_security(dir);
 	const struct task_security_struct *tsec = current_security();
 
 	if ((sbsec->flags & SE_SBINITIALIZED) &&
@@ -1747,7 +1762,7 @@ static int may_create(struct inode *dir,
 	struct common_audit_data ad;
 	int rc;
 
-	dsec = dir->i_security;
+	dsec = inode_security(dir);
 	sbsec = dir->i_sb->s_security;
 
 	sid = tsec->sid;
@@ -1800,8 +1815,8 @@ static int may_link(struct inode *dir,
 	u32 av;
 	int rc;
 
-	dsec = dir->i_security;
-	isec = d_backing_inode(dentry)->i_security;
+	dsec = inode_security(dir);
+	isec = dentry_security(dentry);
 
 	ad.type = LSM_AUDIT_DATA_DENTRY;
 	ad.u.dentry = dentry;
@@ -1844,10 +1859,10 @@ static inline int may_rename(struct inode *old_dir,
 	int old_is_dir, new_is_dir;
 	int rc;
 
-	old_dsec = old_dir->i_security;
-	old_isec = d_backing_inode(old_dentry)->i_security;
+	old_dsec = inode_security(old_dir);
+	old_isec = dentry_security(old_dentry);
 	old_is_dir = d_is_dir(old_dentry);
-	new_dsec = new_dir->i_security;
+	new_dsec = inode_security(new_dir);
 
 	ad.type = LSM_AUDIT_DATA_DENTRY;
 
@@ -1875,7 +1890,7 @@ static inline int may_rename(struct inode *old_dir,
 	if (rc)
 		return rc;
 	if (d_is_positive(new_dentry)) {
-		new_isec = d_backing_inode(new_dentry)->i_security;
+		new_isec = dentry_security(new_dentry);
 		new_is_dir = d_is_dir(new_dentry);
 		rc = avc_has_perm(sid, new_isec->sid,
 				  new_isec->sclass,
@@ -2011,8 +2026,8 @@ static int selinux_binder_transfer_file(struct task_struct *from,
 {
 	u32 sid = task_sid(to);
 	struct file_security_struct *fsec = file->f_security;
-	struct inode *inode = d_backing_inode(file->f_path.dentry);
-	struct inode_security_struct *isec = inode->i_security;
+	struct dentry *dentry = file->f_path.dentry;
+	struct inode_security_struct *isec = dentry_security(dentry);
 	struct common_audit_data ad;
 	int rc;
 
@@ -2028,7 +2043,7 @@ static int selinux_binder_transfer_file(struct task_struct *from,
 			return rc;
 	}
 
-	if (unlikely(IS_PRIVATE(inode)))
+	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
 		return 0;
 
 	return avc_has_perm(sid, isec->sid, isec->sclass, file_to_av(file),
@@ -2207,7 +2222,6 @@ static int selinux_bprm_set_creds(struct linux_binprm *bprm)
 	struct task_security_struct *new_tsec;
 	struct inode_security_struct *isec;
 	struct common_audit_data ad;
-	struct inode *inode = file_inode(bprm->file);
 	int rc;
 
 	/* SELinux context only depends on initial program or script and not
@@ -2217,7 +2231,7 @@ static int selinux_bprm_set_creds(struct linux_binprm *bprm)
 
 	old_tsec = current_security();
 	new_tsec = bprm->cred->security;
-	isec = inode->i_security;
+	isec = dentry_security(bprm->file->f_path.dentry);
 
 	/* Default to the current task SID. */
 	new_tsec->sid = old_tsec->sid;
@@ -2642,7 +2656,7 @@ static int selinux_sb_remount(struct super_block *sb, void *data)
 			break;
 		case ROOTCONTEXT_MNT: {
 			struct inode_security_struct *root_isec;
-			root_isec = d_backing_inode(sb->s_root)->i_security;
+			root_isec = dentry_security(sb->s_root);
 
 			if (bad_option(sbsec, ROOTCONTEXT_MNT, root_isec->sid, sid))
 				goto out_bad_option;
@@ -2859,7 +2873,7 @@ static int selinux_inode_follow_link(struct dentry *dentry, struct inode *inode,
 	ad.type = LSM_AUDIT_DATA_DENTRY;
 	ad.u.dentry = dentry;
 	sid = cred_sid(cred);
-	isec = inode->i_security;
+	isec = inode_security(inode);
 
 	return avc_has_perm_flags(sid, isec->sid, isec->sclass, FILE__READ, &ad,
 				  rcu ? MAY_NOT_BLOCK : 0);
@@ -2911,7 +2925,7 @@ static int selinux_inode_permission(struct inode *inode, int mask)
 	perms = file_mask_to_av(inode->i_mode, mask);
 
 	sid = cred_sid(cred);
-	isec = inode->i_security;
+	isec = inode_security(inode);
 
 	rc = avc_has_perm_noaudit(sid, isec->sid, isec->sclass, perms, 0, &avd);
 	audited = avc_audit_required(perms, &avd, rc,
@@ -2980,7 +2994,7 @@ static int selinux_inode_setxattr(struct dentry *dentry, const char *name,
 				  const void *value, size_t size, int flags)
 {
 	struct inode *inode = d_backing_inode(dentry);
-	struct inode_security_struct *isec = inode->i_security;
+	struct inode_security_struct *isec = dentry_security(dentry);
 	struct superblock_security_struct *sbsec;
 	struct common_audit_data ad;
 	u32 newsid, sid = current_sid();
@@ -3057,7 +3071,7 @@ static void selinux_inode_post_setxattr(struct dentry *dentry, const char *name,
 					int flags)
 {
 	struct inode *inode = d_backing_inode(dentry);
-	struct inode_security_struct *isec = inode->i_security;
+	struct inode_security_struct *isec = dentry_security(dentry);
 	u32 newsid;
 	int rc;
 
@@ -3154,7 +3168,7 @@ out_nofree:
 static int selinux_inode_setsecurity(struct inode *inode, const char *name,
 				     const void *value, size_t size, int flags)
 {
-	struct inode_security_struct *isec = inode->i_security;
+	struct inode_security_struct *isec = inode_security(inode);
 	u32 newsid;
 	int rc;
 
@@ -3205,9 +3219,8 @@ static int selinux_revalidate_file_permission(struct file *file, int mask)
 
 static int selinux_file_permission(struct file *file, int mask)
 {
-	struct inode *inode = file_inode(file);
 	struct file_security_struct *fsec = file->f_security;
-	struct inode_security_struct *isec = inode->i_security;
+	struct inode_security_struct *isec = dentry_security(file->f_path.dentry);
 	u32 sid = current_sid();
 
 	if (!mask)
@@ -3241,8 +3254,8 @@ int ioctl_has_perm(const struct cred *cred, struct file *file,
 {
 	struct common_audit_data ad;
 	struct file_security_struct *fsec = file->f_security;
-	struct inode *inode = file_inode(file);
-	struct inode_security_struct *isec = inode->i_security;
+	struct dentry *dentry = file->f_path.dentry;
+	struct inode_security_struct *isec = dentry_security(dentry);
 	struct lsm_ioctlop_audit ioctl;
 	u32 ssid = cred_sid(cred);
 	int rc;
@@ -3263,7 +3276,7 @@ int ioctl_has_perm(const struct cred *cred, struct file *file,
 			goto out;
 	}
 
-	if (unlikely(IS_PRIVATE(inode)))
+	if (unlikely(IS_PRIVATE(dentry->d_inode)))
 		return 0;
 
 	rc = avc_has_extended_perms(ssid, isec->sid, isec->sclass,
@@ -3506,7 +3519,7 @@ static int selinux_file_open(struct file *file, const struct cred *cred)
 	struct inode_security_struct *isec;
 
 	fsec = file->f_security;
-	isec = file_inode(file)->i_security;
+	isec = dentry_security(file->f_path.dentry);
 	/*
 	 * Save inode label and policy sequence number
 	 * at open-time so that selinux_file_permission
@@ -3624,7 +3637,7 @@ static int selinux_kernel_act_as(struct cred *new, u32 secid)
  */
 static int selinux_kernel_create_files_as(struct cred *new, struct inode *inode)
 {
-	struct inode_security_struct *isec = inode->i_security;
+	struct inode_security_struct *isec = inode_security(inode);
 	struct task_security_struct *tsec = new->security;
 	u32 sid = current_sid();
 	int ret;
@@ -4065,7 +4078,7 @@ static int selinux_socket_post_create(struct socket *sock, int family,
 				      int type, int protocol, int kern)
 {
 	const struct task_security_struct *tsec = current_security();
-	struct inode_security_struct *isec = SOCK_INODE(sock)->i_security;
+	struct inode_security_struct *isec = inode_security(SOCK_INODE(sock));
 	struct sk_security_struct *sksec;
 	int err = 0;
 
@@ -4265,9 +4278,9 @@ static int selinux_socket_accept(struct socket *sock, struct socket *newsock)
 	if (err)
 		return err;
 
-	newisec = SOCK_INODE(newsock)->i_security;
+	newisec = inode_security(SOCK_INODE(newsock));
 
-	isec = SOCK_INODE(sock)->i_security;
+	isec = inode_security(SOCK_INODE(sock));
 	newisec->sclass = isec->sclass;
 	newisec->sid = isec->sid;
 	newisec->initialized = 1;
@@ -4605,7 +4618,7 @@ static void selinux_sk_getsecid(struct sock *sk, u32 *secid)
 
 static void selinux_sock_graft(struct sock *sk, struct socket *parent)
 {
-	struct inode_security_struct *isec = SOCK_INODE(parent)->i_security;
+	struct inode_security_struct *isec = inode_security(SOCK_INODE(parent));
 	struct sk_security_struct *sksec = sk->sk_security;
 
 	if (sk->sk_family == PF_INET || sk->sk_family == PF_INET6 ||
-- 
2.5.0

[PATCH v3 3/7] selinux: Get rid of file_path_has_perm

[Andreas Gruenbacher]

Use path_has_perm directly instead.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 security/selinux/hooks.c | 18 +++---------------
 1 file changed, 3 insertions(+), 15 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 65e8689..d6b4dc9 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1673,18 +1673,6 @@ static inline int path_has_perm(const struct cred *cred,
 	return inode_has_perm(cred, inode, av, &ad);
 }
 
-/* Same as path_has_perm, but uses the inode from the file struct. */
-static inline int file_path_has_perm(const struct cred *cred,
-				     struct file *file,
-				     u32 av)
-{
-	struct common_audit_data ad;
-
-	ad.type = LSM_AUDIT_DATA_PATH;
-	ad.u.path = file->f_path;
-	return inode_has_perm(cred, file_inode(file), av, &ad);
-}
-
 /* Check whether a task can use an open file descriptor to
    access an inode in a given way.  Check access to the
    descriptor itself, and then use dentry_has_perm to
@@ -2371,14 +2359,14 @@ static inline void flush_unauthorized_files(const struct cred *cred,
 			struct tty_file_private *file_priv;
 
 			/* Revalidate access to controlling tty.
-			   Use file_path_has_perm on the tty path directly
+			   Use path_has_perm on the tty path directly
 			   rather than using file_has_perm, as this particular
 			   open file may belong to another process and we are
 			   only interested in the inode-based check here. */
 			file_priv = list_first_entry(&tty->tty_files,
 						struct tty_file_private, list);
 			file = file_priv->file;
-			if (file_path_has_perm(cred, file, FILE__READ | FILE__WRITE))
+			if (path_has_perm(cred, &file->f_path, FILE__READ | FILE__WRITE))
 				drop_tty = 1;
 		}
 		spin_unlock(&tty_files_lock);
@@ -3537,7 +3525,7 @@ static int selinux_file_open(struct file *file, const struct cred *cred)
 	 * new inode label or new policy.
 	 * This check is not redundant - do not remove.
 	 */
-	return file_path_has_perm(cred, file, open_file_to_av(file));
+	return path_has_perm(cred, &file->f_path, open_file_to_av(file));
 }
 
 /* task security operations */
-- 
2.5.0

[PATCH v3 4/7] selinux: Push dentry down from {dentry, path, file}_has_perm

[Andreas Gruenbacher]

In dentry_has_perm, path_has_perm, and file_has_perm, push the dentry down
to before avc_has_perm so that dentry_security can be used instead of
inode_security.  Since inode_has_perm now takes a dentry, rename it to
__dentry_has_perm.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 security/selinux/hooks.c | 29 +++++++++++++----------------
 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index d6b4dc9..2a04729 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1621,56 +1621,54 @@ static int task_has_system(struct task_struct *tsk,
 			    SECCLASS_SYSTEM, perms, NULL);
 }
 
-/* Check whether a task has a particular permission to an inode.
-   The 'adp' parameter is optional and allows other audit
+/* Check whether a task has a particular permission to a dentry's
+   inode.  The 'adp' parameter is optional and allows other audit
    data to be passed (e.g. the dentry). */
-static int inode_has_perm(const struct cred *cred,
-			  struct inode *inode,
-			  u32 perms,
-			  struct common_audit_data *adp)
+static int __dentry_has_perm(const struct cred *cred,
+			     struct dentry *dentry,
+			     u32 perms,
+			     struct common_audit_data *adp)
 {
 	struct inode_security_struct *isec;
 	u32 sid;
 
 	validate_creds(cred);
 
-	if (unlikely(IS_PRIVATE(inode)))
+	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
 		return 0;
 
 	sid = cred_sid(cred);
-	isec = inode_security(inode);
+	isec = dentry_security(dentry);
 
 	return avc_has_perm(sid, isec->sid, isec->sclass, perms, adp);
 }
 
-/* Same as inode_has_perm, but pass explicit audit data containing
+/* Same as __dentry_has_perm, but pass explicit audit data containing
    the dentry to help the auditing code to more easily generate the
    pathname if needed. */
 static inline int dentry_has_perm(const struct cred *cred,
 				  struct dentry *dentry,
 				  u32 av)
 {
-	struct inode *inode = d_backing_inode(dentry);
 	struct common_audit_data ad;
 
 	ad.type = LSM_AUDIT_DATA_DENTRY;
 	ad.u.dentry = dentry;
-	return inode_has_perm(cred, inode, av, &ad);
+	return __dentry_has_perm(cred, dentry, av, &ad);
 }
 
-/* Same as inode_has_perm, but pass explicit audit data containing
+/* Same as __dentry_has_perm, but pass explicit audit data containing
    the path to help the auditing code to more easily generate the
    pathname if needed. */
 static inline int path_has_perm(const struct cred *cred,
 				const struct path *path,
 				u32 av)
 {
-	struct inode *inode = d_backing_inode(path->dentry);
 	struct common_audit_data ad;
 
 	ad.type = LSM_AUDIT_DATA_PATH;
 	ad.u.path = *path;
-	return inode_has_perm(cred, inode, av, &ad);
+	return __dentry_has_perm(cred, path->dentry, av, &ad);
 }
 
 /* Check whether a task can use an open file descriptor to
@@ -1686,7 +1684,6 @@ static int file_has_perm(const struct cred *cred,
 			 u32 av)
 {
 	struct file_security_struct *fsec = file->f_security;
-	struct inode *inode = file_inode(file);
 	struct common_audit_data ad;
 	u32 sid = cred_sid(cred);
 	int rc;
@@ -1706,7 +1703,7 @@ static int file_has_perm(const struct cred *cred,
 	/* av is zero if only checking access to the descriptor. */
 	rc = 0;
 	if (av)
-		rc = inode_has_perm(cred, inode, av, &ad);
+		rc = __dentry_has_perm(cred, file->f_path.dentry, av, &ad);
 
 out:
 	return rc;
-- 
2.5.0

[PATCH v3 5/7] security: Add hook to invalidate inode security labels

[Andreas Gruenbacher]

Add a hook to invalidate an inode's security label when the cached
information becomes invalid.

Implement the new hook in selinux: set a flag when a security label becomes
invalid.  When hitting a security label which has been marked as invalid in
inode_has_perm, try reloading the label.

If an inode does not have any dentries attached, we cannot reload its
security label because we cannot use the getxattr inode operation.  In that
case, continue using the old, invalid label until a dentry becomes
available.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 include/linux/lsm_hooks.h         |  6 ++++++
 include/linux/security.h          |  5 +++++
 security/security.c               |  8 ++++++++
 security/selinux/hooks.c          | 30 ++++++++++++++++++++----------
 security/selinux/include/objsec.h |  6 ++++++
 5 files changed, 45 insertions(+), 10 deletions(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index ec3a6ba..945ae1d 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1261,6 +1261,10 @@
  *	audit_rule_init.
  *	@rule contains the allocated rule
  *
+ * @inode_invalidate_secctx:
+ *	Notify the security module that it must revalidate the security context
+ *	of an inode.
+ *
  * @inode_notifysecctx:
  *	Notify the security module of what the security context of an inode
  *	should be.  Initializes the incore security context managed by the
@@ -1516,6 +1520,7 @@ union security_list_options {
 	int (*secctx_to_secid)(const char *secdata, u32 seclen, u32 *secid);
 	void (*release_secctx)(char *secdata, u32 seclen);
 
+	void (*inode_invalidate_secctx)(struct inode *inode);
 	int (*inode_notifysecctx)(struct inode *inode, void *ctx, u32 ctxlen);
 	int (*inode_setsecctx)(struct dentry *dentry, void *ctx, u32 ctxlen);
 	int (*inode_getsecctx)(struct inode *inode, void **ctx, u32 *ctxlen);
@@ -1757,6 +1762,7 @@ struct security_hook_heads {
 	struct list_head secid_to_secctx;
 	struct list_head secctx_to_secid;
 	struct list_head release_secctx;
+	struct list_head inode_invalidate_secctx;
 	struct list_head inode_notifysecctx;
 	struct list_head inode_setsecctx;
 	struct list_head inode_getsecctx;
diff --git a/include/linux/security.h b/include/linux/security.h
index 2f4c1f7..9692571 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -353,6 +353,7 @@ int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen);
 int security_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid);
 void security_release_secctx(char *secdata, u32 seclen);
 
+void security_inode_invalidate_secctx(struct inode *inode);
 int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
 int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen);
 int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen);
@@ -1093,6 +1094,10 @@ static inline void security_release_secctx(char *secdata, u32 seclen)
 {
 }
 
+static inline void security_inode_invalidate_secctx(struct inode *inode)
+{
+}
+
 static inline int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen)
 {
 	return -EOPNOTSUPP;
diff --git a/security/security.c b/security/security.c
index 46f405c..e4371cd 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1161,6 +1161,12 @@ void security_release_secctx(char *secdata, u32 seclen)
 }
 EXPORT_SYMBOL(security_release_secctx);
 
+void security_inode_invalidate_secctx(struct inode *inode)
+{
+	call_void_hook(inode_invalidate_secctx, inode);
+}
+EXPORT_SYMBOL(security_inode_invalidate_secctx);
+
 int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen)
 {
 	return call_int_hook(inode_notifysecctx, 0, inode, ctx, ctxlen);
@@ -1763,6 +1769,8 @@ struct security_hook_heads security_hook_heads = {
 		LIST_HEAD_INIT(security_hook_heads.secctx_to_secid),
 	.release_secctx =
 		LIST_HEAD_INIT(security_hook_heads.release_secctx),
+	.inode_invalidate_secctx =
+		LIST_HEAD_INIT(security_hook_heads.inode_invalidate_secctx),
 	.inode_notifysecctx =
 		LIST_HEAD_INIT(security_hook_heads.inode_notifysecctx),
 	.inode_setsecctx =
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 2a04729..f93dafd 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -820,7 +820,7 @@ static int selinux_set_mnt_opts(struct super_block *sb,
 			goto out;
 
 		root_isec->sid = rootcontext_sid;
-		root_isec->initialized = 1;
+		root_isec->initialized = LABEL_INITIALIZED;
 	}
 
 	if (defcontext_sid) {
@@ -1308,11 +1308,11 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent
 	unsigned len = 0;
 	int rc = 0;
 
-	if (isec->initialized)
+	if (isec->initialized == LABEL_INITIALIZED)
 		goto out;
 
 	mutex_lock(&isec->lock);
-	if (isec->initialized)
+	if (isec->initialized == LABEL_INITIALIZED)
 		goto out_unlock;
 
 	sbsec = inode->i_sb->s_security;
@@ -1484,7 +1484,7 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent
 		break;
 	}
 
-	isec->initialized = 1;
+	isec->initialized = LABEL_INITIALIZED;
 
 out_unlock:
 	mutex_unlock(&isec->lock);
@@ -2777,7 +2777,7 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
 		struct inode_security_struct *isec = inode->i_security;
 		isec->sclass = inode_mode_to_security_class(inode->i_mode);
 		isec->sid = newsid;
-		isec->initialized = 1;
+		isec->initialized = LABEL_INITIALIZED;
 	}
 
 	if (!ss_initialized || !(sbsec->flags & SBLABEL_MNT))
@@ -3075,7 +3075,7 @@ static void selinux_inode_post_setxattr(struct dentry *dentry, const char *name,
 
 	isec->sclass = inode_mode_to_security_class(inode->i_mode);
 	isec->sid = newsid;
-	isec->initialized = 1;
+	isec->initialized = LABEL_INITIALIZED;
 
 	return;
 }
@@ -3169,7 +3169,7 @@ static int selinux_inode_setsecurity(struct inode *inode, const char *name,
 
 	isec->sclass = inode_mode_to_security_class(inode->i_mode);
 	isec->sid = newsid;
-	isec->initialized = 1;
+	isec->initialized = LABEL_INITIALIZED;
 	return 0;
 }
 
@@ -3746,7 +3746,7 @@ static void selinux_task_to_inode(struct task_struct *p,
 	u32 sid = task_sid(p);
 
 	isec->sid = sid;
-	isec->initialized = 1;
+	isec->initialized = LABEL_INITIALIZED;
 }
 
 /* Returns error only if unable to parse addresses */
@@ -4077,7 +4077,7 @@ static int selinux_socket_post_create(struct socket *sock, int family,
 			return err;
 	}
 
-	isec->initialized = 1;
+	isec->initialized = LABEL_INITIALIZED;
 
 	if (sock->sk) {
 		sksec = sock->sk->sk_security;
@@ -4268,7 +4268,7 @@ static int selinux_socket_accept(struct socket *sock, struct socket *newsock)
 	isec = inode_security(SOCK_INODE(sock));
 	newisec->sclass = isec->sclass;
 	newisec->sid = isec->sid;
-	newisec->initialized = 1;
+	newisec->initialized = LABEL_INITIALIZED;
 
 	return 0;
 }
@@ -5758,6 +5758,15 @@ static void selinux_release_secctx(char *secdata, u32 seclen)
 	kfree(secdata);
 }
 
+static void selinux_inode_invalidate_secctx(struct inode *inode)
+{
+	struct inode_security_struct *isec = inode->i_security;
+
+	mutex_lock(&isec->lock);
+	isec->initialized = LABEL_INVALID;
+	mutex_unlock(&isec->lock);
+}
+
 /*
  *	called with inode->i_mutex locked
  */
@@ -5989,6 +5998,7 @@ static struct security_hook_list selinux_hooks[] = {
 	LSM_HOOK_INIT(secid_to_secctx, selinux_secid_to_secctx),
 	LSM_HOOK_INIT(secctx_to_secid, selinux_secctx_to_secid),
 	LSM_HOOK_INIT(release_secctx, selinux_release_secctx),
+	LSM_HOOK_INIT(inode_invalidate_secctx, selinux_inode_invalidate_secctx),
 	LSM_HOOK_INIT(inode_notifysecctx, selinux_inode_notifysecctx),
 	LSM_HOOK_INIT(inode_setsecctx, selinux_inode_setsecctx),
 	LSM_HOOK_INIT(inode_getsecctx, selinux_inode_getsecctx),
diff --git a/security/selinux/include/objsec.h b/security/selinux/include/objsec.h
index 81fa718..a2ae054 100644
--- a/security/selinux/include/objsec.h
+++ b/security/selinux/include/objsec.h
@@ -37,6 +37,12 @@ struct task_security_struct {
 	u32 sockcreate_sid;	/* fscreate SID */
 };
 
+enum label_initialized {
+	LABEL_MISSING,		/* not initialized */
+	LABEL_INITIALIZED,	/* inizialized */
+	LABEL_INVALID		/* invalid */
+};
+
 struct inode_security_struct {
 	struct inode *inode;	/* back pointer to inode object */
 	union {
-- 
2.5.0

[PATCH v3 6/7] selinux: Revalidate invalid inode security labels

[Andreas Gruenbacher]

When fetching inode's security label, check if they are still valid, and try
reloading invalid labels.  Reloading will fail when we are in RCU context which
doesn't allow sleeping, or when we can't find a dentry for the inode.
(Reloading happens via iop->getxattr which takes a dentry parameter.)

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 security/selinux/hooks.c | 47 +++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 39 insertions(+), 8 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index f93dafd..61aead9 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -241,22 +241,51 @@ static int inode_alloc_security(struct inode *inode)
 	return 0;
 }
 
+static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dentry);
+
 /*
- * Get the security label of a dentry's inode.
+ * Get an inode's security label.  When the label has been marked as invalid,
+ * try to reload it.  The @opt_dentry parameter should be set to a dentry of
+ * the inode; when no dentry is available, set it to NULL instead.  The @rcu
+ * parameter indicates when sleeping and thus reloading labels is not allowed;
+ * in that case, this function will return ERR_PTR(-ECHILD).
+ */
+static struct inode_security_struct *__inode_security(struct inode *inode,
+						      struct dentry *opt_dentry,
+						      bool rcu)
+{
+	struct inode_security_struct *isec = inode->i_security;
+
+	if (isec->initialized == LABEL_INVALID) {
+		if (rcu)
+			return ERR_PTR(-ECHILD);
+
+		/*
+		 * Try reloading the inode security label.  This will fail if
+		 * @opt_dentry is NULL and no dentry for this inode can be
+		 * found; in that case, we will continue using the old label.
+		 */
+		inode_doinit_with_dentry(inode, opt_dentry);
+	}
+	return isec;
+}
+
+/*
+ * Get the security label of a dentry's inode.  Function may block.
  */
 static struct inode_security_struct *dentry_security(struct dentry *dentry)
 {
 	struct inode *inode = d_backing_inode(dentry);
 
-	return inode->i_security;
+	return __inode_security(inode, dentry, false);
 }
 
 /*
- * Get the security label of an inode.
+ * Get the security label of an inode.  Function may block.
  */
 static struct inode_security_struct *inode_security(struct inode *inode)
 {
-	return inode->i_security;
+	return __inode_security(inode, NULL, false);
 }
 
 static void inode_free_rcu(struct rcu_head *head)
@@ -362,8 +391,6 @@ static const char *labeling_behaviors[7] = {
 	"uses native labeling",
 };
 
-static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dentry);
-
 static inline int inode_doinit(struct inode *inode)
 {
 	return inode_doinit_with_dentry(inode, NULL);
@@ -2858,7 +2885,9 @@ static int selinux_inode_follow_link(struct dentry *dentry, struct inode *inode,
 	ad.type = LSM_AUDIT_DATA_DENTRY;
 	ad.u.dentry = dentry;
 	sid = cred_sid(cred);
-	isec = inode_security(inode);
+	isec = __inode_security(inode, NULL, rcu);
+	if (IS_ERR(isec))
+		return PTR_ERR(isec);
 
 	return avc_has_perm_flags(sid, isec->sid, isec->sclass, FILE__READ, &ad,
 				  rcu ? MAY_NOT_BLOCK : 0);
@@ -2910,7 +2939,9 @@ static int selinux_inode_permission(struct inode *inode, int mask)
 	perms = file_mask_to_av(inode->i_mode, mask);
 
 	sid = cred_sid(cred);
-	isec = inode_security(inode);
+	isec = __inode_security(inode, NULL, flags & MAY_NOT_BLOCK);
+	if (IS_ERR(isec))
+		return PTR_ERR(isec);
 
 	rc = avc_has_perm_noaudit(sid, isec->sid, isec->sclass, perms, 0, &avd);
 	audited = avc_audit_required(perms, &avd, rc,
-- 
2.5.0

[PATCH v3 7/7] gfs2: Invalide security labels of inodes when they go invalid

[Andreas Gruenbacher]

When gfs2 releases the glock of an inode, it must invalidate all
information cached for that inode, including the page cache and acls.  Use
the new security_inode_invalidate_secctx hook to also invalidate security
labels in that case.  These items will be reread from disk when needed
after reacquiring the glock.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
Cc: Steven Whitehouse <swhiteho@redhat.com>
Cc: Bob Peterson <rpeterso@redhat.com>
Cc: cluster-devel@redhat.com
---
 fs/gfs2/glops.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/gfs2/glops.c b/fs/gfs2/glops.c
index 1f6c9c3..0833076 100644
--- a/fs/gfs2/glops.c
+++ b/fs/gfs2/glops.c
@@ -13,6 +13,7 @@
 #include <linux/gfs2_ondisk.h>
 #include <linux/bio.h>
 #include <linux/posix_acl.h>
+#include <linux/security.h>
 
 #include "gfs2.h"
 #include "incore.h"
@@ -262,6 +263,7 @@ static void inode_go_inval(struct gfs2_glock *gl, int flags)
 		if (ip) {
 			set_bit(GIF_INVALID, &ip->i_flags);
 			forget_all_cached_acls(&ip->i_inode);
+			security_inode_invalidate_secctx(&ip->i_inode);
 			gfs2_dir_hash_inval(ip);
 		}
 	}
-- 
2.5.0

[Cluster-devel] [PATCH v3 7/7] gfs2: Invalide security labels of inodes when they go invalid

[Andreas Gruenbacher]

When gfs2 releases the glock of an inode, it must invalidate all
information cached for that inode, including the page cache and acls.  Use
the new security_inode_invalidate_secctx hook to also invalidate security
labels in that case.  These items will be reread from disk when needed
after reacquiring the glock.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
Cc: Steven Whitehouse <swhiteho@redhat.com>
Cc: Bob Peterson <rpeterso@redhat.com>
Cc: cluster-devel at redhat.com
---
 fs/gfs2/glops.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/gfs2/glops.c b/fs/gfs2/glops.c
index 1f6c9c3..0833076 100644
--- a/fs/gfs2/glops.c
+++ b/fs/gfs2/glops.c
@@ -13,6 +13,7 @@
 #include <linux/gfs2_ondisk.h>
 #include <linux/bio.h>
 #include <linux/posix_acl.h>
+#include <linux/security.h>
 
 #include "gfs2.h"
 #include "incore.h"
@@ -262,6 +263,7 @@ static void inode_go_inval(struct gfs2_glock *gl, int flags)
 		if (ip) {
 			set_bit(GIF_INVALID, &ip->i_flags);
 			forget_all_cached_acls(&ip->i_inode);
+			security_inode_invalidate_secctx(&ip->i_inode);
 			gfs2_dir_hash_inval(ip);
 		}
 	}
-- 
2.5.0

Re: [PATCH v3 0/7] Inode security label invalidation

[Stephen Smalley]

On 10/26/2015 05:15 PM, Andreas Gruenbacher wrote:
> Here is another version of the patch queue to make gfs2 and similar file
> systems work with SELinux.  As suggested by Stephen Smalley [*], the relevant
> uses of inode->security are wrapped in function calls that try to revalidate
> invalid labels.
>
>    [*] http://marc.info/?l=linux-kernel&m=144416710207686&w=2
>
> The patches are looking good from my point of view; is there anything else that
> needs addressing?
>
> Does SELinux have test suites that these patches could be tested agains?

git clone https://github.com/SELinuxProject/selinux-testsuite
sudo yum install perl-Test perl-Test-Harness selinux-policy-devel gcc 
libselinux-devel net-tools netlabel_tools iptables
cd selinux-testsuite
sudo make test

>
> Thanks,
> Andreas
>
> Andreas Gruenbacher (7):
>    selinux: Remove unused variable in selinux_inode_init_security
>    selinux: Add accessor functions for inode->i_security
>    selinux: Get rid of file_path_has_perm
>    selinux: Push dentry down from {dentry,path,file}_has_perm
>    security: Add hook to invalidate inode security labels
>    selinux: Revalidate invalid inode security labels
>    gfs2: Invalide security labels of inodes when they go invalid
>
>   fs/gfs2/glops.c                   |   2 +
>   include/linux/lsm_hooks.h         |   6 ++
>   include/linux/security.h          |   5 +
>   security/security.c               |   8 ++
>   security/selinux/hooks.c          | 213 ++++++++++++++++++++++----------------
>   security/selinux/include/objsec.h |   6 ++
>   6 files changed, 152 insertions(+), 88 deletions(-)
>

[Ocfs2-devel] [PATCH v3 0/7] Inode security label invalidation

[Stephen Smalley]

On 10/26/2015 05:15 PM, Andreas Gruenbacher wrote:
> Here is another version of the patch queue to make gfs2 and similar file
> systems work with SELinux.  As suggested by Stephen Smalley [*], the relevant
> uses of inode->security are wrapped in function calls that try to revalidate
> invalid labels.
>
>    [*] http://marc.info/?l=linux-kernel&m=144416710207686&w=2
>
> The patches are looking good from my point of view; is there anything else that
> needs addressing?
>
> Does SELinux have test suites that these patches could be tested agains?

git clone https://github.com/SELinuxProject/selinux-testsuite
sudo yum install perl-Test perl-Test-Harness selinux-policy-devel gcc 
libselinux-devel net-tools netlabel_tools iptables
cd selinux-testsuite
sudo make test

>
> Thanks,
> Andreas
>
> Andreas Gruenbacher (7):
>    selinux: Remove unused variable in selinux_inode_init_security
>    selinux: Add accessor functions for inode->i_security
>    selinux: Get rid of file_path_has_perm
>    selinux: Push dentry down from {dentry,path,file}_has_perm
>    security: Add hook to invalidate inode security labels
>    selinux: Revalidate invalid inode security labels
>    gfs2: Invalide security labels of inodes when they go invalid
>
>   fs/gfs2/glops.c                   |   2 +
>   include/linux/lsm_hooks.h         |   6 ++
>   include/linux/security.h          |   5 +
>   security/security.c               |   8 ++
>   security/selinux/hooks.c          | 213 ++++++++++++++++++++++----------------
>   security/selinux/include/objsec.h |   6 ++
>   6 files changed, 152 insertions(+), 88 deletions(-)
>

Re: [PATCH v3 1/7] selinux: Remove unused variable in selinux_inode_init_security

[Stephen Smalley]

On 10/26/2015 05:15 PM, Andreas Gruenbacher wrote:
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>

Acked-by:  Stephen Smalley <sds@tycho.nsa.gov>

> ---
>   security/selinux/hooks.c | 2 --
>   1 file changed, 2 deletions(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index e4369d8..fc8f626 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2756,13 +2756,11 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
>   				       void **value, size_t *len)
>   {
>   	const struct task_security_struct *tsec = current_security();
> -	struct inode_security_struct *dsec;
>   	struct superblock_security_struct *sbsec;
>   	u32 sid, newsid, clen;
>   	int rc;
>   	char *context;
>
> -	dsec = dir->i_security;
>   	sbsec = dir->i_sb->s_security;
>
>   	sid = tsec->sid;
>

[Ocfs2-devel] [PATCH v3 1/7] selinux: Remove unused variable in selinux_inode_init_security

[Stephen Smalley]

On 10/26/2015 05:15 PM, Andreas Gruenbacher wrote:
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>

Acked-by:  Stephen Smalley <sds@tycho.nsa.gov>

> ---
>   security/selinux/hooks.c | 2 --
>   1 file changed, 2 deletions(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index e4369d8..fc8f626 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2756,13 +2756,11 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
>   				       void **value, size_t *len)
>   {
>   	const struct task_security_struct *tsec = current_security();
> -	struct inode_security_struct *dsec;
>   	struct superblock_security_struct *sbsec;
>   	u32 sid, newsid, clen;
>   	int rc;
>   	char *context;
>
> -	dsec = dir->i_security;
>   	sbsec = dir->i_sb->s_security;
>
>   	sid = tsec->sid;
>

Re: [PATCH v3 3/7] selinux: Get rid of file_path_has_perm

[Stephen Smalley]

On 10/26/2015 05:15 PM, Andreas Gruenbacher wrote:
> Use path_has_perm directly instead.

This reverts:

commit 13f8e9810bff12d01807b6f92329111f45218235
Author: David Howells <dhowells@redhat.com>
Date:   Thu Jun 13 23:37:55 2013 +0100

     SELinux: Institute file_path_has_perm()

     Create a file_path_has_perm() function that is like path_has_perm() but
     instead takes a file struct that is the source of both the path and the
     inode (rather than getting the inode from the dentry in the path). 
  This
     is then used where appropriate.

     This will be useful for situations like unionmount where it will be
     possible to have an apparently-negative dentry (eg. a fallthrough) 
that is
     open with the file struct pointing to an inode on the lower fs.

     Signed-off-by: David Howells <dhowells@redhat.com>
     Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

which I think David was intending to use as part of his 
SELinux/overlayfs support.

path_has_perm() uses d_backing_inode(path->dentry), while 
file_path_has_perm() uses file_inode(file).

>
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> ---
>   security/selinux/hooks.c | 18 +++---------------
>   1 file changed, 3 insertions(+), 15 deletions(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 65e8689..d6b4dc9 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -1673,18 +1673,6 @@ static inline int path_has_perm(const struct cred *cred,
>   	return inode_has_perm(cred, inode, av, &ad);
>   }
>
> -/* Same as path_has_perm, but uses the inode from the file struct. */
> -static inline int file_path_has_perm(const struct cred *cred,
> -				     struct file *file,
> -				     u32 av)
> -{
> -	struct common_audit_data ad;
> -
> -	ad.type = LSM_AUDIT_DATA_PATH;
> -	ad.u.path = file->f_path;
> -	return inode_has_perm(cred, file_inode(file), av, &ad);
> -}
> -
>   /* Check whether a task can use an open file descriptor to
>      access an inode in a given way.  Check access to the
>      descriptor itself, and then use dentry_has_perm to
> @@ -2371,14 +2359,14 @@ static inline void flush_unauthorized_files(const struct cred *cred,
>   			struct tty_file_private *file_priv;
>
>   			/* Revalidate access to controlling tty.
> -			   Use file_path_has_perm on the tty path directly
> +			   Use path_has_perm on the tty path directly
>   			   rather than using file_has_perm, as this particular
>   			   open file may belong to another process and we are
>   			   only interested in the inode-based check here. */
>   			file_priv = list_first_entry(&tty->tty_files,
>   						struct tty_file_private, list);
>   			file = file_priv->file;
> -			if (file_path_has_perm(cred, file, FILE__READ | FILE__WRITE))
> +			if (path_has_perm(cred, &file->f_path, FILE__READ | FILE__WRITE))
>   				drop_tty = 1;
>   		}
>   		spin_unlock(&tty_files_lock);
> @@ -3537,7 +3525,7 @@ static int selinux_file_open(struct file *file, const struct cred *cred)
>   	 * new inode label or new policy.
>   	 * This check is not redundant - do not remove.
>   	 */
> -	return file_path_has_perm(cred, file, open_file_to_av(file));
> +	return path_has_perm(cred, &file->f_path, open_file_to_av(file));
>   }
>
>   /* task security operations */
>

[Ocfs2-devel] [PATCH v3 3/7] selinux: Get rid of file_path_has_perm

[Stephen Smalley]

On 10/26/2015 05:15 PM, Andreas Gruenbacher wrote:
> Use path_has_perm directly instead.

This reverts:

commit 13f8e9810bff12d01807b6f92329111f45218235
Author: David Howells <dhowells@redhat.com>
Date:   Thu Jun 13 23:37:55 2013 +0100

     SELinux: Institute file_path_has_perm()

     Create a file_path_has_perm() function that is like path_has_perm() but
     instead takes a file struct that is the source of both the path and the
     inode (rather than getting the inode from the dentry in the path). 
  This
     is then used where appropriate.

     This will be useful for situations like unionmount where it will be
     possible to have an apparently-negative dentry (eg. a fallthrough) 
that is
     open with the file struct pointing to an inode on the lower fs.

     Signed-off-by: David Howells <dhowells@redhat.com>
     Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

which I think David was intending to use as part of his 
SELinux/overlayfs support.

path_has_perm() uses d_backing_inode(path->dentry), while 
file_path_has_perm() uses file_inode(file).

>
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> ---
>   security/selinux/hooks.c | 18 +++---------------
>   1 file changed, 3 insertions(+), 15 deletions(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 65e8689..d6b4dc9 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -1673,18 +1673,6 @@ static inline int path_has_perm(const struct cred *cred,
>   	return inode_has_perm(cred, inode, av, &ad);
>   }
>
> -/* Same as path_has_perm, but uses the inode from the file struct. */
> -static inline int file_path_has_perm(const struct cred *cred,
> -				     struct file *file,
> -				     u32 av)
> -{
> -	struct common_audit_data ad;
> -
> -	ad.type = LSM_AUDIT_DATA_PATH;
> -	ad.u.path = file->f_path;
> -	return inode_has_perm(cred, file_inode(file), av, &ad);
> -}
> -
>   /* Check whether a task can use an open file descriptor to
>      access an inode in a given way.  Check access to the
>      descriptor itself, and then use dentry_has_perm to
> @@ -2371,14 +2359,14 @@ static inline void flush_unauthorized_files(const struct cred *cred,
>   			struct tty_file_private *file_priv;
>
>   			/* Revalidate access to controlling tty.
> -			   Use file_path_has_perm on the tty path directly
> +			   Use path_has_perm on the tty path directly
>   			   rather than using file_has_perm, as this particular
>   			   open file may belong to another process and we are
>   			   only interested in the inode-based check here. */
>   			file_priv = list_first_entry(&tty->tty_files,
>   						struct tty_file_private, list);
>   			file = file_priv->file;
> -			if (file_path_has_perm(cred, file, FILE__READ | FILE__WRITE))
> +			if (path_has_perm(cred, &file->f_path, FILE__READ | FILE__WRITE))
>   				drop_tty = 1;
>   		}
>   		spin_unlock(&tty_files_lock);
> @@ -3537,7 +3525,7 @@ static int selinux_file_open(struct file *file, const struct cred *cred)
>   	 * new inode label or new policy.
>   	 * This check is not redundant - do not remove.
>   	 */
> -	return file_path_has_perm(cred, file, open_file_to_av(file));
> +	return path_has_perm(cred, &file->f_path, open_file_to_av(file));
>   }
>
>   /* task security operations */
>

Re: [PATCH v3 2/7] selinux: Add accessor functions for inode->i_security

[Stephen Smalley]

On 10/26/2015 05:15 PM, Andreas Gruenbacher wrote:
> Add functions dentry_security and inode_security for accessing
> inode->i_security.  These functions initially don't do much, but they
> will later be used to revalidate the security labels when necessary.
>
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> ---
>   security/selinux/hooks.c | 101 ++++++++++++++++++++++++++---------------------
>   1 file changed, 57 insertions(+), 44 deletions(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index fc8f626..65e8689 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -241,6 +241,24 @@ static int inode_alloc_security(struct inode *inode)
>   	return 0;
>   }
>
> +/*
> + * Get the security label of a dentry's inode.
> + */
> +static struct inode_security_struct *dentry_security(struct dentry *dentry)
> +{
> +	struct inode *inode = d_backing_inode(dentry);
> +
> +	return inode->i_security;
> +}
> +
> +/*
> + * Get the security label of an inode.
> + */
> +static struct inode_security_struct *inode_security(struct inode *inode)
> +{
> +	return inode->i_security;
> +}
> +
>   static void inode_free_rcu(struct rcu_head *head)
>   {
>   	struct inode_security_struct *isec;
<snip>
> @@ -2207,7 +2222,6 @@ static int selinux_bprm_set_creds(struct linux_binprm *bprm)
>   	struct task_security_struct *new_tsec;
>   	struct inode_security_struct *isec;
>   	struct common_audit_data ad;
> -	struct inode *inode = file_inode(bprm->file);
>   	int rc;
>
>   	/* SELinux context only depends on initial program or script and not
> @@ -2217,7 +2231,7 @@ static int selinux_bprm_set_creds(struct linux_binprm *bprm)
>
>   	old_tsec = current_security();
>   	new_tsec = bprm->cred->security;
> -	isec = inode->i_security;
> +	isec = dentry_security(bprm->file->f_path.dentry);

IIUC, this could change which inode label gets used when using overlayfs 
(the overlay inode or the underlying inode).  Not sure whether the 
current code is correct for overlayfs (overlayfs + SELinux support still 
in progress).

> @@ -3154,7 +3168,7 @@ out_nofree:
>   static int selinux_inode_setsecurity(struct inode *inode, const char *name,
>   				     const void *value, size_t size, int flags)
>   {
> -	struct inode_security_struct *isec = inode->i_security;
> +	struct inode_security_struct *isec = inode_security(inode);

Was it intentional to not do this for selinux_inode_getsecurity() and 
selinux_inode_getsecid()?

> @@ -3241,8 +3254,8 @@ int ioctl_has_perm(const struct cred *cred, struct file *file,
>   {
>   	struct common_audit_data ad;
>   	struct file_security_struct *fsec = file->f_security;
> -	struct inode *inode = file_inode(file);
> -	struct inode_security_struct *isec = inode->i_security;
> +	struct dentry *dentry = file->f_path.dentry;
> +	struct inode_security_struct *isec = dentry_security(dentry);
>   	struct lsm_ioctlop_audit ioctl;
>   	u32 ssid = cred_sid(cred);
>   	int rc;
> @@ -3263,7 +3276,7 @@ int ioctl_has_perm(const struct cred *cred, struct file *file,
>   			goto out;
>   	}
>
> -	if (unlikely(IS_PRIVATE(inode)))
> +	if (unlikely(IS_PRIVATE(dentry->d_inode)))
>   		return 0;
>
>   	rc = avc_has_extended_perms(ssid, isec->sid, isec->sclass,
> @@ -3506,7 +3519,7 @@ static int selinux_file_open(struct file *file, const struct cred *cred)
>   	struct inode_security_struct *isec;
>
>   	fsec = file->f_security;
> -	isec = file_inode(file)->i_security;
> +	isec = dentry_security(file->f_path.dentry);

Similarly for these cases, switching from file_inode(file) to 
d_backing_inode(dentry) could affect overlayfs interaction IIUC.  cc'd 
David for clarification.

[Ocfs2-devel] [PATCH v3 2/7] selinux: Add accessor functions for inode->i_security

[Stephen Smalley]

On 10/26/2015 05:15 PM, Andreas Gruenbacher wrote:
> Add functions dentry_security and inode_security for accessing
> inode->i_security.  These functions initially don't do much, but they
> will later be used to revalidate the security labels when necessary.
>
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> ---
>   security/selinux/hooks.c | 101 ++++++++++++++++++++++++++---------------------
>   1 file changed, 57 insertions(+), 44 deletions(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index fc8f626..65e8689 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -241,6 +241,24 @@ static int inode_alloc_security(struct inode *inode)
>   	return 0;
>   }
>
> +/*
> + * Get the security label of a dentry's inode.
> + */
> +static struct inode_security_struct *dentry_security(struct dentry *dentry)
> +{
> +	struct inode *inode = d_backing_inode(dentry);
> +
> +	return inode->i_security;
> +}
> +
> +/*
> + * Get the security label of an inode.
> + */
> +static struct inode_security_struct *inode_security(struct inode *inode)
> +{
> +	return inode->i_security;
> +}
> +
>   static void inode_free_rcu(struct rcu_head *head)
>   {
>   	struct inode_security_struct *isec;
<snip>
> @@ -2207,7 +2222,6 @@ static int selinux_bprm_set_creds(struct linux_binprm *bprm)
>   	struct task_security_struct *new_tsec;
>   	struct inode_security_struct *isec;
>   	struct common_audit_data ad;
> -	struct inode *inode = file_inode(bprm->file);
>   	int rc;
>
>   	/* SELinux context only depends on initial program or script and not
> @@ -2217,7 +2231,7 @@ static int selinux_bprm_set_creds(struct linux_binprm *bprm)
>
>   	old_tsec = current_security();
>   	new_tsec = bprm->cred->security;
> -	isec = inode->i_security;
> +	isec = dentry_security(bprm->file->f_path.dentry);

IIUC, this could change which inode label gets used when using overlayfs 
(the overlay inode or the underlying inode).  Not sure whether the 
current code is correct for overlayfs (overlayfs + SELinux support still 
in progress).

> @@ -3154,7 +3168,7 @@ out_nofree:
>   static int selinux_inode_setsecurity(struct inode *inode, const char *name,
>   				     const void *value, size_t size, int flags)
>   {
> -	struct inode_security_struct *isec = inode->i_security;
> +	struct inode_security_struct *isec = inode_security(inode);

Was it intentional to not do this for selinux_inode_getsecurity() and 
selinux_inode_getsecid()?

> @@ -3241,8 +3254,8 @@ int ioctl_has_perm(const struct cred *cred, struct file *file,
>   {
>   	struct common_audit_data ad;
>   	struct file_security_struct *fsec = file->f_security;
> -	struct inode *inode = file_inode(file);
> -	struct inode_security_struct *isec = inode->i_security;
> +	struct dentry *dentry = file->f_path.dentry;
> +	struct inode_security_struct *isec = dentry_security(dentry);
>   	struct lsm_ioctlop_audit ioctl;
>   	u32 ssid = cred_sid(cred);
>   	int rc;
> @@ -3263,7 +3276,7 @@ int ioctl_has_perm(const struct cred *cred, struct file *file,
>   			goto out;
>   	}
>
> -	if (unlikely(IS_PRIVATE(inode)))
> +	if (unlikely(IS_PRIVATE(dentry->d_inode)))
>   		return 0;
>
>   	rc = avc_has_extended_perms(ssid, isec->sid, isec->sclass,
> @@ -3506,7 +3519,7 @@ static int selinux_file_open(struct file *file, const struct cred *cred)
>   	struct inode_security_struct *isec;
>
>   	fsec = file->f_security;
> -	isec = file_inode(file)->i_security;
> +	isec = dentry_security(file->f_path.dentry);

Similarly for these cases, switching from file_inode(file) to 
d_backing_inode(dentry) could affect overlayfs interaction IIUC.  cc'd 
David for clarification.

Re: [PATCH v3 5/7] security: Add hook to invalidate inode security labels

[James Morris]

On Mon, 26 Oct 2015, Andreas Gruenbacher wrote:

> Add a hook to invalidate an inode's security label when the cached
> information becomes invalid.
> 
> Implement the new hook in selinux: set a flag when a security label becomes
> invalid.  When hitting a security label which has been marked as invalid in
> inode_has_perm, try reloading the label.
> 
> If an inode does not have any dentries attached, we cannot reload its
> security label because we cannot use the getxattr inode operation.  In that
> case, continue using the old, invalid label until a dentry becomes
> available.
> 
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>


Reviewed-by: James Morris <james.l.morris@oracle.com>

-- 
James Morris
<jmorris@namei.org>

[Ocfs2-devel] [PATCH v3 5/7] security: Add hook to invalidate inode security labels

[James Morris]

On Mon, 26 Oct 2015, Andreas Gruenbacher wrote:

> Add a hook to invalidate an inode's security label when the cached
> information becomes invalid.
> 
> Implement the new hook in selinux: set a flag when a security label becomes
> invalid.  When hitting a security label which has been marked as invalid in
> inode_has_perm, try reloading the label.
> 
> If an inode does not have any dentries attached, we cannot reload its
> security label because we cannot use the getxattr inode operation.  In that
> case, continue using the old, invalid label until a dentry becomes
> available.
> 
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>


Reviewed-by: James Morris <james.l.morris@oracle.com>

-- 
James Morris
<jmorris@namei.org>

Re: [PATCH v3 5/7] security: Add hook to invalidate inode security labels

[James Morris]

On Mon, 26 Oct 2015, Andreas Gruenbacher wrote:

> Add a hook to invalidate an inode's security label when the cached
> information becomes invalid.
> 
> Implement the new hook in selinux: set a flag when a security label becomes
> invalid.  When hitting a security label which has been marked as invalid in
> inode_has_perm, try reloading the label.
> 
> If an inode does not have any dentries attached, we cannot reload its
> security label because we cannot use the getxattr inode operation.  In that
> case, continue using the old, invalid label until a dentry becomes
> available.
> 
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>


Reviewed-by: James Morris <james.l.morris@oracle.com>

-- 
James Morris
<jmorris@namei.org>

[Ocfs2-devel] [PATCH v3 5/7] security: Add hook to invalidate inode security labels

[James Morris]

On Mon, 26 Oct 2015, Andreas Gruenbacher wrote:

> Add a hook to invalidate an inode's security label when the cached
> information becomes invalid.
> 
> Implement the new hook in selinux: set a flag when a security label becomes
> invalid.  When hitting a security label which has been marked as invalid in
> inode_has_perm, try reloading the label.
> 
> If an inode does not have any dentries attached, we cannot reload its
> security label because we cannot use the getxattr inode operation.  In that
> case, continue using the old, invalid label until a dentry becomes
> available.
> 
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>


Reviewed-by: James Morris <james.l.morris@oracle.com>

-- 
James Morris
<jmorris@namei.org>

Re: [PATCH v3 3/7] selinux: Get rid of file_path_has_perm

[Andreas Gruenbacher]

On Tue, Oct 27, 2015 at 5:40 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On 10/26/2015 05:15 PM, Andreas Gruenbacher wrote:
>>
>> Use path_has_perm directly instead.
>
>
> This reverts:
>
> commit 13f8e9810bff12d01807b6f92329111f45218235
> Author: David Howells <dhowells@redhat.com>
> Date:   Thu Jun 13 23:37:55 2013 +0100
>
>     SELinux: Institute file_path_has_perm()
>
>     Create a file_path_has_perm() function that is like path_has_perm() but
>     instead takes a file struct that is the source of both the path and the
>     inode (rather than getting the inode from the dentry in the path).  This
>     is then used where appropriate.
>
>     This will be useful for situations like unionmount where it will be
>     possible to have an apparently-negative dentry (eg. a fallthrough) that
> is
>     open with the file struct pointing to an inode on the lower fs.
>
>     Signed-off-by: David Howells <dhowells@redhat.com>
>     Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
>
> which I think David was intending to use as part of his SELinux/overlayfs
> support.

Okay. As long as overlayfs support in SELinux is in half-finished
state, let's leave this alone.

Thanks,
Andreas

Re: [PATCH v3 2/7] selinux: Add accessor functions for inode->i_security

[Andreas Gruenbacher]

On Tue, Oct 27, 2015 at 6:20 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On 10/26/2015 05:15 PM, Andreas Gruenbacher wrote:
>> @@ -2217,7 +2231,7 @@ static int selinux_bprm_set_creds(struct
>> linux_binprm *bprm)
>>
>>         old_tsec = current_security();
>>         new_tsec = bprm->cred->security;
>> -       isec = inode->i_security;
>> +       isec = dentry_security(bprm->file->f_path.dentry);
>
> IIUC, this could change which inode label gets used when using overlayfs
> (the overlay inode or the underlying inode).  Not sure whether the current
> code is correct for overlayfs (overlayfs + SELinux support still in
> progress).

Okay, let's stick with inode_security, at least for now.

>> @@ -3154,7 +3168,7 @@ out_nofree:
>>   static int selinux_inode_setsecurity(struct inode *inode, const char
>> *name,
>>                                      const void *value, size_t size, int
>> flags)
>>   {
>> -       struct inode_security_struct *isec = inode->i_security;
>> +       struct inode_security_struct *isec = inode_security(inode);
>
> Was it intentional to not do this for selinux_inode_getsecurity() and
> selinux_inode_getsecid()?

These two hooks both pass in a const inode *, so that needs to be
changed first. Then, selinux_inode_getsecurity should obviously use
inode_security.

I'm not really sure about selinux_inode_getsecid though: can it be
call it from a non-sleeping context?

>> @@ -3241,8 +3254,8 @@ int ioctl_has_perm(const struct cred *cred, struct
>> file *file,
>>   {
>>         struct common_audit_data ad;
>>         struct file_security_struct *fsec = file->f_security;
>> -       struct inode *inode = file_inode(file);
>> -       struct inode_security_struct *isec = inode->i_security;
>> +       struct dentry *dentry = file->f_path.dentry;
>> +       struct inode_security_struct *isec = dentry_security(dentry);
>>         struct lsm_ioctlop_audit ioctl;
>>         u32 ssid = cred_sid(cred);
>>         int rc;
>> @@ -3263,7 +3276,7 @@ int ioctl_has_perm(const struct cred *cred, struct
>> file *file,
>>                         goto out;
>>         }
>>
>> -       if (unlikely(IS_PRIVATE(inode)))
>> +       if (unlikely(IS_PRIVATE(dentry->d_inode)))
>>                 return 0;
>>
>>         rc = avc_has_extended_perms(ssid, isec->sid, isec->sclass,
>> @@ -3506,7 +3519,7 @@ static int selinux_file_open(struct file *file,
>> const struct cred *cred)
>>         struct inode_security_struct *isec;
>>
>>         fsec = file->f_security;
>> -       isec = file_inode(file)->i_security;
>> +       isec = dentry_security(file->f_path.dentry);
>
>
> Similarly for these cases, switching from file_inode(file) to
> d_backing_inode(dentry) could affect overlayfs interaction IIUC.

Okay, let's stick with inode_security as well for now.

Thanks,
Andreas

Re: [PATCH v3 3/7] selinux: Get rid of file_path_has_perm

[Stephen Smalley]

On 10/28/2015 07:48 AM, Andreas Gruenbacher wrote:
> On Tue, Oct 27, 2015 at 5:40 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
>> On 10/26/2015 05:15 PM, Andreas Gruenbacher wrote:
>>>
>>> Use path_has_perm directly instead.
>>
>>
>> This reverts:
>>
>> commit 13f8e9810bff12d01807b6f92329111f45218235
>> Author: David Howells <dhowells@redhat.com>
>> Date:   Thu Jun 13 23:37:55 2013 +0100
>>
>>     SELinux: Institute file_path_has_perm()
>>
>>     Create a file_path_has_perm() function that is like path_has_perm() but
>>     instead takes a file struct that is the source of both the path and the
>>     inode (rather than getting the inode from the dentry in the path).  This
>>     is then used where appropriate.
>>
>>     This will be useful for situations like unionmount where it will be
>>     possible to have an apparently-negative dentry (eg. a fallthrough) that
>> is
>>     open with the file struct pointing to an inode on the lower fs.
>>
>>     Signed-off-by: David Howells <dhowells@redhat.com>
>>     Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
>>
>> which I think David was intending to use as part of his SELinux/overlayfs
>> support.
> 
> Okay. As long as overlayfs support in SELinux is in half-finished
> state, let's leave this alone.

Also, the caller is holding a spinlock (tty_files_lock), so you can't call inode_doinit from
here.

Try stress testing your patch series by just always setting isec->initialized to LABEL_INVALID.
Previously the *has_perm functions could be called under essentially any condition, with the exception
of when in a RCU walk and needing to audit the dname (but they did not previously block/sleep).

[Ocfs2-devel] [PATCH v3 3/7] selinux: Get rid of file_path_has_perm

[Stephen Smalley]

On 10/28/2015 07:48 AM, Andreas Gruenbacher wrote:
> On Tue, Oct 27, 2015 at 5:40 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
>> On 10/26/2015 05:15 PM, Andreas Gruenbacher wrote:
>>>
>>> Use path_has_perm directly instead.
>>
>>
>> This reverts:
>>
>> commit 13f8e9810bff12d01807b6f92329111f45218235
>> Author: David Howells <dhowells@redhat.com>
>> Date:   Thu Jun 13 23:37:55 2013 +0100
>>
>>     SELinux: Institute file_path_has_perm()
>>
>>     Create a file_path_has_perm() function that is like path_has_perm() but
>>     instead takes a file struct that is the source of both the path and the
>>     inode (rather than getting the inode from the dentry in the path).  This
>>     is then used where appropriate.
>>
>>     This will be useful for situations like unionmount where it will be
>>     possible to have an apparently-negative dentry (eg. a fallthrough) that
>> is
>>     open with the file struct pointing to an inode on the lower fs.
>>
>>     Signed-off-by: David Howells <dhowells@redhat.com>
>>     Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
>>
>> which I think David was intending to use as part of his SELinux/overlayfs
>> support.
> 
> Okay. As long as overlayfs support in SELinux is in half-finished
> state, let's leave this alone.

Also, the caller is holding a spinlock (tty_files_lock), so you can't call inode_doinit from
here.

Try stress testing your patch series by just always setting isec->initialized to LABEL_INVALID.
Previously the *has_perm functions could be called under essentially any condition, with the exception
of when in a RCU walk and needing to audit the dname (but they did not previously block/sleep).

Re: [PATCH v3 3/7] selinux: Get rid of file_path_has_perm

[Stephen Smalley]

On 10/28/2015 01:31 PM, Stephen Smalley wrote:
> On 10/28/2015 07:48 AM, Andreas Gruenbacher wrote:
>> On Tue, Oct 27, 2015 at 5:40 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
>>> On 10/26/2015 05:15 PM, Andreas Gruenbacher wrote:
>>>>
>>>> Use path_has_perm directly instead.
>>>
>>>
>>> This reverts:
>>>
>>> commit 13f8e9810bff12d01807b6f92329111f45218235
>>> Author: David Howells <dhowells@redhat.com>
>>> Date:   Thu Jun 13 23:37:55 2013 +0100
>>>
>>>      SELinux: Institute file_path_has_perm()
>>>
>>>      Create a file_path_has_perm() function that is like path_has_perm() but
>>>      instead takes a file struct that is the source of both the path and the
>>>      inode (rather than getting the inode from the dentry in the path).  This
>>>      is then used where appropriate.
>>>
>>>      This will be useful for situations like unionmount where it will be
>>>      possible to have an apparently-negative dentry (eg. a fallthrough) that
>>> is
>>>      open with the file struct pointing to an inode on the lower fs.
>>>
>>>      Signed-off-by: David Howells <dhowells@redhat.com>
>>>      Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
>>>
>>> which I think David was intending to use as part of his SELinux/overlayfs
>>> support.
>>
>> Okay. As long as overlayfs support in SELinux is in half-finished
>> state, let's leave this alone.
>
> Also, the caller is holding a spinlock (tty_files_lock), so you can't call inode_doinit from
> here.
>
> Try stress testing your patch series by just always setting isec->initialized to LABEL_INVALID.
> Previously the *has_perm functions could be called under essentially any condition, with the exception
> of when in a RCU walk and needing to audit the dname (but they did not previously block/sleep).

file_has_perm() also gets called from match_file() callback to 
iterate_fd(), which holds files->file_lock.

[Ocfs2-devel] [PATCH v3 3/7] selinux: Get rid of file_path_has_perm

[Stephen Smalley]

On 10/28/2015 01:31 PM, Stephen Smalley wrote:
> On 10/28/2015 07:48 AM, Andreas Gruenbacher wrote:
>> On Tue, Oct 27, 2015 at 5:40 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
>>> On 10/26/2015 05:15 PM, Andreas Gruenbacher wrote:
>>>>
>>>> Use path_has_perm directly instead.
>>>
>>>
>>> This reverts:
>>>
>>> commit 13f8e9810bff12d01807b6f92329111f45218235
>>> Author: David Howells <dhowells@redhat.com>
>>> Date:   Thu Jun 13 23:37:55 2013 +0100
>>>
>>>      SELinux: Institute file_path_has_perm()
>>>
>>>      Create a file_path_has_perm() function that is like path_has_perm() but
>>>      instead takes a file struct that is the source of both the path and the
>>>      inode (rather than getting the inode from the dentry in the path).  This
>>>      is then used where appropriate.
>>>
>>>      This will be useful for situations like unionmount where it will be
>>>      possible to have an apparently-negative dentry (eg. a fallthrough) that
>>> is
>>>      open with the file struct pointing to an inode on the lower fs.
>>>
>>>      Signed-off-by: David Howells <dhowells@redhat.com>
>>>      Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
>>>
>>> which I think David was intending to use as part of his SELinux/overlayfs
>>> support.
>>
>> Okay. As long as overlayfs support in SELinux is in half-finished
>> state, let's leave this alone.
>
> Also, the caller is holding a spinlock (tty_files_lock), so you can't call inode_doinit from
> here.
>
> Try stress testing your patch series by just always setting isec->initialized to LABEL_INVALID.
> Previously the *has_perm functions could be called under essentially any condition, with the exception
> of when in a RCU walk and needing to audit the dname (but they did not previously block/sleep).

file_has_perm() also gets called from match_file() callback to 
iterate_fd(), which holds files->file_lock.

Re: [PATCH v3 0/7] Inode security label invalidation

[Paul Moore]

On Mon, Oct 26, 2015 at 5:15 PM, Andreas Gruenbacher
<agruenba@redhat.com> wrote:
> Here is another version of the patch queue to make gfs2 and similar file
> systems work with SELinux.  As suggested by Stephen Smalley [*], the relevant
> uses of inode->security are wrapped in function calls that try to revalidate
> invalid labels.
>
>   [*] http://marc.info/?l=linux-kernel&m=144416710207686&w=2
>
> The patches are looking good from my point of view; is there anything else that
> needs addressing?

Hi Andreas,

I'm largely staying out of the way on this patchset as Stephen has
been providing good review and feedback (I see he identified a few
more things in this latest revision), however, before I accept this
upstream I'd like to see an ACK from one of the GFS developers on the
last patch which touches the code under fs/gfs2.  Yes, it's a minor
change, and probably not strictly necessary, but I would like for us
to be good neighbors when possible.

Thanks.

-- 
paul moore
www.paul-moore.com

Re: [PATCH v3 0/7] Inode security label invalidation

[Andreas Gruenbacher]

On Wed, Oct 28, 2015 at 10:12 PM, Paul Moore <paul@paul-moore.com> wrote:
> On Mon, Oct 26, 2015 at 5:15 PM, Andreas Gruenbacher
> <agruenba@redhat.com> wrote:
>> Here is another version of the patch queue to make gfs2 and similar file
>> systems work with SELinux.  As suggested by Stephen Smalley [*], the relevant
>> uses of inode->security are wrapped in function calls that try to revalidate
>> invalid labels.
>>
>>   [*] http://marc.info/?l=linux-kernel&m=144416710207686&w=2
>>
>> The patches are looking good from my point of view; is there anything else that
>> needs addressing?
>
> Hi Andreas,
>
> I'm largely staying out of the way on this patchset as Stephen has
> been providing good review and feedback (I see he identified a few
> more things in this latest revision),

Yes, Stephen is being very helpful.

> however, before I accept this
> upstream I'd like to see an ACK from one of the GFS developers on the
> last patch which touches the code under fs/gfs2.

Sure, no worries there ...

Thanks,
Andreas

Re: [PATCH v3 3/7] selinux: Get rid of file_path_has_perm

[Andreas Gruenbacher]

On Wed, Oct 28, 2015 at 7:56 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On 10/28/2015 01:31 PM, Stephen Smalley wrote:
>>
>> On 10/28/2015 07:48 AM, Andreas Gruenbacher wrote:
>>>
>>> On Tue, Oct 27, 2015 at 5:40 PM, Stephen Smalley <sds@tycho.nsa.gov>
>>> wrote:
>>>>
>>>> On 10/26/2015 05:15 PM, Andreas Gruenbacher wrote:
>>>>>
>>>>>
>>>>> Use path_has_perm directly instead.
>>>>
>>>>
>>>>
>>>> This reverts:
>>>>
>>>> commit 13f8e9810bff12d01807b6f92329111f45218235
>>>> Author: David Howells <dhowells@redhat.com>
>>>> Date:   Thu Jun 13 23:37:55 2013 +0100
>>>>
>>>>      SELinux: Institute file_path_has_perm()
>>>>
>>>>      Create a file_path_has_perm() function that is like path_has_perm()
>>>> but
>>>>      instead takes a file struct that is the source of both the path and
>>>> the
>>>>      inode (rather than getting the inode from the dentry in the path).
>>>> This
>>>>      is then used where appropriate.
>>>>
>>>>      This will be useful for situations like unionmount where it will be
>>>>      possible to have an apparently-negative dentry (eg. a fallthrough)
>>>> that
>>>> is
>>>>      open with the file struct pointing to an inode on the lower fs.
>>>>
>>>>      Signed-off-by: David Howells <dhowells@redhat.com>
>>>>      Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
>>>>
>>>> which I think David was intending to use as part of his
>>>> SELinux/overlayfs
>>>> support.
>>>
>>>
>>> Okay. As long as overlayfs support in SELinux is in half-finished
>>> state, let's leave this alone.
>>
>>
>> Also, the caller is holding a spinlock (tty_files_lock), so you can't call
>> inode_doinit from
>> here.
>>
>> Try stress testing your patch series by just always setting
>> isec->initialized to LABEL_INVALID.
>> Previously the *has_perm functions could be called under essentially any
>> condition, with the exception
>> of when in a RCU walk and needing to audit the dname (but they did not
>> previously block/sleep).

Using might_sleep() is even better, then CONFIG_DEBUG_ATOMIC_SLEEP
will catch any remaining problems.

> file_has_perm() also gets called from match_file() callback to iterate_fd(),
> which holds files->file_lock.

Yes, thanks.

Andreas