Re: [PATCH 1/2] selinux: treat atomic flags more carefully

[Stephen Smalley <sds@xxxxxxxxxxxxx>]

On 1/7/20 8:31 AM, Ondrej Mosnacek wrote:
> The disabled/enforcing/initialized flags are all accessed concurrently
> by threads so use the appropriate accessors that ensure atomicity and
> document that it is expected.
>
> Use smp_load/acquire...() helpers (with memory barriers) for the
> initialized flag, since it gates access to the rest of the state
> structures.
>
> Note that the disabled flag is currently not used for anything other
> than avoiding double disable, but it will be used for bailing out of
> hooks once security_delete_hooks() is removed.
>
> Signed-off-by: Ondrej Mosnacek <omosnace@xxxxxxxxxx>

This looks good to me as a cleanup regardless of whether the 2nd patch 
is merged.  Similar treatment is likely needed for state->checkreqprot 
(although I'd like to obsolete and get rid of that entirely at some 
point), maybe state->policycap[], isec->initialized, and others, but 
that can all be done separately.

Acked-by: Stephen Smalley <sds@xxxxxxxxxxxxx>

> ---
>   security/selinux/hooks.c            | 21 ++++++++--------
>   security/selinux/include/security.h | 33 +++++++++++++++++++++++--
>   security/selinux/ss/services.c      | 38 ++++++++++++++---------------
>   3 files changed, 61 insertions(+), 31 deletions(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 659c4a81e897..47ad4db925cf 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -272,7 +272,7 @@ static int __inode_security_revalidate(struct inode *inode,
>   
>   	might_sleep_if(may_sleep);
>   
> -	if (selinux_state.initialized &&
> +	if (selinux_initialized(&selinux_state) &&
>   	    isec->initialized != LABEL_INITIALIZED) {
>   		if (!may_sleep)
>   			return -ECHILD;
> @@ -659,7 +659,7 @@ static int selinux_set_mnt_opts(struct super_block *sb,
>   
>   	mutex_lock(&sbsec->lock);
>   
> -	if (!selinux_state.initialized) {
> +	if (!selinux_initialized(&selinux_state)) {
>   		if (!opts) {
>   			/* Defer initialization until selinux_complete_init,
>   			   after the initial policy is loaded and the security
> @@ -928,7 +928,7 @@ static int selinux_sb_clone_mnt_opts(const struct super_block *oldsb,
>   	 * if the parent was able to be mounted it clearly had no special lsm
>   	 * mount options.  thus we can safely deal with this superblock later
>   	 */
> -	if (!selinux_state.initialized)
> +	if (!selinux_initialized(&selinux_state))
>   		return 0;
>   
>   	/*
> @@ -1103,7 +1103,7 @@ static int selinux_sb_show_options(struct seq_file *m, struct super_block *sb)
>   	if (!(sbsec->flags & SE_SBINITIALIZED))
>   		return 0;
>   
> -	if (!selinux_state.initialized)
> +	if (!selinux_initialized(&selinux_state))
>   		return 0;
>   
>   	if (sbsec->flags & FSCONTEXT_MNT) {
> @@ -2920,7 +2920,8 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
>   		isec->initialized = LABEL_INITIALIZED;
>   	}
>   
> -	if (!selinux_state.initialized || !(sbsec->flags & SBLABEL_MNT))
> +	if (!selinux_initialized(&selinux_state) ||
> +	    !(sbsec->flags & SBLABEL_MNT))
>   		return -EOPNOTSUPP;
>   
>   	if (name)
> @@ -3143,7 +3144,7 @@ static int selinux_inode_setxattr(struct dentry *dentry, const char *name,
>   		return dentry_has_perm(current_cred(), dentry, FILE__SETATTR);
>   	}
>   
> -	if (!selinux_state.initialized)
> +	if (!selinux_initialized(&selinux_state))
>   		return (inode_owner_or_capable(inode) ? 0 : -EPERM);
>   
>   	sbsec = inode->i_sb->s_security;
> @@ -3229,7 +3230,7 @@ static void selinux_inode_post_setxattr(struct dentry *dentry, const char *name,
>   		return;
>   	}
>   
> -	if (!selinux_state.initialized) {
> +	if (!selinux_initialized(&selinux_state)) {
>   		/* If we haven't even been initialized, then we can't validate
>   		 * against a policy, so leave the label as invalid. It may
>   		 * resolve to a valid label on the next revalidation try if
> @@ -7304,17 +7305,17 @@ static void selinux_nf_ip_exit(void)
>   #ifdef CONFIG_SECURITY_SELINUX_DISABLE
>   int selinux_disable(struct selinux_state *state)
>   {
> -	if (state->initialized) {
> +	if (selinux_initialized(state)) {
>   		/* Not permitted after initial policy load. */
>   		return -EINVAL;
>   	}
>   
> -	if (state->disabled) {
> +	if (selinux_disabled(state)) {
>   		/* Only do this once. */
>   		return -EINVAL;
>   	}
>   
> -	state->disabled = 1;
> +	selinux_mark_disabled(state);
>   
>   	pr_info("SELinux:  Disabled at runtime.\n");
>   
> diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
> index ecdd610e6449..a39f9565d80b 100644
> --- a/security/selinux/include/security.h
> +++ b/security/selinux/include/security.h
> @@ -117,15 +117,27 @@ void selinux_avc_init(struct selinux_avc **avc);
>   
>   extern struct selinux_state selinux_state;
>   
> +static inline bool selinux_initialized(const struct selinux_state *state)
> +{
> +	/* do a synchronized load to avoid race conditions */
> +	return smp_load_acquire(&state->initialized);
> +}
> +
> +static inline void selinux_mark_initialized(struct selinux_state *state)
> +{
> +	/* do a synchronized write to avoid race conditions */
> +	smp_store_release(&state->initialized, true);
> +}
> +
>   #ifdef CONFIG_SECURITY_SELINUX_DEVELOP
>   static inline bool enforcing_enabled(struct selinux_state *state)
>   {
> -	return state->enforcing;
> +	return READ_ONCE(state->enforcing);
>   }
>   
>   static inline void enforcing_set(struct selinux_state *state, bool value)
>   {
> -	state->enforcing = value;
> +	WRITE_ONCE(state->enforcing, value);
>   }
>   #else
>   static inline bool enforcing_enabled(struct selinux_state *state)
> @@ -138,6 +150,23 @@ static inline void enforcing_set(struct selinux_state *state, bool value)
>   }
>   #endif
>   
> +#ifdef CONFIG_SECURITY_SELINUX_DISABLE
> +static inline bool selinux_disabled(struct selinux_state *state)
> +{
> +	return READ_ONCE(state->disabled);
> +}
> +
> +static inline void selinux_mark_disabled(struct selinux_state *state)
> +{
> +	WRITE_ONCE(state->disabled, true);
> +}
> +#else
> +static inline bool selinux_disabled(struct selinux_state *state)
> +{
> +	return false;
> +}
> +#endif
> +
>   static inline bool selinux_policycap_netpeer(void)
>   {
>   	struct selinux_state *state = &selinux_state;
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index 55cf42945cba..0e8b94e8e156 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -767,7 +767,7 @@ static int security_compute_validatetrans(struct selinux_state *state,
>   	int rc = 0;
>   
>   
> -	if (!state->initialized)
> +	if (!selinux_initialized(state))
>   		return 0;
>   
>   	read_lock(&state->ss->policy_rwlock);
> @@ -868,7 +868,7 @@ int security_bounded_transition(struct selinux_state *state,
>   	int index;
>   	int rc;
>   
> -	if (!state->initialized)
> +	if (!selinux_initialized(state))
>   		return 0;
>   
>   	read_lock(&state->ss->policy_rwlock);
> @@ -1027,7 +1027,7 @@ void security_compute_xperms_decision(struct selinux_state *state,
>   	memset(xpermd->dontaudit->p, 0, sizeof(xpermd->dontaudit->p));
>   
>   	read_lock(&state->ss->policy_rwlock);
> -	if (!state->initialized)
> +	if (!selinux_initialized(state))
>   		goto allow;
>   
>   	policydb = &state->ss->policydb;
> @@ -1112,7 +1112,7 @@ void security_compute_av(struct selinux_state *state,
>   	read_lock(&state->ss->policy_rwlock);
>   	avd_init(state, avd);
>   	xperms->len = 0;
> -	if (!state->initialized)
> +	if (!selinux_initialized(state))
>   		goto allow;
>   
>   	policydb = &state->ss->policydb;
> @@ -1166,7 +1166,7 @@ void security_compute_av_user(struct selinux_state *state,
>   
>   	read_lock(&state->ss->policy_rwlock);
>   	avd_init(state, avd);
> -	if (!state->initialized)
> +	if (!selinux_initialized(state))
>   		goto allow;
>   
>   	policydb = &state->ss->policydb;
> @@ -1286,7 +1286,7 @@ int security_sidtab_hash_stats(struct selinux_state *state, char *page)
>   {
>   	int rc;
>   
> -	if (!state->initialized) {
> +	if (!selinux_initialized(state)) {
>   		pr_err("SELinux: %s:  called before initial load_policy\n",
>   		       __func__);
>   		return -EINVAL;
> @@ -1320,7 +1320,7 @@ static int security_sid_to_context_core(struct selinux_state *state,
>   		*scontext = NULL;
>   	*scontext_len  = 0;
>   
> -	if (!state->initialized) {
> +	if (!selinux_initialized(state)) {
>   		if (sid <= SECINITSID_NUM) {
>   			char *scontextp;
>   
> @@ -1549,7 +1549,7 @@ static int security_context_to_sid_core(struct selinux_state *state,
>   	if (!scontext2)
>   		return -ENOMEM;
>   
> -	if (!state->initialized) {
> +	if (!selinux_initialized(state)) {
>   		int i;
>   
>   		for (i = 1; i < SECINITSID_NUM; i++) {
> @@ -1736,7 +1736,7 @@ static int security_compute_sid(struct selinux_state *state,
>   	int rc = 0;
>   	bool sock;
>   
> -	if (!state->initialized) {
> +	if (!selinux_initialized(state)) {
>   		switch (orig_tclass) {
>   		case SECCLASS_PROCESS: /* kernel value */
>   			*out_sid = ssid;
> @@ -2198,7 +2198,7 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len)
>   		goto out;
>   	}
>   
> -	if (!state->initialized) {
> +	if (!selinux_initialized(state)) {
>   		rc = policydb_read(policydb, fp);
>   		if (rc) {
>   			kfree(newsidtab);
> @@ -2223,7 +2223,7 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len)
>   
>   		state->ss->sidtab = newsidtab;
>   		security_load_policycaps(state);
> -		state->initialized = 1;
> +		selinux_mark_initialized(state);
>   		seqno = ++state->ss->latest_granting;
>   		selinux_complete_init();
>   		avc_ss_reset(state->avc, seqno);
> @@ -2639,7 +2639,7 @@ int security_get_user_sids(struct selinux_state *state,
>   	*sids = NULL;
>   	*nel = 0;
>   
> -	if (!state->initialized)
> +	if (!selinux_initialized(state))
>   		goto out;
>   
>   	read_lock(&state->ss->policy_rwlock);
> @@ -2875,7 +2875,7 @@ int security_get_bools(struct selinux_state *state,
>   	struct policydb *policydb;
>   	int i, rc;
>   
> -	if (!state->initialized) {
> +	if (!selinux_initialized(state)) {
>   		*len = 0;
>   		*names = NULL;
>   		*values = NULL;
> @@ -3050,7 +3050,7 @@ int security_sid_mls_copy(struct selinux_state *state,
>   	int rc;
>   
>   	rc = 0;
> -	if (!state->initialized || !policydb->mls_enabled) {
> +	if (!selinux_initialized(state) || !policydb->mls_enabled) {
>   		*new_sid = sid;
>   		goto out;
>   	}
> @@ -3217,7 +3217,7 @@ int security_get_classes(struct selinux_state *state,
>   	struct policydb *policydb = &state->ss->policydb;
>   	int rc;
>   
> -	if (!state->initialized) {
> +	if (!selinux_initialized(state)) {
>   		*nclasses = 0;
>   		*classes = NULL;
>   		return 0;
> @@ -3366,7 +3366,7 @@ int selinux_audit_rule_init(u32 field, u32 op, char *rulestr, void **vrule)
>   
>   	*rule = NULL;
>   
> -	if (!state->initialized)
> +	if (!selinux_initialized(state))
>   		return -EOPNOTSUPP;
>   
>   	switch (field) {
> @@ -3665,7 +3665,7 @@ int security_netlbl_secattr_to_sid(struct selinux_state *state,
>   	struct context *ctx;
>   	struct context ctx_new;
>   
> -	if (!state->initialized) {
> +	if (!selinux_initialized(state)) {
>   		*sid = SECSID_NULL;
>   		return 0;
>   	}
> @@ -3732,7 +3732,7 @@ int security_netlbl_sid_to_secattr(struct selinux_state *state,
>   	int rc;
>   	struct context *ctx;
>   
> -	if (!state->initialized)
> +	if (!selinux_initialized(state))
>   		return 0;
>   
>   	read_lock(&state->ss->policy_rwlock);
> @@ -3771,7 +3771,7 @@ int security_read_policy(struct selinux_state *state,
>   	int rc;
>   	struct policy_file fp;
>   
> -	if (!state->initialized)
> +	if (!selinux_initialized(state))
>   		return -EINVAL;
>   
>   	*len = security_policydb_len(state);