On 8/6/20 11:41 PM, Paul Moore wrote:
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.
Me either, but I see no alternative other than taking/releasing the
mutex in selinuxfs, at which point it is truly no different than
fsi->mutex.
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.
I think if you look at the 2nd patch, you'll see that this won't work
because security_load_policy() is then changed to return newpolicy to
the caller, and selinuxfs then calls selinux_commit_policy() after
updating selinuxfs. In order to provide the full exclusion guarantee,
we need the mutex held across both security_load_policy() and
selinux_commit_policy(). At that point we'd have to take the mutex
lock/unlock up to selinuxfs and we already have fsi->mutex there. This
load_mutex was just to document the exclusion guarantee at the
security/ss/services.c level and provide something we could pass to
lockdep for safety checking when/if we convert to RCU. If you really
don't like it (and I'm not super excited about it myself), then I think
I'll just put a comment in security_load_policy() explaining why we
don't need to take policy read-lock there and drop load_mutex. Thoughts?
