[PATCH v2] selinux: move policy mutex to selinux_state, use in lockdep checks

[PATCH v2] selinux: move policy mutex to selinux_state, use in lockdep checks

[Stephen Smalley]

Move the mutex used to synchronize policy changes (reloads and setting
of booleans) from selinux_fs_info to selinux_state and use it in
lockdep checks for rcu_dereference_protected() calls in the security
server functions.  This makes the dependency on the mutex explicit
in the code rather than relying on comments.

Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>
---
v2 switches selinuxfs from using &selinux_state.policy_mutex to using
fsi->state->policy_mutex.  selinuxfs operates on fsi->state->policy
for all policy reading or modifying operations.  It only acts on
&selinux_state for checking permissions in the current context.
At present, fsi->state is always &selinux_state; this will change
when selinux namespaces are introduced.

 security/selinux/hooks.c            |  1 +
 security/selinux/include/security.h |  1 +
 security/selinux/selinuxfs.c        | 26 ++++++++++----------
 security/selinux/ss/services.c      | 37 +++++++----------------------
 4 files changed, 22 insertions(+), 43 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 6f30ba1a38dc..6210e98219a5 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -7237,6 +7237,7 @@ static __init int selinux_init(void)
 	selinux_state.checkreqprot = selinux_checkreqprot_boot;
 	selinux_avc_init(&selinux_state.avc);
 	mutex_init(&selinux_state.status_lock);
+	mutex_init(&selinux_state.policy_mutex);
 
 	/* Set the security state for the initial task. */
 	cred_init_security();
diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
index 505e51264d51..bbbf7141ccdb 100644
--- a/security/selinux/include/security.h
+++ b/security/selinux/include/security.h
@@ -103,6 +103,7 @@ struct selinux_state {
 
 	struct selinux_avc *avc;
 	struct selinux_policy __rcu *policy;
+	struct mutex policy_mutex;
 } __randomize_layout;
 
 void selinux_avc_init(struct selinux_avc **avc);
diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index d1872adf0c47..29567acdda21 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -75,7 +75,6 @@ struct selinux_fs_info {
 	unsigned long last_class_ino;
 	bool policy_opened;
 	struct dentry *policycap_dir;
-	struct mutex mutex;
 	unsigned long last_ino;
 	struct selinux_state *state;
 	struct super_block *sb;
@@ -89,7 +88,6 @@ static int selinux_fs_info_create(struct super_block *sb)
 	if (!fsi)
 		return -ENOMEM;
 
-	mutex_init(&fsi->mutex);
 	fsi->last_ino = SEL_INO_NEXT - 1;
 	fsi->state = &selinux_state;
 	fsi->sb = sb;
@@ -400,7 +398,7 @@ static int sel_open_policy(struct inode *inode, struct file *filp)
 
 	BUG_ON(filp->private_data);
 
-	mutex_lock(&fsi->mutex);
+	mutex_lock(&fsi->state->policy_mutex);
 
 	rc = avc_has_perm(&selinux_state,
 			  current_sid(), SECINITSID_SECURITY,
@@ -431,11 +429,11 @@ static int sel_open_policy(struct inode *inode, struct file *filp)
 
 	filp->private_data = plm;
 
-	mutex_unlock(&fsi->mutex);
+	mutex_unlock(&fsi->state->policy_mutex);
 
 	return 0;
 err:
-	mutex_unlock(&fsi->mutex);
+	mutex_unlock(&fsi->state->policy_mutex);
 
 	if (plm)
 		vfree(plm->data);
@@ -622,7 +620,7 @@ static ssize_t sel_write_load(struct file *file, const char __user *buf,
 	ssize_t length;
 	void *data = NULL;
 
-	mutex_lock(&fsi->mutex);
+	mutex_lock(&fsi->state->policy_mutex);
 
 	length = avc_has_perm(&selinux_state,
 			      current_sid(), SECINITSID_SECURITY,
@@ -666,7 +664,7 @@ static ssize_t sel_write_load(struct file *file, const char __user *buf,
 		from_kuid(&init_user_ns, audit_get_loginuid(current)),
 		audit_get_sessionid(current));
 out:
-	mutex_unlock(&fsi->mutex);
+	mutex_unlock(&fsi->state->policy_mutex);
 	vfree(data);
 	return length;
 }
@@ -1271,7 +1269,7 @@ static ssize_t sel_read_bool(struct file *filep, char __user *buf,
 	unsigned index = file_inode(filep)->i_ino & SEL_INO_MASK;
 	const char *name = filep->f_path.dentry->d_name.name;
 
-	mutex_lock(&fsi->mutex);
+	mutex_lock(&fsi->state->policy_mutex);
 
 	ret = -EINVAL;
 	if (index >= fsi->bool_num || strcmp(name,
@@ -1290,14 +1288,14 @@ static ssize_t sel_read_bool(struct file *filep, char __user *buf,
 	}
 	length = scnprintf(page, PAGE_SIZE, "%d %d", cur_enforcing,
 			  fsi->bool_pending_values[index]);
-	mutex_unlock(&fsi->mutex);
+	mutex_unlock(&fsi->state->policy_mutex);
 	ret = simple_read_from_buffer(buf, count, ppos, page, length);
 out_free:
 	free_page((unsigned long)page);
 	return ret;
 
 out_unlock:
-	mutex_unlock(&fsi->mutex);
+	mutex_unlock(&fsi->state->policy_mutex);
 	goto out_free;
 }
 
@@ -1322,7 +1320,7 @@ static ssize_t sel_write_bool(struct file *filep, const char __user *buf,
 	if (IS_ERR(page))
 		return PTR_ERR(page);
 
-	mutex_lock(&fsi->mutex);
+	mutex_lock(&fsi->state->policy_mutex);
 
 	length = avc_has_perm(&selinux_state,
 			      current_sid(), SECINITSID_SECURITY,
@@ -1347,7 +1345,7 @@ static ssize_t sel_write_bool(struct file *filep, const char __user *buf,
 	length = count;
 
 out:
-	mutex_unlock(&fsi->mutex);
+	mutex_unlock(&fsi->state->policy_mutex);
 	kfree(page);
 	return length;
 }
@@ -1378,7 +1376,7 @@ static ssize_t sel_commit_bools_write(struct file *filep,
 	if (IS_ERR(page))
 		return PTR_ERR(page);
 
-	mutex_lock(&fsi->mutex);
+	mutex_lock(&fsi->state->policy_mutex);
 
 	length = avc_has_perm(&selinux_state,
 			      current_sid(), SECINITSID_SECURITY,
@@ -1400,7 +1398,7 @@ static ssize_t sel_commit_bools_write(struct file *filep,
 		length = count;
 
 out:
-	mutex_unlock(&fsi->mutex);
+	mutex_unlock(&fsi->state->policy_mutex);
 	kfree(page);
 	return length;
 }
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index e730204f060b..85cfd46836c7 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -2163,13 +2163,8 @@ void selinux_policy_cancel(struct selinux_state *state,
 {
 	struct selinux_policy *oldpolicy;
 
-	/*
-	 * NOTE: We do not need to take the rcu read lock
-	 * around the code below because other policy-modifying
-	 * operations are already excluded by selinuxfs via
-	 * fsi->mutex.
-	 */
-	oldpolicy = rcu_dereference_check(state->policy, 1);
+	oldpolicy = rcu_dereference_protected(state->policy,
+					lockdep_is_held(&state->policy_mutex));
 
 	sidtab_cancel_convert(oldpolicy->sidtab);
 	selinux_policy_free(policy);
@@ -2192,13 +2187,8 @@ void selinux_policy_commit(struct selinux_state *state,
 	struct selinux_policy *oldpolicy;
 	u32 seqno;
 
-	/*
-	 * NOTE: We do not need to take the rcu read lock
-	 * around the code below because other policy-modifying
-	 * operations are already excluded by selinuxfs via
-	 * fsi->mutex.
-	 */
-	oldpolicy = rcu_dereference_check(state->policy, 1);
+	oldpolicy = rcu_dereference_protected(state->policy,
+					lockdep_is_held(&state->policy_mutex));
 
 	/* If switching between different policy types, log MLS status */
 	if (oldpolicy) {
@@ -2291,13 +2281,8 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len,
 		return 0;
 	}
 
-	/*
-	 * NOTE: We do not need to take the rcu read lock
-	 * around the code below because other policy-modifying
-	 * operations are already excluded by selinuxfs via
-	 * fsi->mutex.
-	 */
-	oldpolicy = rcu_dereference_check(state->policy, 1);
+	oldpolicy = rcu_dereference_protected(state->policy,
+					lockdep_is_held(&state->policy_mutex));
 
 	/* Preserve active boolean values from the old policy */
 	rc = security_preserve_bools(oldpolicy, newpolicy);
@@ -3013,14 +2998,8 @@ int security_set_bools(struct selinux_state *state, u32 len, int *values)
 	if (!selinux_initialized(state))
 		return -EINVAL;
 
-	/*
-	 * NOTE: We do not need to take the rcu read lock
-	 * around the code below because other policy-modifying
-	 * operations are already excluded by selinuxfs via
-	 * fsi->mutex.
-	 */
-
-	oldpolicy = rcu_dereference_check(state->policy, 1);
+	oldpolicy = rcu_dereference_protected(state->policy,
+					lockdep_is_held(&state->policy_mutex));
 
 	/* Consistency check on number of booleans, should never fail */
 	if (WARN_ON(len != oldpolicy->policydb.p_bools.nprim))
-- 
2.25.1

Re: [PATCH v2] selinux: move policy mutex to selinux_state, use in lockdep checks

[Ondrej Mosnacek]

On Wed, Aug 26, 2020 at 7:29 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
> Move the mutex used to synchronize policy changes (reloads and setting
> of booleans) from selinux_fs_info to selinux_state and use it in
> lockdep checks for rcu_dereference_protected() calls in the security
> server functions.  This makes the dependency on the mutex explicit
> in the code rather than relying on comments.
>
> Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> ---
> v2 switches selinuxfs from using &selinux_state.policy_mutex to using
> fsi->state->policy_mutex.  selinuxfs operates on fsi->state->policy
> for all policy reading or modifying operations.  It only acts on
> &selinux_state for checking permissions in the current context.
> At present, fsi->state is always &selinux_state; this will change
> when selinux namespaces are introduced.
>
>  security/selinux/hooks.c            |  1 +
>  security/selinux/include/security.h |  1 +
>  security/selinux/selinuxfs.c        | 26 ++++++++++----------
>  security/selinux/ss/services.c      | 37 +++++++----------------------
>  4 files changed, 22 insertions(+), 43 deletions(-)

Seeing this is in an actual patch, it now looks logical to me. *thumbsup*

Reviewed-by: Ondrej Mosnacek <omosnace@redhat.com>

-- 
Ondrej Mosnacek
Software Engineer, Platform Security - SELinux kernel
Red Hat, Inc.

Re: [PATCH v2] selinux: move policy mutex to selinux_state, use in lockdep checks

[Paul Moore]

On Wed, Aug 26, 2020 at 1:29 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> Move the mutex used to synchronize policy changes (reloads and setting
> of booleans) from selinux_fs_info to selinux_state and use it in
> lockdep checks for rcu_dereference_protected() calls in the security
> server functions.  This makes the dependency on the mutex explicit
> in the code rather than relying on comments.
>
> Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> ---
> v2 switches selinuxfs from using &selinux_state.policy_mutex to using
> fsi->state->policy_mutex.  selinuxfs operates on fsi->state->policy
> for all policy reading or modifying operations.  It only acts on
> &selinux_state for checking permissions in the current context.
> At present, fsi->state is always &selinux_state; this will change
> when selinux namespaces are introduced.
>
>  security/selinux/hooks.c            |  1 +
>  security/selinux/include/security.h |  1 +
>  security/selinux/selinuxfs.c        | 26 ++++++++++----------
>  security/selinux/ss/services.c      | 37 +++++++----------------------
>  4 files changed, 22 insertions(+), 43 deletions(-)

Thanks Stephen, merged into selinux/next.

-- 
paul moore
www.paul-moore.com