Re: [PATCH] selinux: implement new format of filename transitions

[Paul Moore <paul@paul-moore.com>]

On Thu, Apr 16, 2020 at 5:53 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Thu, Apr 16, 2020 at 4:23 AM Paul Moore <paul@paul-moore.com> wrote:
> > On Fri, Mar 27, 2020 at 11:19 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > >
> > > Implement a new, more space-efficient way of storing filename
> > > transitions in the binary policy. The internal structures have already
> > > been converted to this new representation; this patch just implements
> > > reading/writing an equivalent represntation from/to the binary policy.
> > >
> > > This new format reduces the size of Fedora policy from 7.6 MB to only
> > > 3.3 MB (with policy optimization enabled in both cases). With the
> > > unconfined module disabled, the size is reduced from 3.3 MB to 2.4 MB.
> > >
> > > The time to load policy into kernel is also shorter with the new format.
> > > On Fedora Rawhide x86_64 it dropped from 157 ms to 106 ms; without the
> > > unconfined module from 115 ms to 105 ms.
> > >
> > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > > ---
> > >  security/selinux/include/security.h |   3 +-
> > >  security/selinux/ss/policydb.c      | 212 ++++++++++++++++++++++++----
> > >  2 files changed, 189 insertions(+), 26 deletions(-)
> >
> > ...
> >
> > > diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
> > > index d6036c018cf2..b0e02cfe3ce1 100644
> > > --- a/security/selinux/include/security.h
> > > +++ b/security/selinux/include/security.h
> > > @@ -41,10 +41,11 @@
> > >  #define POLICYDB_VERSION_XPERMS_IOCTL  30
> > >  #define POLICYDB_VERSION_INFINIBAND            31
> > >  #define POLICYDB_VERSION_GLBLUB                32
> > > +#define POLICYDB_VERSION_COMP_FTRANS   33 /* compressed filename transitions */
> > >
> > >  /* Range of policy versions we understand*/
> > >  #define POLICYDB_VERSION_MIN   POLICYDB_VERSION_BASE
> > > -#define POLICYDB_VERSION_MAX   POLICYDB_VERSION_GLBLUB
> > > +#define POLICYDB_VERSION_MAX   POcould still help in case of coredump analysisLICYDB_VERSION_COMP_FTRANS

Errant middle mouse clicks are always fun :)

> > > +{
> > > +       struct filename_trans_key *ft = NULL;
> > > +       struct filename_trans_datum **dst, *datum, *first = NULL;
> > > +       char *name = NULL;
> > > +       u32 len, ttype, tclass, ndatum, i;
> > > +       __le32 buf[3];
> > > +       int rc;
> > > +
> > > +       /* length of the path component string */
> > > +       rc = next_entry(buf, fp, sizeof(u32));
> > > +       if (rc)
> > > +               return rc;
> > > +       len = le32_to_cpu(buf[0]);
> > > +
> > > +       /* path component string */
> > > +       rc = str_read(&name, GFP_KERNEL, fp, len);
> > > +       if (rc)
> > > +               return rc;
> > > +
> > > +       rc = next_entry(buf, fp, sizeof(u32) * 3);
> > > +       if (rc)
> > > +               goto out;
> > > +
> > > +       ttype = le32_to_cpu(buf[0]);
> > > +       tclass = le32_to_cpu(buf[1]);
> > > +
> > > +       rc = ebitmap_set_bit(&p->filename_trans_ttypes, ttype, 1);
> > > +       if (rc)
> > > +               goto out;
> >
> > Should we move the p->filename_trans_ttypes update to the bottom of
> > the function where we increment filename_trans_count?
>
> I think it doesn't matter in practice, since on error the whole
> policydb is just thrown away anyway, but I agree it fits there a
> little better.

Yes, it shouldn't matter purely from a functional perspective, but
from a human perspective it helps reinforce the link between those two
variables in the code and if something were to ever change in the
caller's error handling this function would be less likely to break.

> Also, looking at the filename_trans_count increment, it should be
> adding 'ndatum' instead of just 1 to match what the compat path puts
> there. (Again this doesn't matter in practice, since the policyvers
> will always stay the same and nothing will use that value, but it
> should still be consistent.)

Sounds like a good change to me.

-- 
paul moore
www.paul-moore.com