On Fri, Mar 27, 2020 at 11:19 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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
