Re: [RFC PATCH] selinux: encapsulate policy state, refactor policy load

[Paul Moore <paul@paul-moore.com>]

On Thu, Jul 30, 2020 at 6:33 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
> On 7/30/20 6:09 PM, Stephen Smalley 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.
> >
> > 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@gmail.com>
> > ---
>
> > @@ -2098,10 +2104,12 @@ static int convert_context(struct context *oldc, struct context *newc, void *p)
> >
> >   static void security_load_policycaps(struct selinux_state *state)
> >   {
> > -     struct policydb *p = &state->ss->policydb;
> > +     struct policydb *p = &state->ss->policy->policydb;
> >       unsigned int i;
> >       struct ebitmap_node *node;
> >
> > +     read_lock(&state->ss->policy_rwlock);
> > +
>
> Oops, should have moved the dereferencing of policy after taking the
> read-lock here; fixed it everywhere else I think but missed this one.
> Will fix in the next version but will wait for other comments on this
> version.

While I haven't looked at this patch in detail, I'm generally in favor
of cleanups and the encapsulation work has generally been a good thing
in my opinion.  It also should go without saying that fixing, or
improving, the atomic load issue would be a very good thing.

> > @@ -2132,112 +2200,58 @@ static int security_preserve_bools(struct selinux_state *state,
> >    */
> >   int security_load_policy(struct selinux_state *state, void *data, size_t len)
> >   {
> <snip>
> >       /*
> >        * Convert the internal representations of contexts
> >        * in the new SID table.
> >        */
> >       args.state = state;
> > -     args.oldp = policydb;
> > -     args.newp = newpolicydb;
> > +     args.oldp = &state->ss->policy->policydb;
> > +     args.newp = &newpolicy->policydb;
> >
> >       convert_params.func = convert_context;
> >       convert_params.args = &args;
> > -     convert_params.target = newsidtab;
> > +     convert_params.target = &newpolicy->sidtab;
> >
> > -     rc = sidtab_convert(oldsidtab, &convert_params);
> > +     rc = sidtab_convert(&state->ss->policy->sidtab, &convert_params);
>
> Should sidtab_convert() be called while holding policy read-lock since
> we are passing state->ss->policy->policydb via the args field of
> convert_params and using it within the callback?  I think it happens to
> be safe currently by virtue of the fact that nothing else can write to
> it since selinuxfs is holding its semaphore during the entire policy
> reload but it seems inconsistent. However, if we do take policy
> read-lock across the call, then sidtab_convert() needs to use GFP_ATOMIC
> allocations instead of GFP_KERNEL.

Without looking too closely at the call chains, or the allocation
sizes, I would vote for taking the lock in the name of consistency.

-- 
paul moore
www.paul-moore.com