On Tue, Nov 3, 2020 at 8:14 AM Sven Schnelle <svens@xxxxxxxxxxxxx> wrote:
> Paul Moore <paul@xxxxxxxxxxxxxx> writes:
>
> > On Thu, Oct 8, 2020 at 9:37 PM <rentianyue@xxxxxxxxxxxxx> wrote:
> >> From: Tianyue Ren <rentianyue@xxxxxxxxxx>
> >>
> >> 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@xxxxxxxxxx>
> >> ---
> >> security/selinux/hooks.c | 19 ++++++++++++++++---
> >> 1 file changed, 16 insertions(+), 3 deletions(-)
> >
> > Merged into selinux/next with some minor tweaks to the comments.
> > Thanks for your help!
>
> This seems to break booting on s390:
>
> Welcome to Fedora 32 (Thirty Two)!
>
> [ 1.434571] systemd[1]: Set hostname to <xxx.xxx>
> [ 1.436839] audit: type=1400 audit(1604408868.681:4): avc: denied { write } for pid=1 comm="systemd" dev="cgroup2" ino=2 scontext=system_u:sys
> tem_r:init_t:s0 tcontext=system_u:object_r:unlabeled_t:s0 tclass=file permissive=0
> [ 1.436840] systemd[1]: Failed to create /init.scope control group: Permission denied
> [ 1.438039] systemd[1]: Failed to allocate manager object: Permission denied
> [ [0;1;31m!!!!!! [0m] Failed to allocate manager object.
> [ 1.438281] systemd[1]: Freezing execution.
>
> Any ideas? If i revert 83370b31a915493231e5b9addc72e4bef69f8d31 from
> linux-next-20201103 it works fine...
Thanks for the report.
Looking at this again, I'm thinking that setting the isec->initialized
field outside of the spinlock is probably a bad idea. My guess is
that your system is racing on inode_doinit_with_dentry() and the
initialized field is getting messed up.
Any chance you could try the attached (completely untested) patch?
--
paul moore
www.paul-moore.com
selinux: fix inode_doinit_with_dentry() error case locking
From: Paul Moore <paul@xxxxxxxxxxxxxx>
XXX - testing only patch, work in progress
Fixes: 83370b31a915 ("selinux: fix error initialization in inode_doinit_with_dentry()")
Reported-by: Sven Schnelle <svens@xxxxxxxxxxxxx>
Signed-off-by: Paul Moore <paul@xxxxxxxxxxxxxx>
---
security/selinux/hooks.c | 29 +++++++++++------------------
1 file changed, 11 insertions(+), 18 deletions(-)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 158fc47d8620..0294da2aaacd 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1451,13 +1451,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;
}
rc = inode_doinit_use_xattr(inode, dentry, sbsec->def_sid,
@@ -1513,15 +1507,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);
if (rc) {
@@ -1546,11 +1533,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;
}
@@ -1558,6 +1544,13 @@ 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;
+ spin_unlock(&isec->lock);
+ return 0;
}
/* Convert a Linux signal to an access vector. */
