On Fri, Aug 7, 2020 at 8:20 AM Stephen Smalley
<stephen.smalley.work@xxxxxxxxx> wrote:
> 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?
I think this is a case where the separation of the security server
from the Linux integration code is not doing us any favors.
While not taking a policy lock does seem a bit awkward here, I agree
that given the way the code is structured we are pretty much stuck
with doing the locking in the selinuxfs code ... which it appears is
what you've done in v5. I'm dealing with limited network access at
the moment, and the merge window is still open, so I'll defer a proper
review of v5 until after the merge window closes but if the mutex
removal is the only significant change I don't suspect there will be
any significant comments.
--
paul moore
www.paul-moore.com
