Re: [PATCH v2 4/5] selinux: Use pointer to switch policydb and sidtab

[Stephen Smalley]

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(_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 = >policydb;
> > > - args.newp = newpolicydb;
> > > + args.newp = _rcu->policydb;
> > >   rc = sidtab_map(, convert_context, );
> > >   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(_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(_rwlock);
> > >   write_lock_irq(_rwlock);
> > > - memcpy(_rcu->policydb, newpolicydb, sizeof(struct
> > > policydb));
> > >   sidtab_set(_rcu->sidtab, );
> > >   security_load_policycaps(_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(_rwlock);
> > > + old_rcu = crm;
> > >   crm = next_rcu;
> > > + write_unlock_irq(_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();
> > > - policydb_destroy(newpolicydb);
> > > -
> > > + 
> 
> 

Re: [PATCH v2 4/5] selinux: Use pointer to switch policydb and sidtab

[peter enderborg]

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(_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 = >policydb;
>> -args.newp = newpolicydb;
>> +args.newp = _rcu->policydb;
>>  rc = sidtab_map(, convert_context, );
>>  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(_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.
>> +read_unlock(_rwlock);
>>  write_lock_irq(_rwlock);
>> -memcpy(_rcu->policydb, newpolicydb, sizeof(struct
>> policydb));
>>  sidtab_set(_rcu->sidtab, );
>>  security_load_policycaps(_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(_rwlock);
>> +old_rcu = crm;
>>  crm = next_rcu;
>> +write_unlock_irq(_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();
>> -policydb_destroy(newpolicydb);
>> -
>> +

Re: [PATCH v2 4/5] selinux: Use pointer to switch policydb and sidtab

[Stephen Smalley]

On Fri, 2018-01-26 at 15:32 +0100, peter.enderb...@sony.com wrote:
> From: Peter Enderborg 
> 
> This i preparation for switching to RCU locks. To be able to use
> RCU we need atomic switched pointer. This adds the dynamic
> memory copying to be a single pointer. It copy all the
> data structures in to new ones. This is an overhead
> for writing rules but the benifit is RCU.
> 
> Signed-off-by: Peter Enderborg 
> ---
>  security/selinux/ss/services.c | 139 +++--
> 
>  1 file changed, 78 insertions(+), 61 deletions(-)
> 
> diff --git a/security/selinux/ss/services.c
> b/security/selinux/ss/services.c
> index 2a8486c..81c5717 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -2064,76 +2064,67 @@ static int security_preserve_bools(struct
> policydb *p);
>   */
>  int security_load_policy(void *data, size_t len)
>  {
> - struct policydb *oldpolicydb, *newpolicydb;
> + struct policydb *oldpolicydb;
>   struct sidtab oldsidtab, newsidtab;
>   struct selinux_mapping *oldmap = NULL, *map = NULL;
>   struct convert_context_args args;
> - struct shared_current_mapping *new_mapping;
>   struct shared_current_mapping *next_rcu;
> -
> + struct shared_current_mapping *old_rcu;
>   u32 seqno;
>   u16 map_size;
>   int rc = 0;
>   struct policy_file file = { data, len }, *fp = 
>  
> - oldpolicydb = kzalloc(2 * sizeof(*oldpolicydb), GFP_KERNEL);
> - if (!oldpolicydb) {
> - rc = -ENOMEM;
> - goto out;
> - }
> - new_mapping = kzalloc(sizeof(struct shared_current_mapping),
> -   GFP_KERNEL);
> - if (!new_mapping) {
> - rc = -ENOMEM;
> - goto out;
> - }
> - newpolicydb = oldpolicydb + 1;
> - next_rcu = kmalloc(sizeof(struct shared_current_mapping),
> GFP_KERNEL);
> - if (!next_rcu) {
> - rc = -ENOMEM;
> - goto out;
> - }
> -
>   if (!ss_initialized) {
> - crm = kzalloc(sizeof(struct shared_current_mapping),
> -   GFP_KERNEL);
> - if (!crm) {
> + struct shared_current_mapping *first_mapping;
> +
> + first_mapping = kzalloc(sizeof(struct
> shared_current_mapping),
> + GFP_KERNEL);
> + if (!first_mapping) {
>   rc = -ENOMEM;
>   goto out;
>   }
>   avtab_cache_init();
>   ebitmap_cache_init();
>   hashtab_cache_init();
> - rc = policydb_read(>policydb, fp);
> + rc = policydb_read(_mapping->policydb, fp);
>   if (rc) {
>   avtab_cache_destroy();
>   ebitmap_cache_destroy();
>   hashtab_cache_destroy();
> + kfree(first_mapping);
>   goto out;
>   }
>  
> - crm->policydb.len = len;
> - rc = selinux_set_mapping(>policydb,
> secclass_map,
> -  >current_mapping,
> -  
> >current_mapping_size);
> + first_mapping->policydb.len = len;
> + rc = selinux_set_mapping(_mapping->policydb,
> secclass_map,
> +  _mapping-
> >current_mapping,
> +  _mapping-
> >current_mapping_size);
>   if (rc) {
> - policydb_destroy(>policydb);
> + policydb_destroy(_mapping->policydb);
>   avtab_cache_destroy();
>   ebitmap_cache_destroy();
>   hashtab_cache_destroy();
> + kfree(first_mapping);
>   goto out;
>   }
>  
> - rc = policydb_load_isids(>policydb, 
> >sidtab);
> + rc = policydb_load_isids(_mapping->policydb,
> +  _mapping->sidtab);
>   if (rc) {
> - policydb_destroy(>policydb);
> + policydb_destroy(_mapping->policydb);
>   avtab_cache_destroy();
>   ebitmap_cache_destroy();
>   hashtab_cache_destroy();
> + kfree(first_mapping);
>   goto out;
>   }
>  
> - security_load_policycaps(>policydb);
> + security_load_policycaps(_mapping->policydb);
> + crm = first_mapping;
> +
> + smp_mb(); /* make sure that crm exist before we */
> +   /* switch ss_initialized */
>   ss_initialized = 1;
>   seqno = ++latest_granting;
>   selinux_complete_init();
> @@ -2148,30 +2139,44 @@ int security_load_policy(void *data, size_t
> len)
>  #if 0