Re: [PATCH v3] selinux: simplify mls_context_to_sid()
On Tue, Nov 20, 2018 at 10:06 PM Paul Moore wrote:
> On Mon, Nov 12, 2018 at 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(-)
>
> What was the original motivation for this patch? Is there a performance
> issue?
No, there is no performance issue that I know of. I simply wanted to
move the sidtab_search() reference out of mls.c when I was adding the
sid_to_context/content_to_sid wrappers to services.c. I have now
dropped the wrappers in favor of just rewriting the sidtab functions,
but the mls_context_to_sid() interface looked really awkward to me,
especially considering that the 'ugly' part of the function is really
used by only one caller, so I decided to post the patch anyway.
>
> I'm asking because I'm not really convinced this is an improvement.
> While I agree the number of function arguments is a bordering on "too
> many", I think I like having the logic in mls_context_to_sid() for
> right now.
I disagree, but I don't mind leaving it as it is if that's what you prefer.
>
> > 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
> >
Re: [PATCH v3] selinux: simplify mls_context_to_sid()
On Mon, Nov 12, 2018 at 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(-)
What was the original motivation for this patch? Is there a performance issue?
I'm asking because I'm not really convinced this is an improvement.
While I agree the number of function arguments is a bordering on "too
many", I think I like having the logic in mls_context_to_sid() for
right now.
> 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 =
Re: [PATCH v3] selinux: simplify mls_context_to_sid()
On 11/14/18 10:23 AM, Stephen Smalley wrote:
On 11/13/18 10:14 PM, Paul Moore wrote:
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);
- 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
Re: [PATCH v3] selinux: simplify mls_context_to_sid()
On 11/13/18 10:14 PM, Paul Moore wrote:
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);
- 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
Re: [PATCH v3] selinux: simplify mls_context_to_sid()
On Wed, Nov 14, 2018 at 4:14 AM Paul Moore wrote:
> 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
Re: [PATCH v3] selinux: simplify mls_context_to_sid()
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: [PATCH v3] selinux: simplify mls_context_to_sid()
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
[PATCH v3] selinux: simplify mls_context_to_sid()
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;
+ } else if (!oldc) {
+ /*
+* If a def_sid is provided and no MLS field is present,
+* copy the MLS field of the associated default context.
+* Used for upgrading to MLS systems where objects may lack
+* MLS fields.
+*/
+ struct
