* [PATCH v2 0/2] userspace: Implement new format of filename trans rules
@ 2020-03-28 12:45 Ondrej Mosnacek
2020-03-28 12:45 ` [PATCH v2 1/2] libsepol,checkpolicy: optimize storage of filename transitions Ondrej Mosnacek
2020-03-28 12:45 ` [PATCH v2 2/2] libsepol: implement POLICYDB_VERSION_COMP_FTRANS Ondrej Mosnacek
0 siblings, 2 replies; 12+ messages in thread
From: Ondrej Mosnacek @ 2020-03-28 12:45 UTC (permalink / raw)
To: selinux; +Cc: Stephen Smalley, Chris PeBenito
These patches are the userspace side of the kernel change posted at [1].
The first patch changes libsepol's internal representation of filename
transition rules in a way similar to kernel commit c3a276111ea2
("selinux: optimize storage of filename transitions") [2].
The second patch then builds upon that and implements reading and
writing of a new binary policy format that uses this representation also
in the data layout.
See individual patches for more details.
NOTE: This series unfortunately breaks the build of setools. Moreover,
when an existing build of setools dynamically links against the new
libsepol, it segfaults. Sadly, there doesn't seem to be a nice way of
handling this, since setools relies on non-public libsepol policydb
API/ABI.
Changes in v2:
- fixed counting rules when reading the new policy format
[1] https://lore.kernel.org/selinux/20200327151941.95619-1-omosnace@redhat.com/T/
[2] https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git/commit/?id=c3a276111ea2572399281988b3129683e2a6b60b
Ondrej Mosnacek (2):
libsepol,checkpolicy: optimize storage of filename transitions
libsepol: implement POLICYDB_VERSION_COMP_FTRANS
checkpolicy/policy_define.c | 52 ++--
checkpolicy/test/dispol.c | 27 +-
libsepol/cil/src/cil_binary.c | 29 +-
libsepol/include/sepol/policydb/policydb.h | 18 +-
libsepol/src/expand.c | 60 +---
libsepol/src/kernel_to_cil.c | 24 +-
libsepol/src/kernel_to_conf.c | 24 +-
libsepol/src/policydb.c | 314 ++++++++++++++++-----
libsepol/src/write.c | 100 +++++--
9 files changed, 441 insertions(+), 207 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 1/2] libsepol,checkpolicy: optimize storage of filename transitions
2020-03-28 12:45 [PATCH v2 0/2] userspace: Implement new format of filename trans rules Ondrej Mosnacek
@ 2020-03-28 12:45 ` Ondrej Mosnacek
2020-03-31 20:30 ` James Carter
2020-04-30 17:34 ` James Carter
2020-03-28 12:45 ` [PATCH v2 2/2] libsepol: implement POLICYDB_VERSION_COMP_FTRANS Ondrej Mosnacek
1 sibling, 2 replies; 12+ messages in thread
From: Ondrej Mosnacek @ 2020-03-28 12:45 UTC (permalink / raw)
To: selinux; +Cc: Stephen Smalley, Chris PeBenito
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@redhat.com>
---
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);
+}
+
static void display_filename_trans(policydb_t *p, FILE *fp)
{
diff --git a/libsepol/cil/src/cil_binary.c b/libsepol/cil/src/cil_binary.c
index 62178d99..bedff628 100644
--- a/libsepol/cil/src/cil_binary.c
+++ b/libsepol/cil/src/cil_binary.c
@@ -1131,13 +1131,13 @@ int __cil_typetransition_to_avtab(policydb_t *pdb, const struct cil_db *db, stru
class_datum_t *sepol_obj = NULL;
struct cil_list *class_list;
type_datum_t *sepol_result = NULL;
- filename_trans_t *newkey = NULL;
- filename_trans_datum_t *newdatum = NULL, *otype = NULL;
ebitmap_t src_bitmap, tgt_bitmap;
ebitmap_node_t *node1, *node2;
unsigned int i, j;
+ uint32_t otype;
struct cil_list_item *c;
char *name = DATUM(typetrans->name)->name;
+ char *dup_name;
if (name == CIL_KEY_STAR) {
struct cil_type_rule trans;
@@ -1176,22 +1176,16 @@ int __cil_typetransition_to_avtab(policydb_t *pdb, const struct cil_db *db, stru
rc = __cil_get_sepol_class_datum(pdb, DATUM(c->data), &sepol_obj);
if (rc != SEPOL_OK) goto exit;
- newkey = cil_calloc(1, sizeof(*newkey));
- newdatum = cil_calloc(1, sizeof(*newdatum));
- newkey->stype = sepol_src->s.value;
- newkey->ttype = sepol_tgt->s.value;
- newkey->tclass = sepol_obj->s.value;
- newkey->name = cil_strdup(name);
- newdatum->otype = sepol_result->s.value;
-
- rc = hashtab_insert(pdb->filename_trans,
- (hashtab_key_t)newkey,
- newdatum);
+ dup_name = NULL;
+ rc = policydb_filetrans_insert(
+ pdb, sepol_src->s.value, sepol_tgt->s.value,
+ sepol_obj->s.value, name, &dup_name,
+ sepol_result->s.value, &otype
+ );
+ free(dup_name);
if (rc != SEPOL_OK) {
if (rc == SEPOL_EEXIST) {
- otype = hashtab_search(pdb->filename_trans,
- (hashtab_key_t)newkey);
- if (newdatum->otype != otype->otype) {
+ if (sepol_result->s.value!= otype) {
cil_log(CIL_ERR, "Conflicting name type transition rules\n");
} else {
rc = SEPOL_OK;
@@ -1199,9 +1193,6 @@ int __cil_typetransition_to_avtab(policydb_t *pdb, const struct cil_db *db, stru
} else {
cil_log(CIL_ERR, "Out of memory\n");
}
- free(newkey->name);
- free(newkey);
- free(newdatum);
if (rc != SEPOL_OK) {
goto exit;
}
diff --git a/libsepol/include/sepol/policydb/policydb.h b/libsepol/include/sepol/policydb/policydb.h
index 81b63fef..c3180c61 100644
--- a/libsepol/include/sepol/policydb/policydb.h
+++ b/libsepol/include/sepol/policydb/policydb.h
@@ -162,15 +162,16 @@ typedef struct role_allow {
} role_allow_t;
/* filename_trans rules */
-typedef struct filename_trans {
- uint32_t stype;
+typedef struct filename_trans_key {
uint32_t ttype;
uint32_t tclass;
char *name;
-} filename_trans_t;
+} filename_trans_key_t;
typedef struct filename_trans_datum {
- uint32_t otype; /* expected of new object */
+ ebitmap_t stypes;
+ uint32_t otype;
+ struct filename_trans_datum *next;
} filename_trans_datum_t;
/* Type attributes */
@@ -591,6 +592,7 @@ typedef struct policydb {
/* file transitions with the last path component */
hashtab_t filename_trans;
+ uint32_t filename_trans_count;
ebitmap_t *type_attr_map;
@@ -650,6 +652,11 @@ extern int policydb_load_isids(policydb_t * p, sidtab_t * s);
extern int policydb_sort_ocontexts(policydb_t *p);
+extern int policydb_filetrans_insert(policydb_t *p, uint32_t stype,
+ uint32_t ttype, uint32_t tclass,
+ const char *name, char **name_alloc,
+ uint32_t otype, uint32_t *present_otype);
+
/* Deprecated */
extern int policydb_context_isvalid(const policydb_t * p,
const context_struct_t * c);
diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c
index 529e1d35..28f93acb 100644
--- a/libsepol/src/expand.c
+++ b/libsepol/src/expand.c
@@ -1371,16 +1371,15 @@ static int copy_role_trans(expand_state_t * state, role_trans_rule_t * rules)
static int expand_filename_trans(expand_state_t *state, filename_trans_rule_t *rules)
{
unsigned int i, j;
- filename_trans_t key, *new_trans;
- filename_trans_datum_t *otype;
filename_trans_rule_t *cur_rule;
ebitmap_t stypes, ttypes;
ebitmap_node_t *snode, *tnode;
+ char *name;
int rc;
cur_rule = rules;
while (cur_rule) {
- uint32_t mapped_otype;
+ uint32_t mapped_otype, present_otype;
ebitmap_init(&stypes);
ebitmap_init(&ttypes);
@@ -1401,15 +1400,17 @@ static int expand_filename_trans(expand_state_t *state, filename_trans_rule_t *r
ebitmap_for_each_positive_bit(&stypes, snode, i) {
ebitmap_for_each_positive_bit(&ttypes, tnode, j) {
- key.stype = i + 1;
- key.ttype = j + 1;
- key.tclass = cur_rule->tclass;
- key.name = cur_rule->name;
- otype = hashtab_search(state->out->filename_trans,
- (hashtab_key_t) &key);
- if (otype) {
+ name = NULL;
+
+ rc = policydb_filetrans_insert(
+ state->out, i + 1, j + 1,
+ cur_rule->tclass, cur_rule->name,
+ &name, mapped_otype, &present_otype
+ );
+ free(name);
+ if (rc == SEPOL_EEXIST) {
/* duplicate rule, ignore */
- if (otype->otype == mapped_otype)
+ if (present_otype == mapped_otype)
continue;
ERR(state->handle, "Conflicting name-based type_transition %s %s:%s \"%s\": %s vs %s",
@@ -1417,44 +1418,11 @@ static int expand_filename_trans(expand_state_t *state, filename_trans_rule_t *r
state->out->p_type_val_to_name[j],
state->out->p_class_val_to_name[cur_rule->tclass - 1],
cur_rule->name,
- state->out->p_type_val_to_name[otype->otype - 1],
+ state->out->p_type_val_to_name[present_otype - 1],
state->out->p_type_val_to_name[mapped_otype - 1]);
return -1;
- }
-
- new_trans = calloc(1, sizeof(*new_trans));
- if (!new_trans) {
- ERR(state->handle, "Out of memory!");
- return -1;
- }
-
- new_trans->name = strdup(cur_rule->name);
- if (!new_trans->name) {
- ERR(state->handle, "Out of memory!");
- free(new_trans);
- return -1;
- }
- new_trans->stype = i + 1;
- new_trans->ttype = j + 1;
- new_trans->tclass = cur_rule->tclass;
-
- otype = calloc(1, sizeof(*otype));
- if (!otype) {
- ERR(state->handle, "Out of memory!");
- free(new_trans->name);
- free(new_trans);
- return -1;
- }
- otype->otype = mapped_otype;
-
- rc = hashtab_insert(state->out->filename_trans,
- (hashtab_key_t)new_trans,
- otype);
- if (rc) {
+ } else if (rc < 0) {
ERR(state->handle, "Out of memory!");
- free(otype);
- free(new_trans->name);
- free(new_trans);
return -1;
}
}
diff --git a/libsepol/src/kernel_to_cil.c b/libsepol/src/kernel_to_cil.c
index ede78a20..718d3481 100644
--- a/libsepol/src/kernel_to_cil.c
+++ b/libsepol/src/kernel_to_cil.c
@@ -1821,21 +1821,35 @@ struct map_filename_trans_args {
static int map_filename_trans_to_str(hashtab_key_t key, void *data, void *arg)
{
- filename_trans_t *ft = (filename_trans_t *)key;
+ filename_trans_key_t *ft = (filename_trans_key_t *)key;
filename_trans_datum_t *datum = data;
struct map_filename_trans_args *map_args = arg;
struct policydb *pdb = map_args->pdb;
struct strs *strs = map_args->strs;
char *src, *tgt, *class, *filename, *new;
+ struct ebitmap_node *node;
+ uint32_t bit;
+ int rc;
- src = pdb->p_type_val_to_name[ft->stype - 1];
tgt = pdb->p_type_val_to_name[ft->ttype - 1];
class = pdb->p_class_val_to_name[ft->tclass - 1];
filename = ft->name;
- new = pdb->p_type_val_to_name[datum->otype - 1];
+ do {
+ new = pdb->p_type_val_to_name[datum->otype - 1];
+
+ ebitmap_for_each_positive_bit(&datum->stypes, node, bit) {
+ src = pdb->p_type_val_to_name[bit];
+ rc = strs_create_and_add(strs,
+ "(typetransition %s %s %s %s %s)",
+ 5, src, tgt, class, filename, new);
+ if (rc)
+ return rc;
+ }
+
+ datum = datum->next;
+ } while (datum);
- return strs_create_and_add(strs, "(typetransition %s %s %s %s %s)", 5,
- src, tgt, class, filename, new);
+ return 0;
}
static int write_filename_trans_rules_to_cil(FILE *out, struct policydb *pdb)
diff --git a/libsepol/src/kernel_to_conf.c b/libsepol/src/kernel_to_conf.c
index 9de64832..9fb3ed22 100644
--- a/libsepol/src/kernel_to_conf.c
+++ b/libsepol/src/kernel_to_conf.c
@@ -1801,21 +1801,35 @@ struct map_filename_trans_args {
static int map_filename_trans_to_str(hashtab_key_t key, void *data, void *arg)
{
- filename_trans_t *ft = (filename_trans_t *)key;
+ filename_trans_key_t *ft = (filename_trans_key_t *)key;
filename_trans_datum_t *datum = data;
struct map_filename_trans_args *map_args = arg;
struct policydb *pdb = map_args->pdb;
struct strs *strs = map_args->strs;
char *src, *tgt, *class, *filename, *new;
+ struct ebitmap_node *node;
+ uint32_t bit;
+ int rc;
- src = pdb->p_type_val_to_name[ft->stype - 1];
tgt = pdb->p_type_val_to_name[ft->ttype - 1];
class = pdb->p_class_val_to_name[ft->tclass - 1];
filename = ft->name;
- new = pdb->p_type_val_to_name[datum->otype - 1];
+ do {
+ new = pdb->p_type_val_to_name[datum->otype - 1];
+
+ ebitmap_for_each_positive_bit(&datum->stypes, node, bit) {
+ src = pdb->p_type_val_to_name[bit];
+ rc = strs_create_and_add(strs,
+ "type_transition %s %s:%s %s \"%s\";",
+ 5, src, tgt, class, new, filename);
+ if (rc)
+ return rc;
+ }
+
+ datum = datum->next;
+ } while (datum);
- return strs_create_and_add(strs, "type_transition %s %s:%s %s \"%s\";", 5,
- src, tgt, class, new, filename);
+ return 0;
}
static int write_filename_trans_rules_to_conf(FILE *out, struct policydb *pdb)
diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c
index 5b289a52..6b121d66 100644
--- a/libsepol/src/policydb.c
+++ b/libsepol/src/policydb.c
@@ -789,12 +789,12 @@ partial_name_hash(unsigned long c, unsigned long prevhash)
static unsigned int filenametr_hash(hashtab_t h, const_hashtab_key_t k)
{
- const struct filename_trans *ft = (const struct filename_trans *)k;
+ const filename_trans_key_t *ft = (const filename_trans_key_t *)k;
unsigned long hash;
unsigned int byte_num;
unsigned char focus;
- hash = ft->stype ^ ft->ttype ^ ft->tclass;
+ hash = ft->ttype ^ ft->tclass;
byte_num = 0;
while ((focus = ft->name[byte_num++]))
@@ -805,14 +805,10 @@ static unsigned int filenametr_hash(hashtab_t h, const_hashtab_key_t k)
static int filenametr_cmp(hashtab_t h __attribute__ ((unused)),
const_hashtab_key_t k1, const_hashtab_key_t k2)
{
- const struct filename_trans *ft1 = (const struct filename_trans *)k1;
- const struct filename_trans *ft2 = (const struct filename_trans *)k2;
+ const filename_trans_key_t *ft1 = (const filename_trans_key_t *)k1;
+ const filename_trans_key_t *ft2 = (const filename_trans_key_t *)k2;
int v;
- v = ft1->stype - ft2->stype;
- if (v)
- return v;
-
v = ft1->ttype - ft2->ttype;
if (v)
return v;
@@ -1409,9 +1405,12 @@ common_destroy, class_destroy, role_destroy, type_destroy, user_destroy,
static int filenametr_destroy(hashtab_key_t key, hashtab_datum_t datum,
void *p __attribute__ ((unused)))
{
- struct filename_trans *ft = (struct filename_trans *)key;
+ filename_trans_key_t *ft = (filename_trans_key_t *)key;
+ filename_trans_datum_t *fd = datum;
+
free(ft->name);
free(key);
+ ebitmap_destroy(&fd->stypes);
free(datum);
return 0;
}
@@ -2595,12 +2594,77 @@ int role_allow_read(role_allow_t ** r, struct policy_file *fp)
return 0;
}
+int policydb_filetrans_insert(policydb_t *p, uint32_t stype, uint32_t ttype,
+ uint32_t tclass, const char *name,
+ char **name_alloc, uint32_t otype,
+ uint32_t *present_otype)
+{
+ filename_trans_key_t *ft, key;
+ filename_trans_datum_t *datum, *last;
+
+ key.ttype = ttype;
+ key.tclass = tclass;
+ key.name = (char *)name;
+
+ last = NULL;
+ datum = hashtab_search(p->filename_trans, (hashtab_key_t)&key);
+ while (datum) {
+ if (ebitmap_get_bit(&datum->stypes, stype - 1)) {
+ if (present_otype)
+ *present_otype = datum->otype;
+ return SEPOL_EEXIST;
+ }
+ if (datum->otype == otype)
+ break;
+ last = datum;
+ datum = datum->next;
+ }
+ if (!datum) {
+ if (!*name_alloc) {
+ *name_alloc = strdup(name);
+ if (!*name_alloc)
+ return SEPOL_ENOMEM;
+ }
+
+ datum = malloc(sizeof(*datum));
+ if (!datum)
+ return SEPOL_ENOMEM;
+
+ ebitmap_init(&datum->stypes);
+ datum->otype = otype;
+ datum->next = NULL;
+
+ if (last) {
+ last->next = datum;
+ } else {
+ ft = malloc(sizeof(*ft));
+ if (!ft) {
+ free(datum);
+ return SEPOL_ENOMEM;
+ }
+
+ ft->ttype = ttype;
+ ft->tclass = tclass;
+ ft->name = *name_alloc;
+
+ if (hashtab_insert(p->filename_trans, (hashtab_key_t)ft,
+ (hashtab_datum_t)datum)) {
+ free(datum);
+ free(ft);
+ return SEPOL_ENOMEM;
+ }
+ *name_alloc = NULL;
+ }
+ }
+
+ p->filename_trans_count++;
+ return ebitmap_set_bit(&datum->stypes, stype - 1, 1);
+}
+
int filename_trans_read(policydb_t *p, struct policy_file *fp)
{
unsigned int i;
- uint32_t buf[4], nel, len;
- filename_trans_t *ft;
- filename_trans_datum_t *otype;
+ uint32_t buf[4], nel, len, stype, ttype, tclass, otype;
int rc;
char *name;
@@ -2610,16 +2674,8 @@ int filename_trans_read(policydb_t *p, struct policy_file *fp)
nel = le32_to_cpu(buf[0]);
for (i = 0; i < nel; i++) {
- ft = NULL;
- otype = NULL;
name = NULL;
- ft = calloc(1, sizeof(*ft));
- if (!ft)
- goto err;
- otype = calloc(1, sizeof(*otype));
- if (!otype)
- goto err;
rc = next_entry(buf, fp, sizeof(uint32_t));
if (rc < 0)
goto err;
@@ -2631,8 +2687,6 @@ int filename_trans_read(policydb_t *p, struct policy_file *fp)
if (!name)
goto err;
- ft->name = name;
-
rc = next_entry(name, fp, len);
if (rc < 0)
goto err;
@@ -2641,13 +2695,13 @@ int filename_trans_read(policydb_t *p, struct policy_file *fp)
if (rc < 0)
goto err;
- ft->stype = le32_to_cpu(buf[0]);
- ft->ttype = le32_to_cpu(buf[1]);
- ft->tclass = le32_to_cpu(buf[2]);
- otype->otype = le32_to_cpu(buf[3]);
+ stype = le32_to_cpu(buf[0]);
+ ttype = le32_to_cpu(buf[1]);
+ tclass = le32_to_cpu(buf[2]);
+ otype = le32_to_cpu(buf[3]);
- rc = hashtab_insert(p->filename_trans, (hashtab_key_t) ft,
- otype);
+ rc = policydb_filetrans_insert(p, stype, ttype, tclass, name,
+ &name, otype, NULL);
if (rc) {
if (rc != SEPOL_EEXIST)
goto err;
@@ -2659,21 +2713,17 @@ int filename_trans_read(policydb_t *p, struct policy_file *fp)
*/
WARN(fp->handle,
"Duplicate name-based type_transition %s %s:%s \"%s\": %s, ignoring",
- p->p_type_val_to_name[ft->stype - 1],
- p->p_type_val_to_name[ft->ttype - 1],
- p->p_class_val_to_name[ft->tclass - 1],
- ft->name,
- p->p_type_val_to_name[otype->otype - 1]);
- free(ft);
- free(name);
- free(otype);
+ p->p_type_val_to_name[stype - 1],
+ p->p_type_val_to_name[ttype - 1],
+ p->p_class_val_to_name[tclass - 1],
+ name,
+ p->p_type_val_to_name[otype - 1]);
/* continue, ignoring this one */
}
+ free(name);
}
return 0;
err:
- free(ft);
- free(otype);
free(name);
return -1;
}
diff --git a/libsepol/src/write.c b/libsepol/src/write.c
index 1fd6a16a..d3aee8d5 100644
--- a/libsepol/src/write.c
+++ b/libsepol/src/write.c
@@ -571,44 +571,50 @@ static int role_allow_write(role_allow_t * r, struct policy_file *fp)
static int filename_write_helper(hashtab_key_t key, void *data, void *ptr)
{
- uint32_t buf[4];
+ uint32_t bit, buf[4];
size_t items, len;
- struct filename_trans *ft = (struct filename_trans *)key;
- struct filename_trans_datum *otype = data;
+ filename_trans_key_t *ft = (filename_trans_key_t *)key;
+ filename_trans_datum_t *datum = data;
+ ebitmap_node_t *node;
void *fp = ptr;
len = strlen(ft->name);
- buf[0] = cpu_to_le32(len);
- items = put_entry(buf, sizeof(uint32_t), 1, fp);
- if (items != 1)
- return POLICYDB_ERROR;
+ do {
+ ebitmap_for_each_positive_bit(&datum->stypes, node, bit) {
+ buf[0] = cpu_to_le32(len);
+ items = put_entry(buf, sizeof(uint32_t), 1, fp);
+ if (items != 1)
+ return POLICYDB_ERROR;
- items = put_entry(ft->name, sizeof(char), len, fp);
- if (items != len)
- return POLICYDB_ERROR;
+ items = put_entry(ft->name, sizeof(char), len, fp);
+ if (items != len)
+ return POLICYDB_ERROR;
- buf[0] = cpu_to_le32(ft->stype);
- buf[1] = cpu_to_le32(ft->ttype);
- buf[2] = cpu_to_le32(ft->tclass);
- buf[3] = cpu_to_le32(otype->otype);
- items = put_entry(buf, sizeof(uint32_t), 4, fp);
- if (items != 4)
- return POLICYDB_ERROR;
+ buf[0] = cpu_to_le32(bit + 1);
+ buf[1] = cpu_to_le32(ft->ttype);
+ buf[2] = cpu_to_le32(ft->tclass);
+ buf[3] = cpu_to_le32(datum->otype);
+ items = put_entry(buf, sizeof(uint32_t), 4, fp);
+ if (items != 4)
+ return POLICYDB_ERROR;
+ }
+
+ datum = datum->next;
+ } while (datum);
return 0;
}
static int filename_trans_write(struct policydb *p, void *fp)
{
- size_t nel, items;
+ size_t items;
uint32_t buf[1];
int rc;
if (p->policyvers < POLICYDB_VERSION_FILENAME_TRANS)
return 0;
- nel = p->filename_trans->nel;
- buf[0] = cpu_to_le32(nel);
+ buf[0] = cpu_to_le32(p->filename_trans_count);
items = put_entry(buf, sizeof(uint32_t), 1, fp);
if (items != 1)
return POLICYDB_ERROR;
--
2.25.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 2/2] libsepol: implement POLICYDB_VERSION_COMP_FTRANS
2020-03-28 12:45 [PATCH v2 0/2] userspace: Implement new format of filename trans rules Ondrej Mosnacek
2020-03-28 12:45 ` [PATCH v2 1/2] libsepol,checkpolicy: optimize storage of filename transitions Ondrej Mosnacek
@ 2020-03-28 12:45 ` Ondrej Mosnacek
2020-03-31 20:30 ` James Carter
1 sibling, 1 reply; 12+ messages in thread
From: Ondrej Mosnacek @ 2020-03-28 12:45 UTC (permalink / raw)
To: selinux; +Cc: Stephen Smalley, Chris PeBenito
Implement a new, more space-efficient form 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 representation 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.
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
libsepol/include/sepol/policydb/policydb.h | 3 +-
libsepol/src/policydb.c | 218 +++++++++++++++++----
libsepol/src/write.c | 72 ++++++-
3 files changed, 242 insertions(+), 51 deletions(-)
diff --git a/libsepol/include/sepol/policydb/policydb.h b/libsepol/include/sepol/policydb/policydb.h
index c3180c61..9ef43abc 100644
--- a/libsepol/include/sepol/policydb/policydb.h
+++ b/libsepol/include/sepol/policydb/policydb.h
@@ -755,10 +755,11 @@ extern int policydb_set_target_platform(policydb_t *p, int platform);
#define POLICYDB_VERSION_XPERMS_IOCTL 30 /* Linux-specific */
#define POLICYDB_VERSION_INFINIBAND 31 /* Linux-specific */
#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
/* Module versions and specific changes*/
#define MOD_POLICYDB_VERSION_BASE 4
diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c
index 6b121d66..18ce1912 100644
--- a/libsepol/src/policydb.c
+++ b/libsepol/src/policydb.c
@@ -200,6 +200,13 @@ static struct policydb_compat_info policydb_compat[] = {
.ocon_num = OCON_IBENDPORT + 1,
.target_platform = SEPOL_TARGET_SELINUX,
},
+ {
+ .type = POLICY_KERN,
+ .version = POLICYDB_VERSION_COMP_FTRANS,
+ .sym_num = SYM_NUM,
+ .ocon_num = OCON_IBENDPORT + 1,
+ .target_platform = SEPOL_TARGET_SELINUX,
+ },
{
.type = POLICY_BASE,
.version = MOD_POLICYDB_VERSION_BASE,
@@ -2661,73 +2668,202 @@ int policydb_filetrans_insert(policydb_t *p, uint32_t stype, uint32_t ttype,
return ebitmap_set_bit(&datum->stypes, stype - 1, 1);
}
-int filename_trans_read(policydb_t *p, struct policy_file *fp)
+static int filename_trans_read_one_old(policydb_t *p, struct policy_file *fp)
{
- unsigned int i;
- uint32_t buf[4], nel, len, stype, ttype, tclass, otype;
+ uint32_t buf[4], len, stype, ttype, tclass, otype;
+ char *name = NULL;
int rc;
- char *name;
rc = next_entry(buf, fp, sizeof(uint32_t));
if (rc < 0)
return -1;
- nel = le32_to_cpu(buf[0]);
+ len = le32_to_cpu(buf[0]);
+ if (zero_or_saturated(len))
+ return -1;
- for (i = 0; i < nel; i++) {
- name = NULL;
+ name = calloc(len + 1, sizeof(*name));
+ if (!name)
+ return -1;
- rc = next_entry(buf, fp, sizeof(uint32_t));
- if (rc < 0)
- goto err;
- len = le32_to_cpu(buf[0]);
- if (zero_or_saturated(len))
+ rc = next_entry(name, fp, len);
+ if (rc < 0)
+ goto err;
+
+ rc = next_entry(buf, fp, sizeof(uint32_t) * 4);
+ if (rc < 0)
+ goto err;
+
+ stype = le32_to_cpu(buf[0]);
+ ttype = le32_to_cpu(buf[1]);
+ tclass = le32_to_cpu(buf[2]);
+ otype = le32_to_cpu(buf[3]);
+
+ rc = policydb_filetrans_insert(p, stype, ttype, tclass, name, &name,
+ otype, NULL);
+ if (rc) {
+ if (rc != SEPOL_EEXIST)
goto err;
+ /*
+ * Some old policies were wrongly generated with
+ * duplicate filename transition rules. For backward
+ * compatibility, do not reject such policies, just
+ * issue a warning and ignore the duplicate.
+ */
+ WARN(fp->handle,
+ "Duplicate name-based type_transition %s %s:%s \"%s\": %s, ignoring",
+ p->p_type_val_to_name[stype - 1],
+ p->p_type_val_to_name[ttype - 1],
+ p->p_class_val_to_name[tclass - 1],
+ name,
+ p->p_type_val_to_name[otype - 1]);
+ /* continue, ignoring this one */
+ }
+ free(name);
+ return 0;
+err:
+ free(name);
+ return -1;
+}
+
+static int filename_trans_check_datum(filename_trans_datum_t *datum)
+{
+ ebitmap_t stypes, otypes;
+
+ ebitmap_init(&stypes);
+ ebitmap_init(&otypes);
+
+ while (datum) {
+ if (ebitmap_get_bit(&otypes, datum->otype))
+ return -1;
+
+ if (ebitmap_set_bit(&otypes, datum->otype, 1))
+ return -1;
- name = calloc(len + 1, sizeof(*name));
- if (!name)
+ if (ebitmap_match_any(&stypes, &datum->stypes))
+ return -1;
+
+ if (ebitmap_union(&stypes, &datum->stypes))
+ return -1;
+ }
+ return 0;
+}
+
+static int filename_trans_read_one_new(policydb_t *p, struct policy_file *fp)
+{
+ filename_trans_key_t *ft = NULL;
+ filename_trans_datum_t **dst, *datum, *first = NULL;
+ unsigned int i;
+ uint32_t buf[3], len, ttype, tclass, ndatum;
+ char *name = NULL;
+ int rc;
+
+ rc = next_entry(buf, fp, sizeof(uint32_t));
+ if (rc < 0)
+ return -1;
+ len = le32_to_cpu(buf[0]);
+ if (zero_or_saturated(len))
+ return -1;
+
+ name = calloc(len + 1, sizeof(*name));
+ if (!name)
+ return -1;
+
+ rc = next_entry(name, fp, len);
+ if (rc < 0)
+ goto err;
+
+ rc = next_entry(buf, fp, sizeof(uint32_t) * 3);
+ if (rc < 0)
+ goto err;
+
+ ttype = le32_to_cpu(buf[0]);
+ tclass = le32_to_cpu(buf[1]);
+ ndatum = le32_to_cpu(buf[2]);
+ if (ndatum == 0)
+ goto err;
+
+ dst = &first;
+ for (i = 0; i < ndatum; i++) {
+ datum = malloc(sizeof(*datum));
+ if (!datum)
goto err;
- rc = next_entry(name, fp, len);
+ *dst = datum;
+
+ /* ebitmap_read() will at least init the bitmap */
+ rc = ebitmap_read(&datum->stypes, fp);
if (rc < 0)
goto err;
- rc = next_entry(buf, fp, sizeof(uint32_t) * 4);
+ rc = next_entry(buf, fp, sizeof(uint32_t));
if (rc < 0)
goto err;
- stype = le32_to_cpu(buf[0]);
- ttype = le32_to_cpu(buf[1]);
- tclass = le32_to_cpu(buf[2]);
- otype = le32_to_cpu(buf[3]);
+ datum->otype = le32_to_cpu(buf[0]);
- rc = policydb_filetrans_insert(p, stype, ttype, tclass, name,
- &name, otype, NULL);
- if (rc) {
- if (rc != SEPOL_EEXIST)
- goto err;
- /*
- * Some old policies were wrongly generated with
- * duplicate filename transition rules. For backward
- * compatibility, do not reject such policies, just
- * issue a warning and ignore the duplicate.
- */
- WARN(fp->handle,
- "Duplicate name-based type_transition %s %s:%s \"%s\": %s, ignoring",
- p->p_type_val_to_name[stype - 1],
- p->p_type_val_to_name[ttype - 1],
- p->p_class_val_to_name[tclass - 1],
- name,
- p->p_type_val_to_name[otype - 1]);
- /* continue, ignoring this one */
- }
- free(name);
+ p->filename_trans_count += ebitmap_cardinality(&datum->stypes);
+
+ dst = &datum->next;
}
+ *dst = NULL;
+
+ if (ndatum > 1 && filename_trans_check_datum(first))
+ goto err;
+
+ ft = malloc(sizeof(*ft));
+ if (!ft)
+ goto err;
+
+ ft->ttype = ttype;
+ ft->tclass = tclass;
+ ft->name = name;
+
+ rc = hashtab_insert(p->filename_trans, (hashtab_key_t)ft,
+ (hashtab_datum_t)first);
+ if (rc)
+ goto err;
+
return 0;
err:
+ free(ft);
free(name);
+ while (first) {
+ datum = first;
+ first = first->next;
+
+ ebitmap_destroy(&datum->stypes);
+ free(datum);
+ }
return -1;
}
+int filename_trans_read(policydb_t *p, struct policy_file *fp)
+{
+ unsigned int i;
+ uint32_t buf[1], nel;
+ int rc;
+
+ rc = next_entry(buf, fp, sizeof(uint32_t));
+ if (rc < 0)
+ return -1;
+ nel = le32_to_cpu(buf[0]);
+
+ if (p->policyvers < POLICYDB_VERSION_COMP_FTRANS) {
+ for (i = 0; i < nel; i++) {
+ rc = filename_trans_read_one_old(p, fp);
+ if (rc < 0)
+ return -1;
+ }
+ } else {
+ for (i = 0; i < nel; i++) {
+ rc = filename_trans_read_one_new(p, fp);
+ if (rc < 0)
+ return -1;
+ }
+ }
+ return 0;
+}
+
static int ocontext_read_xen(struct policydb_compat_info *info,
policydb_t *p, struct policy_file *fp)
{
diff --git a/libsepol/src/write.c b/libsepol/src/write.c
index d3aee8d5..9c74d9f5 100644
--- a/libsepol/src/write.c
+++ b/libsepol/src/write.c
@@ -569,7 +569,7 @@ static int role_allow_write(role_allow_t * r, struct policy_file *fp)
return POLICYDB_SUCCESS;
}
-static int filename_write_helper(hashtab_key_t key, void *data, void *ptr)
+static int filename_write_one_old(hashtab_key_t key, void *data, void *ptr)
{
uint32_t bit, buf[4];
size_t items, len;
@@ -605,6 +605,54 @@ static int filename_write_helper(hashtab_key_t key, void *data, void *ptr)
return 0;
}
+static int filename_write_one_new(hashtab_key_t key, void *data, void *ptr)
+{
+ uint32_t buf[3];
+ size_t items, len, ndatum;
+ filename_trans_key_t *ft = (filename_trans_key_t *)key;
+ filename_trans_datum_t *datum;
+ void *fp = ptr;
+
+ len = strlen(ft->name);
+ buf[0] = cpu_to_le32(len);
+ items = put_entry(buf, sizeof(uint32_t), 1, fp);
+ if (items != 1)
+ return POLICYDB_ERROR;
+
+ items = put_entry(ft->name, sizeof(char), len, fp);
+ if (items != len)
+ return POLICYDB_ERROR;
+
+ ndatum = 0;
+ datum = data;
+ do {
+ ndatum++;
+ datum = datum->next;
+ } while (datum);
+
+ buf[0] = cpu_to_le32(ft->ttype);
+ buf[1] = cpu_to_le32(ft->tclass);
+ buf[2] = cpu_to_le32(ndatum);
+ items = put_entry(buf, sizeof(uint32_t), 3, fp);
+ if (items != 3)
+ return POLICYDB_ERROR;
+
+ datum = data;
+ do {
+ if (ebitmap_write(&datum->stypes, fp))
+ return POLICYDB_ERROR;
+
+ buf[0] = cpu_to_le32(datum->otype);
+ items = put_entry(buf, sizeof(uint32_t), 1, fp);
+ if (items != 1)
+ return POLICYDB_ERROR;
+
+ datum = datum->next;
+ } while (datum);
+
+ return 0;
+}
+
static int filename_trans_write(struct policydb *p, void *fp)
{
size_t items;
@@ -614,16 +662,22 @@ static int filename_trans_write(struct policydb *p, void *fp)
if (p->policyvers < POLICYDB_VERSION_FILENAME_TRANS)
return 0;
- buf[0] = cpu_to_le32(p->filename_trans_count);
- items = put_entry(buf, sizeof(uint32_t), 1, fp);
- if (items != 1)
- return POLICYDB_ERROR;
+ if (p->policyvers < POLICYDB_VERSION_COMP_FTRANS) {
+ buf[0] = cpu_to_le32(p->filename_trans_count);
+ items = put_entry(buf, sizeof(uint32_t), 1, fp);
+ if (items != 1)
+ return POLICYDB_ERROR;
- rc = hashtab_map(p->filename_trans, filename_write_helper, 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);
+ items = put_entry(buf, sizeof(uint32_t), 1, fp);
+ if (items != 1)
+ return POLICYDB_ERROR;
- return 0;
+ rc = hashtab_map(p->filename_trans, filename_write_one_new, fp);
+ }
+ return rc;
}
static int role_set_write(role_set_t * x, struct policy_file *fp)
--
2.25.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] libsepol,checkpolicy: optimize storage of filename transitions
2020-03-28 12:45 ` [PATCH v2 1/2] libsepol,checkpolicy: optimize storage of filename transitions Ondrej Mosnacek
@ 2020-03-31 20:30 ` James Carter
2020-04-30 17:34 ` James Carter
1 sibling, 0 replies; 12+ messages in thread
From: James Carter @ 2020-03-31 20:30 UTC (permalink / raw)
To: Ondrej Mosnacek; +Cc: SElinux list, Stephen Smalley, Chris PeBenito
On Sat, Mar 28, 2020 at 8:46 AM Ondrej Mosnacek <omosnace@redhat.com> 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@redhat.com>
Acked-by: James Carter <jwcart2@gmail.com>
> ---
> 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);
> +}
> +
>
> static void display_filename_trans(policydb_t *p, FILE *fp)
> {
> diff --git a/libsepol/cil/src/cil_binary.c b/libsepol/cil/src/cil_binary.c
> index 62178d99..bedff628 100644
> --- a/libsepol/cil/src/cil_binary.c
> +++ b/libsepol/cil/src/cil_binary.c
> @@ -1131,13 +1131,13 @@ int __cil_typetransition_to_avtab(policydb_t *pdb, const struct cil_db *db, stru
> class_datum_t *sepol_obj = NULL;
> struct cil_list *class_list;
> type_datum_t *sepol_result = NULL;
> - filename_trans_t *newkey = NULL;
> - filename_trans_datum_t *newdatum = NULL, *otype = NULL;
> ebitmap_t src_bitmap, tgt_bitmap;
> ebitmap_node_t *node1, *node2;
> unsigned int i, j;
> + uint32_t otype;
> struct cil_list_item *c;
> char *name = DATUM(typetrans->name)->name;
> + char *dup_name;
>
> if (name == CIL_KEY_STAR) {
> struct cil_type_rule trans;
> @@ -1176,22 +1176,16 @@ int __cil_typetransition_to_avtab(policydb_t *pdb, const struct cil_db *db, stru
> rc = __cil_get_sepol_class_datum(pdb, DATUM(c->data), &sepol_obj);
> if (rc != SEPOL_OK) goto exit;
>
> - newkey = cil_calloc(1, sizeof(*newkey));
> - newdatum = cil_calloc(1, sizeof(*newdatum));
> - newkey->stype = sepol_src->s.value;
> - newkey->ttype = sepol_tgt->s.value;
> - newkey->tclass = sepol_obj->s.value;
> - newkey->name = cil_strdup(name);
> - newdatum->otype = sepol_result->s.value;
> -
> - rc = hashtab_insert(pdb->filename_trans,
> - (hashtab_key_t)newkey,
> - newdatum);
> + dup_name = NULL;
> + rc = policydb_filetrans_insert(
> + pdb, sepol_src->s.value, sepol_tgt->s.value,
> + sepol_obj->s.value, name, &dup_name,
> + sepol_result->s.value, &otype
> + );
> + free(dup_name);
> if (rc != SEPOL_OK) {
> if (rc == SEPOL_EEXIST) {
> - otype = hashtab_search(pdb->filename_trans,
> - (hashtab_key_t)newkey);
> - if (newdatum->otype != otype->otype) {
> + if (sepol_result->s.value!= otype) {
> cil_log(CIL_ERR, "Conflicting name type transition rules\n");
> } else {
> rc = SEPOL_OK;
> @@ -1199,9 +1193,6 @@ int __cil_typetransition_to_avtab(policydb_t *pdb, const struct cil_db *db, stru
> } else {
> cil_log(CIL_ERR, "Out of memory\n");
> }
> - free(newkey->name);
> - free(newkey);
> - free(newdatum);
> if (rc != SEPOL_OK) {
> goto exit;
> }
> diff --git a/libsepol/include/sepol/policydb/policydb.h b/libsepol/include/sepol/policydb/policydb.h
> index 81b63fef..c3180c61 100644
> --- a/libsepol/include/sepol/policydb/policydb.h
> +++ b/libsepol/include/sepol/policydb/policydb.h
> @@ -162,15 +162,16 @@ typedef struct role_allow {
> } role_allow_t;
>
> /* filename_trans rules */
> -typedef struct filename_trans {
> - uint32_t stype;
> +typedef struct filename_trans_key {
> uint32_t ttype;
> uint32_t tclass;
> char *name;
> -} filename_trans_t;
> +} filename_trans_key_t;
>
> typedef struct filename_trans_datum {
> - uint32_t otype; /* expected of new object */
> + ebitmap_t stypes;
> + uint32_t otype;
> + struct filename_trans_datum *next;
> } filename_trans_datum_t;
>
> /* Type attributes */
> @@ -591,6 +592,7 @@ typedef struct policydb {
>
> /* file transitions with the last path component */
> hashtab_t filename_trans;
> + uint32_t filename_trans_count;
>
> ebitmap_t *type_attr_map;
>
> @@ -650,6 +652,11 @@ extern int policydb_load_isids(policydb_t * p, sidtab_t * s);
>
> extern int policydb_sort_ocontexts(policydb_t *p);
>
> +extern int policydb_filetrans_insert(policydb_t *p, uint32_t stype,
> + uint32_t ttype, uint32_t tclass,
> + const char *name, char **name_alloc,
> + uint32_t otype, uint32_t *present_otype);
> +
> /* Deprecated */
> extern int policydb_context_isvalid(const policydb_t * p,
> const context_struct_t * c);
> diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c
> index 529e1d35..28f93acb 100644
> --- a/libsepol/src/expand.c
> +++ b/libsepol/src/expand.c
> @@ -1371,16 +1371,15 @@ static int copy_role_trans(expand_state_t * state, role_trans_rule_t * rules)
> static int expand_filename_trans(expand_state_t *state, filename_trans_rule_t *rules)
> {
> unsigned int i, j;
> - filename_trans_t key, *new_trans;
> - filename_trans_datum_t *otype;
> filename_trans_rule_t *cur_rule;
> ebitmap_t stypes, ttypes;
> ebitmap_node_t *snode, *tnode;
> + char *name;
> int rc;
>
> cur_rule = rules;
> while (cur_rule) {
> - uint32_t mapped_otype;
> + uint32_t mapped_otype, present_otype;
>
> ebitmap_init(&stypes);
> ebitmap_init(&ttypes);
> @@ -1401,15 +1400,17 @@ static int expand_filename_trans(expand_state_t *state, filename_trans_rule_t *r
>
> ebitmap_for_each_positive_bit(&stypes, snode, i) {
> ebitmap_for_each_positive_bit(&ttypes, tnode, j) {
> - key.stype = i + 1;
> - key.ttype = j + 1;
> - key.tclass = cur_rule->tclass;
> - key.name = cur_rule->name;
> - otype = hashtab_search(state->out->filename_trans,
> - (hashtab_key_t) &key);
> - if (otype) {
> + name = NULL;
> +
> + rc = policydb_filetrans_insert(
> + state->out, i + 1, j + 1,
> + cur_rule->tclass, cur_rule->name,
> + &name, mapped_otype, &present_otype
> + );
> + free(name);
> + if (rc == SEPOL_EEXIST) {
> /* duplicate rule, ignore */
> - if (otype->otype == mapped_otype)
> + if (present_otype == mapped_otype)
> continue;
>
> ERR(state->handle, "Conflicting name-based type_transition %s %s:%s \"%s\": %s vs %s",
> @@ -1417,44 +1418,11 @@ static int expand_filename_trans(expand_state_t *state, filename_trans_rule_t *r
> state->out->p_type_val_to_name[j],
> state->out->p_class_val_to_name[cur_rule->tclass - 1],
> cur_rule->name,
> - state->out->p_type_val_to_name[otype->otype - 1],
> + state->out->p_type_val_to_name[present_otype - 1],
> state->out->p_type_val_to_name[mapped_otype - 1]);
> return -1;
> - }
> -
> - new_trans = calloc(1, sizeof(*new_trans));
> - if (!new_trans) {
> - ERR(state->handle, "Out of memory!");
> - return -1;
> - }
> -
> - new_trans->name = strdup(cur_rule->name);
> - if (!new_trans->name) {
> - ERR(state->handle, "Out of memory!");
> - free(new_trans);
> - return -1;
> - }
> - new_trans->stype = i + 1;
> - new_trans->ttype = j + 1;
> - new_trans->tclass = cur_rule->tclass;
> -
> - otype = calloc(1, sizeof(*otype));
> - if (!otype) {
> - ERR(state->handle, "Out of memory!");
> - free(new_trans->name);
> - free(new_trans);
> - return -1;
> - }
> - otype->otype = mapped_otype;
> -
> - rc = hashtab_insert(state->out->filename_trans,
> - (hashtab_key_t)new_trans,
> - otype);
> - if (rc) {
> + } else if (rc < 0) {
> ERR(state->handle, "Out of memory!");
> - free(otype);
> - free(new_trans->name);
> - free(new_trans);
> return -1;
> }
> }
> diff --git a/libsepol/src/kernel_to_cil.c b/libsepol/src/kernel_to_cil.c
> index ede78a20..718d3481 100644
> --- a/libsepol/src/kernel_to_cil.c
> +++ b/libsepol/src/kernel_to_cil.c
> @@ -1821,21 +1821,35 @@ struct map_filename_trans_args {
>
> static int map_filename_trans_to_str(hashtab_key_t key, void *data, void *arg)
> {
> - filename_trans_t *ft = (filename_trans_t *)key;
> + filename_trans_key_t *ft = (filename_trans_key_t *)key;
> filename_trans_datum_t *datum = data;
> struct map_filename_trans_args *map_args = arg;
> struct policydb *pdb = map_args->pdb;
> struct strs *strs = map_args->strs;
> char *src, *tgt, *class, *filename, *new;
> + struct ebitmap_node *node;
> + uint32_t bit;
> + int rc;
>
> - src = pdb->p_type_val_to_name[ft->stype - 1];
> tgt = pdb->p_type_val_to_name[ft->ttype - 1];
> class = pdb->p_class_val_to_name[ft->tclass - 1];
> filename = ft->name;
> - new = pdb->p_type_val_to_name[datum->otype - 1];
> + do {
> + new = pdb->p_type_val_to_name[datum->otype - 1];
> +
> + ebitmap_for_each_positive_bit(&datum->stypes, node, bit) {
> + src = pdb->p_type_val_to_name[bit];
> + rc = strs_create_and_add(strs,
> + "(typetransition %s %s %s %s %s)",
> + 5, src, tgt, class, filename, new);
> + if (rc)
> + return rc;
> + }
> +
> + datum = datum->next;
> + } while (datum);
>
> - return strs_create_and_add(strs, "(typetransition %s %s %s %s %s)", 5,
> - src, tgt, class, filename, new);
> + return 0;
> }
>
> static int write_filename_trans_rules_to_cil(FILE *out, struct policydb *pdb)
> diff --git a/libsepol/src/kernel_to_conf.c b/libsepol/src/kernel_to_conf.c
> index 9de64832..9fb3ed22 100644
> --- a/libsepol/src/kernel_to_conf.c
> +++ b/libsepol/src/kernel_to_conf.c
> @@ -1801,21 +1801,35 @@ struct map_filename_trans_args {
>
> static int map_filename_trans_to_str(hashtab_key_t key, void *data, void *arg)
> {
> - filename_trans_t *ft = (filename_trans_t *)key;
> + filename_trans_key_t *ft = (filename_trans_key_t *)key;
> filename_trans_datum_t *datum = data;
> struct map_filename_trans_args *map_args = arg;
> struct policydb *pdb = map_args->pdb;
> struct strs *strs = map_args->strs;
> char *src, *tgt, *class, *filename, *new;
> + struct ebitmap_node *node;
> + uint32_t bit;
> + int rc;
>
> - src = pdb->p_type_val_to_name[ft->stype - 1];
> tgt = pdb->p_type_val_to_name[ft->ttype - 1];
> class = pdb->p_class_val_to_name[ft->tclass - 1];
> filename = ft->name;
> - new = pdb->p_type_val_to_name[datum->otype - 1];
> + do {
> + new = pdb->p_type_val_to_name[datum->otype - 1];
> +
> + ebitmap_for_each_positive_bit(&datum->stypes, node, bit) {
> + src = pdb->p_type_val_to_name[bit];
> + rc = strs_create_and_add(strs,
> + "type_transition %s %s:%s %s \"%s\";",
> + 5, src, tgt, class, new, filename);
> + if (rc)
> + return rc;
> + }
> +
> + datum = datum->next;
> + } while (datum);
>
> - return strs_create_and_add(strs, "type_transition %s %s:%s %s \"%s\";", 5,
> - src, tgt, class, new, filename);
> + return 0;
> }
>
> static int write_filename_trans_rules_to_conf(FILE *out, struct policydb *pdb)
> diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c
> index 5b289a52..6b121d66 100644
> --- a/libsepol/src/policydb.c
> +++ b/libsepol/src/policydb.c
> @@ -789,12 +789,12 @@ partial_name_hash(unsigned long c, unsigned long prevhash)
>
> static unsigned int filenametr_hash(hashtab_t h, const_hashtab_key_t k)
> {
> - const struct filename_trans *ft = (const struct filename_trans *)k;
> + const filename_trans_key_t *ft = (const filename_trans_key_t *)k;
> unsigned long hash;
> unsigned int byte_num;
> unsigned char focus;
>
> - hash = ft->stype ^ ft->ttype ^ ft->tclass;
> + hash = ft->ttype ^ ft->tclass;
>
> byte_num = 0;
> while ((focus = ft->name[byte_num++]))
> @@ -805,14 +805,10 @@ static unsigned int filenametr_hash(hashtab_t h, const_hashtab_key_t k)
> static int filenametr_cmp(hashtab_t h __attribute__ ((unused)),
> const_hashtab_key_t k1, const_hashtab_key_t k2)
> {
> - const struct filename_trans *ft1 = (const struct filename_trans *)k1;
> - const struct filename_trans *ft2 = (const struct filename_trans *)k2;
> + const filename_trans_key_t *ft1 = (const filename_trans_key_t *)k1;
> + const filename_trans_key_t *ft2 = (const filename_trans_key_t *)k2;
> int v;
>
> - v = ft1->stype - ft2->stype;
> - if (v)
> - return v;
> -
> v = ft1->ttype - ft2->ttype;
> if (v)
> return v;
> @@ -1409,9 +1405,12 @@ common_destroy, class_destroy, role_destroy, type_destroy, user_destroy,
> static int filenametr_destroy(hashtab_key_t key, hashtab_datum_t datum,
> void *p __attribute__ ((unused)))
> {
> - struct filename_trans *ft = (struct filename_trans *)key;
> + filename_trans_key_t *ft = (filename_trans_key_t *)key;
> + filename_trans_datum_t *fd = datum;
> +
> free(ft->name);
> free(key);
> + ebitmap_destroy(&fd->stypes);
> free(datum);
> return 0;
> }
> @@ -2595,12 +2594,77 @@ int role_allow_read(role_allow_t ** r, struct policy_file *fp)
> return 0;
> }
>
> +int policydb_filetrans_insert(policydb_t *p, uint32_t stype, uint32_t ttype,
> + uint32_t tclass, const char *name,
> + char **name_alloc, uint32_t otype,
> + uint32_t *present_otype)
> +{
> + filename_trans_key_t *ft, key;
> + filename_trans_datum_t *datum, *last;
> +
> + key.ttype = ttype;
> + key.tclass = tclass;
> + key.name = (char *)name;
> +
> + last = NULL;
> + datum = hashtab_search(p->filename_trans, (hashtab_key_t)&key);
> + while (datum) {
> + if (ebitmap_get_bit(&datum->stypes, stype - 1)) {
> + if (present_otype)
> + *present_otype = datum->otype;
> + return SEPOL_EEXIST;
> + }
> + if (datum->otype == otype)
> + break;
> + last = datum;
> + datum = datum->next;
> + }
> + if (!datum) {
> + if (!*name_alloc) {
> + *name_alloc = strdup(name);
> + if (!*name_alloc)
> + return SEPOL_ENOMEM;
> + }
> +
> + datum = malloc(sizeof(*datum));
> + if (!datum)
> + return SEPOL_ENOMEM;
> +
> + ebitmap_init(&datum->stypes);
> + datum->otype = otype;
> + datum->next = NULL;
> +
> + if (last) {
> + last->next = datum;
> + } else {
> + ft = malloc(sizeof(*ft));
> + if (!ft) {
> + free(datum);
> + return SEPOL_ENOMEM;
> + }
> +
> + ft->ttype = ttype;
> + ft->tclass = tclass;
> + ft->name = *name_alloc;
> +
> + if (hashtab_insert(p->filename_trans, (hashtab_key_t)ft,
> + (hashtab_datum_t)datum)) {
> + free(datum);
> + free(ft);
> + return SEPOL_ENOMEM;
> + }
> + *name_alloc = NULL;
> + }
> + }
> +
> + p->filename_trans_count++;
> + return ebitmap_set_bit(&datum->stypes, stype - 1, 1);
> +}
> +
> int filename_trans_read(policydb_t *p, struct policy_file *fp)
> {
> unsigned int i;
> - uint32_t buf[4], nel, len;
> - filename_trans_t *ft;
> - filename_trans_datum_t *otype;
> + uint32_t buf[4], nel, len, stype, ttype, tclass, otype;
> int rc;
> char *name;
>
> @@ -2610,16 +2674,8 @@ int filename_trans_read(policydb_t *p, struct policy_file *fp)
> nel = le32_to_cpu(buf[0]);
>
> for (i = 0; i < nel; i++) {
> - ft = NULL;
> - otype = NULL;
> name = NULL;
>
> - ft = calloc(1, sizeof(*ft));
> - if (!ft)
> - goto err;
> - otype = calloc(1, sizeof(*otype));
> - if (!otype)
> - goto err;
> rc = next_entry(buf, fp, sizeof(uint32_t));
> if (rc < 0)
> goto err;
> @@ -2631,8 +2687,6 @@ int filename_trans_read(policydb_t *p, struct policy_file *fp)
> if (!name)
> goto err;
>
> - ft->name = name;
> -
> rc = next_entry(name, fp, len);
> if (rc < 0)
> goto err;
> @@ -2641,13 +2695,13 @@ int filename_trans_read(policydb_t *p, struct policy_file *fp)
> if (rc < 0)
> goto err;
>
> - ft->stype = le32_to_cpu(buf[0]);
> - ft->ttype = le32_to_cpu(buf[1]);
> - ft->tclass = le32_to_cpu(buf[2]);
> - otype->otype = le32_to_cpu(buf[3]);
> + stype = le32_to_cpu(buf[0]);
> + ttype = le32_to_cpu(buf[1]);
> + tclass = le32_to_cpu(buf[2]);
> + otype = le32_to_cpu(buf[3]);
>
> - rc = hashtab_insert(p->filename_trans, (hashtab_key_t) ft,
> - otype);
> + rc = policydb_filetrans_insert(p, stype, ttype, tclass, name,
> + &name, otype, NULL);
> if (rc) {
> if (rc != SEPOL_EEXIST)
> goto err;
> @@ -2659,21 +2713,17 @@ int filename_trans_read(policydb_t *p, struct policy_file *fp)
> */
> WARN(fp->handle,
> "Duplicate name-based type_transition %s %s:%s \"%s\": %s, ignoring",
> - p->p_type_val_to_name[ft->stype - 1],
> - p->p_type_val_to_name[ft->ttype - 1],
> - p->p_class_val_to_name[ft->tclass - 1],
> - ft->name,
> - p->p_type_val_to_name[otype->otype - 1]);
> - free(ft);
> - free(name);
> - free(otype);
> + p->p_type_val_to_name[stype - 1],
> + p->p_type_val_to_name[ttype - 1],
> + p->p_class_val_to_name[tclass - 1],
> + name,
> + p->p_type_val_to_name[otype - 1]);
> /* continue, ignoring this one */
> }
> + free(name);
> }
> return 0;
> err:
> - free(ft);
> - free(otype);
> free(name);
> return -1;
> }
> diff --git a/libsepol/src/write.c b/libsepol/src/write.c
> index 1fd6a16a..d3aee8d5 100644
> --- a/libsepol/src/write.c
> +++ b/libsepol/src/write.c
> @@ -571,44 +571,50 @@ static int role_allow_write(role_allow_t * r, struct policy_file *fp)
>
> static int filename_write_helper(hashtab_key_t key, void *data, void *ptr)
> {
> - uint32_t buf[4];
> + uint32_t bit, buf[4];
> size_t items, len;
> - struct filename_trans *ft = (struct filename_trans *)key;
> - struct filename_trans_datum *otype = data;
> + filename_trans_key_t *ft = (filename_trans_key_t *)key;
> + filename_trans_datum_t *datum = data;
> + ebitmap_node_t *node;
> void *fp = ptr;
>
> len = strlen(ft->name);
> - buf[0] = cpu_to_le32(len);
> - items = put_entry(buf, sizeof(uint32_t), 1, fp);
> - if (items != 1)
> - return POLICYDB_ERROR;
> + do {
> + ebitmap_for_each_positive_bit(&datum->stypes, node, bit) {
> + buf[0] = cpu_to_le32(len);
> + items = put_entry(buf, sizeof(uint32_t), 1, fp);
> + if (items != 1)
> + return POLICYDB_ERROR;
>
> - items = put_entry(ft->name, sizeof(char), len, fp);
> - if (items != len)
> - return POLICYDB_ERROR;
> + items = put_entry(ft->name, sizeof(char), len, fp);
> + if (items != len)
> + return POLICYDB_ERROR;
>
> - buf[0] = cpu_to_le32(ft->stype);
> - buf[1] = cpu_to_le32(ft->ttype);
> - buf[2] = cpu_to_le32(ft->tclass);
> - buf[3] = cpu_to_le32(otype->otype);
> - items = put_entry(buf, sizeof(uint32_t), 4, fp);
> - if (items != 4)
> - return POLICYDB_ERROR;
> + buf[0] = cpu_to_le32(bit + 1);
> + buf[1] = cpu_to_le32(ft->ttype);
> + buf[2] = cpu_to_le32(ft->tclass);
> + buf[3] = cpu_to_le32(datum->otype);
> + items = put_entry(buf, sizeof(uint32_t), 4, fp);
> + if (items != 4)
> + return POLICYDB_ERROR;
> + }
> +
> + datum = datum->next;
> + } while (datum);
>
> return 0;
> }
>
> static int filename_trans_write(struct policydb *p, void *fp)
> {
> - size_t nel, items;
> + size_t items;
> uint32_t buf[1];
> int rc;
>
> if (p->policyvers < POLICYDB_VERSION_FILENAME_TRANS)
> return 0;
>
> - nel = p->filename_trans->nel;
> - buf[0] = cpu_to_le32(nel);
> + buf[0] = cpu_to_le32(p->filename_trans_count);
> items = put_entry(buf, sizeof(uint32_t), 1, fp);
> if (items != 1)
> return POLICYDB_ERROR;
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] libsepol: implement POLICYDB_VERSION_COMP_FTRANS
2020-03-28 12:45 ` [PATCH v2 2/2] libsepol: implement POLICYDB_VERSION_COMP_FTRANS Ondrej Mosnacek
@ 2020-03-31 20:30 ` James Carter
2020-04-27 19:39 ` James Carter
0 siblings, 1 reply; 12+ messages in thread
From: James Carter @ 2020-03-31 20:30 UTC (permalink / raw)
To: Ondrej Mosnacek; +Cc: SElinux list, Stephen Smalley, Chris PeBenito
On Sat, Mar 28, 2020 at 8:46 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> Implement a new, more space-efficient form 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 representation 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.
>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Acked-by: James Carter <jwcart2@gmail.com>
> ---
> libsepol/include/sepol/policydb/policydb.h | 3 +-
> libsepol/src/policydb.c | 218 +++++++++++++++++----
> libsepol/src/write.c | 72 ++++++-
> 3 files changed, 242 insertions(+), 51 deletions(-)
>
> diff --git a/libsepol/include/sepol/policydb/policydb.h b/libsepol/include/sepol/policydb/policydb.h
> index c3180c61..9ef43abc 100644
> --- a/libsepol/include/sepol/policydb/policydb.h
> +++ b/libsepol/include/sepol/policydb/policydb.h
> @@ -755,10 +755,11 @@ extern int policydb_set_target_platform(policydb_t *p, int platform);
> #define POLICYDB_VERSION_XPERMS_IOCTL 30 /* Linux-specific */
> #define POLICYDB_VERSION_INFINIBAND 31 /* Linux-specific */
> #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
>
> /* Module versions and specific changes*/
> #define MOD_POLICYDB_VERSION_BASE 4
> diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c
> index 6b121d66..18ce1912 100644
> --- a/libsepol/src/policydb.c
> +++ b/libsepol/src/policydb.c
> @@ -200,6 +200,13 @@ static struct policydb_compat_info policydb_compat[] = {
> .ocon_num = OCON_IBENDPORT + 1,
> .target_platform = SEPOL_TARGET_SELINUX,
> },
> + {
> + .type = POLICY_KERN,
> + .version = POLICYDB_VERSION_COMP_FTRANS,
> + .sym_num = SYM_NUM,
> + .ocon_num = OCON_IBENDPORT + 1,
> + .target_platform = SEPOL_TARGET_SELINUX,
> + },
> {
> .type = POLICY_BASE,
> .version = MOD_POLICYDB_VERSION_BASE,
> @@ -2661,73 +2668,202 @@ int policydb_filetrans_insert(policydb_t *p, uint32_t stype, uint32_t ttype,
> return ebitmap_set_bit(&datum->stypes, stype - 1, 1);
> }
>
> -int filename_trans_read(policydb_t *p, struct policy_file *fp)
> +static int filename_trans_read_one_old(policydb_t *p, struct policy_file *fp)
> {
> - unsigned int i;
> - uint32_t buf[4], nel, len, stype, ttype, tclass, otype;
> + uint32_t buf[4], len, stype, ttype, tclass, otype;
> + char *name = NULL;
> int rc;
> - char *name;
>
> rc = next_entry(buf, fp, sizeof(uint32_t));
> if (rc < 0)
> return -1;
> - nel = le32_to_cpu(buf[0]);
> + len = le32_to_cpu(buf[0]);
> + if (zero_or_saturated(len))
> + return -1;
>
> - for (i = 0; i < nel; i++) {
> - name = NULL;
> + name = calloc(len + 1, sizeof(*name));
> + if (!name)
> + return -1;
>
> - rc = next_entry(buf, fp, sizeof(uint32_t));
> - if (rc < 0)
> - goto err;
> - len = le32_to_cpu(buf[0]);
> - if (zero_or_saturated(len))
> + rc = next_entry(name, fp, len);
> + if (rc < 0)
> + goto err;
> +
> + rc = next_entry(buf, fp, sizeof(uint32_t) * 4);
> + if (rc < 0)
> + goto err;
> +
> + stype = le32_to_cpu(buf[0]);
> + ttype = le32_to_cpu(buf[1]);
> + tclass = le32_to_cpu(buf[2]);
> + otype = le32_to_cpu(buf[3]);
> +
> + rc = policydb_filetrans_insert(p, stype, ttype, tclass, name, &name,
> + otype, NULL);
> + if (rc) {
> + if (rc != SEPOL_EEXIST)
> goto err;
> + /*
> + * Some old policies were wrongly generated with
> + * duplicate filename transition rules. For backward
> + * compatibility, do not reject such policies, just
> + * issue a warning and ignore the duplicate.
> + */
> + WARN(fp->handle,
> + "Duplicate name-based type_transition %s %s:%s \"%s\": %s, ignoring",
> + p->p_type_val_to_name[stype - 1],
> + p->p_type_val_to_name[ttype - 1],
> + p->p_class_val_to_name[tclass - 1],
> + name,
> + p->p_type_val_to_name[otype - 1]);
> + /* continue, ignoring this one */
> + }
> + free(name);
> + return 0;
> +err:
> + free(name);
> + return -1;
> +}
> +
> +static int filename_trans_check_datum(filename_trans_datum_t *datum)
> +{
> + ebitmap_t stypes, otypes;
> +
> + ebitmap_init(&stypes);
> + ebitmap_init(&otypes);
> +
> + while (datum) {
> + if (ebitmap_get_bit(&otypes, datum->otype))
> + return -1;
> +
> + if (ebitmap_set_bit(&otypes, datum->otype, 1))
> + return -1;
>
> - name = calloc(len + 1, sizeof(*name));
> - if (!name)
> + if (ebitmap_match_any(&stypes, &datum->stypes))
> + return -1;
> +
> + if (ebitmap_union(&stypes, &datum->stypes))
> + return -1;
> + }
> + return 0;
> +}
> +
> +static int filename_trans_read_one_new(policydb_t *p, struct policy_file *fp)
> +{
> + filename_trans_key_t *ft = NULL;
> + filename_trans_datum_t **dst, *datum, *first = NULL;
> + unsigned int i;
> + uint32_t buf[3], len, ttype, tclass, ndatum;
> + char *name = NULL;
> + int rc;
> +
> + rc = next_entry(buf, fp, sizeof(uint32_t));
> + if (rc < 0)
> + return -1;
> + len = le32_to_cpu(buf[0]);
> + if (zero_or_saturated(len))
> + return -1;
> +
> + name = calloc(len + 1, sizeof(*name));
> + if (!name)
> + return -1;
> +
> + rc = next_entry(name, fp, len);
> + if (rc < 0)
> + goto err;
> +
> + rc = next_entry(buf, fp, sizeof(uint32_t) * 3);
> + if (rc < 0)
> + goto err;
> +
> + ttype = le32_to_cpu(buf[0]);
> + tclass = le32_to_cpu(buf[1]);
> + ndatum = le32_to_cpu(buf[2]);
> + if (ndatum == 0)
> + goto err;
> +
> + dst = &first;
> + for (i = 0; i < ndatum; i++) {
> + datum = malloc(sizeof(*datum));
> + if (!datum)
> goto err;
>
> - rc = next_entry(name, fp, len);
> + *dst = datum;
> +
> + /* ebitmap_read() will at least init the bitmap */
> + rc = ebitmap_read(&datum->stypes, fp);
> if (rc < 0)
> goto err;
>
> - rc = next_entry(buf, fp, sizeof(uint32_t) * 4);
> + rc = next_entry(buf, fp, sizeof(uint32_t));
> if (rc < 0)
> goto err;
>
> - stype = le32_to_cpu(buf[0]);
> - ttype = le32_to_cpu(buf[1]);
> - tclass = le32_to_cpu(buf[2]);
> - otype = le32_to_cpu(buf[3]);
> + datum->otype = le32_to_cpu(buf[0]);
>
> - rc = policydb_filetrans_insert(p, stype, ttype, tclass, name,
> - &name, otype, NULL);
> - if (rc) {
> - if (rc != SEPOL_EEXIST)
> - goto err;
> - /*
> - * Some old policies were wrongly generated with
> - * duplicate filename transition rules. For backward
> - * compatibility, do not reject such policies, just
> - * issue a warning and ignore the duplicate.
> - */
> - WARN(fp->handle,
> - "Duplicate name-based type_transition %s %s:%s \"%s\": %s, ignoring",
> - p->p_type_val_to_name[stype - 1],
> - p->p_type_val_to_name[ttype - 1],
> - p->p_class_val_to_name[tclass - 1],
> - name,
> - p->p_type_val_to_name[otype - 1]);
> - /* continue, ignoring this one */
> - }
> - free(name);
> + p->filename_trans_count += ebitmap_cardinality(&datum->stypes);
> +
> + dst = &datum->next;
> }
> + *dst = NULL;
> +
> + if (ndatum > 1 && filename_trans_check_datum(first))
> + goto err;
> +
> + ft = malloc(sizeof(*ft));
> + if (!ft)
> + goto err;
> +
> + ft->ttype = ttype;
> + ft->tclass = tclass;
> + ft->name = name;
> +
> + rc = hashtab_insert(p->filename_trans, (hashtab_key_t)ft,
> + (hashtab_datum_t)first);
> + if (rc)
> + goto err;
> +
> return 0;
> err:
> + free(ft);
> free(name);
> + while (first) {
> + datum = first;
> + first = first->next;
> +
> + ebitmap_destroy(&datum->stypes);
> + free(datum);
> + }
> return -1;
> }
>
> +int filename_trans_read(policydb_t *p, struct policy_file *fp)
> +{
> + unsigned int i;
> + uint32_t buf[1], nel;
> + int rc;
> +
> + rc = next_entry(buf, fp, sizeof(uint32_t));
> + if (rc < 0)
> + return -1;
> + nel = le32_to_cpu(buf[0]);
> +
> + if (p->policyvers < POLICYDB_VERSION_COMP_FTRANS) {
> + for (i = 0; i < nel; i++) {
> + rc = filename_trans_read_one_old(p, fp);
> + if (rc < 0)
> + return -1;
> + }
> + } else {
> + for (i = 0; i < nel; i++) {
> + rc = filename_trans_read_one_new(p, fp);
> + if (rc < 0)
> + return -1;
> + }
> + }
> + return 0;
> +}
> +
> static int ocontext_read_xen(struct policydb_compat_info *info,
> policydb_t *p, struct policy_file *fp)
> {
> diff --git a/libsepol/src/write.c b/libsepol/src/write.c
> index d3aee8d5..9c74d9f5 100644
> --- a/libsepol/src/write.c
> +++ b/libsepol/src/write.c
> @@ -569,7 +569,7 @@ static int role_allow_write(role_allow_t * r, struct policy_file *fp)
> return POLICYDB_SUCCESS;
> }
>
> -static int filename_write_helper(hashtab_key_t key, void *data, void *ptr)
> +static int filename_write_one_old(hashtab_key_t key, void *data, void *ptr)
> {
> uint32_t bit, buf[4];
> size_t items, len;
> @@ -605,6 +605,54 @@ static int filename_write_helper(hashtab_key_t key, void *data, void *ptr)
> return 0;
> }
>
> +static int filename_write_one_new(hashtab_key_t key, void *data, void *ptr)
> +{
> + uint32_t buf[3];
> + size_t items, len, ndatum;
> + filename_trans_key_t *ft = (filename_trans_key_t *)key;
> + filename_trans_datum_t *datum;
> + void *fp = ptr;
> +
> + len = strlen(ft->name);
> + buf[0] = cpu_to_le32(len);
> + items = put_entry(buf, sizeof(uint32_t), 1, fp);
> + if (items != 1)
> + return POLICYDB_ERROR;
> +
> + items = put_entry(ft->name, sizeof(char), len, fp);
> + if (items != len)
> + return POLICYDB_ERROR;
> +
> + ndatum = 0;
> + datum = data;
> + do {
> + ndatum++;
> + datum = datum->next;
> + } while (datum);
> +
> + buf[0] = cpu_to_le32(ft->ttype);
> + buf[1] = cpu_to_le32(ft->tclass);
> + buf[2] = cpu_to_le32(ndatum);
> + items = put_entry(buf, sizeof(uint32_t), 3, fp);
> + if (items != 3)
> + return POLICYDB_ERROR;
> +
> + datum = data;
> + do {
> + if (ebitmap_write(&datum->stypes, fp))
> + return POLICYDB_ERROR;
> +
> + buf[0] = cpu_to_le32(datum->otype);
> + items = put_entry(buf, sizeof(uint32_t), 1, fp);
> + if (items != 1)
> + return POLICYDB_ERROR;
> +
> + datum = datum->next;
> + } while (datum);
> +
> + return 0;
> +}
> +
> static int filename_trans_write(struct policydb *p, void *fp)
> {
> size_t items;
> @@ -614,16 +662,22 @@ static int filename_trans_write(struct policydb *p, void *fp)
> if (p->policyvers < POLICYDB_VERSION_FILENAME_TRANS)
> return 0;
>
> - buf[0] = cpu_to_le32(p->filename_trans_count);
> - items = put_entry(buf, sizeof(uint32_t), 1, fp);
> - if (items != 1)
> - return POLICYDB_ERROR;
> + if (p->policyvers < POLICYDB_VERSION_COMP_FTRANS) {
> + buf[0] = cpu_to_le32(p->filename_trans_count);
> + items = put_entry(buf, sizeof(uint32_t), 1, fp);
> + if (items != 1)
> + return POLICYDB_ERROR;
>
> - rc = hashtab_map(p->filename_trans, filename_write_helper, 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);
> + items = put_entry(buf, sizeof(uint32_t), 1, fp);
> + if (items != 1)
> + return POLICYDB_ERROR;
>
> - return 0;
> + rc = hashtab_map(p->filename_trans, filename_write_one_new, fp);
> + }
> + return rc;
> }
>
> static int role_set_write(role_set_t * x, struct policy_file *fp)
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] libsepol: implement POLICYDB_VERSION_COMP_FTRANS
2020-03-31 20:30 ` James Carter
@ 2020-04-27 19:39 ` James Carter
2020-04-28 8:49 ` Ondrej Mosnacek
0 siblings, 1 reply; 12+ messages in thread
From: James Carter @ 2020-04-27 19:39 UTC (permalink / raw)
To: Ondrej Mosnacek; +Cc: SElinux list, Stephen Smalley, Chris PeBenito
On Tue, Mar 31, 2020 at 4:30 PM James Carter <jwcart2@gmail.com> wrote:
>
> On Sat, Mar 28, 2020 at 8:46 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> >
> > Implement a new, more space-efficient form 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 representation 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.
> >
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
>
> Acked-by: James Carter <jwcart2@gmail.com>
>
Sorry, it has been a while.
I was going to merge these today, but upon further testing, I noticed
a couple of problems.
First, creating a policy.conf from a binary like this:
~/local/usr/bin/checkpolicy -F -M -b -o policy.31.conf policy.31
give the following error:
libsepol.avtab_read_item: invalid item count
Second, creating a binary from a policy.conf and then trying to create
a policy.conf from that binary:
~/local/usr/bin/checkpolicy -o policy.bin policy.conf
~/local/usr/bin/checkpolicy -F -b -o policy.bin.conf policy.bin
gives the following error:
security: ebitmap: high bit (32) is not a multiple of the map size (64)
I don't know if this is related to the problems you had on the kernel
side or not. I'll keep looking into this, but, for now, I'll have to
remove my Acked-by.
Jim
> > ---
> > libsepol/include/sepol/policydb/policydb.h | 3 +-
> > libsepol/src/policydb.c | 218 +++++++++++++++++----
> > libsepol/src/write.c | 72 ++++++-
> > 3 files changed, 242 insertions(+), 51 deletions(-)
> >
> > diff --git a/libsepol/include/sepol/policydb/policydb.h b/libsepol/include/sepol/policydb/policydb.h
> > index c3180c61..9ef43abc 100644
> > --- a/libsepol/include/sepol/policydb/policydb.h
> > +++ b/libsepol/include/sepol/policydb/policydb.h
> > @@ -755,10 +755,11 @@ extern int policydb_set_target_platform(policydb_t *p, int platform);
> > #define POLICYDB_VERSION_XPERMS_IOCTL 30 /* Linux-specific */
> > #define POLICYDB_VERSION_INFINIBAND 31 /* Linux-specific */
> > #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
> >
> > /* Module versions and specific changes*/
> > #define MOD_POLICYDB_VERSION_BASE 4
> > diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c
> > index 6b121d66..18ce1912 100644
> > --- a/libsepol/src/policydb.c
> > +++ b/libsepol/src/policydb.c
> > @@ -200,6 +200,13 @@ static struct policydb_compat_info policydb_compat[] = {
> > .ocon_num = OCON_IBENDPORT + 1,
> > .target_platform = SEPOL_TARGET_SELINUX,
> > },
> > + {
> > + .type = POLICY_KERN,
> > + .version = POLICYDB_VERSION_COMP_FTRANS,
> > + .sym_num = SYM_NUM,
> > + .ocon_num = OCON_IBENDPORT + 1,
> > + .target_platform = SEPOL_TARGET_SELINUX,
> > + },
> > {
> > .type = POLICY_BASE,
> > .version = MOD_POLICYDB_VERSION_BASE,
> > @@ -2661,73 +2668,202 @@ int policydb_filetrans_insert(policydb_t *p, uint32_t stype, uint32_t ttype,
> > return ebitmap_set_bit(&datum->stypes, stype - 1, 1);
> > }
> >
> > -int filename_trans_read(policydb_t *p, struct policy_file *fp)
> > +static int filename_trans_read_one_old(policydb_t *p, struct policy_file *fp)
> > {
> > - unsigned int i;
> > - uint32_t buf[4], nel, len, stype, ttype, tclass, otype;
> > + uint32_t buf[4], len, stype, ttype, tclass, otype;
> > + char *name = NULL;
> > int rc;
> > - char *name;
> >
> > rc = next_entry(buf, fp, sizeof(uint32_t));
> > if (rc < 0)
> > return -1;
> > - nel = le32_to_cpu(buf[0]);
> > + len = le32_to_cpu(buf[0]);
> > + if (zero_or_saturated(len))
> > + return -1;
> >
> > - for (i = 0; i < nel; i++) {
> > - name = NULL;
> > + name = calloc(len + 1, sizeof(*name));
> > + if (!name)
> > + return -1;
> >
> > - rc = next_entry(buf, fp, sizeof(uint32_t));
> > - if (rc < 0)
> > - goto err;
> > - len = le32_to_cpu(buf[0]);
> > - if (zero_or_saturated(len))
> > + rc = next_entry(name, fp, len);
> > + if (rc < 0)
> > + goto err;
> > +
> > + rc = next_entry(buf, fp, sizeof(uint32_t) * 4);
> > + if (rc < 0)
> > + goto err;
> > +
> > + stype = le32_to_cpu(buf[0]);
> > + ttype = le32_to_cpu(buf[1]);
> > + tclass = le32_to_cpu(buf[2]);
> > + otype = le32_to_cpu(buf[3]);
> > +
> > + rc = policydb_filetrans_insert(p, stype, ttype, tclass, name, &name,
> > + otype, NULL);
> > + if (rc) {
> > + if (rc != SEPOL_EEXIST)
> > goto err;
> > + /*
> > + * Some old policies were wrongly generated with
> > + * duplicate filename transition rules. For backward
> > + * compatibility, do not reject such policies, just
> > + * issue a warning and ignore the duplicate.
> > + */
> > + WARN(fp->handle,
> > + "Duplicate name-based type_transition %s %s:%s \"%s\": %s, ignoring",
> > + p->p_type_val_to_name[stype - 1],
> > + p->p_type_val_to_name[ttype - 1],
> > + p->p_class_val_to_name[tclass - 1],
> > + name,
> > + p->p_type_val_to_name[otype - 1]);
> > + /* continue, ignoring this one */
> > + }
> > + free(name);
> > + return 0;
> > +err:
> > + free(name);
> > + return -1;
> > +}
> > +
> > +static int filename_trans_check_datum(filename_trans_datum_t *datum)
> > +{
> > + ebitmap_t stypes, otypes;
> > +
> > + ebitmap_init(&stypes);
> > + ebitmap_init(&otypes);
> > +
> > + while (datum) {
> > + if (ebitmap_get_bit(&otypes, datum->otype))
> > + return -1;
> > +
> > + if (ebitmap_set_bit(&otypes, datum->otype, 1))
> > + return -1;
> >
> > - name = calloc(len + 1, sizeof(*name));
> > - if (!name)
> > + if (ebitmap_match_any(&stypes, &datum->stypes))
> > + return -1;
> > +
> > + if (ebitmap_union(&stypes, &datum->stypes))
> > + return -1;
> > + }
> > + return 0;
> > +}
> > +
> > +static int filename_trans_read_one_new(policydb_t *p, struct policy_file *fp)
> > +{
> > + filename_trans_key_t *ft = NULL;
> > + filename_trans_datum_t **dst, *datum, *first = NULL;
> > + unsigned int i;
> > + uint32_t buf[3], len, ttype, tclass, ndatum;
> > + char *name = NULL;
> > + int rc;
> > +
> > + rc = next_entry(buf, fp, sizeof(uint32_t));
> > + if (rc < 0)
> > + return -1;
> > + len = le32_to_cpu(buf[0]);
> > + if (zero_or_saturated(len))
> > + return -1;
> > +
> > + name = calloc(len + 1, sizeof(*name));
> > + if (!name)
> > + return -1;
> > +
> > + rc = next_entry(name, fp, len);
> > + if (rc < 0)
> > + goto err;
> > +
> > + rc = next_entry(buf, fp, sizeof(uint32_t) * 3);
> > + if (rc < 0)
> > + goto err;
> > +
> > + ttype = le32_to_cpu(buf[0]);
> > + tclass = le32_to_cpu(buf[1]);
> > + ndatum = le32_to_cpu(buf[2]);
> > + if (ndatum == 0)
> > + goto err;
> > +
> > + dst = &first;
> > + for (i = 0; i < ndatum; i++) {
> > + datum = malloc(sizeof(*datum));
> > + if (!datum)
> > goto err;
> >
> > - rc = next_entry(name, fp, len);
> > + *dst = datum;
> > +
> > + /* ebitmap_read() will at least init the bitmap */
> > + rc = ebitmap_read(&datum->stypes, fp);
> > if (rc < 0)
> > goto err;
> >
> > - rc = next_entry(buf, fp, sizeof(uint32_t) * 4);
> > + rc = next_entry(buf, fp, sizeof(uint32_t));
> > if (rc < 0)
> > goto err;
> >
> > - stype = le32_to_cpu(buf[0]);
> > - ttype = le32_to_cpu(buf[1]);
> > - tclass = le32_to_cpu(buf[2]);
> > - otype = le32_to_cpu(buf[3]);
> > + datum->otype = le32_to_cpu(buf[0]);
> >
> > - rc = policydb_filetrans_insert(p, stype, ttype, tclass, name,
> > - &name, otype, NULL);
> > - if (rc) {
> > - if (rc != SEPOL_EEXIST)
> > - goto err;
> > - /*
> > - * Some old policies were wrongly generated with
> > - * duplicate filename transition rules. For backward
> > - * compatibility, do not reject such policies, just
> > - * issue a warning and ignore the duplicate.
> > - */
> > - WARN(fp->handle,
> > - "Duplicate name-based type_transition %s %s:%s \"%s\": %s, ignoring",
> > - p->p_type_val_to_name[stype - 1],
> > - p->p_type_val_to_name[ttype - 1],
> > - p->p_class_val_to_name[tclass - 1],
> > - name,
> > - p->p_type_val_to_name[otype - 1]);
> > - /* continue, ignoring this one */
> > - }
> > - free(name);
> > + p->filename_trans_count += ebitmap_cardinality(&datum->stypes);
> > +
> > + dst = &datum->next;
> > }
> > + *dst = NULL;
> > +
> > + if (ndatum > 1 && filename_trans_check_datum(first))
> > + goto err;
> > +
> > + ft = malloc(sizeof(*ft));
> > + if (!ft)
> > + goto err;
> > +
> > + ft->ttype = ttype;
> > + ft->tclass = tclass;
> > + ft->name = name;
> > +
> > + rc = hashtab_insert(p->filename_trans, (hashtab_key_t)ft,
> > + (hashtab_datum_t)first);
> > + if (rc)
> > + goto err;
> > +
> > return 0;
> > err:
> > + free(ft);
> > free(name);
> > + while (first) {
> > + datum = first;
> > + first = first->next;
> > +
> > + ebitmap_destroy(&datum->stypes);
> > + free(datum);
> > + }
> > return -1;
> > }
> >
> > +int filename_trans_read(policydb_t *p, struct policy_file *fp)
> > +{
> > + unsigned int i;
> > + uint32_t buf[1], nel;
> > + int rc;
> > +
> > + rc = next_entry(buf, fp, sizeof(uint32_t));
> > + if (rc < 0)
> > + return -1;
> > + nel = le32_to_cpu(buf[0]);
> > +
> > + if (p->policyvers < POLICYDB_VERSION_COMP_FTRANS) {
> > + for (i = 0; i < nel; i++) {
> > + rc = filename_trans_read_one_old(p, fp);
> > + if (rc < 0)
> > + return -1;
> > + }
> > + } else {
> > + for (i = 0; i < nel; i++) {
> > + rc = filename_trans_read_one_new(p, fp);
> > + if (rc < 0)
> > + return -1;
> > + }
> > + }
> > + return 0;
> > +}
> > +
> > static int ocontext_read_xen(struct policydb_compat_info *info,
> > policydb_t *p, struct policy_file *fp)
> > {
> > diff --git a/libsepol/src/write.c b/libsepol/src/write.c
> > index d3aee8d5..9c74d9f5 100644
> > --- a/libsepol/src/write.c
> > +++ b/libsepol/src/write.c
> > @@ -569,7 +569,7 @@ static int role_allow_write(role_allow_t * r, struct policy_file *fp)
> > return POLICYDB_SUCCESS;
> > }
> >
> > -static int filename_write_helper(hashtab_key_t key, void *data, void *ptr)
> > +static int filename_write_one_old(hashtab_key_t key, void *data, void *ptr)
> > {
> > uint32_t bit, buf[4];
> > size_t items, len;
> > @@ -605,6 +605,54 @@ static int filename_write_helper(hashtab_key_t key, void *data, void *ptr)
> > return 0;
> > }
> >
> > +static int filename_write_one_new(hashtab_key_t key, void *data, void *ptr)
> > +{
> > + uint32_t buf[3];
> > + size_t items, len, ndatum;
> > + filename_trans_key_t *ft = (filename_trans_key_t *)key;
> > + filename_trans_datum_t *datum;
> > + void *fp = ptr;
> > +
> > + len = strlen(ft->name);
> > + buf[0] = cpu_to_le32(len);
> > + items = put_entry(buf, sizeof(uint32_t), 1, fp);
> > + if (items != 1)
> > + return POLICYDB_ERROR;
> > +
> > + items = put_entry(ft->name, sizeof(char), len, fp);
> > + if (items != len)
> > + return POLICYDB_ERROR;
> > +
> > + ndatum = 0;
> > + datum = data;
> > + do {
> > + ndatum++;
> > + datum = datum->next;
> > + } while (datum);
> > +
> > + buf[0] = cpu_to_le32(ft->ttype);
> > + buf[1] = cpu_to_le32(ft->tclass);
> > + buf[2] = cpu_to_le32(ndatum);
> > + items = put_entry(buf, sizeof(uint32_t), 3, fp);
> > + if (items != 3)
> > + return POLICYDB_ERROR;
> > +
> > + datum = data;
> > + do {
> > + if (ebitmap_write(&datum->stypes, fp))
> > + return POLICYDB_ERROR;
> > +
> > + buf[0] = cpu_to_le32(datum->otype);
> > + items = put_entry(buf, sizeof(uint32_t), 1, fp);
> > + if (items != 1)
> > + return POLICYDB_ERROR;
> > +
> > + datum = datum->next;
> > + } while (datum);
> > +
> > + return 0;
> > +}
> > +
> > static int filename_trans_write(struct policydb *p, void *fp)
> > {
> > size_t items;
> > @@ -614,16 +662,22 @@ static int filename_trans_write(struct policydb *p, void *fp)
> > if (p->policyvers < POLICYDB_VERSION_FILENAME_TRANS)
> > return 0;
> >
> > - buf[0] = cpu_to_le32(p->filename_trans_count);
> > - items = put_entry(buf, sizeof(uint32_t), 1, fp);
> > - if (items != 1)
> > - return POLICYDB_ERROR;
> > + if (p->policyvers < POLICYDB_VERSION_COMP_FTRANS) {
> > + buf[0] = cpu_to_le32(p->filename_trans_count);
> > + items = put_entry(buf, sizeof(uint32_t), 1, fp);
> > + if (items != 1)
> > + return POLICYDB_ERROR;
> >
> > - rc = hashtab_map(p->filename_trans, filename_write_helper, 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);
> > + items = put_entry(buf, sizeof(uint32_t), 1, fp);
> > + if (items != 1)
> > + return POLICYDB_ERROR;
> >
> > - return 0;
> > + rc = hashtab_map(p->filename_trans, filename_write_one_new, fp);
> > + }
> > + return rc;
> > }
> >
> > static int role_set_write(role_set_t * x, struct policy_file *fp)
> > --
> > 2.25.1
> >
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] libsepol: implement POLICYDB_VERSION_COMP_FTRANS
2020-04-27 19:39 ` James Carter
@ 2020-04-28 8:49 ` Ondrej Mosnacek
2020-04-28 15:28 ` James Carter
0 siblings, 1 reply; 12+ messages in thread
From: Ondrej Mosnacek @ 2020-04-28 8:49 UTC (permalink / raw)
To: James Carter; +Cc: SElinux list, Stephen Smalley, Chris PeBenito
On Mon, Apr 27, 2020 at 9:39 PM James Carter <jwcart2@gmail.com> wrote:
> On Tue, Mar 31, 2020 at 4:30 PM James Carter <jwcart2@gmail.com> wrote:
> >
> > On Sat, Mar 28, 2020 at 8:46 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > >
> > > Implement a new, more space-efficient form 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 representation 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.
> > >
> > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> >
> > Acked-by: James Carter <jwcart2@gmail.com>
> >
>
> Sorry, it has been a while.
No problem. I would've merged it myself, I just wasn't sure if we
shouldn't try to do something about the setools issue somehow... But I
really don't feel like touching that code, so if the consensus is that
this is worth the breakage then I'm fine with it :)
>
> I was going to merge these today, but upon further testing, I noticed
> a couple of problems.
>
> First, creating a policy.conf from a binary like this:
> ~/local/usr/bin/checkpolicy -F -M -b -o policy.31.conf policy.31
> give the following error:
> libsepol.avtab_read_item: invalid item count
Hm... I can't reproduce that on my side. Are you sure you're not using
a broken policy generated by the buggy v1 of this series?
>
> Second, creating a binary from a policy.conf and then trying to create
> a policy.conf from that binary:
> ~/local/usr/bin/checkpolicy -o policy.bin policy.conf
> ~/local/usr/bin/checkpolicy -F -b -o policy.bin.conf policy.bin
> gives the following error:
> security: ebitmap: high bit (32) is not a multiple of the map size (64)
Likewise, couldn't reproduce it with the conf file I tried... Can you
share the policy.conf that triggers this?
>
> I don't know if this is related to the problems you had on the kernel
> side or not. I'll keep looking into this, but, for now, I'll have to
> remove my Acked-by.
>
> Jim
>
>
> > > ---
> > > libsepol/include/sepol/policydb/policydb.h | 3 +-
> > > libsepol/src/policydb.c | 218 +++++++++++++++++----
> > > libsepol/src/write.c | 72 ++++++-
> > > 3 files changed, 242 insertions(+), 51 deletions(-)
> > >
> > > diff --git a/libsepol/include/sepol/policydb/policydb.h b/libsepol/include/sepol/policydb/policydb.h
> > > index c3180c61..9ef43abc 100644
> > > --- a/libsepol/include/sepol/policydb/policydb.h
> > > +++ b/libsepol/include/sepol/policydb/policydb.h
> > > @@ -755,10 +755,11 @@ extern int policydb_set_target_platform(policydb_t *p, int platform);
> > > #define POLICYDB_VERSION_XPERMS_IOCTL 30 /* Linux-specific */
> > > #define POLICYDB_VERSION_INFINIBAND 31 /* Linux-specific */
> > > #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
> > >
> > > /* Module versions and specific changes*/
> > > #define MOD_POLICYDB_VERSION_BASE 4
> > > diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c
> > > index 6b121d66..18ce1912 100644
> > > --- a/libsepol/src/policydb.c
> > > +++ b/libsepol/src/policydb.c
> > > @@ -200,6 +200,13 @@ static struct policydb_compat_info policydb_compat[] = {
> > > .ocon_num = OCON_IBENDPORT + 1,
> > > .target_platform = SEPOL_TARGET_SELINUX,
> > > },
> > > + {
> > > + .type = POLICY_KERN,
> > > + .version = POLICYDB_VERSION_COMP_FTRANS,
> > > + .sym_num = SYM_NUM,
> > > + .ocon_num = OCON_IBENDPORT + 1,
> > > + .target_platform = SEPOL_TARGET_SELINUX,
> > > + },
> > > {
> > > .type = POLICY_BASE,
> > > .version = MOD_POLICYDB_VERSION_BASE,
> > > @@ -2661,73 +2668,202 @@ int policydb_filetrans_insert(policydb_t *p, uint32_t stype, uint32_t ttype,
> > > return ebitmap_set_bit(&datum->stypes, stype - 1, 1);
> > > }
> > >
> > > -int filename_trans_read(policydb_t *p, struct policy_file *fp)
> > > +static int filename_trans_read_one_old(policydb_t *p, struct policy_file *fp)
> > > {
> > > - unsigned int i;
> > > - uint32_t buf[4], nel, len, stype, ttype, tclass, otype;
> > > + uint32_t buf[4], len, stype, ttype, tclass, otype;
> > > + char *name = NULL;
> > > int rc;
> > > - char *name;
> > >
> > > rc = next_entry(buf, fp, sizeof(uint32_t));
> > > if (rc < 0)
> > > return -1;
> > > - nel = le32_to_cpu(buf[0]);
> > > + len = le32_to_cpu(buf[0]);
> > > + if (zero_or_saturated(len))
> > > + return -1;
> > >
> > > - for (i = 0; i < nel; i++) {
> > > - name = NULL;
> > > + name = calloc(len + 1, sizeof(*name));
> > > + if (!name)
> > > + return -1;
> > >
> > > - rc = next_entry(buf, fp, sizeof(uint32_t));
> > > - if (rc < 0)
> > > - goto err;
> > > - len = le32_to_cpu(buf[0]);
> > > - if (zero_or_saturated(len))
> > > + rc = next_entry(name, fp, len);
> > > + if (rc < 0)
> > > + goto err;
> > > +
> > > + rc = next_entry(buf, fp, sizeof(uint32_t) * 4);
> > > + if (rc < 0)
> > > + goto err;
> > > +
> > > + stype = le32_to_cpu(buf[0]);
> > > + ttype = le32_to_cpu(buf[1]);
> > > + tclass = le32_to_cpu(buf[2]);
> > > + otype = le32_to_cpu(buf[3]);
> > > +
> > > + rc = policydb_filetrans_insert(p, stype, ttype, tclass, name, &name,
> > > + otype, NULL);
> > > + if (rc) {
> > > + if (rc != SEPOL_EEXIST)
> > > goto err;
> > > + /*
> > > + * Some old policies were wrongly generated with
> > > + * duplicate filename transition rules. For backward
> > > + * compatibility, do not reject such policies, just
> > > + * issue a warning and ignore the duplicate.
> > > + */
> > > + WARN(fp->handle,
> > > + "Duplicate name-based type_transition %s %s:%s \"%s\": %s, ignoring",
> > > + p->p_type_val_to_name[stype - 1],
> > > + p->p_type_val_to_name[ttype - 1],
> > > + p->p_class_val_to_name[tclass - 1],
> > > + name,
> > > + p->p_type_val_to_name[otype - 1]);
> > > + /* continue, ignoring this one */
> > > + }
> > > + free(name);
> > > + return 0;
> > > +err:
> > > + free(name);
> > > + return -1;
> > > +}
> > > +
> > > +static int filename_trans_check_datum(filename_trans_datum_t *datum)
> > > +{
> > > + ebitmap_t stypes, otypes;
> > > +
> > > + ebitmap_init(&stypes);
> > > + ebitmap_init(&otypes);
> > > +
> > > + while (datum) {
> > > + if (ebitmap_get_bit(&otypes, datum->otype))
> > > + return -1;
> > > +
> > > + if (ebitmap_set_bit(&otypes, datum->otype, 1))
> > > + return -1;
> > >
> > > - name = calloc(len + 1, sizeof(*name));
> > > - if (!name)
> > > + if (ebitmap_match_any(&stypes, &datum->stypes))
> > > + return -1;
> > > +
> > > + if (ebitmap_union(&stypes, &datum->stypes))
> > > + return -1;
> > > + }
> > > + return 0;
> > > +}
> > > +
> > > +static int filename_trans_read_one_new(policydb_t *p, struct policy_file *fp)
> > > +{
> > > + filename_trans_key_t *ft = NULL;
> > > + filename_trans_datum_t **dst, *datum, *first = NULL;
> > > + unsigned int i;
> > > + uint32_t buf[3], len, ttype, tclass, ndatum;
> > > + char *name = NULL;
> > > + int rc;
> > > +
> > > + rc = next_entry(buf, fp, sizeof(uint32_t));
> > > + if (rc < 0)
> > > + return -1;
> > > + len = le32_to_cpu(buf[0]);
> > > + if (zero_or_saturated(len))
> > > + return -1;
> > > +
> > > + name = calloc(len + 1, sizeof(*name));
> > > + if (!name)
> > > + return -1;
> > > +
> > > + rc = next_entry(name, fp, len);
> > > + if (rc < 0)
> > > + goto err;
> > > +
> > > + rc = next_entry(buf, fp, sizeof(uint32_t) * 3);
> > > + if (rc < 0)
> > > + goto err;
> > > +
> > > + ttype = le32_to_cpu(buf[0]);
> > > + tclass = le32_to_cpu(buf[1]);
> > > + ndatum = le32_to_cpu(buf[2]);
> > > + if (ndatum == 0)
> > > + goto err;
> > > +
> > > + dst = &first;
> > > + for (i = 0; i < ndatum; i++) {
> > > + datum = malloc(sizeof(*datum));
> > > + if (!datum)
> > > goto err;
> > >
> > > - rc = next_entry(name, fp, len);
> > > + *dst = datum;
> > > +
> > > + /* ebitmap_read() will at least init the bitmap */
> > > + rc = ebitmap_read(&datum->stypes, fp);
> > > if (rc < 0)
> > > goto err;
> > >
> > > - rc = next_entry(buf, fp, sizeof(uint32_t) * 4);
> > > + rc = next_entry(buf, fp, sizeof(uint32_t));
> > > if (rc < 0)
> > > goto err;
> > >
> > > - stype = le32_to_cpu(buf[0]);
> > > - ttype = le32_to_cpu(buf[1]);
> > > - tclass = le32_to_cpu(buf[2]);
> > > - otype = le32_to_cpu(buf[3]);
> > > + datum->otype = le32_to_cpu(buf[0]);
> > >
> > > - rc = policydb_filetrans_insert(p, stype, ttype, tclass, name,
> > > - &name, otype, NULL);
> > > - if (rc) {
> > > - if (rc != SEPOL_EEXIST)
> > > - goto err;
> > > - /*
> > > - * Some old policies were wrongly generated with
> > > - * duplicate filename transition rules. For backward
> > > - * compatibility, do not reject such policies, just
> > > - * issue a warning and ignore the duplicate.
> > > - */
> > > - WARN(fp->handle,
> > > - "Duplicate name-based type_transition %s %s:%s \"%s\": %s, ignoring",
> > > - p->p_type_val_to_name[stype - 1],
> > > - p->p_type_val_to_name[ttype - 1],
> > > - p->p_class_val_to_name[tclass - 1],
> > > - name,
> > > - p->p_type_val_to_name[otype - 1]);
> > > - /* continue, ignoring this one */
> > > - }
> > > - free(name);
> > > + p->filename_trans_count += ebitmap_cardinality(&datum->stypes);
> > > +
> > > + dst = &datum->next;
> > > }
> > > + *dst = NULL;
> > > +
> > > + if (ndatum > 1 && filename_trans_check_datum(first))
> > > + goto err;
> > > +
> > > + ft = malloc(sizeof(*ft));
> > > + if (!ft)
> > > + goto err;
> > > +
> > > + ft->ttype = ttype;
> > > + ft->tclass = tclass;
> > > + ft->name = name;
> > > +
> > > + rc = hashtab_insert(p->filename_trans, (hashtab_key_t)ft,
> > > + (hashtab_datum_t)first);
> > > + if (rc)
> > > + goto err;
> > > +
> > > return 0;
> > > err:
> > > + free(ft);
> > > free(name);
> > > + while (first) {
> > > + datum = first;
> > > + first = first->next;
> > > +
> > > + ebitmap_destroy(&datum->stypes);
> > > + free(datum);
> > > + }
> > > return -1;
> > > }
> > >
> > > +int filename_trans_read(policydb_t *p, struct policy_file *fp)
> > > +{
> > > + unsigned int i;
> > > + uint32_t buf[1], nel;
> > > + int rc;
> > > +
> > > + rc = next_entry(buf, fp, sizeof(uint32_t));
> > > + if (rc < 0)
> > > + return -1;
> > > + nel = le32_to_cpu(buf[0]);
> > > +
> > > + if (p->policyvers < POLICYDB_VERSION_COMP_FTRANS) {
> > > + for (i = 0; i < nel; i++) {
> > > + rc = filename_trans_read_one_old(p, fp);
> > > + if (rc < 0)
> > > + return -1;
> > > + }
> > > + } else {
> > > + for (i = 0; i < nel; i++) {
> > > + rc = filename_trans_read_one_new(p, fp);
> > > + if (rc < 0)
> > > + return -1;
> > > + }
> > > + }
> > > + return 0;
> > > +}
> > > +
> > > static int ocontext_read_xen(struct policydb_compat_info *info,
> > > policydb_t *p, struct policy_file *fp)
> > > {
> > > diff --git a/libsepol/src/write.c b/libsepol/src/write.c
> > > index d3aee8d5..9c74d9f5 100644
> > > --- a/libsepol/src/write.c
> > > +++ b/libsepol/src/write.c
> > > @@ -569,7 +569,7 @@ static int role_allow_write(role_allow_t * r, struct policy_file *fp)
> > > return POLICYDB_SUCCESS;
> > > }
> > >
> > > -static int filename_write_helper(hashtab_key_t key, void *data, void *ptr)
> > > +static int filename_write_one_old(hashtab_key_t key, void *data, void *ptr)
> > > {
> > > uint32_t bit, buf[4];
> > > size_t items, len;
> > > @@ -605,6 +605,54 @@ static int filename_write_helper(hashtab_key_t key, void *data, void *ptr)
> > > return 0;
> > > }
> > >
> > > +static int filename_write_one_new(hashtab_key_t key, void *data, void *ptr)
> > > +{
> > > + uint32_t buf[3];
> > > + size_t items, len, ndatum;
> > > + filename_trans_key_t *ft = (filename_trans_key_t *)key;
> > > + filename_trans_datum_t *datum;
> > > + void *fp = ptr;
> > > +
> > > + len = strlen(ft->name);
> > > + buf[0] = cpu_to_le32(len);
> > > + items = put_entry(buf, sizeof(uint32_t), 1, fp);
> > > + if (items != 1)
> > > + return POLICYDB_ERROR;
> > > +
> > > + items = put_entry(ft->name, sizeof(char), len, fp);
> > > + if (items != len)
> > > + return POLICYDB_ERROR;
> > > +
> > > + ndatum = 0;
> > > + datum = data;
> > > + do {
> > > + ndatum++;
> > > + datum = datum->next;
> > > + } while (datum);
> > > +
> > > + buf[0] = cpu_to_le32(ft->ttype);
> > > + buf[1] = cpu_to_le32(ft->tclass);
> > > + buf[2] = cpu_to_le32(ndatum);
> > > + items = put_entry(buf, sizeof(uint32_t), 3, fp);
> > > + if (items != 3)
> > > + return POLICYDB_ERROR;
> > > +
> > > + datum = data;
> > > + do {
> > > + if (ebitmap_write(&datum->stypes, fp))
> > > + return POLICYDB_ERROR;
> > > +
> > > + buf[0] = cpu_to_le32(datum->otype);
> > > + items = put_entry(buf, sizeof(uint32_t), 1, fp);
> > > + if (items != 1)
> > > + return POLICYDB_ERROR;
> > > +
> > > + datum = datum->next;
> > > + } while (datum);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > static int filename_trans_write(struct policydb *p, void *fp)
> > > {
> > > size_t items;
> > > @@ -614,16 +662,22 @@ static int filename_trans_write(struct policydb *p, void *fp)
> > > if (p->policyvers < POLICYDB_VERSION_FILENAME_TRANS)
> > > return 0;
> > >
> > > - buf[0] = cpu_to_le32(p->filename_trans_count);
> > > - items = put_entry(buf, sizeof(uint32_t), 1, fp);
> > > - if (items != 1)
> > > - return POLICYDB_ERROR;
> > > + if (p->policyvers < POLICYDB_VERSION_COMP_FTRANS) {
> > > + buf[0] = cpu_to_le32(p->filename_trans_count);
> > > + items = put_entry(buf, sizeof(uint32_t), 1, fp);
> > > + if (items != 1)
> > > + return POLICYDB_ERROR;
> > >
> > > - rc = hashtab_map(p->filename_trans, filename_write_helper, 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);
> > > + items = put_entry(buf, sizeof(uint32_t), 1, fp);
> > > + if (items != 1)
> > > + return POLICYDB_ERROR;
> > >
> > > - return 0;
> > > + rc = hashtab_map(p->filename_trans, filename_write_one_new, fp);
> > > + }
> > > + return rc;
> > > }
> > >
> > > static int role_set_write(role_set_t * x, struct policy_file *fp)
> > > --
> > > 2.25.1
> > >
>
--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] libsepol: implement POLICYDB_VERSION_COMP_FTRANS
2020-04-28 8:49 ` Ondrej Mosnacek
@ 2020-04-28 15:28 ` James Carter
2020-04-30 14:25 ` Stephen Smalley
0 siblings, 1 reply; 12+ messages in thread
From: James Carter @ 2020-04-28 15:28 UTC (permalink / raw)
To: Ondrej Mosnacek; +Cc: SElinux list, Stephen Smalley, Chris PeBenito
On Tue, Apr 28, 2020 at 4:49 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> On Mon, Apr 27, 2020 at 9:39 PM James Carter <jwcart2@gmail.com> wrote:
> > On Tue, Mar 31, 2020 at 4:30 PM James Carter <jwcart2@gmail.com> wrote:
> > >
> > > On Sat, Mar 28, 2020 at 8:46 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > >
> > > > Implement a new, more space-efficient form 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 representation 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.
> > > >
> > > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > >
> > > Acked-by: James Carter <jwcart2@gmail.com>
> > >
> >
> > Sorry, it has been a while.
>
> No problem. I would've merged it myself, I just wasn't sure if we
> shouldn't try to do something about the setools issue somehow... But I
> really don't feel like touching that code, so if the consensus is that
> this is worth the breakage then I'm fine with it :)
>
I thought the consensus was to apply this now, but it would be nice to
not break setools, so I am going to hold off merging for day or two
and take a look at how much work it would be to export what setools
needs.
> >
> > I was going to merge these today, but upon further testing, I noticed
> > a couple of problems.
> >
> > First, creating a policy.conf from a binary like this:
> > ~/local/usr/bin/checkpolicy -F -M -b -o policy.31.conf policy.31
> > give the following error:
> > libsepol.avtab_read_item: invalid item count
>
> Hm... I can't reproduce that on my side. Are you sure you're not using
> a broken policy generated by the buggy v1 of this series?
>
No, it wasn't that. It turns out that I didn't do "make clean" between
building master and building the branch with this patch series. I
don't see any problems now.
Jim
> >
> > Second, creating a binary from a policy.conf and then trying to create
> > a policy.conf from that binary:
> > ~/local/usr/bin/checkpolicy -o policy.bin policy.conf
> > ~/local/usr/bin/checkpolicy -F -b -o policy.bin.conf policy.bin
> > gives the following error:
> > security: ebitmap: high bit (32) is not a multiple of the map size (64)
>
> Likewise, couldn't reproduce it with the conf file I tried... Can you
> share the policy.conf that triggers this?
>
> >
> > I don't know if this is related to the problems you had on the kernel
> > side or not. I'll keep looking into this, but, for now, I'll have to
> > remove my Acked-by.
> >
> > Jim
> >
> >
> > > > ---
> > > > libsepol/include/sepol/policydb/policydb.h | 3 +-
> > > > libsepol/src/policydb.c | 218 +++++++++++++++++----
> > > > libsepol/src/write.c | 72 ++++++-
> > > > 3 files changed, 242 insertions(+), 51 deletions(-)
> > > >
> > > > diff --git a/libsepol/include/sepol/policydb/policydb.h b/libsepol/include/sepol/policydb/policydb.h
> > > > index c3180c61..9ef43abc 100644
> > > > --- a/libsepol/include/sepol/policydb/policydb.h
> > > > +++ b/libsepol/include/sepol/policydb/policydb.h
> > > > @@ -755,10 +755,11 @@ extern int policydb_set_target_platform(policydb_t *p, int platform);
> > > > #define POLICYDB_VERSION_XPERMS_IOCTL 30 /* Linux-specific */
> > > > #define POLICYDB_VERSION_INFINIBAND 31 /* Linux-specific */
> > > > #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
> > > >
> > > > /* Module versions and specific changes*/
> > > > #define MOD_POLICYDB_VERSION_BASE 4
> > > > diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c
> > > > index 6b121d66..18ce1912 100644
> > > > --- a/libsepol/src/policydb.c
> > > > +++ b/libsepol/src/policydb.c
> > > > @@ -200,6 +200,13 @@ static struct policydb_compat_info policydb_compat[] = {
> > > > .ocon_num = OCON_IBENDPORT + 1,
> > > > .target_platform = SEPOL_TARGET_SELINUX,
> > > > },
> > > > + {
> > > > + .type = POLICY_KERN,
> > > > + .version = POLICYDB_VERSION_COMP_FTRANS,
> > > > + .sym_num = SYM_NUM,
> > > > + .ocon_num = OCON_IBENDPORT + 1,
> > > > + .target_platform = SEPOL_TARGET_SELINUX,
> > > > + },
> > > > {
> > > > .type = POLICY_BASE,
> > > > .version = MOD_POLICYDB_VERSION_BASE,
> > > > @@ -2661,73 +2668,202 @@ int policydb_filetrans_insert(policydb_t *p, uint32_t stype, uint32_t ttype,
> > > > return ebitmap_set_bit(&datum->stypes, stype - 1, 1);
> > > > }
> > > >
> > > > -int filename_trans_read(policydb_t *p, struct policy_file *fp)
> > > > +static int filename_trans_read_one_old(policydb_t *p, struct policy_file *fp)
> > > > {
> > > > - unsigned int i;
> > > > - uint32_t buf[4], nel, len, stype, ttype, tclass, otype;
> > > > + uint32_t buf[4], len, stype, ttype, tclass, otype;
> > > > + char *name = NULL;
> > > > int rc;
> > > > - char *name;
> > > >
> > > > rc = next_entry(buf, fp, sizeof(uint32_t));
> > > > if (rc < 0)
> > > > return -1;
> > > > - nel = le32_to_cpu(buf[0]);
> > > > + len = le32_to_cpu(buf[0]);
> > > > + if (zero_or_saturated(len))
> > > > + return -1;
> > > >
> > > > - for (i = 0; i < nel; i++) {
> > > > - name = NULL;
> > > > + name = calloc(len + 1, sizeof(*name));
> > > > + if (!name)
> > > > + return -1;
> > > >
> > > > - rc = next_entry(buf, fp, sizeof(uint32_t));
> > > > - if (rc < 0)
> > > > - goto err;
> > > > - len = le32_to_cpu(buf[0]);
> > > > - if (zero_or_saturated(len))
> > > > + rc = next_entry(name, fp, len);
> > > > + if (rc < 0)
> > > > + goto err;
> > > > +
> > > > + rc = next_entry(buf, fp, sizeof(uint32_t) * 4);
> > > > + if (rc < 0)
> > > > + goto err;
> > > > +
> > > > + stype = le32_to_cpu(buf[0]);
> > > > + ttype = le32_to_cpu(buf[1]);
> > > > + tclass = le32_to_cpu(buf[2]);
> > > > + otype = le32_to_cpu(buf[3]);
> > > > +
> > > > + rc = policydb_filetrans_insert(p, stype, ttype, tclass, name, &name,
> > > > + otype, NULL);
> > > > + if (rc) {
> > > > + if (rc != SEPOL_EEXIST)
> > > > goto err;
> > > > + /*
> > > > + * Some old policies were wrongly generated with
> > > > + * duplicate filename transition rules. For backward
> > > > + * compatibility, do not reject such policies, just
> > > > + * issue a warning and ignore the duplicate.
> > > > + */
> > > > + WARN(fp->handle,
> > > > + "Duplicate name-based type_transition %s %s:%s \"%s\": %s, ignoring",
> > > > + p->p_type_val_to_name[stype - 1],
> > > > + p->p_type_val_to_name[ttype - 1],
> > > > + p->p_class_val_to_name[tclass - 1],
> > > > + name,
> > > > + p->p_type_val_to_name[otype - 1]);
> > > > + /* continue, ignoring this one */
> > > > + }
> > > > + free(name);
> > > > + return 0;
> > > > +err:
> > > > + free(name);
> > > > + return -1;
> > > > +}
> > > > +
> > > > +static int filename_trans_check_datum(filename_trans_datum_t *datum)
> > > > +{
> > > > + ebitmap_t stypes, otypes;
> > > > +
> > > > + ebitmap_init(&stypes);
> > > > + ebitmap_init(&otypes);
> > > > +
> > > > + while (datum) {
> > > > + if (ebitmap_get_bit(&otypes, datum->otype))
> > > > + return -1;
> > > > +
> > > > + if (ebitmap_set_bit(&otypes, datum->otype, 1))
> > > > + return -1;
> > > >
> > > > - name = calloc(len + 1, sizeof(*name));
> > > > - if (!name)
> > > > + if (ebitmap_match_any(&stypes, &datum->stypes))
> > > > + return -1;
> > > > +
> > > > + if (ebitmap_union(&stypes, &datum->stypes))
> > > > + return -1;
> > > > + }
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int filename_trans_read_one_new(policydb_t *p, struct policy_file *fp)
> > > > +{
> > > > + filename_trans_key_t *ft = NULL;
> > > > + filename_trans_datum_t **dst, *datum, *first = NULL;
> > > > + unsigned int i;
> > > > + uint32_t buf[3], len, ttype, tclass, ndatum;
> > > > + char *name = NULL;
> > > > + int rc;
> > > > +
> > > > + rc = next_entry(buf, fp, sizeof(uint32_t));
> > > > + if (rc < 0)
> > > > + return -1;
> > > > + len = le32_to_cpu(buf[0]);
> > > > + if (zero_or_saturated(len))
> > > > + return -1;
> > > > +
> > > > + name = calloc(len + 1, sizeof(*name));
> > > > + if (!name)
> > > > + return -1;
> > > > +
> > > > + rc = next_entry(name, fp, len);
> > > > + if (rc < 0)
> > > > + goto err;
> > > > +
> > > > + rc = next_entry(buf, fp, sizeof(uint32_t) * 3);
> > > > + if (rc < 0)
> > > > + goto err;
> > > > +
> > > > + ttype = le32_to_cpu(buf[0]);
> > > > + tclass = le32_to_cpu(buf[1]);
> > > > + ndatum = le32_to_cpu(buf[2]);
> > > > + if (ndatum == 0)
> > > > + goto err;
> > > > +
> > > > + dst = &first;
> > > > + for (i = 0; i < ndatum; i++) {
> > > > + datum = malloc(sizeof(*datum));
> > > > + if (!datum)
> > > > goto err;
> > > >
> > > > - rc = next_entry(name, fp, len);
> > > > + *dst = datum;
> > > > +
> > > > + /* ebitmap_read() will at least init the bitmap */
> > > > + rc = ebitmap_read(&datum->stypes, fp);
> > > > if (rc < 0)
> > > > goto err;
> > > >
> > > > - rc = next_entry(buf, fp, sizeof(uint32_t) * 4);
> > > > + rc = next_entry(buf, fp, sizeof(uint32_t));
> > > > if (rc < 0)
> > > > goto err;
> > > >
> > > > - stype = le32_to_cpu(buf[0]);
> > > > - ttype = le32_to_cpu(buf[1]);
> > > > - tclass = le32_to_cpu(buf[2]);
> > > > - otype = le32_to_cpu(buf[3]);
> > > > + datum->otype = le32_to_cpu(buf[0]);
> > > >
> > > > - rc = policydb_filetrans_insert(p, stype, ttype, tclass, name,
> > > > - &name, otype, NULL);
> > > > - if (rc) {
> > > > - if (rc != SEPOL_EEXIST)
> > > > - goto err;
> > > > - /*
> > > > - * Some old policies were wrongly generated with
> > > > - * duplicate filename transition rules. For backward
> > > > - * compatibility, do not reject such policies, just
> > > > - * issue a warning and ignore the duplicate.
> > > > - */
> > > > - WARN(fp->handle,
> > > > - "Duplicate name-based type_transition %s %s:%s \"%s\": %s, ignoring",
> > > > - p->p_type_val_to_name[stype - 1],
> > > > - p->p_type_val_to_name[ttype - 1],
> > > > - p->p_class_val_to_name[tclass - 1],
> > > > - name,
> > > > - p->p_type_val_to_name[otype - 1]);
> > > > - /* continue, ignoring this one */
> > > > - }
> > > > - free(name);
> > > > + p->filename_trans_count += ebitmap_cardinality(&datum->stypes);
> > > > +
> > > > + dst = &datum->next;
> > > > }
> > > > + *dst = NULL;
> > > > +
> > > > + if (ndatum > 1 && filename_trans_check_datum(first))
> > > > + goto err;
> > > > +
> > > > + ft = malloc(sizeof(*ft));
> > > > + if (!ft)
> > > > + goto err;
> > > > +
> > > > + ft->ttype = ttype;
> > > > + ft->tclass = tclass;
> > > > + ft->name = name;
> > > > +
> > > > + rc = hashtab_insert(p->filename_trans, (hashtab_key_t)ft,
> > > > + (hashtab_datum_t)first);
> > > > + if (rc)
> > > > + goto err;
> > > > +
> > > > return 0;
> > > > err:
> > > > + free(ft);
> > > > free(name);
> > > > + while (first) {
> > > > + datum = first;
> > > > + first = first->next;
> > > > +
> > > > + ebitmap_destroy(&datum->stypes);
> > > > + free(datum);
> > > > + }
> > > > return -1;
> > > > }
> > > >
> > > > +int filename_trans_read(policydb_t *p, struct policy_file *fp)
> > > > +{
> > > > + unsigned int i;
> > > > + uint32_t buf[1], nel;
> > > > + int rc;
> > > > +
> > > > + rc = next_entry(buf, fp, sizeof(uint32_t));
> > > > + if (rc < 0)
> > > > + return -1;
> > > > + nel = le32_to_cpu(buf[0]);
> > > > +
> > > > + if (p->policyvers < POLICYDB_VERSION_COMP_FTRANS) {
> > > > + for (i = 0; i < nel; i++) {
> > > > + rc = filename_trans_read_one_old(p, fp);
> > > > + if (rc < 0)
> > > > + return -1;
> > > > + }
> > > > + } else {
> > > > + for (i = 0; i < nel; i++) {
> > > > + rc = filename_trans_read_one_new(p, fp);
> > > > + if (rc < 0)
> > > > + return -1;
> > > > + }
> > > > + }
> > > > + return 0;
> > > > +}
> > > > +
> > > > static int ocontext_read_xen(struct policydb_compat_info *info,
> > > > policydb_t *p, struct policy_file *fp)
> > > > {
> > > > diff --git a/libsepol/src/write.c b/libsepol/src/write.c
> > > > index d3aee8d5..9c74d9f5 100644
> > > > --- a/libsepol/src/write.c
> > > > +++ b/libsepol/src/write.c
> > > > @@ -569,7 +569,7 @@ static int role_allow_write(role_allow_t * r, struct policy_file *fp)
> > > > return POLICYDB_SUCCESS;
> > > > }
> > > >
> > > > -static int filename_write_helper(hashtab_key_t key, void *data, void *ptr)
> > > > +static int filename_write_one_old(hashtab_key_t key, void *data, void *ptr)
> > > > {
> > > > uint32_t bit, buf[4];
> > > > size_t items, len;
> > > > @@ -605,6 +605,54 @@ static int filename_write_helper(hashtab_key_t key, void *data, void *ptr)
> > > > return 0;
> > > > }
> > > >
> > > > +static int filename_write_one_new(hashtab_key_t key, void *data, void *ptr)
> > > > +{
> > > > + uint32_t buf[3];
> > > > + size_t items, len, ndatum;
> > > > + filename_trans_key_t *ft = (filename_trans_key_t *)key;
> > > > + filename_trans_datum_t *datum;
> > > > + void *fp = ptr;
> > > > +
> > > > + len = strlen(ft->name);
> > > > + buf[0] = cpu_to_le32(len);
> > > > + items = put_entry(buf, sizeof(uint32_t), 1, fp);
> > > > + if (items != 1)
> > > > + return POLICYDB_ERROR;
> > > > +
> > > > + items = put_entry(ft->name, sizeof(char), len, fp);
> > > > + if (items != len)
> > > > + return POLICYDB_ERROR;
> > > > +
> > > > + ndatum = 0;
> > > > + datum = data;
> > > > + do {
> > > > + ndatum++;
> > > > + datum = datum->next;
> > > > + } while (datum);
> > > > +
> > > > + buf[0] = cpu_to_le32(ft->ttype);
> > > > + buf[1] = cpu_to_le32(ft->tclass);
> > > > + buf[2] = cpu_to_le32(ndatum);
> > > > + items = put_entry(buf, sizeof(uint32_t), 3, fp);
> > > > + if (items != 3)
> > > > + return POLICYDB_ERROR;
> > > > +
> > > > + datum = data;
> > > > + do {
> > > > + if (ebitmap_write(&datum->stypes, fp))
> > > > + return POLICYDB_ERROR;
> > > > +
> > > > + buf[0] = cpu_to_le32(datum->otype);
> > > > + items = put_entry(buf, sizeof(uint32_t), 1, fp);
> > > > + if (items != 1)
> > > > + return POLICYDB_ERROR;
> > > > +
> > > > + datum = datum->next;
> > > > + } while (datum);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > static int filename_trans_write(struct policydb *p, void *fp)
> > > > {
> > > > size_t items;
> > > > @@ -614,16 +662,22 @@ static int filename_trans_write(struct policydb *p, void *fp)
> > > > if (p->policyvers < POLICYDB_VERSION_FILENAME_TRANS)
> > > > return 0;
> > > >
> > > > - buf[0] = cpu_to_le32(p->filename_trans_count);
> > > > - items = put_entry(buf, sizeof(uint32_t), 1, fp);
> > > > - if (items != 1)
> > > > - return POLICYDB_ERROR;
> > > > + if (p->policyvers < POLICYDB_VERSION_COMP_FTRANS) {
> > > > + buf[0] = cpu_to_le32(p->filename_trans_count);
> > > > + items = put_entry(buf, sizeof(uint32_t), 1, fp);
> > > > + if (items != 1)
> > > > + return POLICYDB_ERROR;
> > > >
> > > > - rc = hashtab_map(p->filename_trans, filename_write_helper, 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);
> > > > + items = put_entry(buf, sizeof(uint32_t), 1, fp);
> > > > + if (items != 1)
> > > > + return POLICYDB_ERROR;
> > > >
> > > > - return 0;
> > > > + rc = hashtab_map(p->filename_trans, filename_write_one_new, fp);
> > > > + }
> > > > + return rc;
> > > > }
> > > >
> > > > static int role_set_write(role_set_t * x, struct policy_file *fp)
> > > > --
> > > > 2.25.1
> > > >
> >
>
>
> --
> Ondrej Mosnacek <omosnace at redhat dot com>
> Software Engineer, Security Technologies
> Red Hat, Inc.
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] libsepol: implement POLICYDB_VERSION_COMP_FTRANS
2020-04-28 15:28 ` James Carter
@ 2020-04-30 14:25 ` Stephen Smalley
2020-04-30 15:06 ` James Carter
0 siblings, 1 reply; 12+ messages in thread
From: Stephen Smalley @ 2020-04-30 14:25 UTC (permalink / raw)
To: James Carter, Petr Lautrbach
Cc: Ondrej Mosnacek, SElinux list, Stephen Smalley, Chris PeBenito
On Tue, Apr 28, 2020 at 11:29 AM James Carter <jwcart2@gmail.com> wrote:
>
> On Tue, Apr 28, 2020 at 4:49 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> >
> > On Mon, Apr 27, 2020 at 9:39 PM James Carter <jwcart2@gmail.com> wrote:
> > > On Tue, Mar 31, 2020 at 4:30 PM James Carter <jwcart2@gmail.com> wrote:
> > > >
> > > > On Sat, Mar 28, 2020 at 8:46 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > > >
> > > > > Implement a new, more space-efficient form 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 representation 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.
> > > > >
> > > > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > > >
> > > > Acked-by: James Carter <jwcart2@gmail.com>
> > > >
> > >
> > > Sorry, it has been a while.
> >
> > No problem. I would've merged it myself, I just wasn't sure if we
> > shouldn't try to do something about the setools issue somehow... But I
> > really don't feel like touching that code, so if the consensus is that
> > this is worth the breakage then I'm fine with it :)
> >
>
> I thought the consensus was to apply this now, but it would be nice to
> not break setools, so I am going to hold off merging for day or two
> and take a look at how much work it would be to export what setools
> needs.
There was an earlier discussion about whether we ought to make another
selinux userspace release soon.
If so, we should defer applying this change until after that release
to avoid breaking setools with no fix
in sight for it.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] libsepol: implement POLICYDB_VERSION_COMP_FTRANS
2020-04-30 14:25 ` Stephen Smalley
@ 2020-04-30 15:06 ` James Carter
0 siblings, 0 replies; 12+ messages in thread
From: James Carter @ 2020-04-30 15:06 UTC (permalink / raw)
To: Stephen Smalley
Cc: Petr Lautrbach, Ondrej Mosnacek, SElinux list, Stephen Smalley,
Chris PeBenito
On Thu, Apr 30, 2020 at 10:25 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Tue, Apr 28, 2020 at 11:29 AM James Carter <jwcart2@gmail.com> wrote:
> >
> > On Tue, Apr 28, 2020 at 4:49 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > >
> > > On Mon, Apr 27, 2020 at 9:39 PM James Carter <jwcart2@gmail.com> wrote:
> > > > On Tue, Mar 31, 2020 at 4:30 PM James Carter <jwcart2@gmail.com> wrote:
> > > > >
> > > > > On Sat, Mar 28, 2020 at 8:46 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > > > >
> > > > > > Implement a new, more space-efficient form 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 representation 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.
> > > > > >
> > > > > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > > > >
> > > > > Acked-by: James Carter <jwcart2@gmail.com>
> > > > >
> > > >
> > > > Sorry, it has been a while.
> > >
> > > No problem. I would've merged it myself, I just wasn't sure if we
> > > shouldn't try to do something about the setools issue somehow... But I
> > > really don't feel like touching that code, so if the consensus is that
> > > this is worth the breakage then I'm fine with it :)
> > >
> >
> > I thought the consensus was to apply this now, but it would be nice to
> > not break setools, so I am going to hold off merging for day or two
> > and take a look at how much work it would be to export what setools
> > needs.
>
> There was an earlier discussion about whether we ought to make another
> selinux userspace release soon.
> If so, we should defer applying this change until after that release
> to avoid breaking setools with no fix
> in sight for it.
I agree. That discussion was what got me really thinking about what to
do about setools.
Jim
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] libsepol,checkpolicy: optimize storage of filename transitions
2020-03-28 12:45 ` [PATCH v2 1/2] libsepol,checkpolicy: optimize storage of filename transitions Ondrej Mosnacek
2020-03-31 20:30 ` James Carter
@ 2020-04-30 17:34 ` James Carter
2020-04-30 18:18 ` Ondrej Mosnacek
1 sibling, 1 reply; 12+ messages in thread
From: James Carter @ 2020-04-30 17:34 UTC (permalink / raw)
To: Ondrej Mosnacek; +Cc: SElinux list, Stephen Smalley, Chris PeBenito
On Sat, Mar 28, 2020 at 8:46 AM Ondrej Mosnacek <omosnace@redhat.com> 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@redhat.com>
> ---
> 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.
Jim
> static void display_filename_trans(policydb_t *p, FILE *fp)
> {
> diff --git a/libsepol/cil/src/cil_binary.c b/libsepol/cil/src/cil_binary.c
> index 62178d99..bedff628 100644
> --- a/libsepol/cil/src/cil_binary.c
> +++ b/libsepol/cil/src/cil_binary.c
> @@ -1131,13 +1131,13 @@ int __cil_typetransition_to_avtab(policydb_t *pdb, const struct cil_db *db, stru
> class_datum_t *sepol_obj = NULL;
> struct cil_list *class_list;
> type_datum_t *sepol_result = NULL;
> - filename_trans_t *newkey = NULL;
> - filename_trans_datum_t *newdatum = NULL, *otype = NULL;
> ebitmap_t src_bitmap, tgt_bitmap;
> ebitmap_node_t *node1, *node2;
> unsigned int i, j;
> + uint32_t otype;
> struct cil_list_item *c;
> char *name = DATUM(typetrans->name)->name;
> + char *dup_name;
>
> if (name == CIL_KEY_STAR) {
> struct cil_type_rule trans;
> @@ -1176,22 +1176,16 @@ int __cil_typetransition_to_avtab(policydb_t *pdb, const struct cil_db *db, stru
> rc = __cil_get_sepol_class_datum(pdb, DATUM(c->data), &sepol_obj);
> if (rc != SEPOL_OK) goto exit;
>
> - newkey = cil_calloc(1, sizeof(*newkey));
> - newdatum = cil_calloc(1, sizeof(*newdatum));
> - newkey->stype = sepol_src->s.value;
> - newkey->ttype = sepol_tgt->s.value;
> - newkey->tclass = sepol_obj->s.value;
> - newkey->name = cil_strdup(name);
> - newdatum->otype = sepol_result->s.value;
> -
> - rc = hashtab_insert(pdb->filename_trans,
> - (hashtab_key_t)newkey,
> - newdatum);
> + dup_name = NULL;
> + rc = policydb_filetrans_insert(
> + pdb, sepol_src->s.value, sepol_tgt->s.value,
> + sepol_obj->s.value, name, &dup_name,
> + sepol_result->s.value, &otype
> + );
> + free(dup_name);
> if (rc != SEPOL_OK) {
> if (rc == SEPOL_EEXIST) {
> - otype = hashtab_search(pdb->filename_trans,
> - (hashtab_key_t)newkey);
> - if (newdatum->otype != otype->otype) {
> + if (sepol_result->s.value!= otype) {
> cil_log(CIL_ERR, "Conflicting name type transition rules\n");
> } else {
> rc = SEPOL_OK;
> @@ -1199,9 +1193,6 @@ int __cil_typetransition_to_avtab(policydb_t *pdb, const struct cil_db *db, stru
> } else {
> cil_log(CIL_ERR, "Out of memory\n");
> }
> - free(newkey->name);
> - free(newkey);
> - free(newdatum);
> if (rc != SEPOL_OK) {
> goto exit;
> }
> diff --git a/libsepol/include/sepol/policydb/policydb.h b/libsepol/include/sepol/policydb/policydb.h
> index 81b63fef..c3180c61 100644
> --- a/libsepol/include/sepol/policydb/policydb.h
> +++ b/libsepol/include/sepol/policydb/policydb.h
> @@ -162,15 +162,16 @@ typedef struct role_allow {
> } role_allow_t;
>
> /* filename_trans rules */
> -typedef struct filename_trans {
> - uint32_t stype;
> +typedef struct filename_trans_key {
> uint32_t ttype;
> uint32_t tclass;
> char *name;
> -} filename_trans_t;
> +} filename_trans_key_t;
>
> typedef struct filename_trans_datum {
> - uint32_t otype; /* expected of new object */
> + ebitmap_t stypes;
> + uint32_t otype;
> + struct filename_trans_datum *next;
> } filename_trans_datum_t;
>
> /* Type attributes */
> @@ -591,6 +592,7 @@ typedef struct policydb {
>
> /* file transitions with the last path component */
> hashtab_t filename_trans;
> + uint32_t filename_trans_count;
>
> ebitmap_t *type_attr_map;
>
> @@ -650,6 +652,11 @@ extern int policydb_load_isids(policydb_t * p, sidtab_t * s);
>
> extern int policydb_sort_ocontexts(policydb_t *p);
>
> +extern int policydb_filetrans_insert(policydb_t *p, uint32_t stype,
> + uint32_t ttype, uint32_t tclass,
> + const char *name, char **name_alloc,
> + uint32_t otype, uint32_t *present_otype);
> +
> /* Deprecated */
> extern int policydb_context_isvalid(const policydb_t * p,
> const context_struct_t * c);
> diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c
> index 529e1d35..28f93acb 100644
> --- a/libsepol/src/expand.c
> +++ b/libsepol/src/expand.c
> @@ -1371,16 +1371,15 @@ static int copy_role_trans(expand_state_t * state, role_trans_rule_t * rules)
> static int expand_filename_trans(expand_state_t *state, filename_trans_rule_t *rules)
> {
> unsigned int i, j;
> - filename_trans_t key, *new_trans;
> - filename_trans_datum_t *otype;
> filename_trans_rule_t *cur_rule;
> ebitmap_t stypes, ttypes;
> ebitmap_node_t *snode, *tnode;
> + char *name;
> int rc;
>
> cur_rule = rules;
> while (cur_rule) {
> - uint32_t mapped_otype;
> + uint32_t mapped_otype, present_otype;
>
> ebitmap_init(&stypes);
> ebitmap_init(&ttypes);
> @@ -1401,15 +1400,17 @@ static int expand_filename_trans(expand_state_t *state, filename_trans_rule_t *r
>
> ebitmap_for_each_positive_bit(&stypes, snode, i) {
> ebitmap_for_each_positive_bit(&ttypes, tnode, j) {
> - key.stype = i + 1;
> - key.ttype = j + 1;
> - key.tclass = cur_rule->tclass;
> - key.name = cur_rule->name;
> - otype = hashtab_search(state->out->filename_trans,
> - (hashtab_key_t) &key);
> - if (otype) {
> + name = NULL;
> +
> + rc = policydb_filetrans_insert(
> + state->out, i + 1, j + 1,
> + cur_rule->tclass, cur_rule->name,
> + &name, mapped_otype, &present_otype
> + );
> + free(name);
> + if (rc == SEPOL_EEXIST) {
> /* duplicate rule, ignore */
> - if (otype->otype == mapped_otype)
> + if (present_otype == mapped_otype)
> continue;
>
> ERR(state->handle, "Conflicting name-based type_transition %s %s:%s \"%s\": %s vs %s",
> @@ -1417,44 +1418,11 @@ static int expand_filename_trans(expand_state_t *state, filename_trans_rule_t *r
> state->out->p_type_val_to_name[j],
> state->out->p_class_val_to_name[cur_rule->tclass - 1],
> cur_rule->name,
> - state->out->p_type_val_to_name[otype->otype - 1],
> + state->out->p_type_val_to_name[present_otype - 1],
> state->out->p_type_val_to_name[mapped_otype - 1]);
> return -1;
> - }
> -
> - new_trans = calloc(1, sizeof(*new_trans));
> - if (!new_trans) {
> - ERR(state->handle, "Out of memory!");
> - return -1;
> - }
> -
> - new_trans->name = strdup(cur_rule->name);
> - if (!new_trans->name) {
> - ERR(state->handle, "Out of memory!");
> - free(new_trans);
> - return -1;
> - }
> - new_trans->stype = i + 1;
> - new_trans->ttype = j + 1;
> - new_trans->tclass = cur_rule->tclass;
> -
> - otype = calloc(1, sizeof(*otype));
> - if (!otype) {
> - ERR(state->handle, "Out of memory!");
> - free(new_trans->name);
> - free(new_trans);
> - return -1;
> - }
> - otype->otype = mapped_otype;
> -
> - rc = hashtab_insert(state->out->filename_trans,
> - (hashtab_key_t)new_trans,
> - otype);
> - if (rc) {
> + } else if (rc < 0) {
> ERR(state->handle, "Out of memory!");
> - free(otype);
> - free(new_trans->name);
> - free(new_trans);
> return -1;
> }
> }
> diff --git a/libsepol/src/kernel_to_cil.c b/libsepol/src/kernel_to_cil.c
> index ede78a20..718d3481 100644
> --- a/libsepol/src/kernel_to_cil.c
> +++ b/libsepol/src/kernel_to_cil.c
> @@ -1821,21 +1821,35 @@ struct map_filename_trans_args {
>
> static int map_filename_trans_to_str(hashtab_key_t key, void *data, void *arg)
> {
> - filename_trans_t *ft = (filename_trans_t *)key;
> + filename_trans_key_t *ft = (filename_trans_key_t *)key;
> filename_trans_datum_t *datum = data;
> struct map_filename_trans_args *map_args = arg;
> struct policydb *pdb = map_args->pdb;
> struct strs *strs = map_args->strs;
> char *src, *tgt, *class, *filename, *new;
> + struct ebitmap_node *node;
> + uint32_t bit;
> + int rc;
>
> - src = pdb->p_type_val_to_name[ft->stype - 1];
> tgt = pdb->p_type_val_to_name[ft->ttype - 1];
> class = pdb->p_class_val_to_name[ft->tclass - 1];
> filename = ft->name;
> - new = pdb->p_type_val_to_name[datum->otype - 1];
> + do {
> + new = pdb->p_type_val_to_name[datum->otype - 1];
> +
> + ebitmap_for_each_positive_bit(&datum->stypes, node, bit) {
> + src = pdb->p_type_val_to_name[bit];
> + rc = strs_create_and_add(strs,
> + "(typetransition %s %s %s %s %s)",
> + 5, src, tgt, class, filename, new);
> + if (rc)
> + return rc;
> + }
> +
> + datum = datum->next;
> + } while (datum);
>
> - return strs_create_and_add(strs, "(typetransition %s %s %s %s %s)", 5,
> - src, tgt, class, filename, new);
> + return 0;
> }
>
> static int write_filename_trans_rules_to_cil(FILE *out, struct policydb *pdb)
> diff --git a/libsepol/src/kernel_to_conf.c b/libsepol/src/kernel_to_conf.c
> index 9de64832..9fb3ed22 100644
> --- a/libsepol/src/kernel_to_conf.c
> +++ b/libsepol/src/kernel_to_conf.c
> @@ -1801,21 +1801,35 @@ struct map_filename_trans_args {
>
> static int map_filename_trans_to_str(hashtab_key_t key, void *data, void *arg)
> {
> - filename_trans_t *ft = (filename_trans_t *)key;
> + filename_trans_key_t *ft = (filename_trans_key_t *)key;
> filename_trans_datum_t *datum = data;
> struct map_filename_trans_args *map_args = arg;
> struct policydb *pdb = map_args->pdb;
> struct strs *strs = map_args->strs;
> char *src, *tgt, *class, *filename, *new;
> + struct ebitmap_node *node;
> + uint32_t bit;
> + int rc;
>
> - src = pdb->p_type_val_to_name[ft->stype - 1];
> tgt = pdb->p_type_val_to_name[ft->ttype - 1];
> class = pdb->p_class_val_to_name[ft->tclass - 1];
> filename = ft->name;
> - new = pdb->p_type_val_to_name[datum->otype - 1];
> + do {
> + new = pdb->p_type_val_to_name[datum->otype - 1];
> +
> + ebitmap_for_each_positive_bit(&datum->stypes, node, bit) {
> + src = pdb->p_type_val_to_name[bit];
> + rc = strs_create_and_add(strs,
> + "type_transition %s %s:%s %s \"%s\";",
> + 5, src, tgt, class, new, filename);
> + if (rc)
> + return rc;
> + }
> +
> + datum = datum->next;
> + } while (datum);
>
> - return strs_create_and_add(strs, "type_transition %s %s:%s %s \"%s\";", 5,
> - src, tgt, class, new, filename);
> + return 0;
> }
>
> static int write_filename_trans_rules_to_conf(FILE *out, struct policydb *pdb)
> diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c
> index 5b289a52..6b121d66 100644
> --- a/libsepol/src/policydb.c
> +++ b/libsepol/src/policydb.c
> @@ -789,12 +789,12 @@ partial_name_hash(unsigned long c, unsigned long prevhash)
>
> static unsigned int filenametr_hash(hashtab_t h, const_hashtab_key_t k)
> {
> - const struct filename_trans *ft = (const struct filename_trans *)k;
> + const filename_trans_key_t *ft = (const filename_trans_key_t *)k;
> unsigned long hash;
> unsigned int byte_num;
> unsigned char focus;
>
> - hash = ft->stype ^ ft->ttype ^ ft->tclass;
> + hash = ft->ttype ^ ft->tclass;
>
> byte_num = 0;
> while ((focus = ft->name[byte_num++]))
> @@ -805,14 +805,10 @@ static unsigned int filenametr_hash(hashtab_t h, const_hashtab_key_t k)
> static int filenametr_cmp(hashtab_t h __attribute__ ((unused)),
> const_hashtab_key_t k1, const_hashtab_key_t k2)
> {
> - const struct filename_trans *ft1 = (const struct filename_trans *)k1;
> - const struct filename_trans *ft2 = (const struct filename_trans *)k2;
> + const filename_trans_key_t *ft1 = (const filename_trans_key_t *)k1;
> + const filename_trans_key_t *ft2 = (const filename_trans_key_t *)k2;
> int v;
>
> - v = ft1->stype - ft2->stype;
> - if (v)
> - return v;
> -
> v = ft1->ttype - ft2->ttype;
> if (v)
> return v;
> @@ -1409,9 +1405,12 @@ common_destroy, class_destroy, role_destroy, type_destroy, user_destroy,
> static int filenametr_destroy(hashtab_key_t key, hashtab_datum_t datum,
> void *p __attribute__ ((unused)))
> {
> - struct filename_trans *ft = (struct filename_trans *)key;
> + filename_trans_key_t *ft = (filename_trans_key_t *)key;
> + filename_trans_datum_t *fd = datum;
> +
> free(ft->name);
> free(key);
> + ebitmap_destroy(&fd->stypes);
> free(datum);
> return 0;
> }
> @@ -2595,12 +2594,77 @@ int role_allow_read(role_allow_t ** r, struct policy_file *fp)
> return 0;
> }
>
> +int policydb_filetrans_insert(policydb_t *p, uint32_t stype, uint32_t ttype,
> + uint32_t tclass, const char *name,
> + char **name_alloc, uint32_t otype,
> + uint32_t *present_otype)
> +{
> + filename_trans_key_t *ft, key;
> + filename_trans_datum_t *datum, *last;
> +
> + key.ttype = ttype;
> + key.tclass = tclass;
> + key.name = (char *)name;
> +
> + last = NULL;
> + datum = hashtab_search(p->filename_trans, (hashtab_key_t)&key);
> + while (datum) {
> + if (ebitmap_get_bit(&datum->stypes, stype - 1)) {
> + if (present_otype)
> + *present_otype = datum->otype;
> + return SEPOL_EEXIST;
> + }
> + if (datum->otype == otype)
> + break;
> + last = datum;
> + datum = datum->next;
> + }
> + if (!datum) {
> + if (!*name_alloc) {
> + *name_alloc = strdup(name);
> + if (!*name_alloc)
> + return SEPOL_ENOMEM;
> + }
> +
> + datum = malloc(sizeof(*datum));
> + if (!datum)
> + return SEPOL_ENOMEM;
> +
> + ebitmap_init(&datum->stypes);
> + datum->otype = otype;
> + datum->next = NULL;
> +
> + if (last) {
> + last->next = datum;
> + } else {
> + ft = malloc(sizeof(*ft));
> + if (!ft) {
> + free(datum);
> + return SEPOL_ENOMEM;
> + }
> +
> + ft->ttype = ttype;
> + ft->tclass = tclass;
> + ft->name = *name_alloc;
> +
> + if (hashtab_insert(p->filename_trans, (hashtab_key_t)ft,
> + (hashtab_datum_t)datum)) {
> + free(datum);
> + free(ft);
> + return SEPOL_ENOMEM;
> + }
> + *name_alloc = NULL;
> + }
> + }
> +
> + p->filename_trans_count++;
> + return ebitmap_set_bit(&datum->stypes, stype - 1, 1);
> +}
> +
> int filename_trans_read(policydb_t *p, struct policy_file *fp)
> {
> unsigned int i;
> - uint32_t buf[4], nel, len;
> - filename_trans_t *ft;
> - filename_trans_datum_t *otype;
> + uint32_t buf[4], nel, len, stype, ttype, tclass, otype;
> int rc;
> char *name;
>
> @@ -2610,16 +2674,8 @@ int filename_trans_read(policydb_t *p, struct policy_file *fp)
> nel = le32_to_cpu(buf[0]);
>
> for (i = 0; i < nel; i++) {
> - ft = NULL;
> - otype = NULL;
> name = NULL;
>
> - ft = calloc(1, sizeof(*ft));
> - if (!ft)
> - goto err;
> - otype = calloc(1, sizeof(*otype));
> - if (!otype)
> - goto err;
> rc = next_entry(buf, fp, sizeof(uint32_t));
> if (rc < 0)
> goto err;
> @@ -2631,8 +2687,6 @@ int filename_trans_read(policydb_t *p, struct policy_file *fp)
> if (!name)
> goto err;
>
> - ft->name = name;
> -
> rc = next_entry(name, fp, len);
> if (rc < 0)
> goto err;
> @@ -2641,13 +2695,13 @@ int filename_trans_read(policydb_t *p, struct policy_file *fp)
> if (rc < 0)
> goto err;
>
> - ft->stype = le32_to_cpu(buf[0]);
> - ft->ttype = le32_to_cpu(buf[1]);
> - ft->tclass = le32_to_cpu(buf[2]);
> - otype->otype = le32_to_cpu(buf[3]);
> + stype = le32_to_cpu(buf[0]);
> + ttype = le32_to_cpu(buf[1]);
> + tclass = le32_to_cpu(buf[2]);
> + otype = le32_to_cpu(buf[3]);
>
> - rc = hashtab_insert(p->filename_trans, (hashtab_key_t) ft,
> - otype);
> + rc = policydb_filetrans_insert(p, stype, ttype, tclass, name,
> + &name, otype, NULL);
> if (rc) {
> if (rc != SEPOL_EEXIST)
> goto err;
> @@ -2659,21 +2713,17 @@ int filename_trans_read(policydb_t *p, struct policy_file *fp)
> */
> WARN(fp->handle,
> "Duplicate name-based type_transition %s %s:%s \"%s\": %s, ignoring",
> - p->p_type_val_to_name[ft->stype - 1],
> - p->p_type_val_to_name[ft->ttype - 1],
> - p->p_class_val_to_name[ft->tclass - 1],
> - ft->name,
> - p->p_type_val_to_name[otype->otype - 1]);
> - free(ft);
> - free(name);
> - free(otype);
> + p->p_type_val_to_name[stype - 1],
> + p->p_type_val_to_name[ttype - 1],
> + p->p_class_val_to_name[tclass - 1],
> + name,
> + p->p_type_val_to_name[otype - 1]);
> /* continue, ignoring this one */
> }
> + free(name);
> }
> return 0;
> err:
> - free(ft);
> - free(otype);
> free(name);
> return -1;
> }
> diff --git a/libsepol/src/write.c b/libsepol/src/write.c
> index 1fd6a16a..d3aee8d5 100644
> --- a/libsepol/src/write.c
> +++ b/libsepol/src/write.c
> @@ -571,44 +571,50 @@ static int role_allow_write(role_allow_t * r, struct policy_file *fp)
>
> static int filename_write_helper(hashtab_key_t key, void *data, void *ptr)
> {
> - uint32_t buf[4];
> + uint32_t bit, buf[4];
> size_t items, len;
> - struct filename_trans *ft = (struct filename_trans *)key;
> - struct filename_trans_datum *otype = data;
> + filename_trans_key_t *ft = (filename_trans_key_t *)key;
> + filename_trans_datum_t *datum = data;
> + ebitmap_node_t *node;
> void *fp = ptr;
>
> len = strlen(ft->name);
> - buf[0] = cpu_to_le32(len);
> - items = put_entry(buf, sizeof(uint32_t), 1, fp);
> - if (items != 1)
> - return POLICYDB_ERROR;
> + do {
> + ebitmap_for_each_positive_bit(&datum->stypes, node, bit) {
> + buf[0] = cpu_to_le32(len);
> + items = put_entry(buf, sizeof(uint32_t), 1, fp);
> + if (items != 1)
> + return POLICYDB_ERROR;
>
> - items = put_entry(ft->name, sizeof(char), len, fp);
> - if (items != len)
> - return POLICYDB_ERROR;
> + items = put_entry(ft->name, sizeof(char), len, fp);
> + if (items != len)
> + return POLICYDB_ERROR;
>
> - buf[0] = cpu_to_le32(ft->stype);
> - buf[1] = cpu_to_le32(ft->ttype);
> - buf[2] = cpu_to_le32(ft->tclass);
> - buf[3] = cpu_to_le32(otype->otype);
> - items = put_entry(buf, sizeof(uint32_t), 4, fp);
> - if (items != 4)
> - return POLICYDB_ERROR;
> + buf[0] = cpu_to_le32(bit + 1);
> + buf[1] = cpu_to_le32(ft->ttype);
> + buf[2] = cpu_to_le32(ft->tclass);
> + buf[3] = cpu_to_le32(datum->otype);
> + items = put_entry(buf, sizeof(uint32_t), 4, fp);
> + if (items != 4)
> + return POLICYDB_ERROR;
> + }
> +
> + datum = datum->next;
> + } while (datum);
>
> return 0;
> }
>
> static int filename_trans_write(struct policydb *p, void *fp)
> {
> - size_t nel, items;
> + size_t items;
> uint32_t buf[1];
> int rc;
>
> if (p->policyvers < POLICYDB_VERSION_FILENAME_TRANS)
> return 0;
>
> - nel = p->filename_trans->nel;
> - buf[0] = cpu_to_le32(nel);
> + buf[0] = cpu_to_le32(p->filename_trans_count);
> items = put_entry(buf, sizeof(uint32_t), 1, fp);
> if (items != 1)
> return POLICYDB_ERROR;
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] libsepol,checkpolicy: optimize storage of filename transitions
2020-04-30 17:34 ` James Carter
@ 2020-04-30 18:18 ` Ondrej Mosnacek
0 siblings, 0 replies; 12+ messages in thread
From: Ondrej Mosnacek @ 2020-04-30 18:18 UTC (permalink / raw)
To: James Carter; +Cc: SElinux list, Stephen Smalley, Chris PeBenito
On Thu, Apr 30, 2020 at 7:35 PM James Carter <jwcart2@gmail.com> wrote:
> On Sat, Mar 28, 2020 at 8:46 AM Ondrej Mosnacek <omosnace@redhat.com> 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@redhat.com>
> > ---
> > 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...
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-04-30 18:19 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-28 12:45 [PATCH v2 0/2] userspace: Implement new format of filename trans rules Ondrej Mosnacek
2020-03-28 12:45 ` [PATCH v2 1/2] libsepol,checkpolicy: optimize storage of filename transitions Ondrej Mosnacek
2020-03-31 20:30 ` James Carter
2020-04-30 17:34 ` James Carter
2020-04-30 18:18 ` Ondrej Mosnacek
2020-03-28 12:45 ` [PATCH v2 2/2] libsepol: implement POLICYDB_VERSION_COMP_FTRANS Ondrej Mosnacek
2020-03-31 20:30 ` James Carter
2020-04-27 19:39 ` James Carter
2020-04-28 8:49 ` Ondrej Mosnacek
2020-04-28 15:28 ` James Carter
2020-04-30 14:25 ` Stephen Smalley
2020-04-30 15:06 ` James Carter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.