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

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

[Sven Schnelle]

Paul Moore <paul@paul-moore.com> writes:

> On Thu, Oct 8, 2020 at 9:37 PM <rentianyue@tj.kylinos.cn> wrote:
>> From: Tianyue Ren <rentianyue@kylinos.cn>
>>
>> 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>
>> ---
>>  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
Sven

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

[Paul Moore]

[-- Attachment #1: Type: text/plain, Size: 1900 bytes --]

On Tue, Nov 3, 2020 at 8:14 AM Sven Schnelle <svens@linux.ibm.com> wrote:
> Paul Moore <paul@paul-moore.com> writes:
>
> > On Thu, Oct 8, 2020 at 9:37 PM <rentianyue@tj.kylinos.cn> wrote:
> >> From: Tianyue Ren <rentianyue@kylinos.cn>
> >>
> >> 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>
> >> ---
> >>  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

[-- Attachment #2: 01-selinux-inode_dentry_init_fix.patch --]
[-- Type: text/x-patch, Size: 2378 bytes --]

selinux: fix inode_doinit_with_dentry() error case locking

From: Paul Moore <paul@paul-moore.com>

XXX - testing only patch, work in progress

Fixes: 83370b31a915 ("selinux: fix error initialization in inode_doinit_with_dentry()")
Reported-by: Sven Schnelle <svens@linux.ibm.com>
Signed-off-by: Paul Moore <paul@paul-moore.com>
---
 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. */

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

[Sven Schnelle]

Hi Paul,

Paul Moore <paul@paul-moore.com> writes:

> On Tue, Nov 3, 2020 at 8:14 AM Sven Schnelle <svens@linux.ibm.com> wrote:
>> Paul Moore <paul@paul-moore.com> writes:
>>
>> > On Thu, Oct 8, 2020 at 9:37 PM <rentianyue@tj.kylinos.cn> wrote:
>> >> From: Tianyue Ren <rentianyue@kylinos.cn>
>> >>
>> >> 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>
>> >> ---
>> >>  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?

Thanks for the patch. Unfortunately it doesn't seem to change anything
for me. I can take a look into this tomorrow, but i don't know much
about the internals of selinux, so i'm not sure whether i'm of much help.

Regards
Sven

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

[Paul Moore]

[-- Attachment #1: Type: text/plain, Size: 850 bytes --]

On Tue, Nov 3, 2020 at 2:02 PM Sven Schnelle <svens@linux.ibm.com> wrote:
> Thanks for the patch. Unfortunately it doesn't seem to change anything
> for me. I can take a look into this tomorrow, but i don't know much
> about the internals of selinux, so i'm not sure whether i'm of much help.

I'm sorry that patch didn't work out.  I just spent some more time
looking at the code+patch and the only other thing that I can see is
that if we mark the isec invalid, we don't bother setting the
isec->sid value to whatever default we may have already found.  In a
perfect world this shouldn't matter, but if for whatever reason the
kernel can't revalidate the inode's label when it tries later it will
fallback to that default isec->sid.

I'm sorry to ask this again, but would you be able to test the attached patch?

-- 
paul moore
www.paul-moore.com

[-- Attachment #2: 01-selinux-inode_dentry_init_fix.patch --]
[-- Type: text/x-patch, Size: 2406 bytes --]

selinux: fix inode_doinit_with_dentry() error case locking

From: Paul Moore <paul@paul-moore.com>

XXX - testing only patch, work in progress

Fixes: 83370b31a915 ("selinux: fix error initialization in inode_doinit_with_dentry()")
Reported-by: Sven Schnelle <svens@linux.ibm.com>
Signed-off-by: Paul Moore <paul@paul-moore.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 158fc47d8620..c46312710e73 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,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. */

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

[Sven Schnelle]

Hi Paul,

Paul Moore <paul@paul-moore.com> writes:

> On Tue, Nov 3, 2020 at 2:02 PM Sven Schnelle <svens@linux.ibm.com> wrote:
>> Thanks for the patch. Unfortunately it doesn't seem to change anything
>> for me. I can take a look into this tomorrow, but i don't know much
>> about the internals of selinux, so i'm not sure whether i'm of much help.
>
> I'm sorry that patch didn't work out.  I just spent some more time
> looking at the code+patch and the only other thing that I can see is
> that if we mark the isec invalid, we don't bother setting the
> isec->sid value to whatever default we may have already found.  In a
> perfect world this shouldn't matter, but if for whatever reason the
> kernel can't revalidate the inode's label when it tries later it will
> fallback to that default isec->sid.
>
> I'm sorry to ask this again, but would you be able to test the attached patch?

This patch fixes the issue. So it looks like your assumption is right.

Thanks
Sven

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

[Paul Moore]

On Wed, Nov 4, 2020 at 2:02 AM Sven Schnelle <svens@linux.ibm.com> wrote:
> Paul Moore <paul@paul-moore.com> writes:
> > On Tue, Nov 3, 2020 at 2:02 PM Sven Schnelle <svens@linux.ibm.com> wrote:
> >> Thanks for the patch. Unfortunately it doesn't seem to change anything
> >> for me. I can take a look into this tomorrow, but i don't know much
> >> about the internals of selinux, so i'm not sure whether i'm of much help.
> >
> > I'm sorry that patch didn't work out.  I just spent some more time
> > looking at the code+patch and the only other thing that I can see is
> > that if we mark the isec invalid, we don't bother setting the
> > isec->sid value to whatever default we may have already found.  In a
> > perfect world this shouldn't matter, but if for whatever reason the
> > kernel can't revalidate the inode's label when it tries later it will
> > fallback to that default isec->sid.
> >
> > I'm sorry to ask this again, but would you be able to test the attached patch?
>
> This patch fixes the issue. So it looks like your assumption is right.

Great, I'm glad that fixed the problem you were seeing; thanks for
your help with testing!  I'll post a proper version of the patch to
the list later today.

-- 
paul moore
www.paul-moore.com