diff mbox series

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

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

Commit Message

rentianyue@tj.kylinos.cn Sept. 30, 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 | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Stephen Smalley Sept. 30, 2020, 1:49 p.m. UTC | #1
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>
Paul Moore Oct. 1, 2020, 9:14 p.m. UTC | #2
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 Oct. 1, 2020, 9:45 p.m. UTC | #3
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 mbox series

Patch

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