linux-kernel - Re: [PATCH] selinux: consilidate comments from inode_doinit_with

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAHC9VhQngdvJCDWtguSH8EXwYhgeQwG8Zbp5jA3bheNVCZaigA@mail.gmail.com>
Date:   Mon, 11 Oct 2021 19:13:34 -0400
From:   Paul Moore <paul@...l-moore.com>
To:     Davidlohr Bueso <dave@...olabs.net>
Cc:     Stephen Smalley <stephen.smalley.work@...il.com>,
        selinux@...r.kernel.org, linux-kernel@...r.kernel.org,
        Davidlohr Bueso <dbueso@...e.de>
Subject: Re: [PATCH] selinux: consilidate comments from inode_doinit_with_dentry
 wrt !dentry

On Mon, Oct 11, 2021 at 3:02 PM Davidlohr Bueso <dave@...olabs.net> wrote:
>
> Instead of having the same big comment twice for the same scenario,
> just have it in out_invalid.
>
> Signed-off-by: Davidlohr Bueso <dbueso@...e.de>
> ---
>  security/selinux/hooks.c | 30 ++++++++++--------------------
>  1 file changed, 10 insertions(+), 20 deletions(-)

Hi Davidlohr,

Thanks for the patch, but I think the comment is a bit more
discoverable/helpful when it is integrated into the main flow of the
code and not the error handling at the bottom.  However, if you wanted
to change the second instance to refer to the prior instance I think
that would be okay.

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index e7ebd45ca345..0e0013cc7ba8 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -1502,18 +1502,8 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent
>                         if (!dentry)
>                                 dentry = d_find_any_alias(inode);
>                 }
> -               if (!dentry) {
> -                       /*
> -                        * this is can be hit on boot when a file is accessed
> -                        * before the policy is loaded.  When we load policy we
> -                        * may find inodes that have no dentry on the
> -                        * sbsec->isec_head list.  No reason to complain as these
> -                        * will get fixed up the next time we go through
> -                        * inode_doinit with a dentry, before these inodes could
> -                        * be used again by userspace.
> -                        */
> +               if (!dentry)
>                         goto out_invalid;
> -               }
>
>                 rc = inode_doinit_use_xattr(inode, dentry, sbsec->def_sid,
>                                             &sid);
> @@ -1559,15 +1549,6 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent
>                                 if (!dentry)
>                                         dentry = d_find_any_alias(inode);
>                         }
> -                       /*
> -                        * This can be hit on boot when a file is accessed
> -                        * before the policy is loaded.  When we load policy we
> -                        * may find inodes that have no dentry on the
> -                        * sbsec->isec_head list.  No reason to complain as
> -                        * these will get fixed up the next time we go through
> -                        * inode_doinit() with a dentry, before these inodes
> -                        * could be used again by userspace.
> -                        */
>                         if (!dentry)
>                                 goto out_invalid;
>                         rc = selinux_genfs_get_sid(dentry, sclass,
> @@ -1607,6 +1588,15 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent
>         return rc;
>
>  out_invalid:
> +       /*
> +        * This is can be hit on boot when a file is accessed
> +        * before the policy is loaded.  When we load policy we
> +        * may find inodes that have no dentry on the
> +        * sbsec->isec_head list.  No reason to complain as these
> +        * will get fixed up the next time we go through
> +        * inode_doinit with a dentry, before these inodes could
> +        * be used again by userspace.
> +        */
>         spin_lock(&isec->lock);
>         if (isec->initialized == LABEL_PENDING) {
>                 isec->initialized = LABEL_INVALID;
> --
> 2.26.2

-- 
paul moore
www.paul-moore.com