Re: [RFC PATCH] selinux: clean up selinux_enabled/disabled

[Stephen Smalley <sds@xxxxxxxxxxxxx>]

On 12/13/19 9:32 AM, Stephen Smalley wrote:
> Rename selinux_enabled to selinux_enabled_boot to make it clear that
> it only reflects whether SELinux was enabled at boot.  Further make it
> unconditionally read-only-after-init and stop needlessly clearing it
> in selinux_disable() since it is only used to skip other SELinux
> initialization code.
>
> Wrap the disabled field in the struct selinux_state with
> CONFIG_SECURITY_SELINUX_DISABLE since it is only used for
> runtime disable.
>
> Introduce a selinux_is_enabled() static inline that tests both
> selinux_enabled_boot and selinux_state.disabled as appropriate
> to determine whether SELinux is in an enabled state.  Use this function
> in the MAC_STATUS audit log message in place of selinux_enabled(_boot).
> It is unclear why this information is included in that audit record
> since selinuxfs is never registered at all if !selinux_enabled_boot
> and is unregistered in the runtime disable case, so this code should never
> be reached if SELinux is disabled.  It is also unclear why it is logged
> twice under enabled/old-enabled since setenforce does not change its
> value. Regardless, we leave it as is for compatibility.

Just noticed that there is another AUDIT_MAC_STATUS audit log in 
sel_write_disable() that uses hardcoded 0, 1 for enabled and old-enabled 
values in the audit record.  Don't know if it should also use 
selinux_is_enabled(), or if we should likewise just hardcode the values 
used in the sel_write_enforce() case (to 1, 1, since this code shouldn't 
be reachable if SELinux were disabled).  If the latter, we don't need 
selinux_is_enabled() at all.

> As a further cleanup, we could make selinux_enabled_boot __initdata if
> we were to stop testing it for the MAC_STATUS audit record since that
> is the only non-init code that uses it.  The selinux_is_enabled()
> static inline function could be reduced to only testing
> selinux_state.disabled or always being 1 if
> CONFIG_SECURITY_SELINUX_DISABLE=n, with it being implicit that we would
> not have reached the test if selinux_enabled_boot was not 1.
>
> Signed-off-by: Stephen Smalley <sds@xxxxxxxxxxxxx>
> ---
>   security/selinux/hooks.c            | 10 ++++------
>   security/selinux/ibpkey.c           |  2 +-
>   security/selinux/include/security.h | 16 +++++++++++++++-
>   security/selinux/netif.c            |  2 +-
>   security/selinux/netnode.c          |  2 +-
>   security/selinux/netport.c          |  2 +-
>   security/selinux/selinuxfs.c        |  4 ++--
>   7 files changed, 25 insertions(+), 13 deletions(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 40ec866e48da..7984e72312da 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -123,13 +123,13 @@ __setup("enforcing=", enforcing_setup);
>   #define selinux_enforcing_boot 1
>   #endif
>   
> -int selinux_enabled __lsm_ro_after_init = 1;
> +int selinux_enabled_boot __ro_after_init = 1;
>   #ifdef CONFIG_SECURITY_SELINUX_BOOTPARAM
>   static int __init selinux_enabled_setup(char *str)
>   {
>   	unsigned long enabled;
>   	if (!kstrtoul(str, 0, &enabled))
> -		selinux_enabled = enabled ? 1 : 0;
> +		selinux_enabled_boot = enabled ? 1 : 0;
>   	return 1;
>   }
>   __setup("selinux=", selinux_enabled_setup);
> @@ -7202,7 +7202,7 @@ void selinux_complete_init(void)
>   DEFINE_LSM(selinux) = {
>   	.name = "selinux",
>   	.flags = LSM_FLAG_LEGACY_MAJOR | LSM_FLAG_EXCLUSIVE,
> -	.enabled = &selinux_enabled,
> +	.enabled = &selinux_enabled_boot,
>   	.blobs = &selinux_blob_sizes,
>   	.init = selinux_init,
>   };
> @@ -7271,7 +7271,7 @@ static int __init selinux_nf_ip_init(void)
>   {
>   	int err;
>   
> -	if (!selinux_enabled)
> +	if (!selinux_enabled_boot)
>   		return 0;
>   
>   	pr_debug("SELinux:  Registering netfilter hooks\n");
> @@ -7318,8 +7318,6 @@ int selinux_disable(struct selinux_state *state)
>   
>   	pr_info("SELinux:  Disabled at runtime.\n");
>   
> -	selinux_enabled = 0;
> -
>   	security_delete_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks));
>   
>   	/* Try to destroy the avc node cache */
> diff --git a/security/selinux/ibpkey.c b/security/selinux/ibpkey.c
> index de92365e4324..f68a7617cfb9 100644
> --- a/security/selinux/ibpkey.c
> +++ b/security/selinux/ibpkey.c
> @@ -222,7 +222,7 @@ static __init int sel_ib_pkey_init(void)
>   {
>   	int iter;
>   
> -	if (!selinux_enabled)
> +	if (!selinux_enabled_boot)
>   		return 0;
>   
>   	for (iter = 0; iter < SEL_PKEY_HASH_SIZE; iter++) {
> diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
> index 8c0dbbd076c6..49737087ad33 100644
> --- a/security/selinux/include/security.h
> +++ b/security/selinux/include/security.h
> @@ -69,7 +69,7 @@
>   
>   struct netlbl_lsm_secattr;
>   
> -extern int selinux_enabled;
> +extern int selinux_enabled_boot;
>   
>   /* Policy capabilities */
>   enum {
> @@ -99,7 +99,9 @@ struct selinux_avc;
>   struct selinux_ss;
>   
>   struct selinux_state {
> +#ifdef CONFIG_SECURITY_SELINUX_DISABLE
>   	bool disabled;
> +#endif
>   #ifdef CONFIG_SECURITY_SELINUX_DEVELOP
>   	bool enforcing;
>   #endif
> @@ -136,6 +138,18 @@ static inline void enforcing_set(struct selinux_state *state, bool value)
>   }
>   #endif
>   
> +#ifdef CONFIG_SECURITY_SELINUX_DISABLE
> +static inline bool selinux_is_enabled(struct selinux_state *state)
> +{
> +	return selinux_enabled_boot && !state->disabled;
> +}
> +#else
> +static inline bool selinux_is_enabled(struct selinux_state *state)
> +{
> +	return selinux_enabled_boot;
> +}
> +#endif
> +
>   static inline bool selinux_policycap_netpeer(void)
>   {
>   	struct selinux_state *state = &selinux_state;
> diff --git a/security/selinux/netif.c b/security/selinux/netif.c
> index e40fecd73752..15b8c1bcd7d0 100644
> --- a/security/selinux/netif.c
> +++ b/security/selinux/netif.c
> @@ -266,7 +266,7 @@ static __init int sel_netif_init(void)
>   {
>   	int i;
>   
> -	if (!selinux_enabled)
> +	if (!selinux_enabled_boot)
>   		return 0;
>   
>   	for (i = 0; i < SEL_NETIF_HASH_SIZE; i++)
> diff --git a/security/selinux/netnode.c b/security/selinux/netnode.c
> index 9ab84efa46c7..dff587d1e164 100644
> --- a/security/selinux/netnode.c
> +++ b/security/selinux/netnode.c
> @@ -291,7 +291,7 @@ static __init int sel_netnode_init(void)
>   {
>   	int iter;
>   
> -	if (!selinux_enabled)
> +	if (!selinux_enabled_boot)
>   		return 0;
>   
>   	for (iter = 0; iter < SEL_NETNODE_HASH_SIZE; iter++) {
> diff --git a/security/selinux/netport.c b/security/selinux/netport.c
> index 3f8b2c0458c8..de727f7489b7 100644
> --- a/security/selinux/netport.c
> +++ b/security/selinux/netport.c
> @@ -225,7 +225,7 @@ static __init int sel_netport_init(void)
>   {
>   	int iter;
>   
> -	if (!selinux_enabled)
> +	if (!selinux_enabled_boot)
>   		return 0;
>   
>   	for (iter = 0; iter < SEL_NETPORT_HASH_SIZE; iter++) {
> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> index dd7bb1f1dc99..3b8397ed87f3 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -172,7 +172,7 @@ static ssize_t sel_write_enforce(struct file *file, const char __user *buf,
>   			new_value, old_value,
>   			from_kuid(&init_user_ns, audit_get_loginuid(current)),
>   			audit_get_sessionid(current),
> -			selinux_enabled, selinux_enabled);
> +			selinux_is_enabled(state), selinux_is_enabled(state));
>   		enforcing_set(state, new_value);
>   		if (new_value)
>   			avc_ss_reset(state->avc, 0);
> @@ -2105,7 +2105,7 @@ static int __init init_sel_fs(void)
>   					  sizeof(NULL_FILE_NAME)-1);
>   	int err;
>   
> -	if (!selinux_enabled)
> +	if (!selinux_enabled_boot)
>   		return 0;
>   
>   	err = sysfs_create_mount_point(fs_kobj, "selinux");