Skip to contentSkip to conversation

Inode security label invalidation

PATCH Superseded Andreas Gruenbacher 4 participants 21 messages Oct 26, 2015 – Oct 29, 2015

Subsystems (4)

v3 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Andreas Gruenbacher

author maintainer
#0 lore raw

v3 cover Inode security label invalidation

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(-)
Signature · 1 line
2.5.0
author maintainer
#1 lore raw

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

---
 security/selinux/hooks.c | 2 --
 1 file changed, 2 deletions(-)
security/selinux/hooks.c +0 -2
index e4369d8..fc8f626 100644
@@ -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;
Signature · 1 line
2.5.0
#2 lore raw

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

Quoted 1 line · Andreas Gruenbacher
Quoted 23 lines
---
  security/selinux/hooks.c | 2 --
  1 file changed, 2 deletions(-)
security/selinux/hooks.c +0 -2
index e4369d8..fc8f626 100644
@@ -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;
author maintainer
#3 lore raw

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

In reply to 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.

---
 security/selinux/hooks.c | 101 ++++++++++++++++++++++++++---------------------
 1 file changed, 57 insertions(+), 44 deletions(-)
security/selinux/hooks.c +57 -44
index fc8f626..65e8689 100644
@@ -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 ||
Signature · 1 line
2.5.0
#4 lore raw

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

Quoted 38 lines · 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.

---
  security/selinux/hooks.c | 101 ++++++++++++++++++++++++++---------------------
  1 file changed, 57 insertions(+), 44 deletions(-)
security/selinux/hooks.c +18 -0
index fc8f626..65e8689 100644
@@ -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>

Quoted 9 lines
security/selinux/hooks.c +0 -1
@@ -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

security/selinux/hooks.c static int selinux_bprm_set_creds(struct linux_binprm *bprm) · line 2234

Quoted 5 lines
security/selinux/hooks.c +1 -1
@@ -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).

security/selinux/hooks.c out_nofree: · line 3171

Quoted 6 lines
security/selinux/hooks.c +1 -1
@@ -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()?

Quoted 21 lines
security/selinux/hooks.c +3 -3
@@ -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,

security/selinux/hooks.c static int selinux_file_open(struct file *file, const struct cred *cred) · line 3522

Quoted 5 lines
security/selinux/hooks.c +1 -1
@@ -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.

author maintainer
#5 lore raw

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

Quoted 4 lines · Stephen Smalley
Quoted 3 lines · Andreas Gruenbacher
(quoted diff) +0 -0
@@ -2217,7 +2231,7 @@ static int selinux_bprm_set_creds(struct
 linux_binprm *bprm)
 
Quoted 9 lines

security/selinux/hooks.c static int selinux_bprm_set_creds(struct linux_binprm *bprm) · line 2234

Quoted 4 lines
security/selinux/hooks.c +1 -1
        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.

Quoted 11 lines

Review out_nofree: · line 3173

Quoted 8 lines
(quoted diff) +1 -1
@@ -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?

Quoted 26 lines
Quoted 26 lines
security/selinux/hooks.c +2 -2
@@ -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;
security/selinux/hooks.c +1 -1
@@ -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,
security/selinux/hooks.c +0 -0
@@ -3506,7 +3519,7 @@ static int selinux_file_open(struct file *file,
 const struct cred *cred)
        struct inode_security_struct *isec;
 
Quoted 7 lines

security/selinux/hooks.c static int selinux_file_open(struct file *file, const struct cred *cred) · line 3522

Quoted 3 lines
security/selinux/hooks.c +1 -1
        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

author maintainer
#6 lore raw

PATCH v3 3/7 selinux: Get rid of file_path_has_perm

In reply to Andreas Gruenbacher

Use path_has_perm directly instead.

---
 security/selinux/hooks.c | 18 +++---------------
 1 file changed, 3 insertions(+), 15 deletions(-)
security/selinux/hooks.c +3 -15
index 65e8689..d6b4dc9 100644
@@ -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 */
Signature · 1 line
2.5.0
#7 lore raw

PATCH v3 3/7 selinux: Get rid of file_path_has_perm

Quoted 1 line · Andreas Gruenbacher

Use path_has_perm directly instead.

This reverts:

commit 13f8e9810bff12d01807b6f92329111f45218235

Author: David Howells <[email protected]>
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.

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).

Quoted 56 lines
---
  security/selinux/hooks.c | 18 +++---------------
  1 file changed, 3 insertions(+), 15 deletions(-)
security/selinux/hooks.c +3 -15
index 65e8689..d6b4dc9 100644
@@ -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 */
author maintainer
#8 lore raw

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

Quoted 23 lines · Stephen Smalley
Quoted 2 lines · Andreas Gruenbacher

Use path_has_perm directly instead.

This reverts:

commit 13f8e9810bff12d01807b6f92329111f45218235

Author: David Howells <[email protected]>
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.

Quoted 5 lines

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

#9 lore raw

PATCH v3 3/7 selinux: Get rid of file_path_has_perm

Quoted 24 lines · Andreas Gruenbacher
Quoted 23 lines · Stephen Smalley
Quoted 2 lines · Andreas Gruenbacher

Use path_has_perm directly instead.

This reverts:

commit 13f8e9810bff12d01807b6f92329111f45218235

Author: David Howells <[email protected]>
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.

Quoted 8 lines
Quoted 5 lines

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).

#10 lore raw

PATCH v3 3/7 selinux: Get rid of file_path_has_perm

Quoted 34 lines · Stephen Smalley
Quoted 24 lines · Andreas Gruenbacher
Quoted 23 lines · Stephen Smalley
Quoted 2 lines · Andreas Gruenbacher

Use path_has_perm directly instead.

This reverts:

commit 13f8e9810bff12d01807b6f92329111f45218235

Author: David Howells <[email protected]>
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.

Quoted 8 lines
Quoted 5 lines

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.

Quoted 6 lines

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.

author maintainer
#11 lore raw

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

Quoted 52 lines · Stephen Smalley
Quoted 51 lines · Stephen Smalley
Quoted 36 lines · Andreas Gruenbacher
Quoted 33 lines · Stephen Smalley
Quoted 3 lines · Andreas Gruenbacher

Use path_has_perm directly instead.

This reverts:

commit 13f8e9810bff12d01807b6f92329111f45218235

Author: David Howells <[email protected]>
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.

Quoted 7 lines
Quoted 3 lines

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.

Quoted 6 lines
Quoted 6 lines

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.

Quoted 2 lines

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

Yes, thanks.

Andreas

author maintainer
#12 lore raw

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

In reply to 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.

---
 security/selinux/hooks.c | 29 +++++++++++++----------------
 1 file changed, 13 insertions(+), 16 deletions(-)
security/selinux/hooks.c +13 -16
index d6b4dc9..2a04729 100644
@@ -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;
Signature · 1 line
2.5.0
author maintainer
#13 lore raw

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

In reply to 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.

---
 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(-)
include/linux/lsm_hooks.h +6 -0
index ec3a6ba..945ae1d 100644
@@ -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;
include/linux/security.h +5 -0
index 2f4c1f7..9692571 100644
@@ -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;
security/security.c +8 -0
index 46f405c..e4371cd 100644
@@ -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 =
security/selinux/hooks.c +20 -10
index 2a04729..f93dafd 100644
@@ -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),
security/selinux/include/objsec.h +6 -0
index 81fa718..a2ae054 100644
@@ -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 {
Signature · 1 line
2.5.0
maintainer
#14 lore raw

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

Quoted 7 lines · 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.

Quoted 6 lines

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.

Signature · 2 lines
James Morris
<[email protected]>
maintainer
#15 lore raw

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

In reply to Andreas Gruenbacher
Quoted 7 lines · 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.

Quoted 6 lines

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.

Signature · 2 lines
James Morris
<[email protected]>
author maintainer
#16 lore raw

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

In reply to 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.)

---
 security/selinux/hooks.c | 47 +++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 39 insertions(+), 8 deletions(-)
security/selinux/hooks.c +39 -8
index f93dafd..61aead9 100644
@@ -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,
Signature · 1 line
2.5.0
author maintainer
#17 lore raw

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

In reply to 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.

Cc Steven Whitehouse <[email protected]>
Cc Bob Peterson <[email protected]>
Cc cluster-devel at redhat.com
---
 fs/gfs2/glops.c | 2 ++
 1 file changed, 2 insertions(+)
fs/gfs2/glops.c +2 -0
index 1f6c9c3..0833076 100644
@@ -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);
 		}
 	}
Signature · 1 line
2.5.0
#18 lore raw

v3 cover Inode security label invalidation

In reply to Andreas Gruenbacher
Quoted 11 lines · 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?

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

Quoted 21 lines

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(-)
maintainer
#19 lore raw
In reply to Andreas Gruenbacher
Quoted 9 lines · 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?

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.

Signature · 2 lines
paul moore
www.paul-moore.com
author maintainer
#20 lore raw
Quoted 9 lines · Paul Moore
Quoted 7 lines · 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

Quoted 8 lines
Quoted 2 lines

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.

Quoted 3 lines

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