| Message ID | 20201009013630.6777-2-rentianyue@tj.kylinos.cn (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | fix error initialization in inode_doinit_with_dentry() | expand |
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 <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
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?
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
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?
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
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 --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) {