* [PATCH] selinux: implement new format of filename transitions
@ 2020-03-27 15:19 Ondrej Mosnacek
2020-04-16 2:22 ` Paul Moore
0 siblings, 1 reply; 5+ messages in thread
From: Ondrej Mosnacek @ 2020-03-27 15:19 UTC (permalink / raw)
To: selinux, Paul Moore; +Cc: Stephen Smalley
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 POLICYDB_VERSION_COMP_FTRANS
/* Mask for just the mount related flags */
#define SE_MNTMASK 0x0f
diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index 932b2b9bcdb2..f355876ed793 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -154,6 +154,11 @@ static struct policydb_compat_info policydb_compat[] = {
.sym_num = SYM_NUM,
.ocon_num = OCON_NUM,
},
+ {
+ .version = POLICYDB_VERSION_COMP_FTRANS,
+ .sym_num = SYM_NUM,
+ .ocon_num = OCON_NUM,
+ },
};
static struct policydb_compat_info *policydb_lookup_compat(int version)
@@ -461,23 +466,16 @@ static int rangetr_cmp(struct hashtab *h, const void *k1, const void *k2)
/*
* Initialize a policy database structure.
*/
-static int policydb_init(struct policydb *p)
+static void policydb_init(struct policydb *p)
{
memset(p, 0, sizeof(*p));
avtab_init(&p->te_avtab);
cond_policydb_init(p);
- p->filename_trans = hashtab_create(filenametr_hash, filenametr_cmp,
- (1 << 11));
- if (!p->filename_trans)
- return -ENOMEM;
-
ebitmap_init(&p->filename_trans_ttypes);
ebitmap_init(&p->policycaps);
ebitmap_init(&p->permissive_map);
-
- return 0;
}
/*
@@ -1842,7 +1840,7 @@ out:
return rc;
}
-static int filename_trans_read_one(struct policydb *p, void *fp)
+static int filename_trans_read_one_old(struct policydb *p, void *fp)
{
struct filename_trans_key key, *ft = NULL;
struct filename_trans_datum *last, *datum = NULL;
@@ -1925,6 +1923,99 @@ out:
return rc;
}
+static int filename_trans_read_one_new(struct policydb *p, void *fp)
+{
+ 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;
+
+ ndatum = le32_to_cpu(buf[2]);
+ if (ndatum == 0) {
+ pr_err("SELinux: Filename transition key with no datum\n");
+ rc = -ENOENT;
+ goto out;
+ }
+
+ dst = &first;
+ for (i = 0; i < ndatum; i++) {
+ rc = -ENOMEM;
+ datum = kmalloc(sizeof(*datum), GFP_KERNEL);
+ if (!datum)
+ goto out;
+
+ *dst = datum;
+
+ /* ebitmap_read() will at least init the bitmap */
+ rc = ebitmap_read(&datum->stypes, fp);
+ if (rc)
+ goto out;
+
+ rc = next_entry(buf, fp, sizeof(u32));
+ if (rc)
+ goto out;
+
+ datum->otype = le32_to_cpu(buf[0]);
+ datum->next = NULL;
+
+ dst = &datum->next;
+ }
+
+ rc = -ENOMEM;
+ ft = kmalloc(sizeof(*ft), GFP_KERNEL);
+ if (!ft)
+ goto out;
+
+ ft->ttype = ttype;
+ ft->tclass = tclass;
+ ft->name = name;
+
+ rc = hashtab_insert(p->filename_trans, ft, first);
+ if (rc == -EEXIST)
+ pr_err("SELinux: Duplicate filename transition key\n");
+ if (rc)
+ goto out;
+
+ p->filename_trans_count++;
+ return 0;
+
+out:
+ kfree(ft);
+ kfree(name);
+ while (first) {
+ datum = first;
+ first = first->next;
+
+ ebitmap_destroy(&datum->stypes);
+ kfree(datum);
+ }
+ return rc;
+}
+
static int filename_trans_read(struct policydb *p, void *fp)
{
u32 nel;
@@ -1939,12 +2030,29 @@ static int filename_trans_read(struct policydb *p, void *fp)
return rc;
nel = le32_to_cpu(buf[0]);
- p->filename_trans_count = nel;
+ if (p->policyvers < POLICYDB_VERSION_COMP_FTRANS) {
+ p->filename_trans_count = nel;
+ p->filename_trans = hashtab_create(filenametr_hash,
+ filenametr_cmp, (1 << 11));
+ if (!p->filename_trans)
+ return -ENOMEM;
- for (i = 0; i < nel; i++) {
- rc = filename_trans_read_one(p, fp);
- if (rc)
- return rc;
+ for (i = 0; i < nel; i++) {
+ rc = filename_trans_read_one_old(p, fp);
+ if (rc)
+ return rc;
+ }
+ } else {
+ p->filename_trans = hashtab_create(filenametr_hash,
+ filenametr_cmp, nel);
+ if (!p->filename_trans)
+ return -ENOMEM;
+
+ for (i = 0; i < nel; i++) {
+ rc = filename_trans_read_one_new(p, fp);
+ if (rc)
+ return rc;
+ }
}
hash_eval(p->filename_trans, "filenametr");
return 0;
@@ -2260,9 +2368,7 @@ int policydb_read(struct policydb *p, void *fp)
char *policydb_str;
struct policydb_compat_info *info;
- rc = policydb_init(p);
- if (rc)
- return rc;
+ policydb_init(p);
/* Read the magic number and string length. */
rc = next_entry(buf, fp, sizeof(u32) * 2);
@@ -3327,7 +3433,7 @@ static int range_write(struct policydb *p, void *fp)
return 0;
}
-static int filename_write_helper(void *key, void *data, void *ptr)
+static int filename_write_one_old(void *key, void *data, void *ptr)
{
struct filename_trans_key *ft = key;
struct filename_trans_datum *datum = data;
@@ -3364,26 +3470,82 @@ static int filename_write_helper(void *key, void *data, void *ptr)
return 0;
}
-static int filename_trans_write(struct policydb *p, void *fp)
+static int filename_write_one_new(void *key, void *data, void *ptr)
{
- __le32 buf[1];
+ struct filename_trans_key *ft = key;
+ struct filename_trans_datum *datum;
+ void *fp = ptr;
+ __le32 buf[3];
int rc;
+ u32 ndatum, len = strlen(ft->name);
- if (p->policyvers < POLICYDB_VERSION_FILENAME_TRANS)
- return 0;
-
- buf[0] = cpu_to_le32(p->filename_trans_count);
+ buf[0] = cpu_to_le32(len);
rc = put_entry(buf, sizeof(u32), 1, fp);
if (rc)
return rc;
- rc = hashtab_map(p->filename_trans, filename_write_helper, fp);
+ rc = put_entry(ft->name, sizeof(char), len, fp);
+ if (rc)
+ return rc;
+
+ ndatum = 0;
+ datum = data;
+ do {
+ ndatum++;
+ datum = datum->next;
+ } while (unlikely(datum));
+
+ buf[0] = cpu_to_le32(ft->ttype);
+ buf[1] = cpu_to_le32(ft->tclass);
+ buf[2] = cpu_to_le32(ndatum);
+ rc = put_entry(buf, sizeof(u32), 3, fp);
if (rc)
return rc;
+ datum = data;
+ do {
+ rc = ebitmap_write(&datum->stypes, fp);
+ if (rc)
+ return rc;
+
+ buf[0] = cpu_to_le32(datum->otype);
+ rc = put_entry(buf, sizeof(u32), 1, fp);
+ if (rc)
+ return rc;
+
+ datum = datum->next;
+ } while (unlikely(datum));
+
return 0;
}
+static int filename_trans_write(struct policydb *p, void *fp)
+{
+ __le32 buf[1];
+ int rc;
+
+ if (p->policyvers < POLICYDB_VERSION_FILENAME_TRANS)
+ return 0;
+
+ if (p->policyvers < POLICYDB_VERSION_COMP_FTRANS) {
+ buf[0] = cpu_to_le32(p->filename_trans_count);
+ rc = put_entry(buf, sizeof(u32), 1, fp);
+ if (rc)
+ return rc;
+
+ rc = hashtab_map(p->filename_trans, filename_write_one_old, fp);
+ } else {
+ buf[0] = cpu_to_le32(p->filename_trans->nel);
+ rc = put_entry(buf, sizeof(u32), 1, fp);
+ if (rc)
+ return rc;
+
+ rc = hashtab_map(p->filename_trans, filename_write_one_new, fp);
+ }
+
+ return rc;
+}
+
/*
* Write the configuration data in a policy database
* structure to a policy database binary representation
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] selinux: implement new format of filename transitions
2020-03-27 15:19 [PATCH] selinux: implement new format of filename transitions Ondrej Mosnacek
@ 2020-04-16 2:22 ` Paul Moore
2020-04-16 9:52 ` Ondrej Mosnacek
0 siblings, 1 reply; 5+ messages in thread
From: Paul Moore @ 2020-04-16 2:22 UTC (permalink / raw)
To: Ondrej Mosnacek; +Cc: selinux, Stephen Smalley
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 POLICYDB_VERSION_COMP_FTRANS
>
> /* Mask for just the mount related flags */
> #define SE_MNTMASK 0x0f
> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 932b2b9bcdb2..f355876ed793 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -154,6 +154,11 @@ static struct policydb_compat_info policydb_compat[] = {
> .sym_num = SYM_NUM,
> .ocon_num = OCON_NUM,
> },
> + {
> + .version = POLICYDB_VERSION_COMP_FTRANS,
> + .sym_num = SYM_NUM,
> + .ocon_num = OCON_NUM,
> + },
> };
>
> static struct policydb_compat_info *policydb_lookup_compat(int version)
> @@ -461,23 +466,16 @@ static int rangetr_cmp(struct hashtab *h, const void *k1, const void *k2)
> /*
> * Initialize a policy database structure.
> */
> -static int policydb_init(struct policydb *p)
> +static void policydb_init(struct policydb *p)
> {
> memset(p, 0, sizeof(*p));
>
> avtab_init(&p->te_avtab);
> cond_policydb_init(p);
>
> - p->filename_trans = hashtab_create(filenametr_hash, filenametr_cmp,
> - (1 << 11));
> - if (!p->filename_trans)
> - return -ENOMEM;
> -
> ebitmap_init(&p->filename_trans_ttypes);
> ebitmap_init(&p->policycaps);
> ebitmap_init(&p->permissive_map);
> -
> - return 0;
> }
>
> /*
> @@ -1842,7 +1840,7 @@ out:
> return rc;
> }
>
> -static int filename_trans_read_one(struct policydb *p, void *fp)
> +static int filename_trans_read_one_old(struct policydb *p, void *fp)
If you have to respin this patch, please change from XXX_old(...) to
XXX_compat(...); there is some precedence for using _compat in the
SELinux kernel code.
Same thing with the _write_ code.
> {
> struct filename_trans_key key, *ft = NULL;
> struct filename_trans_datum *last, *datum = NULL;
> @@ -1925,6 +1923,99 @@ out:
> return rc;
> }
>
> +static int filename_trans_read_one_new(struct policydb *p, void *fp)
No need to call this XXX_new(), just stick to the original name and
XXX_compat().
Same thing with the _write_ code.
> +{
> + 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?
> + ndatum = le32_to_cpu(buf[2]);
> + if (ndatum == 0) {
> + pr_err("SELinux: Filename transition key with no datum\n");
> + rc = -ENOENT;
> + goto out;
> + }
> +
> + dst = &first;
> + for (i = 0; i < ndatum; i++) {
> + rc = -ENOMEM;
> + datum = kmalloc(sizeof(*datum), GFP_KERNEL);
> + if (!datum)
> + goto out;
> +
> + *dst = datum;
> +
> + /* ebitmap_read() will at least init the bitmap */
> + rc = ebitmap_read(&datum->stypes, fp);
> + if (rc)
> + goto out;
> +
> + rc = next_entry(buf, fp, sizeof(u32));
> + if (rc)
> + goto out;
> +
> + datum->otype = le32_to_cpu(buf[0]);
> + datum->next = NULL;
> +
> + dst = &datum->next;
> + }
> +
> + rc = -ENOMEM;
> + ft = kmalloc(sizeof(*ft), GFP_KERNEL);
> + if (!ft)
> + goto out;
> +
> + ft->ttype = ttype;
> + ft->tclass = tclass;
> + ft->name = name;
> +
> + rc = hashtab_insert(p->filename_trans, ft, first);
> + if (rc == -EEXIST)
> + pr_err("SELinux: Duplicate filename transition key\n");
> + if (rc)
> + goto out;
> +
> + p->filename_trans_count++;
> + return 0;
> +
> +out:
> + kfree(ft);
> + kfree(name);
> + while (first) {
> + datum = first;
> + first = first->next;
> +
> + ebitmap_destroy(&datum->stypes);
> + kfree(datum);
> + }
> + return rc;
> +}
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] selinux: implement new format of filename transitions
2020-04-16 2:22 ` Paul Moore
@ 2020-04-16 9:52 ` Ondrej Mosnacek
2020-04-16 13:27 ` Paul Moore
0 siblings, 1 reply; 5+ messages in thread
From: Ondrej Mosnacek @ 2020-04-16 9:52 UTC (permalink / raw)
To: Paul Moore; +Cc: SElinux list, Stephen Smalley
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
> >
> > /* Mask for just the mount related flags */
> > #define SE_MNTMASK 0x0f
> > diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> > index 932b2b9bcdb2..f355876ed793 100644
> > --- a/security/selinux/ss/policydb.c
> > +++ b/security/selinux/ss/policydb.c
> > @@ -154,6 +154,11 @@ static struct policydb_compat_info policydb_compat[] = {
> > .sym_num = SYM_NUM,
> > .ocon_num = OCON_NUM,
> > },
> > + {
> > + .version = POLICYDB_VERSION_COMP_FTRANS,
> > + .sym_num = SYM_NUM,
> > + .ocon_num = OCON_NUM,
> > + },
> > };
> >
> > static struct policydb_compat_info *policydb_lookup_compat(int version)
> > @@ -461,23 +466,16 @@ static int rangetr_cmp(struct hashtab *h, const void *k1, const void *k2)
> > /*
> > * Initialize a policy database structure.
> > */
> > -static int policydb_init(struct policydb *p)
> > +static void policydb_init(struct policydb *p)
> > {
> > memset(p, 0, sizeof(*p));
> >
> > avtab_init(&p->te_avtab);
> > cond_policydb_init(p);
> >
> > - p->filename_trans = hashtab_create(filenametr_hash, filenametr_cmp,
> > - (1 << 11));
> > - if (!p->filename_trans)
> > - return -ENOMEM;
> > -
> > ebitmap_init(&p->filename_trans_ttypes);
> > ebitmap_init(&p->policycaps);
> > ebitmap_init(&p->permissive_map);
> > -
> > - return 0;
> > }
> >
> > /*
> > @@ -1842,7 +1840,7 @@ out:
> > return rc;
> > }
> >
> > -static int filename_trans_read_one(struct policydb *p, void *fp)
> > +static int filename_trans_read_one_old(struct policydb *p, void *fp)
>
> If you have to respin this patch, please change from XXX_old(...) to
> XXX_compat(...); there is some precedence for using _compat in the
> SELinux kernel code.
>
> Same thing with the _write_ code.
>
> > {
> > struct filename_trans_key key, *ft = NULL;
> > struct filename_trans_datum *last, *datum = NULL;
> > @@ -1925,6 +1923,99 @@ out:
> > return rc;
> > }
> >
> > +static int filename_trans_read_one_new(struct policydb *p, void *fp)
>
> No need to call this XXX_new(), just stick to the original name and
> XXX_compat().
>
> Same thing with the _write_ code.
OK, that makes sense.
>
> > +{
> > + 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.
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.)
>
> > + ndatum = le32_to_cpu(buf[2]);
> > + if (ndatum == 0) {
> > + pr_err("SELinux: Filename transition key with no datum\n");
> > + rc = -ENOENT;
> > + goto out;
> > + }
> > +
> > + dst = &first;
> > + for (i = 0; i < ndatum; i++) {
> > + rc = -ENOMEM;
> > + datum = kmalloc(sizeof(*datum), GFP_KERNEL);
> > + if (!datum)
> > + goto out;
> > +
> > + *dst = datum;
> > +
> > + /* ebitmap_read() will at least init the bitmap */
> > + rc = ebitmap_read(&datum->stypes, fp);
> > + if (rc)
> > + goto out;
> > +
> > + rc = next_entry(buf, fp, sizeof(u32));
> > + if (rc)
> > + goto out;
> > +
> > + datum->otype = le32_to_cpu(buf[0]);
> > + datum->next = NULL;
> > +
> > + dst = &datum->next;
> > + }
> > +
> > + rc = -ENOMEM;
> > + ft = kmalloc(sizeof(*ft), GFP_KERNEL);
> > + if (!ft)
> > + goto out;
> > +
> > + ft->ttype = ttype;
> > + ft->tclass = tclass;
> > + ft->name = name;
> > +
> > + rc = hashtab_insert(p->filename_trans, ft, first);
> > + if (rc == -EEXIST)
> > + pr_err("SELinux: Duplicate filename transition key\n");
> > + if (rc)
> > + goto out;
> > +
> > + p->filename_trans_count++;
> > + return 0;
> > +
> > +out:
> > + kfree(ft);
> > + kfree(name);
> > + while (first) {
> > + datum = first;
> > + first = first->next;
> > +
> > + ebitmap_destroy(&datum->stypes);
> > + kfree(datum);
> > + }
> > + return rc;
> > +}
>
> --
> paul moore
> www.paul-moore.com
>
--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] selinux: implement new format of filename transitions
2020-04-16 9:52 ` Ondrej Mosnacek
@ 2020-04-16 13:27 ` Paul Moore
2020-04-16 14:20 ` Ondrej Mosnacek
0 siblings, 1 reply; 5+ messages in thread
From: Paul Moore @ 2020-04-16 13:27 UTC (permalink / raw)
To: Ondrej Mosnacek; +Cc: SElinux list, Stephen Smalley
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] selinux: implement new format of filename transitions
2020-04-16 13:27 ` Paul Moore
@ 2020-04-16 14:20 ` Ondrej Mosnacek
0 siblings, 0 replies; 5+ messages in thread
From: Ondrej Mosnacek @ 2020-04-16 14:20 UTC (permalink / raw)
To: Paul Moore; +Cc: SElinux list, Stephen Smalley
On Thu, Apr 16, 2020 at 3:27 PM Paul Moore <paul@paul-moore.com> wrote:
> 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 :)
Hehe :) Weird coincidence that it occurred to me just yesterday how
easily I could accidentally paste something embarrassing in a reply
with this lousy touchpad I'm using now... I'm surprised that what
ended up there was actually somewhat insightful :) (I wanted to say
that the filename transition count could be useful if you were
analysing a core dump and looking at the struct values, but later I
changed my mind and removed it.)
--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-04-16 16:00 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-27 15:19 [PATCH] selinux: implement new format of filename transitions Ondrej Mosnacek
2020-04-16 2:22 ` Paul Moore
2020-04-16 9:52 ` Ondrej Mosnacek
2020-04-16 13:27 ` Paul Moore
2020-04-16 14:20 ` Ondrej Mosnacek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).