This patchset backports basically upstream commit 9287aed2 (selinux: Convert isec->lock into a spinlock). This "fixes a deadlock between selinux and GFS2 when GFS2 invalidates a security label", see the original discussion at https://lore.kernel.org/all/1478812710-17190-2-git-send-email-agruenba@redha... It also contains the follow-up fixes to make this correct.
The first 3 commits (by Andreas) are valuable on their own too as they fix a documentation bug, avoid partially initialized structs and (slightly) improve performance while making the code cleaner.
Andreas Gruenbacher (4): selinux: Minor cleanups proc: Pass file mode to proc_pid_make_inode selinux: Clean up initialization of isec->sclass selinux: Convert isec->lock into a spinlock
Paul Moore (1): selinux: fix inode_doinit_with_dentry() LABEL_INVALID error handling
Tianyue Ren (1): selinux: fix error initialization in inode_doinit_with_dentry()
fs/proc/base.c | 23 +++--- fs/proc/fd.c | 6 +- fs/proc/internal.h | 2 +- fs/proc/namespaces.c | 3 +- security/selinux/hooks.c | 123 +++++++++++++++++++----------- security/selinux/include/objsec.h | 5 +- security/selinux/selinuxfs.c | 4 +- 7 files changed, 96 insertions(+), 70 deletions(-)
From: Andreas Gruenbacher agruenba@redhat.com
commit 420591128cb206201dc444c2d42fb6f299b2ecd0 upstream.
Fix the comment for function __inode_security_revalidate, which returns an integer.
Use the LABEL_* constants consistently for isec->initialized.
Signed-off-by: Andreas Gruenbacher agruenba@redhat.com Signed-off-by: Paul Moore paul@paul-moore.com Signed-off-by: Alexander Grund theflamefire89@gmail.com --- security/selinux/hooks.c | 3 ++- security/selinux/selinuxfs.c | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index eb503eccbacc..43f81db42169 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -237,6 +237,7 @@ static int inode_alloc_security(struct inode *inode) isec->sid = SECINITSID_UNLABELED; isec->sclass = SECCLASS_FILE; isec->task_sid = sid; + isec->initialized = LABEL_INVALID; inode->i_security = isec;
return 0; @@ -247,7 +248,7 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent /* * Try reloading inode security labels that have been marked as invalid. The * @may_sleep parameter indicates when sleeping and thus reloading labels is - * allowed; when set to false, returns ERR_PTR(-ECHILD) when the label is + * allowed; when set to false, returns -ECHILD when the label is * invalid. The @opt_dentry parameter should be set to a dentry of the inode; * when no dentry is available, set it to NULL instead. */ diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c index ef1226c1c3ad..a033306d14ee 100644 --- a/security/selinux/selinuxfs.c +++ b/security/selinux/selinuxfs.c @@ -1301,7 +1301,7 @@ static int sel_make_bools(void) goto out;
isec->sid = sid; - isec->initialized = 1; + isec->initialized = LABEL_INITIALIZED; inode->i_fop = &sel_bool_ops; inode->i_ino = i|SEL_BOOL_INO_OFFSET; d_add(dentry, inode); @@ -1835,7 +1835,7 @@ static int sel_fill_super(struct super_block *sb, void *data, int silent) isec = (struct inode_security_struct *)inode->i_security; isec->sid = SECINITSID_DEVNULL; isec->sclass = SECCLASS_CHR_FILE; - isec->initialized = 1; + isec->initialized = LABEL_INITIALIZED;
init_special_inode(inode, S_IFCHR | S_IRUGO | S_IWUGO, MKDEV(MEM_MAJOR, 3)); d_add(dentry, inode);
From: Andreas Gruenbacher agruenba@redhat.com
commit db978da8fa1d0819b210c137d31a339149b88875 upstream.
Pass the file mode of the proc inode to be created to proc_pid_make_inode. In proc_pid_make_inode, initialize inode->i_mode before calling security_task_to_inode. This allows selinux to set isec->sclass right away without introducing "half-initialized" inode security structs.
Signed-off-by: Andreas Gruenbacher agruenba@redhat.com Signed-off-by: Paul Moore paul@paul-moore.com Signed-off-by: Alexander Grund theflamefire89@gmail.com --- fs/proc/base.c | 23 +++++++++-------------- fs/proc/fd.c | 6 ++---- fs/proc/internal.h | 2 +- fs/proc/namespaces.c | 3 +-- security/selinux/hooks.c | 1 + 5 files changed, 14 insertions(+), 21 deletions(-)
diff --git a/fs/proc/base.c b/fs/proc/base.c index 886e408f4769..2dd4a2b7222c 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -1676,7 +1676,8 @@ const struct inode_operations proc_pid_link_inode_operations = {
/* building an inode */
-struct inode *proc_pid_make_inode(struct super_block * sb, struct task_struct *task) +struct inode *proc_pid_make_inode(struct super_block * sb, + struct task_struct *task, umode_t mode) { struct inode * inode; struct proc_inode *ei; @@ -1690,6 +1691,7 @@ struct inode *proc_pid_make_inode(struct super_block * sb, struct task_struct *t
/* Common stuff */ ei = PROC_I(inode); + inode->i_mode = mode; inode->i_ino = get_next_ino(); inode->i_mtime = inode->i_atime = inode->i_ctime = current_time(inode); inode->i_op = &proc_def_inode_operations; @@ -2041,7 +2043,9 @@ proc_map_files_instantiate(struct inode *dir, struct dentry *dentry, struct proc_inode *ei; struct inode *inode;
- inode = proc_pid_make_inode(dir->i_sb, task); + inode = proc_pid_make_inode(dir->i_sb, task, S_IFLNK | + ((mode & FMODE_READ ) ? S_IRUSR : 0) | + ((mode & FMODE_WRITE) ? S_IWUSR : 0)); if (!inode) return -ENOENT;
@@ -2050,12 +2054,6 @@ proc_map_files_instantiate(struct inode *dir, struct dentry *dentry,
inode->i_op = &proc_map_files_link_inode_operations; inode->i_size = 64; - inode->i_mode = S_IFLNK; - - if (mode & FMODE_READ) - inode->i_mode |= S_IRUSR; - if (mode & FMODE_WRITE) - inode->i_mode |= S_IWUSR;
d_set_d_op(dentry, &tid_map_files_dentry_operations); d_add(dentry, inode); @@ -2409,12 +2407,11 @@ static int proc_pident_instantiate(struct inode *dir, struct inode *inode; struct proc_inode *ei;
- inode = proc_pid_make_inode(dir->i_sb, task); + inode = proc_pid_make_inode(dir->i_sb, task, p->mode); if (!inode) goto out;
ei = PROC_I(inode); - inode->i_mode = p->mode; if (S_ISDIR(inode->i_mode)) set_nlink(inode, 2); /* Use getattr to fix if necessary */ if (p->iop) @@ -3109,11 +3106,10 @@ static int proc_pid_instantiate(struct inode *dir, { struct inode *inode;
- inode = proc_pid_make_inode(dir->i_sb, task); + inode = proc_pid_make_inode(dir->i_sb, task, S_IFDIR | S_IRUGO | S_IXUGO); if (!inode) goto out;
- inode->i_mode = S_IFDIR|S_IRUGO|S_IXUGO; inode->i_op = &proc_tgid_base_inode_operations; inode->i_fop = &proc_tgid_base_operations; inode->i_flags|=S_IMMUTABLE; @@ -3404,11 +3400,10 @@ static int proc_task_instantiate(struct inode *dir, struct dentry *dentry, struct task_struct *task, const void *ptr) { struct inode *inode; - inode = proc_pid_make_inode(dir->i_sb, task); + inode = proc_pid_make_inode(dir->i_sb, task, S_IFDIR | S_IRUGO | S_IXUGO);
if (!inode) goto out; - inode->i_mode = S_IFDIR|S_IRUGO|S_IXUGO; inode->i_op = &proc_tid_base_inode_operations; inode->i_fop = &proc_tid_base_operations; inode->i_flags|=S_IMMUTABLE; diff --git a/fs/proc/fd.c b/fs/proc/fd.c index d21dafef3102..4274f83bf100 100644 --- a/fs/proc/fd.c +++ b/fs/proc/fd.c @@ -183,14 +183,13 @@ proc_fd_instantiate(struct inode *dir, struct dentry *dentry, struct proc_inode *ei; struct inode *inode;
- inode = proc_pid_make_inode(dir->i_sb, task); + inode = proc_pid_make_inode(dir->i_sb, task, S_IFLNK); if (!inode) goto out;
ei = PROC_I(inode); ei->fd = fd;
- inode->i_mode = S_IFLNK; inode->i_op = &proc_pid_link_inode_operations; inode->i_size = 64;
@@ -322,14 +321,13 @@ proc_fdinfo_instantiate(struct inode *dir, struct dentry *dentry, struct proc_inode *ei; struct inode *inode;
- inode = proc_pid_make_inode(dir->i_sb, task); + inode = proc_pid_make_inode(dir->i_sb, task, S_IFREG | S_IRUSR); if (!inode) goto out;
ei = PROC_I(inode); ei->fd = fd;
- inode->i_mode = S_IFREG | S_IRUSR; inode->i_fop = &proc_fdinfo_file_operations;
d_set_d_op(dentry, &tid_fd_dentry_operations); diff --git a/fs/proc/internal.h b/fs/proc/internal.h index c0bdeceaaeb6..5bc057be6fa3 100644 --- a/fs/proc/internal.h +++ b/fs/proc/internal.h @@ -163,7 +163,7 @@ extern int proc_pid_statm(struct seq_file *, struct pid_namespace *, extern const struct dentry_operations pid_dentry_operations; extern int pid_getattr(struct vfsmount *, struct dentry *, struct kstat *); extern int proc_setattr(struct dentry *, struct iattr *); -extern struct inode *proc_pid_make_inode(struct super_block *, struct task_struct *); +extern struct inode *proc_pid_make_inode(struct super_block *, struct task_struct *, umode_t); extern int pid_revalidate(struct dentry *, unsigned int); extern int pid_delete_dentry(const struct dentry *); extern int proc_pid_readdir(struct file *, struct dir_context *); diff --git a/fs/proc/namespaces.c b/fs/proc/namespaces.c index 51b8b0a8ad91..766f0c637ad1 100644 --- a/fs/proc/namespaces.c +++ b/fs/proc/namespaces.c @@ -92,12 +92,11 @@ static int proc_ns_instantiate(struct inode *dir, struct inode *inode; struct proc_inode *ei;
- inode = proc_pid_make_inode(dir->i_sb, task); + inode = proc_pid_make_inode(dir->i_sb, task, S_IFLNK | S_IRWXUGO); if (!inode) goto out;
ei = PROC_I(inode); - inode->i_mode = S_IFLNK|S_IRWXUGO; inode->i_op = &proc_ns_link_inode_operations; ei->ns_ops = ns_ops;
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 43f81db42169..1997d87b35d5 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -3958,6 +3958,7 @@ static void selinux_task_to_inode(struct task_struct *p, struct inode_security_struct *isec = inode->i_security; u32 sid = task_sid(p);
+ isec->sclass = inode_mode_to_security_class(inode->i_mode); isec->sid = sid; isec->initialized = LABEL_INITIALIZED; }
From: Andreas Gruenbacher agruenba@redhat.com
commit 13457d073c29da92001f6ee809075eaa8757fb96 upstream.
Now that isec->initialized == LABEL_INITIALIZED implies that isec->sclass is valid, skip such inodes immediately in inode_doinit_with_dentry.
For the remaining inodes, initialize isec->sclass at the beginning of inode_doinit_with_dentry to simplify the code.
Signed-off-by: Andreas Gruenbacher agruenba@redhat.com Signed-off-by: Paul Moore paul@paul-moore.com Signed-off-by: Alexander Grund theflamefire89@gmail.com --- security/selinux/hooks.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 1997d87b35d5..d21b95a848b8 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -1395,12 +1395,15 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent int rc = 0;
if (isec->initialized == LABEL_INITIALIZED) - goto out; + return 0;
mutex_lock(&isec->lock); if (isec->initialized == LABEL_INITIALIZED) goto out_unlock;
+ if (isec->sclass == SECCLASS_FILE) + isec->sclass = inode_mode_to_security_class(inode->i_mode); + sbsec = inode->i_sb->s_security; if (!(sbsec->flags & SE_SBINITIALIZED)) { /* Defer initialization until selinux_complete_init, @@ -1518,7 +1521,6 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent isec->sid = sbsec->sid;
/* Try to obtain a transition SID. */ - isec->sclass = inode_mode_to_security_class(inode->i_mode); rc = security_transition_sid(isec->task_sid, sbsec->sid, isec->sclass, NULL, &sid); if (rc) @@ -1554,7 +1556,6 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent */ if (!dentry) goto out_unlock; - isec->sclass = inode_mode_to_security_class(inode->i_mode); rc = selinux_genfs_get_sid(dentry, isec->sclass, sbsec->flags, &sid); dput(dentry); @@ -1569,9 +1570,6 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent
out_unlock: mutex_unlock(&isec->lock); -out: - if (isec->sclass == SECCLASS_FILE) - isec->sclass = inode_mode_to_security_class(inode->i_mode); return rc; }
From: Andreas Gruenbacher agruenba@redhat.com
commit 9287aed2ad1ff1bde5eb190bcd6dccd5f1cf47d3 upstream.
Convert isec->lock from a mutex into a spinlock. Instead of holding the lock while sleeping in inode_doinit_with_dentry, set isec->initialized to LABEL_PENDING and release the lock. Then, when the sid has been determined, re-acquire the lock. If isec->initialized is still set to LABEL_PENDING, set isec->sid; otherwise, the sid has been set by another task (LABEL_INITIALIZED) or invalidated (LABEL_INVALID) in the meantime.
This fixes a deadlock on gfs2 where
* one task is in inode_doinit_with_dentry -> gfs2_getxattr, holds isec->lock, and tries to acquire the inode's glock, and
* another task is in do_xmote -> inode_go_inval -> selinux_inode_invalidate_secctx, holds the inode's glock, and tries to acquire isec->lock.
Signed-off-by: Andreas Gruenbacher agruenba@redhat.com [PM: minor tweaks to keep checkpatch.pl happy] Signed-off-by: Paul Moore paul@paul-moore.com Signed-off-by: Alexander Grund theflamefire89@gmail.com --- security/selinux/hooks.c | 101 +++++++++++++++++++----------- security/selinux/include/objsec.h | 5 +- 2 files changed, 66 insertions(+), 40 deletions(-)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index d21b95a848b8..d252890debf7 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -231,7 +231,7 @@ static int inode_alloc_security(struct inode *inode) if (!isec) return -ENOMEM;
- mutex_init(&isec->lock); + spin_lock_init(&isec->lock); INIT_LIST_HEAD(&isec->list); isec->inode = inode; isec->sid = SECINITSID_UNLABELED; @@ -1387,7 +1387,8 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent { struct superblock_security_struct *sbsec = NULL; struct inode_security_struct *isec = inode->i_security; - u32 sid; + u32 task_sid, sid = 0; + u16 sclass; struct dentry *dentry; #define INITCONTEXTLEN 255 char *context = NULL; @@ -1397,7 +1398,7 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent if (isec->initialized == LABEL_INITIALIZED) return 0;
- mutex_lock(&isec->lock); + spin_lock(&isec->lock); if (isec->initialized == LABEL_INITIALIZED) goto out_unlock;
@@ -1416,12 +1417,18 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent goto out_unlock; }
+ sclass = isec->sclass; + task_sid = isec->task_sid; + sid = isec->sid; + isec->initialized = LABEL_PENDING; + spin_unlock(&isec->lock); + switch (sbsec->behavior) { case SECURITY_FS_USE_NATIVE: break; case SECURITY_FS_USE_XATTR: if (!(inode->i_opflags & IOP_XATTR)) { - isec->sid = sbsec->def_sid; + sid = sbsec->def_sid; break; } /* Need a dentry, since the xattr API requires one. @@ -1443,7 +1450,7 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent * inode_doinit with a dentry, before these inodes could * be used again by userspace. */ - goto out_unlock; + goto out; }
len = INITCONTEXTLEN; @@ -1451,7 +1458,7 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent if (!context) { rc = -ENOMEM; dput(dentry); - goto out_unlock; + goto out; } context[len] = '\0'; rc = __vfs_getxattr(dentry, inode, XATTR_NAME_SELINUX, context, len); @@ -1462,14 +1469,14 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent rc = __vfs_getxattr(dentry, inode, XATTR_NAME_SELINUX, NULL, 0); if (rc < 0) { dput(dentry); - goto out_unlock; + goto out; } len = rc; context = kmalloc(len+1, GFP_NOFS); if (!context) { rc = -ENOMEM; dput(dentry); - goto out_unlock; + goto out; } context[len] = '\0'; rc = __vfs_getxattr(dentry, inode, XATTR_NAME_SELINUX, context, len); @@ -1481,7 +1488,7 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent "%d for dev=%s ino=%ld\n", __func__, -rc, inode->i_sb->s_id, inode->i_ino); kfree(context); - goto out_unlock; + goto out; } /* Map ENODATA to the default file SID */ sid = sbsec->def_sid; @@ -1511,28 +1518,25 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent } } kfree(context); - isec->sid = sid; break; case SECURITY_FS_USE_TASK: - isec->sid = isec->task_sid; + sid = task_sid; break; case SECURITY_FS_USE_TRANS: /* Default to the fs SID. */ - isec->sid = sbsec->sid; + sid = sbsec->sid;
/* Try to obtain a transition SID. */ - rc = security_transition_sid(isec->task_sid, sbsec->sid, - isec->sclass, NULL, &sid); + rc = security_transition_sid(task_sid, sid, sclass, NULL, &sid); if (rc) - goto out_unlock; - isec->sid = sid; + goto out; break; case SECURITY_FS_USE_MNTPOINT: - isec->sid = sbsec->mntpoint_sid; + sid = sbsec->mntpoint_sid; break; default: /* Default to the fs superblock SID. */ - isec->sid = sbsec->sid; + sid = sbsec->sid;
if ((sbsec->flags & SE_SBGENFS) && !S_ISLNK(inode->i_mode)) { /* We must have a dentry to determine the label on @@ -1555,21 +1559,30 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent * could be used again by userspace. */ if (!dentry) - goto out_unlock; - rc = selinux_genfs_get_sid(dentry, isec->sclass, + goto out; + rc = selinux_genfs_get_sid(dentry, sclass, sbsec->flags, &sid); dput(dentry); if (rc) - goto out_unlock; - isec->sid = sid; + goto out; } break; }
- isec->initialized = LABEL_INITIALIZED; +out: + spin_lock(&isec->lock); + if (isec->initialized == LABEL_PENDING) { + if (!sid || rc) { + isec->initialized = LABEL_INVALID; + goto out_unlock; + } + + isec->initialized = LABEL_INITIALIZED; + isec->sid = sid; + }
out_unlock: - mutex_unlock(&isec->lock); + spin_unlock(&isec->lock); return rc; }
@@ -3199,9 +3212,11 @@ static void selinux_inode_post_setxattr(struct dentry *dentry, const char *name, }
isec = backing_inode_security(dentry); + spin_lock(&isec->lock); isec->sclass = inode_mode_to_security_class(inode->i_mode); isec->sid = newsid; isec->initialized = LABEL_INITIALIZED; + spin_unlock(&isec->lock);
return; } @@ -3298,9 +3313,11 @@ static int selinux_inode_setsecurity(struct inode *inode, const char *name, if (rc) return rc;
+ spin_lock(&isec->lock); isec->sclass = inode_mode_to_security_class(inode->i_mode); isec->sid = newsid; isec->initialized = LABEL_INITIALIZED; + spin_unlock(&isec->lock); return 0; }
@@ -3956,9 +3973,11 @@ static void selinux_task_to_inode(struct task_struct *p, struct inode_security_struct *isec = inode->i_security; u32 sid = task_sid(p);
+ spin_lock(&isec->lock); isec->sclass = inode_mode_to_security_class(inode->i_mode); isec->sid = sid; isec->initialized = LABEL_INITIALIZED; + spin_unlock(&isec->lock); }
/* Returns error only if unable to parse addresses */ @@ -4277,24 +4296,24 @@ static int selinux_socket_post_create(struct socket *sock, int family, const struct task_security_struct *tsec = current_security(); struct inode_security_struct *isec = inode_security_novalidate(SOCK_INODE(sock)); struct sk_security_struct *sksec; + u16 sclass = socket_type_to_security_class(family, type, protocol); + u32 sid = SECINITSID_KERNEL; int err = 0;
- isec->sclass = socket_type_to_security_class(family, type, protocol); - - if (kern) - isec->sid = SECINITSID_KERNEL; - else { - err = socket_sockcreate_sid(tsec, isec->sclass, &(isec->sid)); + if (!kern) { + err = socket_sockcreate_sid(tsec, sclass, &sid); if (err) return err; }
+ isec->sclass = sclass; + isec->sid = sid; isec->initialized = LABEL_INITIALIZED;
if (sock->sk) { sksec = sock->sk->sk_security; - sksec->sid = isec->sid; - sksec->sclass = isec->sclass; + sksec->sclass = sclass; + sksec->sid = sid; err = selinux_netlbl_socket_post_create(sock->sk, family); }
@@ -4478,16 +4497,22 @@ static int selinux_socket_accept(struct socket *sock, struct socket *newsock) int err; struct inode_security_struct *isec; struct inode_security_struct *newisec; + u16 sclass; + u32 sid;
err = sock_has_perm(current, sock->sk, SOCKET__ACCEPT); if (err) return err;
- newisec = inode_security_novalidate(SOCK_INODE(newsock)); - isec = inode_security_novalidate(SOCK_INODE(sock)); - newisec->sclass = isec->sclass; - newisec->sid = isec->sid; + spin_lock(&isec->lock); + sclass = isec->sclass; + sid = isec->sid; + spin_unlock(&isec->lock); + + newisec = inode_security_novalidate(SOCK_INODE(newsock)); + newisec->sclass = sclass; + newisec->sid = sid; newisec->initialized = LABEL_INITIALIZED;
return 0; @@ -6010,9 +6035,9 @@ static void selinux_inode_invalidate_secctx(struct inode *inode) { struct inode_security_struct *isec = inode->i_security;
- mutex_lock(&isec->lock); + spin_lock(&isec->lock); isec->initialized = LABEL_INVALID; - mutex_unlock(&isec->lock); + spin_unlock(&isec->lock); }
/* diff --git a/security/selinux/include/objsec.h b/security/selinux/include/objsec.h index c21e135460a5..e8dab0f02c72 100644 --- a/security/selinux/include/objsec.h +++ b/security/selinux/include/objsec.h @@ -39,7 +39,8 @@ struct task_security_struct {
enum label_initialized { LABEL_INVALID, /* invalid or not initialized */ - LABEL_INITIALIZED /* initialized */ + LABEL_INITIALIZED, /* initialized */ + LABEL_PENDING };
struct inode_security_struct { @@ -52,7 +53,7 @@ struct inode_security_struct { u32 sid; /* SID of this object */ u16 sclass; /* security class of this object */ unsigned char initialized; /* initialization flag */ - struct mutex lock; + spinlock_t lock; };
struct file_security_struct {
From: Tianyue Ren rentianyue@kylinos.cn
commit 83370b31a915493231e5b9addc72e4bef69f8d31 upstream.
Mark the inode security label as invalid if we cannot find a dentry so that we will retry later rather than marking it initialized with the unlabeled SID.
Fixes: 9287aed2ad1f ("selinux: Convert isec->lock into a spinlock") Signed-off-by: Tianyue Ren rentianyue@kylinos.cn [PM: minor comment tweaks] Signed-off-by: Paul Moore paul@paul-moore.com Signed-off-by: Alexander Grund theflamefire89@gmail.com --- security/selinux/hooks.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index d252890debf7..b9c7e089906c 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -1450,7 +1450,13 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent * inode_doinit with a dentry, before these inodes could * be used again by userspace. */ - goto out; + isec->initialized = LABEL_INVALID; + /* + * There is nothing useful to jump to the "out" + * label, except a needless spin lock/unlock + * cycle. + */ + return 0; }
len = INITCONTEXTLEN; @@ -1558,8 +1564,15 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent * inode_doinit() with a dentry, before these inodes * could be used again by userspace. */ - if (!dentry) - goto out; + if (!dentry) { + isec->initialized = LABEL_INVALID; + /* + * There is nothing useful to jump to the "out" + * label, except a needless spin lock/unlock + * cycle. + */ + return 0; + } rc = selinux_genfs_get_sid(dentry, sclass, sbsec->flags, &sid); dput(dentry);
From: Paul Moore paul@paul-moore.com
commit 200ea5a2292dc444a818b096ae6a32ba3caa51b9 upstream.
A previous fix, commit 83370b31a915 ("selinux: fix error initialization in inode_doinit_with_dentry()"), changed how failures were handled before a SELinux policy was loaded. Unfortunately that patch was potentially problematic for two reasons: it set the isec->initialized state without holding a lock, and it didn't set the inode's SELinux label to the "default" for the particular filesystem. The later can be a problem if/when a later attempt to revalidate the inode fails and SELinux reverts to the existing inode label.
This patch should restore the default inode labeling that existed before the original fix, without affecting the LABEL_INVALID marking such that revalidation will still be attempted in the future.
Fixes: 83370b31a915 ("selinux: fix error initialization in inode_doinit_with_dentry()") Reported-by: Sven Schnelle svens@linux.ibm.com Tested-by: Sven Schnelle svens@linux.ibm.com Reviewed-by: Ondrej Mosnacek omosnace@redhat.com Signed-off-by: Paul Moore paul@paul-moore.com Signed-off-by: Alexander Grund theflamefire89@gmail.com --- security/selinux/hooks.c | 31 +++++++++++++------------------ 1 file changed, 13 insertions(+), 18 deletions(-)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index b9c7e089906c..ac2381eec27f 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -1450,13 +1450,7 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent * inode_doinit with a dentry, before these inodes could * be used again by userspace. */ - isec->initialized = LABEL_INVALID; - /* - * There is nothing useful to jump to the "out" - * label, except a needless spin lock/unlock - * cycle. - */ - return 0; + goto out_invalid; }
len = INITCONTEXTLEN; @@ -1564,15 +1558,8 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent * inode_doinit() with a dentry, before these inodes * could be used again by userspace. */ - if (!dentry) { - isec->initialized = LABEL_INVALID; - /* - * There is nothing useful to jump to the "out" - * label, except a needless spin lock/unlock - * cycle. - */ - return 0; - } + if (!dentry) + goto out_invalid; rc = selinux_genfs_get_sid(dentry, sclass, sbsec->flags, &sid); dput(dentry); @@ -1585,11 +1572,10 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent out: spin_lock(&isec->lock); if (isec->initialized == LABEL_PENDING) { - if (!sid || rc) { + if (rc) { isec->initialized = LABEL_INVALID; goto out_unlock; } - isec->initialized = LABEL_INITIALIZED; isec->sid = sid; } @@ -1597,6 +1583,15 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent out_unlock: spin_unlock(&isec->lock); return rc; + +out_invalid: + spin_lock(&isec->lock); + if (isec->initialized == LABEL_PENDING) { + isec->initialized = LABEL_INVALID; + isec->sid = sid; + } + spin_unlock(&isec->lock); + return 0; }
/* Convert a Linux signal to an access vector. */
On Sat, Jul 30, 2022 at 07:03:37PM +0200, Alexander Grund wrote:
This patchset backports basically upstream commit 9287aed2 (selinux: Convert isec->lock into a spinlock). This "fixes a deadlock between selinux and GFS2 when GFS2 invalidates a security label", see the original discussion at https://lore.kernel.org/all/1478812710-17190-2-git-send-email-agruenba@redha... It also contains the follow-up fixes to make this correct.
The first 3 commits (by Andreas) are valuable on their own too as they fix a documentation bug, avoid partially initialized structs and (slightly) improve performance while making the code cleaner.
Andreas Gruenbacher (4): selinux: Minor cleanups proc: Pass file mode to proc_pid_make_inode selinux: Clean up initialization of isec->sclass selinux: Convert isec->lock into a spinlock
Paul Moore (1): selinux: fix inode_doinit_with_dentry() LABEL_INVALID error handling
Tianyue Ren (1): selinux: fix error initialization in inode_doinit_with_dentry()
fs/proc/base.c | 23 +++--- fs/proc/fd.c | 6 +- fs/proc/internal.h | 2 +- fs/proc/namespaces.c | 3 +- security/selinux/hooks.c | 123 +++++++++++++++++++----------- security/selinux/include/objsec.h | 5 +- security/selinux/selinuxfs.c | 4 +- 7 files changed, 96 insertions(+), 70 deletions(-)
-- 2.25.1
All look good, now queued up, thanks,
greg k-h
linux-stable-mirror@lists.linaro.org
-
Alexander Grund -
Greg KH