Re: [PATCH 2/2] security,selinux: get rid of security_delete_hooks()

From: James Morris <jmorris@namei.org>
To: Ondrej Mosnacek <omosnace@redhat.com>
Cc: linux-security-module@vger.kernel.org,
	"Serge E. Hallyn" <serge@hallyn.com>,
	Casey Schaufler <casey@schaufler-ca.com>,
	selinux@vger.kernel.org, Paul Moore <paul@paul-moore.com>,
	Stephen Smalley <sds@tycho.nsa.gov>,
	John Johansen <john.johansen@canonical.com>,
	Kees Cook <keescook@chromium.org>,
	Micah Morton <mortonm@chromium.org>,
	Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Subject: Re: [PATCH 2/2] security,selinux: get rid of security_delete_hooks()
Date: Wed, 8 Jan 2020 06:59:48 +1100 (AEDT)	[thread overview]
Message-ID: <alpine.LRH.2.21.2001080653220.575@namei.org> (raw)
In-Reply-To: <20200107133154.588958-3-omosnace@redhat.com>

On Tue, 7 Jan 2020, Ondrej Mosnacek wrote:

> The only user is SELinux, which is hereby converted to check the
> disabled flag in each hook instead of removing the hooks from the list.
> 
> The __lsm_ro_after_init macro is now removed and replaced with
> __ro_after_init directly.
> 
> This fixes a race condition in SELinux runtime disable, which was
> introduced with the switch to hook lists in b1d9e6b0646d ("LSM: Switch
> to lists of hooks").
> 
> Suggested-by: Stephen Smalley <sds@tycho.nsa.gov>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  include/linux/lsm_hooks.h    |  31 --
>  security/Kconfig             |   5 -
>  security/apparmor/lsm.c      |   6 +-
>  security/commoncap.c         |   2 +-
>  security/loadpin/loadpin.c   |   2 +-
>  security/lockdown/lockdown.c |   2 +-
>  security/security.c          |   5 +-
>  security/selinux/Kconfig     |   6 -
>  security/selinux/hooks.c     | 742 ++++++++++++++++++++++++++++++-----
>  security/smack/smack_lsm.c   |   4 +-
>  security/tomoyo/tomoyo.c     |   6 +-
>  security/yama/yama_lsm.c     |   2 +-
>  12 files changed, 654 insertions(+), 159 deletions(-)

Please separate the changes for each LSM into separate patches (the 
__lsm_ro_after_init removal patch can be last).

>  config SECURITY_SELINUX_DEVELOP
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 47ad4db925cf..9ac2b6b69ff9 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -650,13 +650,15 @@ static int selinux_set_mnt_opts(struct super_block *sb,
>  {
>  	const struct cred *cred = current_cred();
>  	struct superblock_security_struct *sbsec = sb->s_security;
> -	struct dentry *root = sbsec->sb->s_root;
>  	struct selinux_mnt_opts *opts = mnt_opts;

Seems like there are a bunch of unrelated cleanups mixed in here.

> -	int set_fscontext =	(oldsbsec->flags & FSCONTEXT_MNT);
> -	int set_context =	(oldsbsec->flags & CONTEXT_MNT);
> -	int set_rootcontext =	(oldsbsec->flags & ROOTCONTEXT_MNT);
> +	set_fscontext =		(oldsbsec->flags & FSCONTEXT_MNT);
> +	set_context =		(oldsbsec->flags & CONTEXT_MNT);
> +	set_rootcontext =	(oldsbsec->flags & ROOTCONTEXT_MNT);
>  

...

>  static int selinux_binder_set_context_mgr(struct task_struct *mgr)
>  {
> -	u32 mysid = current_sid();
> -	u32 mgrsid = task_sid(mgr);
> +	if (selinux_disabled(&selinux_state))
> +		return 0;
>  
>  	return avc_has_perm(&selinux_state,
> -			    mysid, mgrsid, SECCLASS_BINDER,
> +			    current_sid(), task_sid(mgr), SECCLASS_BINDER,
>  			    BINDER__SET_CONTEXT_MGR, NULL);
>  }
>  

Ditto, etc.

Please don't do this.

-- 
James Morris
<jmorris@namei.org>