On Thu, Apr 30, 2020 at 7:35 PM James Carter <jwcart2@xxxxxxxxx> wrote:
> On Sat, Mar 28, 2020 at 8:46 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
> >
> > In preparation to support a new policy format with a more optimal
> > representation of filename transition rules, this patch applies an
> > equivalent change from kernel commit c3a276111ea2 ("selinux: optimize
> > storage of filename transitions").
> >
> > See the kernel commit's description [1] for the rationale behind this
> > representation. This change doesn't bring any measurable difference of
> > policy build performance (semodule -B) on Fedora.
> >
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git/commit/?id=c3a276111ea2572399281988b3129683e2a6b60b
> >
> > Signed-off-by: Ondrej Mosnacek <omosnace@xxxxxxxxxx>
> > ---
> > checkpolicy/policy_define.c | 52 +++------
> > checkpolicy/test/dispol.c | 27 +++--
> > libsepol/cil/src/cil_binary.c | 29 ++---
> > libsepol/include/sepol/policydb/policydb.h | 15 ++-
> > libsepol/src/expand.c | 60 +++-------
> > libsepol/src/kernel_to_cil.c | 24 +++-
> > libsepol/src/kernel_to_conf.c | 24 +++-
> > libsepol/src/policydb.c | 126 ++++++++++++++-------
> > libsepol/src/write.c | 46 ++++----
> > 9 files changed, 223 insertions(+), 180 deletions(-)
> >
> > diff --git a/checkpolicy/policy_define.c b/checkpolicy/policy_define.c
> > index c6733fa4..01a90438 100644
> > --- a/checkpolicy/policy_define.c
> > +++ b/checkpolicy/policy_define.c
> > @@ -3303,10 +3303,9 @@ int define_filename_trans(void)
> > ebitmap_t e_stypes, e_ttypes;
> > ebitmap_t e_tclasses;
> > ebitmap_node_t *snode, *tnode, *cnode;
> > - filename_trans_t *ft;
> > - filename_trans_datum_t *ftdatum;
> > filename_trans_rule_t *ftr;
> > type_datum_t *typdatum;
> > + char *dup_name;
> > uint32_t otype;
> > unsigned int c, s, t;
> > int add, rc;
> > @@ -3388,40 +3387,21 @@ int define_filename_trans(void)
> > ebitmap_for_each_positive_bit(&e_tclasses, cnode, c) {
> > ebitmap_for_each_positive_bit(&e_stypes, snode, s) {
> > ebitmap_for_each_positive_bit(&e_ttypes, tnode, t) {
> > - ft = calloc(1, sizeof(*ft));
> > - if (!ft) {
> > - yyerror("out of memory");
> > - goto bad;
> > - }
> > - ft->stype = s+1;
> > - ft->ttype = t+1;
> > - ft->tclass = c+1;
> > - ft->name = strdup(name);
> > - if (!ft->name) {
> > - yyerror("out of memory");
> > - goto bad;
> > - }
> > -
> > - ftdatum = hashtab_search(policydbp->filename_trans,
> > - (hashtab_key_t)ft);
> > - if (ftdatum) {
> > - yyerror2("duplicate filename transition for: filename_trans %s %s %s:%s",
> > - name,
> > - policydbp->p_type_val_to_name[s],
> > - policydbp->p_type_val_to_name[t],
> > - policydbp->p_class_val_to_name[c]);
> > - goto bad;
> > - }
> > -
> > - ftdatum = calloc(1, sizeof(*ftdatum));
> > - if (!ftdatum) {
> > - yyerror("out of memory");
> > - goto bad;
> > - }
> > - rc = hashtab_insert(policydbp->filename_trans,
> > - (hashtab_key_t)ft,
> > - ftdatum);
> > - if (rc) {
> > + dup_name = NULL;
> > + rc = policydb_filetrans_insert(
> > + policydbp, s+1, t+1, c+1, name,
> > + &dup_name, otype, NULL
> > + );
> > + free(dup_name);
> > + if (rc != SEPOL_OK) {
> > + if (rc == SEPOL_EEXIST) {
> > + yyerror2("duplicate filename transition for: filename_trans %s %s %s:%s",
> > + name,
> > + policydbp->p_type_val_to_name[s],
> > + policydbp->p_type_val_to_name[t],
> > + policydbp->p_class_val_to_name[c]);
> > + goto bad;
> > + }
> > yyerror("out of memory");
> > goto bad;
> > }
> > diff --git a/checkpolicy/test/dispol.c b/checkpolicy/test/dispol.c
> > index d72d9fb3..7c74e9cf 100644
> > --- a/checkpolicy/test/dispol.c
> > +++ b/checkpolicy/test/dispol.c
> > @@ -329,26 +329,39 @@ static void display_role_trans(policydb_t *p, FILE *fp)
> > struct filenametr_display_args {
> > policydb_t *p;
> > FILE *fp;
> > + filename_trans_key_t *ft;
> > };
> >
> > -static int filenametr_display(hashtab_key_t key,
> > - hashtab_datum_t datum,
> > - void *ptr)
> > +static int filenametr_display_sub(hashtab_key_t key,
> > + hashtab_datum_t datum,
> > + void *ptr)
> > {
> > - struct filename_trans *ft = (struct filename_trans *)key;
> > - struct filename_trans_datum *ftdatum = datum;
> > struct filenametr_display_args *args = ptr;
> > + filename_trans_key_t *ft = args->ft;
> > policydb_t *p = args->p;
> > FILE *fp = args->fp;
> > + uint32_t stype = (uintptr_t)key;
> > + uint32_t otype = (uintptr_t)datum;
> >
> > - display_id(p, fp, SYM_TYPES, ft->stype - 1, "");
> > + display_id(p, fp, SYM_TYPES, stype - 1, "");
> > display_id(p, fp, SYM_TYPES, ft->ttype - 1, "");
> > display_id(p, fp, SYM_CLASSES, ft->tclass - 1, ":");
> > - display_id(p, fp, SYM_TYPES, ftdatum->otype - 1, "");
> > + display_id(p, fp, SYM_TYPES, otype - 1, "");
> > fprintf(fp, " %s\n", ft->name);
> > return 0;
> > }
> >
> > +static int filenametr_display(hashtab_key_t key,
> > + hashtab_datum_t datum,
> > + void *ptr)
> > +{
> > + struct filenametr_display_args *args = ptr;
> > + hashtab_t subhash = datum;
> > +
> > + args->ft = (filename_trans_key_t *)key;
> > + return hashtab_map(subhash, filenametr_display_sub, args);
> > +}
> > +
> >
>
> Because of the discussion about setools, I've been playing some with
> dispol and I am getting segfaults with your patch when trying to print
> out the filename transitions.
>
> Looking closer at dispol, this doesn't make sense. The function
> display_filename_trans(), which hasn't changed, passes the function
> filenametr_display() to hashtab_map(), but then filenametr_display()
> takes the datum passed to it to call hashtab_map() again (this time
> with filenametr_display_sub()). The datum is a filename_trans_datum_t
> which doesn't have a hashtab in it.
Ouch, you're right! This must be a leftover of an earlier
implementation of ftrans rules compression I was trying out before...
I must've forgotten to update the dispol code and didn't realize it,
because the damn thing still compiled... I'll have a closer look
tomorrow and get it fixed. Thanks for spotting it, it would've been
much more embarrassing if this was found after merging...
