Re: [PATCH v3] selinux: simplify mls_context_to_sid()

2018-11-13 Thread Paul Moore 
On Tue, Nov 13, 2018 at 4:10 PM Stephen Smalley  wrote:
> On 11/12/18 6:44 AM, Ondrej Mosnacek wrote:
> > This function has only two callers, but only one of them actually needs
> > the special logic at the beginning. Factoring this logic out into
> > string_to_context_struct() allows us to drop the arguments 'oldc', 's',
> > and 'def_sid'.
> >
> > Signed-off-by: Ondrej Mosnacek 
> > ---
> >
> > Changes in v3:
> >   - correct the comment about policy read lock
> >
> > Changes in v2:
> >   - also drop unneeded #include's from mls.c
> >
> >   security/selinux/ss/mls.c  | 49 +-
> >   security/selinux/ss/mls.h  |  5 +---
> >   security/selinux/ss/services.c | 32 +++---
> >   3 files changed, 36 insertions(+), 50 deletions(-)
> >
> > diff --git a/security/selinux/ss/mls.c b/security/selinux/ss/mls.c
> > index 2fe459df3c85..d1da928a7e77 100644
> > --- a/security/selinux/ss/mls.c
> > +++ b/security/selinux/ss/mls.c
> > @@ -24,10 +24,7 @@
> >   #include 
> >   #include 
> >   #include 
> > -#include "sidtab.h"
> >   #include "mls.h"
> > -#include "policydb.h"
> > -#include "services.h"
> >
> >   /*
> >* Return the length in bytes for the MLS fields of the
> > @@ -223,20 +220,12 @@ int mls_context_isvalid(struct policydb *p, struct 
> > context *c)
> >* This function modifies the string in place, inserting
> >* NULL characters to terminate the MLS fields.
> >*
> > - * If a def_sid is provided and no MLS field is present,
> > - * copy the MLS field of the associated default context.
> > - * Used for upgraded to MLS systems where objects may lack
> > - * MLS fields.
> > - *
> > - * Policy read-lock must be held for sidtab lookup.
> > + * Policy read-lock must be held for policy data lookup.
> >*
> >*/
> >   int mls_context_to_sid(struct policydb *pol,
> > -char oldc,
> >  char *scontext,
> > -struct context *context,
> > -struct sidtab *s,
> > -u32 def_sid)
> > +struct context *context)
> >   {
> >   char *sensitivity, *cur_cat, *next_cat, *rngptr;
> >   struct level_datum *levdatum;
> > @@ -244,29 +233,6 @@ int mls_context_to_sid(struct policydb *pol,
> >   int l, rc, i;
> >   char *rangep[2];
> >
> > - if (!pol->mls_enabled) {
> > - if ((def_sid != SECSID_NULL && oldc) || (*scontext) == '\0')
> > - return 0;
> > - return -EINVAL;
> > - }
> > -
> > - /*
> > -  * No MLS component to the security context, try and map to
> > -  * default if provided.
> > -  */
> > - if (!oldc) {
> > - struct context *defcon;
> > -
> > - if (def_sid == SECSID_NULL)
> > - return -EINVAL;
> > -
> > - defcon = sidtab_search(s, def_sid);
> > - if (!defcon)
> > - return -EINVAL;
> > -
> > - return mls_context_cpy(context, defcon);
> > - }
> > -
> >   /*
> >* If we're dealing with a range, figure out where the two parts
> >* of the range begin.
> > @@ -364,14 +330,11 @@ int mls_from_string(struct policydb *p, char *str, 
> > struct context *context,
> >   return -EINVAL;
> >
> >   tmpstr = kstrdup(str, gfp_mask);
> > - if (!tmpstr) {
> > - rc = -ENOMEM;
> > - } else {
> > - rc = mls_context_to_sid(p, ':', tmpstr, context,
> > - NULL, SECSID_NULL);
> > - kfree(tmpstr);
> > - }
> > + if (!tmpstr)
> > + return -ENOMEM;
> >
> > + rc = mls_context_to_sid(p, tmpstr, context);
> > + kfree(tmpstr);
> >   return rc;
> >   }
> >
> > diff --git a/security/selinux/ss/mls.h b/security/selinux/ss/mls.h
> > index 67093647576d..e2498f78e100 100644
> > --- a/security/selinux/ss/mls.h
> > +++ b/security/selinux/ss/mls.h
> > @@ -33,11 +33,8 @@ int mls_range_isvalid(struct policydb *p, struct 
> > mls_range *r);
> >   int mls_level_isvalid(struct policydb *p, struct mls_level *l);
> >
> >   int mls_context_to_sid(struct policydb *p,
> > -char oldc,
> >  char *scontext,
> > -struct context *context,
> > -struct sidtab *s,
> > -u32 def_sid);
> > +struct context *context);
> >
> >   int mls_from_string(struct policydb *p, char *str, struct context 
> > *context,
> >   gfp_t gfp_mask);
> > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> > index 12e414394530..ccad4334f99d 100644
> > --- a/security/selinux/ss/services.c
> > +++ b/security/selinux/ss/services.c
> > @@ -1425,9 +1425,35 @@ static int string_to_context_struct(struct policydb 
> > *pol,
> >
> >   ctx->type = typdatum->value;
> >
> > - rc = mls_context_to_sid(pol, oldc, p, ctx, sidtabp, def_sid);
> > -

Re: [RFC PATCH 2/3] selinux: use separate table for initial SID lookup

2018-11-13 Thread Stephen Smalley 

On 11/13/18 8:52 AM, Ondrej Mosnacek wrote:

This patch is non-functional and moves handling of initial SIDs into a
separate table. Note that the SIDs stored in the main table are now
shifted by SECINITSID_NUM and converted to/from the actual SIDs
transparently by helper functions.


When you say "non-functional", you mean it doesn't make any functional 
changes, right?  As opposed to leaving the kernel in a broken 
intermediate state until your 3rd patch?


I think you need to double check that all references to 
state->ss->sidtab are preceded by a check of state->initialized since it 
could be NULL before first policy load and a system could come up with 
SELinux enabled but never load a policy.


Aren't you still wasting a slot in the initial SIDs array, or did I miss 
something?




This change doesn't make much sense on its own, but it simplifies
further sidtab overhaul in a succeeding patch.

Signed-off-by: Ondrej Mosnacek 
---
  security/selinux/ss/policydb.c |  10 ++-
  security/selinux/ss/services.c |  88 ++
  security/selinux/ss/services.h |   2 +-
  security/selinux/ss/sidtab.c   | 158 +++--
  security/selinux/ss/sidtab.h   |  14 +--
  5 files changed, 162 insertions(+), 110 deletions(-)

diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index f4eadd3f7350..21e4bdbcf994 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -909,13 +909,21 @@ int policydb_load_isids(struct policydb *p, struct sidtab 
*s)
if (!c->context[0].user) {
pr_err("SELinux:  SID %s was never defined.\n",
c->u.name);
+   sidtab_destroy(s);
+   goto out;
+   }
+   if (c->sid[0] > SECINITSID_NUM) {
+   pr_err("SELinux:  Initial SID %s out of range.\n",
+   c->u.name);
+   sidtab_destroy(s);
goto out;
}
  
-		rc = sidtab_insert(s, c->sid[0], >context[0]);

+   rc = sidtab_set_initial(s, c->sid[0], >context[0]);
if (rc) {
pr_err("SELinux:  unable to load initial SID %s.\n",
c->u.name);
+   sidtab_destroy(s);
goto out;
}
}
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 7337db24a6a8..30170d4c567a 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -776,7 +776,7 @@ static int security_compute_validatetrans(struct 
selinux_state *state,
read_lock(>ss->policy_rwlock);
  
  	policydb = >ss->policydb;

-   sidtab = >ss->sidtab;
+   sidtab = state->ss->sidtab;
  
  	if (!user)

tclass = unmap_class(>ss->map, orig_tclass);
@@ -876,7 +876,7 @@ int security_bounded_transition(struct selinux_state *state,
read_lock(>ss->policy_rwlock);
  
  	policydb = >ss->policydb;

-   sidtab = >ss->sidtab;
+   sidtab = state->ss->sidtab;
  
  	rc = -EINVAL;

old_context = sidtab_search(sidtab, old_sid);
@@ -1034,7 +1034,7 @@ void security_compute_xperms_decision(struct 
selinux_state *state,
goto allow;
  
  	policydb = >ss->policydb;

-   sidtab = >ss->sidtab;
+   sidtab = state->ss->sidtab;
  
  	scontext = sidtab_search(sidtab, ssid);

if (!scontext) {
@@ -1123,7 +1123,7 @@ void security_compute_av(struct selinux_state *state,
goto allow;
  
  	policydb = >ss->policydb;

-   sidtab = >ss->sidtab;
+   sidtab = state->ss->sidtab;
  
  	scontext = sidtab_search(sidtab, ssid);

if (!scontext) {
@@ -1177,7 +1177,7 @@ void security_compute_av_user(struct selinux_state *state,
goto allow;
  
  	policydb = >ss->policydb;

-   sidtab = >ss->sidtab;
+   sidtab = state->ss->sidtab;
  
  	scontext = sidtab_search(sidtab, ssid);

if (!scontext) {
@@ -1315,7 +1315,7 @@ static int security_sid_to_context_core(struct 
selinux_state *state,
}
read_lock(>ss->policy_rwlock);
policydb = >ss->policydb;
-   sidtab = >ss->sidtab;
+   sidtab = state->ss->sidtab;
if (force)
context = sidtab_search_force(sidtab, sid);
else
@@ -1483,7 +1483,7 @@ static int security_context_to_sid_core(struct 
selinux_state *state,
}
read_lock(>ss->policy_rwlock);
policydb = >ss->policydb;
-   sidtab = >ss->sidtab;
+   sidtab = state->ss->sidtab;
rc = string_to_context_struct(policydb, sidtab, scontext2,
  , def_sid);
if (rc == -EINVAL && force) {
@@ -1668,7 +1668,7 @@ static int security_compute_sid(struct selinux_state 
*state,
}
  
  	policydb = >ss->policydb;

-   sidtab = >ss->sidtab;
+   sidtab = state->ss->sidtab;

Re: [RFC PATCH 1/3] selinux: refactor sidtab conversion

2018-11-13 Thread Stephen Smalley 

On 11/13/18 8:52 AM, Ondrej Mosnacek wrote:

This is a purely cosmetic change that encapsulates the three-step sidtab
conversion logic (shutdown -> clone -> map) into a single function
defined in sidtab.c (as opposed to services.c).

Signed-off-by: Ondrej Mosnacek 


Acked-by: Stephen Smalley 


---
  security/selinux/ss/services.c | 22 +--
  security/selinux/ss/sidtab.c   | 50 --
  security/selinux/ss/sidtab.h   | 11 
  3 files changed, 42 insertions(+), 41 deletions(-)

diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 12e414394530..7337db24a6a8 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -1880,19 +1880,6 @@ int security_change_sid(struct selinux_state *state,
out_sid, false);
  }
  
-/* Clone the SID into the new SID table. */

-static int clone_sid(u32 sid,
-struct context *context,
-void *arg)
-{
-   struct sidtab *s = arg;
-
-   if (sid > SECINITSID_NUM)
-   return sidtab_insert(s, sid, context);
-   else
-   return 0;
-}
-
  static inline int convert_context_handle_invalid_context(
struct selinux_state *state,
struct context *context)
@@ -2186,13 +2173,6 @@ int security_load_policy(struct selinux_state *state, 
void *data, size_t len)
goto err;
}
  
-	/* Clone the SID table. */

-   sidtab_shutdown(sidtab);
-
-   rc = sidtab_map(sidtab, clone_sid, );
-   if (rc)
-   goto err;
-
/*
 * Convert the internal representations of contexts
 * in the new SID table.
@@ -2200,7 +2180,7 @@ int security_load_policy(struct selinux_state *state, 
void *data, size_t len)
args.state = state;
args.oldp = policydb;
args.newp = newpolicydb;
-   rc = sidtab_map(, convert_context, );
+   rc = sidtab_convert(sidtab, , convert_context, );
if (rc) {
pr_err("SELinux:  unable to convert the internal"
" representation of contexts in the new SID"
diff --git a/security/selinux/ss/sidtab.c b/security/selinux/ss/sidtab.c
index fd75a12fa8fc..e66a2ab3d1c2 100644
--- a/security/selinux/ss/sidtab.c
+++ b/security/selinux/ss/sidtab.c
@@ -116,11 +116,11 @@ struct context *sidtab_search_force(struct sidtab *s, u32 
sid)
return sidtab_search_core(s, sid, 1);
  }
  
-int sidtab_map(struct sidtab *s,

-  int (*apply) (u32 sid,
-struct context *context,
-void *args),
-  void *args)
+static int sidtab_map(struct sidtab *s,
+ int (*apply) (u32 sid,
+   struct context *context,
+   void *args),
+ void *args)
  {
int i, rc = 0;
struct sidtab_node *cur;
@@ -141,6 +141,37 @@ out:
return rc;
  }
  
+/* Clone the SID into the new SID table. */

+static int clone_sid(u32 sid, struct context *context, void *arg)
+{
+   struct sidtab *s = arg;
+
+   if (sid > SECINITSID_NUM)
+   return sidtab_insert(s, sid, context);
+   else
+   return 0;
+}
+
+int sidtab_convert(struct sidtab *s, struct sidtab *news,
+  int (*convert) (u32 sid,
+  struct context *context,
+  void *args),
+  void *args)
+{
+   unsigned long flags;
+   int rc;
+
+   spin_lock_irqsave(>lock, flags);
+   s->shutdown = 1;
+   spin_unlock_irqrestore(>lock, flags);
+
+   rc = sidtab_map(s, clone_sid, news);
+   if (rc)
+   return rc;
+
+   return sidtab_map(news, convert, args);
+}
+
  static void sidtab_update_cache(struct sidtab *s, struct sidtab_node *n, int 
loc)
  {
BUG_ON(loc >= SIDTAB_CACHE_LEN);
@@ -295,12 +326,3 @@ void sidtab_set(struct sidtab *dst, struct sidtab *src)
dst->cache[i] = NULL;
spin_unlock_irqrestore(>lock, flags);
  }
-
-void sidtab_shutdown(struct sidtab *s)
-{
-   unsigned long flags;
-
-   spin_lock_irqsave(>lock, flags);
-   s->shutdown = 1;
-   spin_unlock_irqrestore(>lock, flags);
-}
diff --git a/security/selinux/ss/sidtab.h b/security/selinux/ss/sidtab.h
index a1a1d2617b6f..26c74fe7afc0 100644
--- a/security/selinux/ss/sidtab.h
+++ b/security/selinux/ss/sidtab.h
@@ -37,11 +37,11 @@ int sidtab_insert(struct sidtab *s, u32 sid, struct context 
*context);
  struct context *sidtab_search(struct sidtab *s, u32 sid);
  struct context *sidtab_search_force(struct sidtab *s, u32 sid);
  
-int sidtab_map(struct sidtab *s,

-  int (*apply) (u32 sid,
-struct context *context,
-void *args),
-  void *args);
+int sidtab_convert(struct sidtab *s, struct sidtab *news,

Re: [PATCH v3] selinux: simplify mls_context_to_sid()

2018-11-13 Thread Stephen Smalley 

On 11/12/18 6:44 AM, Ondrej Mosnacek wrote:

This function has only two callers, but only one of them actually needs
the special logic at the beginning. Factoring this logic out into
string_to_context_struct() allows us to drop the arguments 'oldc', 's',
and 'def_sid'.

Signed-off-by: Ondrej Mosnacek 
---

Changes in v3:
  - correct the comment about policy read lock

Changes in v2:
  - also drop unneeded #include's from mls.c

  security/selinux/ss/mls.c  | 49 +-
  security/selinux/ss/mls.h  |  5 +---
  security/selinux/ss/services.c | 32 +++---
  3 files changed, 36 insertions(+), 50 deletions(-)

diff --git a/security/selinux/ss/mls.c b/security/selinux/ss/mls.c
index 2fe459df3c85..d1da928a7e77 100644
--- a/security/selinux/ss/mls.c
+++ b/security/selinux/ss/mls.c
@@ -24,10 +24,7 @@
  #include 
  #include 
  #include 
-#include "sidtab.h"
  #include "mls.h"
-#include "policydb.h"
-#include "services.h"
  
  /*

   * Return the length in bytes for the MLS fields of the
@@ -223,20 +220,12 @@ int mls_context_isvalid(struct policydb *p, struct 
context *c)
   * This function modifies the string in place, inserting
   * NULL characters to terminate the MLS fields.
   *
- * If a def_sid is provided and no MLS field is present,
- * copy the MLS field of the associated default context.
- * Used for upgraded to MLS systems where objects may lack
- * MLS fields.
- *
- * Policy read-lock must be held for sidtab lookup.
+ * Policy read-lock must be held for policy data lookup.
   *
   */
  int mls_context_to_sid(struct policydb *pol,
-  char oldc,
   char *scontext,
-  struct context *context,
-  struct sidtab *s,
-  u32 def_sid)
+  struct context *context)
  {
char *sensitivity, *cur_cat, *next_cat, *rngptr;
struct level_datum *levdatum;
@@ -244,29 +233,6 @@ int mls_context_to_sid(struct policydb *pol,
int l, rc, i;
char *rangep[2];
  
-	if (!pol->mls_enabled) {

-   if ((def_sid != SECSID_NULL && oldc) || (*scontext) == '\0')
-   return 0;
-   return -EINVAL;
-   }
-
-   /*
-* No MLS component to the security context, try and map to
-* default if provided.
-*/
-   if (!oldc) {
-   struct context *defcon;
-
-   if (def_sid == SECSID_NULL)
-   return -EINVAL;
-
-   defcon = sidtab_search(s, def_sid);
-   if (!defcon)
-   return -EINVAL;
-
-   return mls_context_cpy(context, defcon);
-   }
-
/*
 * If we're dealing with a range, figure out where the two parts
 * of the range begin.
@@ -364,14 +330,11 @@ int mls_from_string(struct policydb *p, char *str, struct 
context *context,
return -EINVAL;
  
  	tmpstr = kstrdup(str, gfp_mask);

-   if (!tmpstr) {
-   rc = -ENOMEM;
-   } else {
-   rc = mls_context_to_sid(p, ':', tmpstr, context,
-   NULL, SECSID_NULL);
-   kfree(tmpstr);
-   }
+   if (!tmpstr)
+   return -ENOMEM;
  
+	rc = mls_context_to_sid(p, tmpstr, context);

+   kfree(tmpstr);
return rc;
  }
  
diff --git a/security/selinux/ss/mls.h b/security/selinux/ss/mls.h

index 67093647576d..e2498f78e100 100644
--- a/security/selinux/ss/mls.h
+++ b/security/selinux/ss/mls.h
@@ -33,11 +33,8 @@ int mls_range_isvalid(struct policydb *p, struct mls_range 
*r);
  int mls_level_isvalid(struct policydb *p, struct mls_level *l);
  
  int mls_context_to_sid(struct policydb *p,

-  char oldc,
   char *scontext,
-  struct context *context,
-  struct sidtab *s,
-  u32 def_sid);
+  struct context *context);
  
  int mls_from_string(struct policydb *p, char *str, struct context *context,

gfp_t gfp_mask);
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 12e414394530..ccad4334f99d 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -1425,9 +1425,35 @@ static int string_to_context_struct(struct policydb *pol,
  
  	ctx->type = typdatum->value;
  
-	rc = mls_context_to_sid(pol, oldc, p, ctx, sidtabp, def_sid);

-   if (rc)
-   goto out;
+   if (!pol->mls_enabled) {
+   rc = -EINVAL;
+   if ((def_sid == SECSID_NULL || !oldc) && (*p) != '\0')
+   goto out;


I don't think this is your bug, but unless I'm mistaken, p could be OOB 
and be dereferenced here.  Seems to have been introduced by 
95ffe194204ae3cef88d0b59be209204bbe9b3be.  Likely not caught in testing 
since both Linux distributions and Android enable MLS to use the 
category

[RFC PATCH 2/3] selinux: use separate table for initial SID lookup

2018-11-13 Thread Ondrej Mosnacek 
This patch is non-functional and moves handling of initial SIDs into a
separate table. Note that the SIDs stored in the main table are now
shifted by SECINITSID_NUM and converted to/from the actual SIDs
transparently by helper functions.

This change doesn't make much sense on its own, but it simplifies
further sidtab overhaul in a succeeding patch.

Signed-off-by: Ondrej Mosnacek 
---
 security/selinux/ss/policydb.c |  10 ++-
 security/selinux/ss/services.c |  88 ++
 security/selinux/ss/services.h |   2 +-
 security/selinux/ss/sidtab.c   | 158 +++--
 security/selinux/ss/sidtab.h   |  14 +--
 5 files changed, 162 insertions(+), 110 deletions(-)

diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index f4eadd3f7350..21e4bdbcf994 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -909,13 +909,21 @@ int policydb_load_isids(struct policydb *p, struct sidtab 
*s)
if (!c->context[0].user) {
pr_err("SELinux:  SID %s was never defined.\n",
c->u.name);
+   sidtab_destroy(s);
+   goto out;
+   }
+   if (c->sid[0] > SECINITSID_NUM) {
+   pr_err("SELinux:  Initial SID %s out of range.\n",
+   c->u.name);
+   sidtab_destroy(s);
goto out;
}
 
-   rc = sidtab_insert(s, c->sid[0], >context[0]);
+   rc = sidtab_set_initial(s, c->sid[0], >context[0]);
if (rc) {
pr_err("SELinux:  unable to load initial SID %s.\n",
c->u.name);
+   sidtab_destroy(s);
goto out;
}
}
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 7337db24a6a8..30170d4c567a 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -776,7 +776,7 @@ static int security_compute_validatetrans(struct 
selinux_state *state,
read_lock(>ss->policy_rwlock);
 
policydb = >ss->policydb;
-   sidtab = >ss->sidtab;
+   sidtab = state->ss->sidtab;
 
if (!user)
tclass = unmap_class(>ss->map, orig_tclass);
@@ -876,7 +876,7 @@ int security_bounded_transition(struct selinux_state *state,
read_lock(>ss->policy_rwlock);
 
policydb = >ss->policydb;
-   sidtab = >ss->sidtab;
+   sidtab = state->ss->sidtab;
 
rc = -EINVAL;
old_context = sidtab_search(sidtab, old_sid);
@@ -1034,7 +1034,7 @@ void security_compute_xperms_decision(struct 
selinux_state *state,
goto allow;
 
policydb = >ss->policydb;
-   sidtab = >ss->sidtab;
+   sidtab = state->ss->sidtab;
 
scontext = sidtab_search(sidtab, ssid);
if (!scontext) {
@@ -1123,7 +1123,7 @@ void security_compute_av(struct selinux_state *state,
goto allow;
 
policydb = >ss->policydb;
-   sidtab = >ss->sidtab;
+   sidtab = state->ss->sidtab;
 
scontext = sidtab_search(sidtab, ssid);
if (!scontext) {
@@ -1177,7 +1177,7 @@ void security_compute_av_user(struct selinux_state *state,
goto allow;
 
policydb = >ss->policydb;
-   sidtab = >ss->sidtab;
+   sidtab = state->ss->sidtab;
 
scontext = sidtab_search(sidtab, ssid);
if (!scontext) {
@@ -1315,7 +1315,7 @@ static int security_sid_to_context_core(struct 
selinux_state *state,
}
read_lock(>ss->policy_rwlock);
policydb = >ss->policydb;
-   sidtab = >ss->sidtab;
+   sidtab = state->ss->sidtab;
if (force)
context = sidtab_search_force(sidtab, sid);
else
@@ -1483,7 +1483,7 @@ static int security_context_to_sid_core(struct 
selinux_state *state,
}
read_lock(>ss->policy_rwlock);
policydb = >ss->policydb;
-   sidtab = >ss->sidtab;
+   sidtab = state->ss->sidtab;
rc = string_to_context_struct(policydb, sidtab, scontext2,
  , def_sid);
if (rc == -EINVAL && force) {
@@ -1668,7 +1668,7 @@ static int security_compute_sid(struct selinux_state 
*state,
}
 
policydb = >ss->policydb;
-   sidtab = >ss->sidtab;
+   sidtab = state->ss->sidtab;
 
scontext = sidtab_search(sidtab, ssid);
if (!scontext) {
@@ -1925,10 +1925,7 @@ static int convert_context(u32 key,
struct user_datum *usrdatum;
char *s;
u32 len;
-   int rc = 0;
-
-   if (key <= SECINITSID_NUM)
-   goto out;
+   int rc;
 
args = p;
 
@@ -2090,9 +2087,8 @@ static int security_preserve_bools(struct selinux_state 
*state,
 int security_load_policy(struct selinux_state *state, void *data, size_t len)
 {
struct policydb

[RFC PATCH 3/3] selinux: overhaul sidtab to fix bug and improve performance

2018-11-13 Thread Ondrej Mosnacek 
Before this patch, during a policy reload the sidtab would become frozen
and trying to map a new context to SID would be unable to add a new
entry to sidtab and fail with -ENOMEM.

Such failures are usually propagated into userspace, which has no way of
distignuishing them from actual allocation failures and thus doesn't
handle them gracefully. Such situation can be triggered e.g. by the
following reproducer:

while true; do load_policy; echo -n .; sleep 0.1; done &
for (( i = 0; i < 1024; i++ )); do
runcon -l s0:c$i echo -n x || break
# or:
# chcon -l s0:c$i  || break
done

This patch overhauls the sidtab so it doesn't need to be frozen during
policy reload, thus solving the above problem.

The new SID table leverages the fact that SIDs are allocated
sequentially and are never invalidated and stores them in linear buckets
indexed by a tree structure. This brings several advantages:
  1. Fast SID -> context lookup - this lookup can now be done in
 logarithmic time complexity (usually in less than 4 array lookups)
 and can still be done safely without locking.
  2. No need to re-search the whole table on reverse lookup miss - after
 acquiring the spinlock only the newly added entries need to be
 searched, which means that reverse lookups that end up inserting a
 new entry are now about twice as fast.
  3. No need to freeze sidtab during policy reload - it is now possible
 to handle insertion of new entries even during sidtab conversion.

The tree structure of the new sidtab is able to grow automatically to up
to about 2^31 entries (at which point it should not have more than about
4 tree levels). The old sidtab had a theoretical capacity of almost 2^32
entries, but half of that is still more than enough since by that point
the reverse table lookups would become unusably slow anyway...

The number of entries per tree node is selected automatically so that
each node fits into a single page, which should be the easiest size for
kmalloc() to handle.

Note that currently the new implementation does not have a cache for
recent reverse lookups as the old one had. Since repeated lookups are
quite likely with sidtab, it is an important optimization and I will try
to add it before submitting a final version.

Tested by selinux-testsuite and thoroughly tortured by this simple
stress test:
```
function rand_cat() {
echo $(( $RANDOM % 1024 ))
}

function do_work() {
while true; do
echo -n 
"system_u:system_r:kernel_t:s0:c$(rand_cat),c$(rand_cat)" \
>/sys/fs/selinux/context 2>/dev/null || true
done
}

do_work >/dev/null &
do_work >/dev/null &
do_work >/dev/null &

while load_policy; do echo -n .; sleep 0.1; done

kill %1
kill %2
kill %3
```

Reported-by: Orion Poplawski 
Reported-by: Li Kun 
Link: https://github.com/SELinuxProject/selinux-kernel/issues/38
Signed-off-by: Ondrej Mosnacek 
---
 security/selinux/ss/mls.c  |  23 +-
 security/selinux/ss/mls.h  |   3 +-
 security/selinux/ss/services.c |  90 +++---
 security/selinux/ss/sidtab.c   | 514 +++--
 security/selinux/ss/sidtab.h   |  75 +++--
 5 files changed, 391 insertions(+), 314 deletions(-)

diff --git a/security/selinux/ss/mls.c b/security/selinux/ss/mls.c
index 2fe459df3c85..267ae4f9be79 100644
--- a/security/selinux/ss/mls.c
+++ b/security/selinux/ss/mls.c
@@ -436,16 +436,17 @@ int mls_setup_user_range(struct policydb *p,
 
 /*
  * Convert the MLS fields in the security context
- * structure `c' from the values specified in the
- * policy `oldp' to the values specified in the policy `newp'.
+ * structure `oldc' from the values specified in the
+ * policy `oldp' to the values specified in the policy `newp',
+ * storing the resulting context in `newc'.
  */
 int mls_convert_context(struct policydb *oldp,
struct policydb *newp,
-   struct context *c)
+   struct context *oldc,
+   struct context *newc)
 {
struct level_datum *levdatum;
struct cat_datum *catdatum;
-   struct ebitmap bitmap;
struct ebitmap_node *node;
int l, i;
 
@@ -455,28 +456,24 @@ int mls_convert_context(struct policydb *oldp,
for (l = 0; l < 2; l++) {
levdatum = hashtab_search(newp->p_levels.table,
  sym_name(oldp, SYM_LEVELS,
-  c->range.level[l].sens - 1));
+  oldc->range.level[l].sens - 
1));
 
if (!levdatum)
return -EINVAL;
-   c->range.level[l].sens = levdatum->level->sens;
+   newc->range.level[l].sens = levdatum->level->sens;
 
-   ebitmap_init();
-   ebitmap_for_each_positive_bit(>range.level[l].cat, node, i) {
+

[RFC PATCH 0/3] Fix ENOMEM errors during policy reload

2018-11-13 Thread Ondrej Mosnacek 
This patchset is an alternative, hopefully better (but also more risky),
solution of the ENOMEM problem ([1]) that I first tried to solve in [2].

In this version I encapsulate the initial SID table within sidtab and
also switch back from converting the sidtab in-place to converting into
a new sidtab and then just switching the pointer (keeping the code ready
for switching to RCU locks).

The change is split into three patches for easier review. Some changes
done in the first two patches are effectively undone by the last patch,
so it might actually make more sense to send the final version as one
squashed patch (please let me know which is better for you).

The first patch moves the sidtab conversion logic into sidtab.c. This
allows hiding sidtab_insert() from sidtab.h in the second patch, where
it becomes an internal function.

The second patch separates the handling of initial SIDs into a separate
lookup table inside sidtab. After this change, the main table always
contains N entries with keys from 0 to (N-1). This property is then
leveraged in the last patch.

Finally, the third patch rewrites the main sidtab to a more efficient
implementation that also gracefully handles context conversions during
policy reloads, which no longer produces the ENOMEM errors.

After applying this patchset, the time it takes to insert new sidtab
entries is drastically reduced. I measured the time to populate the
table with N new entries by repeatedly writing to
/sys/fs/selinux/context. A graph of the results is available at [3].

The SID -> context lookups are now also faster. With the old
implementation, these are O(N) once N goes above 128. The new
implementation can handle them theoretically in O(log N), but in
practice the slope is almost flat, so they are practically
almost constant-time.

Review and feedback welcome.

[1] https://github.com/SELinuxProject/selinux-kernel/issues/38
[2] https://lore.kernel.org/selinux/20181031122718.18735-1-omosn...@redhat.com/
[3] 
https://docs.google.com/spreadsheets/d/e/2PACX-1vRUArNJR6kckm2SEs4dRZlijNVdCTmsNuWRGe7X3fC01YkBHpxXHnmcssxEiMF3Z7ivtXN2L5MC0ry-/pubhtml

Ondrej Mosnacek (3):
  selinux: refactor sidtab conversion
  selinux: use separate table for initial SID lookup
  selinux: overhaul sidtab to fix bug and improve performance

 security/selinux/ss/mls.c  |  23 +-
 security/selinux/ss/mls.h  |   3 +-
 security/selinux/ss/policydb.c |  10 +-
 security/selinux/ss/services.c | 188 +--
 security/selinux/ss/services.h |   2 +-
 security/selinux/ss/sidtab.c   | 550 -
 security/selinux/ss/sidtab.h   |  90 --
 7 files changed, 498 insertions(+), 368 deletions(-)

-- 
2.17.2

___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.

[RFC PATCH 1/3] selinux: refactor sidtab conversion

2018-11-13 Thread Ondrej Mosnacek 
This is a purely cosmetic change that encapsulates the three-step sidtab
conversion logic (shutdown -> clone -> map) into a single function
defined in sidtab.c (as opposed to services.c).

Signed-off-by: Ondrej Mosnacek 
---
 security/selinux/ss/services.c | 22 +--
 security/selinux/ss/sidtab.c   | 50 --
 security/selinux/ss/sidtab.h   | 11 
 3 files changed, 42 insertions(+), 41 deletions(-)

diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 12e414394530..7337db24a6a8 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -1880,19 +1880,6 @@ int security_change_sid(struct selinux_state *state,
out_sid, false);
 }
 
-/* Clone the SID into the new SID table. */
-static int clone_sid(u32 sid,
-struct context *context,
-void *arg)
-{
-   struct sidtab *s = arg;
-
-   if (sid > SECINITSID_NUM)
-   return sidtab_insert(s, sid, context);
-   else
-   return 0;
-}
-
 static inline int convert_context_handle_invalid_context(
struct selinux_state *state,
struct context *context)
@@ -2186,13 +2173,6 @@ int security_load_policy(struct selinux_state *state, 
void *data, size_t len)
goto err;
}
 
-   /* Clone the SID table. */
-   sidtab_shutdown(sidtab);
-
-   rc = sidtab_map(sidtab, clone_sid, );
-   if (rc)
-   goto err;
-
/*
 * Convert the internal representations of contexts
 * in the new SID table.
@@ -2200,7 +2180,7 @@ int security_load_policy(struct selinux_state *state, 
void *data, size_t len)
args.state = state;
args.oldp = policydb;
args.newp = newpolicydb;
-   rc = sidtab_map(, convert_context, );
+   rc = sidtab_convert(sidtab, , convert_context, );
if (rc) {
pr_err("SELinux:  unable to convert the internal"
" representation of contexts in the new SID"
diff --git a/security/selinux/ss/sidtab.c b/security/selinux/ss/sidtab.c
index fd75a12fa8fc..e66a2ab3d1c2 100644
--- a/security/selinux/ss/sidtab.c
+++ b/security/selinux/ss/sidtab.c
@@ -116,11 +116,11 @@ struct context *sidtab_search_force(struct sidtab *s, u32 
sid)
return sidtab_search_core(s, sid, 1);
 }
 
-int sidtab_map(struct sidtab *s,
-  int (*apply) (u32 sid,
-struct context *context,
-void *args),
-  void *args)
+static int sidtab_map(struct sidtab *s,
+ int (*apply) (u32 sid,
+   struct context *context,
+   void *args),
+ void *args)
 {
int i, rc = 0;
struct sidtab_node *cur;
@@ -141,6 +141,37 @@ out:
return rc;
 }
 
+/* Clone the SID into the new SID table. */
+static int clone_sid(u32 sid, struct context *context, void *arg)
+{
+   struct sidtab *s = arg;
+
+   if (sid > SECINITSID_NUM)
+   return sidtab_insert(s, sid, context);
+   else
+   return 0;
+}
+
+int sidtab_convert(struct sidtab *s, struct sidtab *news,
+  int (*convert) (u32 sid,
+  struct context *context,
+  void *args),
+  void *args)
+{
+   unsigned long flags;
+   int rc;
+
+   spin_lock_irqsave(>lock, flags);
+   s->shutdown = 1;
+   spin_unlock_irqrestore(>lock, flags);
+
+   rc = sidtab_map(s, clone_sid, news);
+   if (rc)
+   return rc;
+
+   return sidtab_map(news, convert, args);
+}
+
 static void sidtab_update_cache(struct sidtab *s, struct sidtab_node *n, int 
loc)
 {
BUG_ON(loc >= SIDTAB_CACHE_LEN);
@@ -295,12 +326,3 @@ void sidtab_set(struct sidtab *dst, struct sidtab *src)
dst->cache[i] = NULL;
spin_unlock_irqrestore(>lock, flags);
 }
-
-void sidtab_shutdown(struct sidtab *s)
-{
-   unsigned long flags;
-
-   spin_lock_irqsave(>lock, flags);
-   s->shutdown = 1;
-   spin_unlock_irqrestore(>lock, flags);
-}
diff --git a/security/selinux/ss/sidtab.h b/security/selinux/ss/sidtab.h
index a1a1d2617b6f..26c74fe7afc0 100644
--- a/security/selinux/ss/sidtab.h
+++ b/security/selinux/ss/sidtab.h
@@ -37,11 +37,11 @@ int sidtab_insert(struct sidtab *s, u32 sid, struct context 
*context);
 struct context *sidtab_search(struct sidtab *s, u32 sid);
 struct context *sidtab_search_force(struct sidtab *s, u32 sid);
 
-int sidtab_map(struct sidtab *s,
-  int (*apply) (u32 sid,
-struct context *context,
-void *args),
-  void *args);
+int sidtab_convert(struct sidtab *s, struct sidtab *news,
+  int (*apply) (u32 sid,
+struct context