SELinux Archive on lore.kernel.org
 help / color / Atom feed
From: Paul Moore <paul@paul-moore.com>
To: Joshua Brindle <joshua.brindle@crunchydata.com>
Cc: selinux@vger.kernel.org
Subject: Re: [PATCH] default_range glblub implementation
Date: Mon, 26 Aug 2019 17:19:06 -0400
Message-ID: <CAHC9VhRXyRDjj3KJDHvA4ruJg6H+1kzFPzfA-PLZ-NqBicsLrw@mail.gmail.com> (raw)
In-Reply-To: <20190826142008.2198-1-joshua.brindle@crunchydata.com>

On Mon, Aug 26, 2019 at 10:20 AM Joshua Brindle
<joshua.brindle@crunchydata.com> wrote:
> A policy developer can now specify glblub as a default_range default and
> the computed transition will be the intersection of the mls range of
> the two contexts.
>
> Signed-off-by: Joshua Brindle <joshua.brindle@crunchydata.com>
> ---
>  security/selinux/include/security.h |  3 ++-
>  security/selinux/ss/context.h       |  6 ++++++
>  security/selinux/ss/ebitmap.c       | 15 +++++++++++++++
>  security/selinux/ss/ebitmap.h       |  1 +
>  security/selinux/ss/mls.c           |  2 ++
>  security/selinux/ss/mls_types.h     | 28 ++++++++++++++++++++++++++++
>  security/selinux/ss/policydb.c      |  5 +++++
>  security/selinux/ss/policydb.h      |  1 +
>  8 files changed, 60 insertions(+), 1 deletion(-)

Independent from the comments below I think we need to additional
things for this patch:

* A much better description.  At the very least I would like you to
explain the MLS bounding and how one might use this (think sample
code); and don't worry about your description being too long ;)
* Tests (see the selinux-testsuite).

> diff --git a/security/selinux/ss/context.h b/security/selinux/ss/context.h
> index 2260c44a568c..cecb84d8b26c 100644
> --- a/security/selinux/ss/context.h
> +++ b/security/selinux/ss/context.h
> @@ -95,6 +95,12 @@ static inline int mls_context_cpy_high(struct context *dst, struct context *src)
>         return rc;
>  }
>
> +
> +static inline int mls_context_glblub(struct context *dst, struct context *c1, struct context *c2)
> +{
> +       return mls_range_glblub(&dst->range, &c1->range, &c2->range);
> +}

Considering the other functions that are already defined in context.h,
it seems like you could get rid of mls_range_glblub() and put the guts
in mls_context_glblub(), yes?  Unless I missed something
mls_range_glblub() is only ever called from mls_context_glblub() which
makes one of these functions pointless.

>  static inline int mls_context_cmp(struct context *c1, struct context *c2)
>  {
>         return ((c1->range.level[0].sens == c2->range.level[0].sens) &&
> diff --git a/security/selinux/ss/ebitmap.c b/security/selinux/ss/ebitmap.c
> index 09929fc5ab47..2042729b81f8 100644
> --- a/security/selinux/ss/ebitmap.c
> +++ b/security/selinux/ss/ebitmap.c
> @@ -77,6 +77,21 @@ int ebitmap_cpy(struct ebitmap *dst, struct ebitmap *src)
>         return 0;
>  }
>
> +int ebitmap_and(struct ebitmap *dst, struct ebitmap *e1, struct ebitmap *e2)
> +{
> +       unsigned int i, length = min(ebitmap_length(e1), ebitmap_length(e2));

A line of vertical whitespace here would be nice.

> +       ebitmap_init(dst);
> +       for (i=0; i < length; i++) {
> +               if (ebitmap_get_bit(e1, i) && ebitmap_get_bit(e2, i)) {
> +                       int rc = ebitmap_set_bit(dst, i, 1);
> +                       if (rc < 0)
> +                               return rc;
> +               }
> +       }

Same as above, a line of vertical whitespace would be nice.

Beyond that, since this is an AND operation, could we make better use
of things like find_first_bit()/ebitmap_start_positive()/ebitmap_next_positive()
to skip along one of the bitmaps instead of needing to call
ebitmap_get_bit() for each bit?  I imagine it would be quicker that
way.

> +       return 0;
> +}

...

> diff --git a/security/selinux/ss/mls_types.h b/security/selinux/ss/mls_types.h
> index 068e0d7809db..e2a20eb0e87c 100644
> --- a/security/selinux/ss/mls_types.h
> +++ b/security/selinux/ss/mls_types.h
> @@ -39,6 +39,34 @@ static inline int mls_level_dom(struct mls_level *l1, struct mls_level *l2)
>                 ebitmap_contains(&l1->cat, &l2->cat, 0));
>  }
>
> +static inline int mls_range_glblub(struct mls_range *dst, struct mls_range *r1, struct mls_range *r2)
> +{
> +       int rc = 0;
> +
> +       if (r1->level[1].sens < r2->level[0].sens || r2->level[1].sens < r1->level[0].sens) {
> +       {

These braces aren't necessary, take them from the patch and give them
to a child in need.

> +               // These ranges have no common sensitivities

Please use the old C style comments instead of the C++ style comments
(this applies to the whole patch).  I know you don't track kernel
development very closely, but we just had a discussion about this
on-list earlier this summer.

> +               return -1;
> +       }
> +
> +       // Take the greatest of the low
> +       dst->level[0].sens = max(r1->level[0].sens, r2->level[0].sens);
> +
> +        // Take the least of the high
> +       dst->level[1].sens = min(r1->level[1].sens, r2->level[1].sens);
> +
> +       rc = ebitmap_and(&dst->level[0].cat, &r1->level[0].cat, &r2->level[0].cat);
> +       if (rc)
> +               goto out;
> +
> +       rc = ebitmap_and(&dst->level[1].cat, &r1->level[1].cat, &r2->level[1].cat);
> +       if (rc)
> +               goto out;
> +
> +out:
> +       return rc;
> +}

-- 
paul moore
www.paul-moore.com

      reply index

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-26 14:20 Joshua Brindle
2019-08-26 21:19 ` Paul Moore [this message]

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAHC9VhRXyRDjj3KJDHvA4ruJg6H+1kzFPzfA-PLZ-NqBicsLrw@mail.gmail.com \
    --to=paul@paul-moore.com \
    --cc=joshua.brindle@crunchydata.com \
    --cc=selinux@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

SELinux Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/selinux/0 selinux/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 selinux selinux/ https://lore.kernel.org/selinux \
		selinux@vger.kernel.org selinux@archiver.kernel.org
	public-inbox-index selinux

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.selinux


AGPL code for this site: git clone https://public-inbox.org/ public-inbox