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

[James Morris <jmorris@namei.org>]

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>