| Message ID | 20191213143218.149544-1-sds@tycho.nsa.gov (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC] selinux: clean up selinux_enabled/disabled | expand |
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@tycho.nsa.gov> > --- > 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"); >
On Fri, Dec 13, 2019 at 9:44 AM Stephen Smalley <sds@tycho.nsa.gov> wrote: > 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. I see no reason why we can't hardcode the enabled/old-enabled values as they are always going to be "1" here. As for why the values are being logged there at all, it has to do with a desire to have a consistent format for each record type; in sel_write_enforce() we log the enabled/old-enabled values for the MAC_STATUS record, so we need to ensure we log the values in sel_write_disable() as well. Looks fine to me otherwise.
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");
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. 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@tycho.nsa.gov> --- 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(-)