| Message ID | 20181112114426.20887-1-omosnace@redhat.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [v3] selinux: simplify mls_context_to_sid() | expand |
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 <omosnace@redhat.com> > --- > > 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 <linux/string.h> > #include <linux/errno.h> > #include <net/netlabel.h> > -#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 sets for isolation. > + } 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 context *defcon; > + > + rc = -EINVAL; > + if (def_sid == SECSID_NULL) > + goto out; > + > + defcon = sidtab_search(sidtabp, def_sid); > + if (!defcon) > + goto out; > + > + rc = mls_context_cpy(ctx, defcon); > + if (rc) > + goto out; > + } else { > + rc = mls_context_to_sid(pol, p, ctx); > + if (rc) > + goto out; > + } > > /* Check the validity of the new context. */ > rc = -EINVAL; >
On Tue, Nov 13, 2018 at 4:10 PM Stephen Smalley <sds@tycho.nsa.gov> 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 <omosnace@redhat.com> > > --- > > > > 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 <linux/string.h> > > #include <linux/errno.h> > > #include <net/netlabel.h> > > -#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 sets for isolation. Yep, and we should fix that in v4.20-rcX independent of this patch. I think if we simply remove the "(*scontext) == '\0'" from the check we should be okay; I believe the only time we would want to return 0 when not running a MLS policy would be when there is something in the MLS portion of the label. -- paul moore www.paul-moore.com
On Wed, Nov 14, 2018 at 4:14 AM Paul Moore <paul@paul-moore.com> wrote: > On Tue, Nov 13, 2018 at 4:10 PM Stephen Smalley <sds@tycho.nsa.gov> 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 <omosnace@redhat.com> > > > --- > > > > > > 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 <linux/string.h> > > > #include <linux/errno.h> > > > #include <net/netlabel.h> > > > -#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 sets for isolation. > > Yep, and we should fix that in v4.20-rcX independent of this patch. I > think if we simply remove the "(*scontext) == '\0'" from the check we > should be okay; I believe the only time we would want to return 0 when > not running a MLS policy would be when there is something in the MLS > portion of the label. I think I'll leave this part to you guys. I don't feel confident about changing the logic in this part of the code. I can't even parse the meaning of that scary if statement... Bug for bug compatibility was the best I could do here :)
On 11/13/18 10:14 PM, Paul Moore wrote: > On Tue, Nov 13, 2018 at 4:10 PM Stephen Smalley <sds@tycho.nsa.gov> 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 <omosnace@redhat.com> >>> --- >>> >>> 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 <linux/string.h> >>> #include <linux/errno.h> >>> #include <net/netlabel.h> >>> -#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 sets for isolation. > > Yep, and we should fix that in v4.20-rcX independent of this patch. I > think if we simply remove the "(*scontext) == '\0'" from the check we > should be okay; I believe the only time we would want to return 0 when > not running a MLS policy would be when there is something in the MLS > portion of the label. Yes, that should restore the previous behavior. The desired behavior is that if MLS is disabled, we reject the context as invalid if it has a MLS field unless the context was fetched from an xattr, which we only permit to provide filesystem compatibility between MLS and non-MLS systems.
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 <sds@tycho.nsa.gov> >> 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 <omosnace@redhat.com> >>>> --- >>>> >>>> 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 <linux/string.h> >>>> #include <linux/errno.h> >>>> #include <net/netlabel.h> >>>> -#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 sets for isolation. >> >> Yep, and we should fix that in v4.20-rcX independent of this patch. I >> think if we simply remove the "(*scontext) == '\0'" from the check we >> should be okay; I believe the only time we would want to return 0 when >> not running a MLS policy would be when there is something in the MLS >> portion of the label. > > Yes, that should restore the previous behavior. The desired behavior is > that if MLS is disabled, we reject the context as invalid if it has a > MLS field unless the context was fetched from an xattr, which we only > permit to provide filesystem compatibility between MLS and non-MLS systems. Sorry, that isn't correct. If MLS is disabled, then we want to return 0 if either there is no MLS field (!oldc) or the context came from an xattr (def_sid != SECSID_NULL). We should only return -EINVAL if there is a MLS field (oldc) and the context did not come from an xattr (def_sid == SECSID_NULL).
On Mon, Nov 12, 2018 at 6:44 AM Ondrej Mosnacek <omosnace@redhat.com> 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 <omosnace@redhat.com> > --- > > 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 <linux/string.h> > #include <linux/errno.h> > #include <net/netlabel.h> > -#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 context *defcon; > + > + rc = -EINVAL; > + if (def_sid == SECSID_NULL) > + goto out; > + > + defcon = sidtab_search(sidtabp, def_sid); > + if (!defcon) > + goto out; > + > + rc = mls_context_cpy(ctx, defcon); > + if (rc) > + goto out; > + } else { > + rc = mls_context_to_sid(pol, p, ctx); > + if (rc) > + goto out; > + } > > /* Check the validity of the new context. */ > rc = -EINVAL; > -- > 2.17.2 >
On Tue, Nov 20, 2018 at 10:06 PM Paul Moore <paul@paul-moore.com> wrote: > On Mon, Nov 12, 2018 at 6:44 AM Ondrej Mosnacek <omosnace@redhat.com> 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 <omosnace@redhat.com> > > --- > > > > 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 <linux/string.h> > > #include <linux/errno.h> > > #include <net/netlabel.h> > > -#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 context *defcon; > > + > > + rc = -EINVAL; > > + if (def_sid == SECSID_NULL) > > + goto out; > > + > > + defcon = sidtab_search(sidtabp, def_sid); > > + if (!defcon) > > + goto out; > > + > > + rc = mls_context_cpy(ctx, defcon); > > + if (rc) > > + goto out; > > + } else { > > + rc = mls_context_to_sid(pol, p, ctx); > > + if (rc) > > + goto out; > > + } > > > > /* Check the validity of the new context. */ > > rc = -EINVAL; > > -- > > 2.17.2 > > > > > -- > paul moore > www.paul-moore.com
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 <linux/string.h> #include <linux/errno.h> #include <net/netlabel.h> -#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 context *defcon; + + rc = -EINVAL; + if (def_sid == SECSID_NULL) + goto out; + + defcon = sidtab_search(sidtabp, def_sid); + if (!defcon) + goto out; + + rc = mls_context_cpy(ctx, defcon); + if (rc) + goto out; + } else { + rc = mls_context_to_sid(pol, p, ctx); + if (rc) + goto out; + } /* Check the validity of the new context. */ rc = -EINVAL;
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 <omosnace@redhat.com> --- 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(-)