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

From: Paul Moore <paul@paul-moore.com>
To: omosnace@redhat.com
Cc: Stephen Smalley <sds@tycho.nsa.gov>,
	selinux@vger.kernel.org, selinux@tycho.nsa.gov
Subject: Re: [PATCH 1/2] selinux: use separate table for initial SID lookup
Date: Mon, 5 Nov 2018 15:47:09 -0500	[thread overview]
Message-ID: <CAHC9VhTLvAYg7oi9pyRuDSQ1ht5L8seCdjKf=0BZ0nsQFSypxw@mail.gmail.com> (raw)
In-Reply-To: <CAFqZXNvDiQekJTm=4dLx93BWEouiqxWFCc_207oEuaSFuV3BfA@mail.gmail.com>

On Fri, Nov 2, 2018 at 11:35 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Wed, Oct 31, 2018 at 6:09 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> > On 10/31/2018 08:27 AM, Ondrej Mosnacek wrote:
> > > This patch separates the lookup of the initial SIDs into a separate
> > > lookup table (implemented simply by a fixed-size array), in order to
> > > pave the way for improving the process of converting the sidtab to a new
> > > policy during a policy reload.
> > >
> > > The initial SIDs are loaded directly and are skipped during sidtab
> > > conversion, so handling them separately makes things somewhat simpler.
> > > Since there is only a small fixed number of them, they can be stored in
> > > a simple lookup table.
> > >
> > > This patch also moves the fallback-to-unlabeled logic from sidtab.c to
> > > the new helper functions in services.c that now handle the unified
> > > lookup in both sidtab and isidtab, simplifying the sidtab interface.
> > >
> > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > > ---
> > >   security/selinux/include/security.h |   3 +
> > >   security/selinux/ss/mls.c           |   6 +-
> > >   security/selinux/ss/mls.h           |   2 +-
> > >   security/selinux/ss/policydb.c      |  24 ++-
> > >   security/selinux/ss/policydb.h      |  26 ++-
> > >   security/selinux/ss/services.c      | 238 +++++++++++++++-------------
> > >   security/selinux/ss/services.h      |   1 +
> > >   security/selinux/ss/sidtab.c        |  29 +---
> > >   security/selinux/ss/sidtab.h        |   3 +-
> > >   9 files changed, 187 insertions(+), 145 deletions(-)
> > >
> > > diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
> > > index 23e762d529fa..a1b4b13c2300 100644
> > > --- a/security/selinux/include/security.h
> > > +++ b/security/selinux/include/security.h
> > > @@ -221,6 +221,9 @@ struct extended_perms {
> > >   /* definitions of av_decision.flags */
> > >   #define AVD_FLAGS_PERMISSIVE        0x0001
> > >
> > > +struct context *security_sid_to_context_struct(struct selinux_state *state,
> > > +                                            u32 sid, int force);
> >
> > This header is for interfaces exposed by the security server (i.e. the
> > policy engine) to the AVC, hooks, and other policy enforcement code. The
> > context structure is private to the security server in order to
> > encapsulate the policy logic and should never be returned directly to
> > code outside of the security server.  Technically you aren't actually
> > exposing the structure definition but this interface isn't useful
> > without doing so, so it shouldn't live here.
>
> Another option could be to refine mls_context_to_sid() so it doesn't
> need the sidtab lookup at all, moving that part to the call sites.
> That function has two callers and only one of them can really trigger
> the path with the lookup. I planned to look into doing this later (I
> didn't want to include unnecessary changes in this patchset), but now
> I actually tried doing it and it seems like a good simplification. I
> will fold it under these two patches in v2. After this change the
> helper function won't be needed outside services.c.
>
> >
> > You could make this a services_sid_to_context_struct() interface defined
> > in security/selinux/ss/services.h instead.  Or you could keep all of
> > this within the sidtab, just making the isidtab part of its internal
> > state, and moving this logic inside of sidtab_search() instead of
> > splitting it out.
>
> My intention was to not hide too much complexity under sidtab, but
> rethinking it now I agree it would probably make sense to just hide
> isidtab under sidtab. It would need to have a separate insert function
> for initial SIDs (and in the second patch also some logic to switch to
> the new isidtab), but I guess that is less ugly than keeping it
> outside... I'll see if I can make it all a bit nicer.

FWIW, I agree with Stephen about managing the initial sids within the
context of the sidtab; conceptually it just makes sense.

-- 
paul moore
www.paul-moore.com