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

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

[Paul Moore]

On Sun, Sep 27, 2020 at 5:44 AM <rentianyue@tj.kylinos.cn> wrote:
> From: Tianyue Ren <rentianyue@kylinos.cn>
>
> I think there is a error initialization for isec->sid when the dentry is NULL,
> when we do "remount -o rw,remount ${rootmnt}" action before selinux policy loading.It's
> leading kernel getting wrong security label "unlabeled_t" when the userspace access
> files such as /etc/fstab.

Out of curiosity, can you provide some background information
regarding why you made the decision to remount a filesystem before
loading the SELinux policy?

-- 
paul moore
www.paul-moore.com

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

[Stephen Smalley]

On Sun, Sep 27, 2020 at 11:24 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Sun, Sep 27, 2020 at 5:44 AM <rentianyue@tj.kylinos.cn> wrote:
> > From: Tianyue Ren <rentianyue@kylinos.cn>
> >
> > I think there is a error initialization for isec->sid when the dentry is NULL,
> > when we do "remount -o rw,remount ${rootmnt}" action before selinux policy loading.It's
> > leading kernel getting wrong security label "unlabeled_t" when the userspace access
> > files such as /etc/fstab.
>
> Out of curiosity, can you provide some background information
> regarding why you made the decision to remount a filesystem before
> loading the SELinux policy?

And where is the original message and patch?  I didn't receive the
original (just the reply) and I don't see it in the list archives.

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

[Stephen Smalley]

On Mon, Sep 28, 2020 at 11:06 PM 任天悦 <rentianyue@tj.kylinos.cn> wrote:
>
> Sorry, the previous emails sent failed for "connect to sec-jeemsg.eemsg.mail.mil[156.112.250.13]: Connection timed out (port 25)".
> The attchment is the previous emails.

1) Use this gmail address for me, not the tycho.nsa.gov one; always
look at the latest MAINTAINERS file in the current kernel for contact
info.
2) Don't send html email to the lists; the list server will drop it
(which is why it never reached the lists).  Make sure you are sending
plaintext emails and use git-send-email to send your patches directly.

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

[Stephen Smalley]

On 9/27/20 5:42 AM, rentianyue@tj.kylinos.cn wrote:

> From: Tianyue Ren <rentianyue@kylinos.cn>
>
> Fix to initialize isec->class with SECINITSID_UNLABELED other
> than the from the xattr label when then dentry is NULL when
> the filesystem is remounted before the policy loading.

Looks like this was broken by commit 
9287aed2ad1ff1bde5eb190bcd6dccd5f1cf47d3 ("selinux: Convert isec->lock 
into a spinlock").

>
> Signed-off-by: Tianyue Ren <rentianyue@kylinos.cn>
> ---
>   security/selinux/hooks.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index bf8328adad8f..da7295a546e0 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -1499,6 +1499,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;
>   			goto out;
>   		}
>   
> @@ -1553,8 +1554,10 @@ 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)
> +			if (!dentry) {
> +				isec->initialized = LABEL_INVALID;
>   				goto out;
> +			}
>   			rc = selinux_genfs_get_sid(dentry, sclass,
>   						   sbsec->flags, &sid);
>   			if (rc) {

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

[Stephen Smalley]

On Tue, Sep 29, 2020 at 8:54 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On 9/27/20 5:42 AM, rentianyue@tj.kylinos.cn wrote:
>
> > From: Tianyue Ren <rentianyue@kylinos.cn>
> >
> > Fix to initialize isec->class with SECINITSID_UNLABELED other
> > than the from the xattr label when then dentry is NULL when
> > the filesystem is remounted before the policy loading.
>
> Looks like this was broken by commit
> 9287aed2ad1ff1bde5eb190bcd6dccd5f1cf47d3 ("selinux: Convert isec->lock
> into a spinlock").

It appears that the broken commit assumed (wrongly) that isec->sid is
0 initially, sets sid = isec->sid, and then in the out: path, if (!sid
|| rc) it sets isec->initialized to LABEL_INVALID.  In fact, isec->sid
is SECINITSID_UNLABELED initially upon selinux_inode_alloc_security(),
so that !sid test never evaluates to true.  And changing it to compare
with SECINITSID_UNLABELED wouldn't be safe either since it is possible
to end up with SECINITSID_UNLABELED without it being invalid.  I think
your fix resolves the issue with ensuring that we retry upon
subsequent attempts to access the inode but we should likely fix up
this code.

Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>

>
> >
> > Signed-off-by: Tianyue Ren <rentianyue@kylinos.cn>
> > ---
> >   security/selinux/hooks.c | 5 ++++-
> >   1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index bf8328adad8f..da7295a546e0 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -1499,6 +1499,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;
> >                       goto out;
> >               }
> >
> > @@ -1553,8 +1554,10 @@ 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)
> > +                     if (!dentry) {
> > +                             isec->initialized = LABEL_INVALID;
> >                               goto out;
> > +                     }
> >                       rc = selinux_genfs_get_sid(dentry, sclass,
> >                                                  sbsec->flags, &sid);
> >                       if (rc) {

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

[Stephen Smalley]

On Tue, Sep 29, 2020 at 9:31 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Tue, Sep 29, 2020 at 8:54 AM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
> >
> > On 9/27/20 5:42 AM, rentianyue@tj.kylinos.cn wrote:
> >
> > > From: Tianyue Ren <rentianyue@kylinos.cn>
> > >
> > > Fix to initialize isec->class with SECINITSID_UNLABELED other
> > > than the from the xattr label when then dentry is NULL when
> > > the filesystem is remounted before the policy loading.
> >
> > Looks like this was broken by commit
> > 9287aed2ad1ff1bde5eb190bcd6dccd5f1cf47d3 ("selinux: Convert isec->lock
> > into a spinlock").
>
> It appears that the broken commit assumed (wrongly) that isec->sid is
> 0 initially, sets sid = isec->sid, and then in the out: path, if (!sid
> || rc) it sets isec->initialized to LABEL_INVALID.  In fact, isec->sid
> is SECINITSID_UNLABELED initially upon selinux_inode_alloc_security(),
> so that !sid test never evaluates to true.  And changing it to compare
> with SECINITSID_UNLABELED wouldn't be safe either since it is possible
> to end up with SECINITSID_UNLABELED without it being invalid.  I think
> your fix resolves the issue with ensuring that we retry upon
> subsequent attempts to access the inode but we should likely fix up
> this code.
>
> Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>

Please fix the patch description (e.g. "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"), add a
Fixes: line with the commit I cited, and re-post correctly with git
send-email so that it reaches the list.

[PATCH v2 0/1] selinux: fix error initialization in inode_doinit_with_dentry()

[rentianyue]

From: Tianyue Ren <rentianyue@kylinos.cn>

I think there is a error initialization for isec->sid when the dentry is NULL,
when we do "remount -o rw,remount ${rootmnt}" action before selinux policy loading.It's
leading kernel getting wrong security label "unlabeled_t" when the userspace access
files such as /etc/fstab.

This patch lets the label be invalid before it's initializated correctly.

v2: Modify the patch description

Tianyue Ren (1):
  selinux: fix error initialization in inode_doinit_with_dentry()

 security/selinux/hooks.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

-- 
2.28.0

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

[rentianyue]

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 | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index bf8328adad8f..da7295a546e0 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1499,6 +1499,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;
 			goto out;
 		}
 
@@ -1553,8 +1554,10 @@ 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)
+			if (!dentry) {
+				isec->initialized = LABEL_INVALID;
 				goto out;
+			}
 			rc = selinux_genfs_get_sid(dentry, sclass,
 						   sbsec->flags, &sid);
 			if (rc) {
-- 
2.28.0

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

[Stephen Smalley]

On Tue, Sep 29, 2020 at 9:38 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>

Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>

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

[Paul Moore]

On Wed, Sep 30, 2020 at 9:50 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
> On Tue, Sep 29, 2020 at 9:38 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>
>
> Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>

Thank you for the patch, and your patience with the submission process.

Considering that this has been broken for almost four years and we are
currently at -rc7, I'm going to hold this for after the upcoming merge
window in order to limit our risk for the upcoming v5.9 release.

-- 
paul moore
www.paul-moore.com

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

[Paul Moore]

On Tue, Sep 29, 2020 at 9:38 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 | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index bf8328adad8f..da7295a546e0 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -1499,6 +1499,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;
>                         goto out;
>                 }
>
> @@ -1553,8 +1554,10 @@ 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)
> +                       if (!dentry) {
> +                               isec->initialized = LABEL_INVALID;
>                                 goto out;
> +                       }
>                         rc = selinux_genfs_get_sid(dentry, sclass,
>                                                    sbsec->flags, &sid);
>                         if (rc) {

Looking at this some more, in both cases where we mark the isec as
"LABEL_INVALID" we can probably just do a "return 0;" instead of
jumping to "out" as there is nothing useful there except a needless
spin lock/unlock cycle.  I would suggest adding a short explanation to
the comment above each line explaining why this is okay.

-- 
paul moore
www.paul-moore.com

[PATCH v3 0/1] fix error initialization in inode_doinit_with_dentry()

[rentianyue]

From: Tianyue Ren <rentianyue@kylinos.cn>

I think there is a error initialization for isec->sid when the dentry is NULL,
when we do "remount -o rw,remount ${rootmnt}" action before selinux policy loading.It's
leading kernel getting wrong security label "unlabeled_t" when the userspace access
files such as /etc/fstab.

This patch lets the label be invalid before it's initializated correctly.

v2: Modify the patch description
v3: Remove unnecessary "out cycle" that do a needless
    spin lock/unlock cycle when dentry is NULL.

Tianyue Ren (1):
  selinux: fix error initialization in inode_doinit_with_dentry()

 security/selinux/hooks.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

-- 
2.28.0

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

[rentianyue]

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(-)

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) {
-- 
2.28.0

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

[Paul Moore]

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!

-- 
paul moore
www.paul-moore.com

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

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

[Paul Moore]

On Tue, Sep 29, 2020 at 9:31 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
> On Tue, Sep 29, 2020 at 8:54 AM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
> > On 9/27/20 5:42 AM, rentianyue@tj.kylinos.cn wrote:
> > > From: Tianyue Ren <rentianyue@kylinos.cn>
> > >
> > > Fix to initialize isec->class with SECINITSID_UNLABELED other
> > > than the from the xattr label when then dentry is NULL when
> > > the filesystem is remounted before the policy loading.
> >
> > Looks like this was broken by commit
> > 9287aed2ad1ff1bde5eb190bcd6dccd5f1cf47d3 ("selinux: Convert isec->lock
> > into a spinlock").
>
> It appears that the broken commit assumed (wrongly) that isec->sid is
> 0 initially, sets sid = isec->sid, and then in the out: path, if (!sid
> || rc) it sets isec->initialized to LABEL_INVALID.  In fact, isec->sid
> is SECINITSID_UNLABELED initially upon selinux_inode_alloc_security(),
> so that !sid test never evaluates to true.  And changing it to compare
> with SECINITSID_UNLABELED wouldn't be safe either since it is possible
> to end up with SECINITSID_UNLABELED without it being invalid.  I think
> your fix resolves the issue with ensuring that we retry upon
> subsequent attempts to access the inode but we should likely fix up
> this code.

Beyond the patch that has already been posted, I think the fix/clean
up is probably just to change the "!sid || rc" conditional in the
"out" jump target to simply "rc".  All of the code above that appears
to set "rc" correctly on error, which is really the only time (beyond
the posted patch) that we would need to set "isec->initizalized" to
"LABEL_INVALID".

-- 
paul moore
www.paul-moore.com