* Re: [PATCH v3 1/1] selinux: fix error initialization in inode_doinit_with_dentry() [not found] ` <CAHC9VhR2KPKN8ot9WrkjZQ08X-VPDGkXro18C5jhDEwcFH6wog@mail.gmail.com> @ 2020-11-03 13:13 ` Sven Schnelle 2020-11-03 17:11 ` Paul Moore 0 siblings, 1 reply; 6+ messages in thread From: Sven Schnelle @ 2020-11-03 13:13 UTC (permalink / raw) To: rentianyue Cc: Paul Moore, Stephen Smalley, Eric Paris, Andreas Gruenbacher, yangzhao, selinux, Tianyue Ren, linux-s390, hca, borntraeger 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/1] selinux: fix error initialization in inode_doinit_with_dentry() 2020-11-03 13:13 ` [PATCH v3 1/1] selinux: fix error initialization in inode_doinit_with_dentry() Sven Schnelle @ 2020-11-03 17:11 ` Paul Moore 2020-11-03 19:02 ` Sven Schnelle 0 siblings, 1 reply; 6+ messages in thread From: Paul Moore @ 2020-11-03 17:11 UTC (permalink / raw) To: Sven Schnelle Cc: rentianyue, Stephen Smalley, Eric Paris, Andreas Gruenbacher, yangzhao, selinux, Tianyue Ren, linux-s390, hca, borntraeger [-- 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. */ ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/1] selinux: fix error initialization in inode_doinit_with_dentry() 2020-11-03 17:11 ` Paul Moore @ 2020-11-03 19:02 ` Sven Schnelle 2020-11-04 2:42 ` Paul Moore 0 siblings, 1 reply; 6+ messages in thread From: Sven Schnelle @ 2020-11-03 19:02 UTC (permalink / raw) To: Paul Moore Cc: rentianyue, Stephen Smalley, Eric Paris, Andreas Gruenbacher, yangzhao, selinux, Tianyue Ren, linux-s390, hca, borntraeger 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/1] selinux: fix error initialization in inode_doinit_with_dentry() 2020-11-03 19:02 ` Sven Schnelle @ 2020-11-04 2:42 ` Paul Moore 2020-11-04 7:01 ` Sven Schnelle 0 siblings, 1 reply; 6+ messages in thread From: Paul Moore @ 2020-11-04 2:42 UTC (permalink / raw) To: Sven Schnelle Cc: rentianyue, Stephen Smalley, Eric Paris, Andreas Gruenbacher, yangzhao, selinux, Tianyue Ren, linux-s390, hca, borntraeger [-- 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. */ ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/1] selinux: fix error initialization in inode_doinit_with_dentry() 2020-11-04 2:42 ` Paul Moore @ 2020-11-04 7:01 ` Sven Schnelle 2020-11-04 20:40 ` Paul Moore 0 siblings, 1 reply; 6+ messages in thread From: Sven Schnelle @ 2020-11-04 7:01 UTC (permalink / raw) To: Paul Moore Cc: rentianyue, Stephen Smalley, Eric Paris, Andreas Gruenbacher, yangzhao, selinux, Tianyue Ren, linux-s390, hca, borntraeger 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/1] selinux: fix error initialization in inode_doinit_with_dentry() 2020-11-04 7:01 ` Sven Schnelle @ 2020-11-04 20:40 ` Paul Moore 0 siblings, 0 replies; 6+ messages in thread From: Paul Moore @ 2020-11-04 20:40 UTC (permalink / raw) To: Sven Schnelle Cc: rentianyue, Stephen Smalley, Eric Paris, Andreas Gruenbacher, yangzhao, selinux, Tianyue Ren, linux-s390, hca, borntraeger 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-11-04 20:40 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <CAHC9VhQTp3Rc_7zM661Rzur0XSuWRWKJJg=CwLPAQo5ABRpS-w@mail.gmail.com>
[not found] ` <20201009013630.6777-1-rentianyue@tj.kylinos.cn>
[not found] ` <20201009013630.6777-2-rentianyue@tj.kylinos.cn>
[not found] ` <CAHC9VhR2KPKN8ot9WrkjZQ08X-VPDGkXro18C5jhDEwcFH6wog@mail.gmail.com>
2020-11-03 13:13 ` [PATCH v3 1/1] selinux: fix error initialization in inode_doinit_with_dentry() Sven Schnelle
2020-11-03 17:11 ` Paul Moore
2020-11-03 19:02 ` Sven Schnelle
2020-11-04 2:42 ` Paul Moore
2020-11-04 7:01 ` Sven Schnelle
2020-11-04 20:40 ` Paul Moore
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox