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