On Wed, Aug 5, 2020 at 11:52 AM Stephen Smalley
<stephen.smalley.work@xxxxxxxxx> wrote:
>
> Encapsulate the policy state in its own structure (struct
> selinux_policy) that is separately allocated but referenced from the
> selinux_ss structure. The policy state includes the SID table
> (particularly the context structures), the policy database, and the
> mapping between the kernel classes/permissions and the policy values.
> Refactor the security server portion of the policy load logic to
> cleanly separate loading of the new structures from committing the new
> policy. Unify the initial policy load and reload code paths as much
> as possible, avoiding duplicated code. Make sure we are taking the
> policy read-lock prior to any dereferencing of the policy. Move the
> copying of the policy capability booleans into the state structure
> outside of the policy write-lock because they are separate from the
> policy and are read outside of any policy lock; possibly they should
> be using at least READ_ONCE/WRITE_ONCE or smp_load_acquire/store_release.
> Restore the load mutex that was previously removed by
> commit 89abd0acf033 ("SELinux: drop load_mutex in security_load_policy")
> to make explicit the exclusion even though it is currently redundant
> with the fsi->mutex held by selinuxfs; this makes clear that we do
> not need to take the policy read-lock across sidtab_convert() and will
> be useful in the future for lockdep checking.
>
> These changes simplify the policy loading logic, reduce the size of
> the critical section while holding the policy write-lock, and should
> facilitate future changes to e.g. refactor the entire policy reload
> logic including the selinuxfs code to make the updating of the policy
> and the selinuxfs directory tree atomic and/or to convert the policy
> read-write lock to RCU.
>
> Signed-off-by: Stephen Smalley <stephen.smalley.work@xxxxxxxxx>
> ---
> v4 does not take the policy read-lock across sidtab_convert() and
> therefore does not require changing allocations by it to be atomic
> or dropping the cond_resched() call. To make obvious that taking
> the policy read-lock is not necessary in security_load_policy(), restore
> the load mutex to security_load_policy() that was removed back in
> commit 89abd0acf033 ("SELinux: drop load_mutex in security_load_policy").
> However, since we have refactored security_load_policy() in this change
> to split out selinux_policy_commit(), we need to take the mutex in
> security_load_policy() and release it in selinux_policy_commit().
I'm not in love with the idea of splitting the lock/unlock across
different functions, more below in the relevant code section.
> security/selinux/ss/services.c | 403 +++++++++++++++++----------------
> security/selinux/ss/services.h | 11 +-
> 2 files changed, 220 insertions(+), 194 deletions(-)
...
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index 9e76a80db6e1..6dea93fac9e2 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -2115,11 +2126,73 @@ static void security_load_policycaps(struct selinux_state *state)
> pr_info("SELinux: unknown policy capability %u\n",
> i);
> }
> +
> + read_unlock(&state->ss->policy_rwlock);
> }
>
> static int security_preserve_bools(struct selinux_state *state,
> struct policydb *newpolicydb);
>
> +static void selinux_policy_free(struct selinux_policy *policy)
> +{
> + if (!policy)
> + return;
> +
> + policydb_destroy(&policy->policydb);
> + sidtab_destroy(&policy->sidtab);
> + kfree(policy->map.mapping);
> + kfree(policy);
> +}
> +
> +static void selinux_policy_commit(struct selinux_state *state,
> + struct selinux_policy *newpolicy)
> +{
> + struct selinux_policy *oldpolicy;
> + u32 seqno;
> +
> + lockdep_assert_held(&state->ss->load_mutex);
> +
> + /* If switching between different policy types, log MLS status */
> + oldpolicy = state->ss->policy;
> + if (oldpolicy) {
> + if (oldpolicy->policydb.mls_enabled && !newpolicy->policydb.mls_enabled)
> + pr_info("SELinux: Disabling MLS support...\n");
> + else if (!oldpolicy->policydb.mls_enabled && newpolicy->policydb.mls_enabled)
> + pr_info("SELinux: Enabling MLS support...\n");
> + }
> +
> + /* Install the new policy. */
> + write_lock_irq(&state->ss->policy_rwlock);
> + state->ss->policy = newpolicy;
> + seqno = ++state->ss->latest_granting;
> + write_unlock_irq(&state->ss->policy_rwlock);
> +
> + /* Load the policycaps from the new policy */
> + security_load_policycaps(state);
> +
> + if (!selinux_initialized(state)) {
> + /*
> + * After first policy load, the security server is
> + * marked as initialized and ready to handle requests and
> + * any objects created prior to policy load are then labeled.
> + */
> + selinux_mark_initialized(state);
> + mutex_unlock(&state->ss->load_mutex);
> + selinux_complete_init();
> + } else
> + mutex_unlock(&state->ss->load_mutex);
> +
> + /* Free the old policy */
> + selinux_policy_free(oldpolicy);
> +
> + /* Flush external caches and notify userspace of policy load */
> + avc_ss_reset(state->avc, seqno);
> + selnl_notify_policyload(seqno);
> + selinux_status_update_policyload(state, seqno);
> + selinux_netlbl_cache_invalidate();
> + selinux_xfrm_notify_policyload();
> +}
I can somewhat understand if you don't want to have all the old policy
cleanup, reset, and notify code in security_load_policy(), but I
really dislike that the mutex lock/unlock is split across the two
functions.
What if selinux_policy_commit() returned oldpolicy on success and we
created a new function, selinux_policy_retire() (name?), that would be
called from security_load_policy and could handle the cleanup, reset,
and notify code. The mutex unlock could happen between the calls to
selinux_policy_commit() and selinux_policy_retire()?
I'm open to other ideas as well.
--
paul moore
www.paul-moore.com
