On Thu, Jul 30, 2020 at 6:33 PM Stephen Smalley
<stephen.smalley.work@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
>
> > @@ -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
