On Thu, 2018-02-08 at 08:16 +0100, peter enderborg wrote:
> On 01/30/2018 03:37 PM, Stephen Smalley wrote:
> > On Fri, 2018-01-26 at 15:32 +0100, peter.enderb...@sony.com wrote:
> > goto err;
> >  
> > -   rc = security_preserve_bools(newpolicydb);
> > +   rc = security_preserve_bools(&next_rcu->policydb);
> >     if (rc) {
> >             printk(KERN_ERR "SELinux:  unable to preserve
> > booleans\n");
> >             goto err;
> > Most of this shouldn't need to be under the read lock.
> > 
> > > @@ -2189,7 +2194,7 @@ int security_load_policy(void *data, size_t
> > > len)
> > >    * in the new SID table.
> > >    */
> > >   args.oldp = &crm->policydb;
> > > - args.newp = newpolicydb;
> > > + args.newp = &next_rcu->policydb;
> > >   rc = sidtab_map(&newsidtab, convert_context, &args);
> > >   if (rc) {
> > >           printk(KERN_ERR "SELinux:  unable to convert the
> > > internal"
> > > @@ -2204,8 +2209,9 @@ int security_load_policy(void *data, size_t
> > > len)
> > >  
> > >   /* Install the new policydb and SID table. */
> > >   /* next */
> > > + security_load_policycaps(&next_rcu->policydb);
> > 
> > This cannot be done outside of the write lock; it has to be atomic
> > with
> > the policy switch.
> 
> Can you please elaborate, does some else write the policydb without a
> lock?
> Is there any other data that is shared? I see this as a private until
> we switch the pointer.

security_load_policycaps() updates the selinux_policycap_* variables
from the policydb.  Those variables are used by the hooks to
enable/disable policy-specific functionality, like whether to check
open permission or assign finer-grained security classes to sockets. 
We need to atomically update those variables with the active policy;
otherwise, a hook may perform a permission check that wasn't supposed
to be enabled under the old policy against the old policy (yielding an
unexpected denial).  Everything done under the write lock currently is
there for a reason.

> > > + read_unlock(&policy_rwlock);
> > >   write_lock_irq(&policy_rwlock);
> > > - memcpy(&next_rcu->policydb, newpolicydb, sizeof(struct
> > > policydb));
> > >   sidtab_set(&next_rcu->sidtab, &newsidtab);
> > >   security_load_policycaps(&next_rcu->policydb);
> > >   oldmap = crm->current_mapping;
> > > @@ -2213,8 +2219,9 @@ int security_load_policy(void *data, size_t
> > > len)
> > >   next_rcu->current_mapping_size = map_size;
> > >  
> > >   seqno = ++latest_granting;
> > > - write_unlock_irq(&policy_rwlock);
> > > + old_rcu = crm;
> > >   crm = next_rcu;
> > > + write_unlock_irq(&policy_rwlock);
> > >  
> > >   /* Free the old policydb and SID table. */
> > >   policydb_destroy(oldpolicydb);
> > > @@ -2226,17 +2233,16 @@ int security_load_policy(void *data,
> > > size_t
> > > len)
> > >   selinux_status_update_policyload(seqno);
> > >   selinux_netlbl_cache_invalidate();
> > >   selinux_xfrm_notify_policyload();
> > > + kfree(oldpolicydb);
> > > + kfree(old_rcu);
> > >  
> > >   rc = 0;
> > >   goto out;
> > > -
> > >  err:
> > >   kfree(map);
> > >   sidtab_destroy(&newsidtab);
> > > - policydb_destroy(newpolicydb);
> > > -
> > > + 
> 
> 

Reply via email to