diff mbox series

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

Message ID 20201009013630.6777-2-rentianyue@tj.kylinos.cn
State Accepted
Delegated to: Paul Moore
Headers show
Series fix error initialization in inode_doinit_with_dentry() | expand

Commit Message

rentianyue@tj.kylinos.cn Oct. 9, 2020, 1:36 a.m. UTC
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(-)

Comments

Paul Moore Oct. 28, 2020, 2:17 a.m. UTC | #1
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!
Sven Schnelle Nov. 3, 2020, 1:13 p.m. UTC | #2
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
Paul Moore Nov. 3, 2020, 5:11 p.m. UTC | #3
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?
Sven Schnelle Nov. 3, 2020, 7:02 p.m. UTC | #4
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
Paul Moore Nov. 4, 2020, 2:42 a.m. UTC | #5
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?
Sven Schnelle Nov. 4, 2020, 7:01 a.m. UTC | #6
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
Paul Moore Nov. 4, 2020, 8:40 p.m. UTC | #7
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.
diff mbox series

Patch

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index bf8328adad8f..c3ca2957a79d 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1499,7 +1499,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 "out"
+			 * label that except a needless spin
+			 * lock/unlock cycle.
+			 */
+			return 0;
 		}
 
 		rc = inode_doinit_use_xattr(inode, dentry, sbsec->def_sid,
@@ -1553,8 +1559,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 "out"
+				 * label that except a needless spin
+				 * lock/unlock cycle.
+				 */
+				return 0;
+			}
 			rc = selinux_genfs_get_sid(dentry, sclass,
 						   sbsec->flags, &sid);
 			if (rc) {