Re: [PATCH v3 1/1] selinux: fix error initialization in inode_doinit_with_dentry()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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. */

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Kernel Development]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Info]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Linux Media]     [Device Mapper]

  Powered by Linux