* [PATCH v2 0/3] selinux: policydb - fix memory leak and do some cleanup
@ 2019-07-29 8:41 Ondrej Mosnacek
2019-07-29 8:41 ` [PATCH v2 1/3] selinux: policydb - fix memory leak in policydb_init() Ondrej Mosnacek
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Ondrej Mosnacek @ 2019-07-29 8:41 UTC (permalink / raw)
To: selinux, Paul Moore; +Cc: Dmitry Vyukov
Cahnges in v2:
- move code around to avoid a forward declaration
- add two patches to clean up checpatch.pl warnings detected in the
moved code
v1: https://lore.kernel.org/selinux/20190725105243.28404-1-omosnace@redhat.com/T/
Ondrej Mosnacek (3):
selinux: policydb - fix memory leak in policydb_init()
selinux: policydb - fix some checkpatch.pl warnings
selinux: policydb - rename type_val_to_struct_array
security/selinux/ss/policydb.c | 980 +++++++++++++++++----------------
security/selinux/ss/policydb.h | 2 +-
security/selinux/ss/services.c | 6 +-
3 files changed, 497 insertions(+), 491 deletions(-)
--
2.21.0
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 1/3] selinux: policydb - fix memory leak in policydb_init()
2019-07-29 8:41 [PATCH v2 0/3] selinux: policydb - fix memory leak and do some cleanup Ondrej Mosnacek
@ 2019-07-29 8:41 ` Ondrej Mosnacek
2019-07-29 22:48 ` Paul Moore
2019-07-29 8:41 ` [PATCH v2 2/3] selinux: policydb - fix some checkpatch.pl warnings Ondrej Mosnacek
2019-07-29 8:41 ` [PATCH v2 3/3] selinux: policydb - rename type_val_to_struct_array Ondrej Mosnacek
2 siblings, 1 reply; 12+ messages in thread
From: Ondrej Mosnacek @ 2019-07-29 8:41 UTC (permalink / raw)
To: selinux, Paul Moore; +Cc: Dmitry Vyukov, syzbot+fee3a14d4cdf92646287
Since roles_init() adds some entries to the role hash table, we need to
destroy also its keys/values on error, otherwise we get a memory leak in
the error path.
To avoid a forward declaration and maintain a sane layout, move all the
destroy stuff above policydb_init. No changes are made to the moved code
in this patch. Note that this triggers some pre-existing checkpatch.pl
warnings - these will be fixed in follow-up patches.
Reported-by: syzbot+fee3a14d4cdf92646287@syzkaller.appspotmail.com
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
security/selinux/ss/policydb.c | 976 +++++++++++++++++----------------
1 file changed, 489 insertions(+), 487 deletions(-)
diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index daecdfb15a9c..451f2bcd2d83 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -179,664 +179,666 @@ static struct policydb_compat_info *policydb_lookup_compat(int version)
}
/*
- * Initialize the role table.
+ * The following *_destroy functions are used to
+ * free any memory allocated for each kind of
+ * symbol data in the policy database.
*/
-static int roles_init(struct policydb *p)
+
+static int perm_destroy(void *key, void *datum, void *p)
{
- char *key = NULL;
- int rc;
- struct role_datum *role;
+ kfree(key);
+ kfree(datum);
+ return 0;
+}
- role = kzalloc(sizeof(*role), GFP_KERNEL);
- if (!role)
- return -ENOMEM;
+static int common_destroy(void *key, void *datum, void *p)
+{
+ struct common_datum *comdatum;
- rc = -EINVAL;
- role->value = ++p->p_roles.nprim;
- if (role->value != OBJECT_R_VAL)
- goto out;
+ kfree(key);
+ if (datum) {
+ comdatum = datum;
+ hashtab_map(comdatum->permissions.table, perm_destroy, NULL);
+ hashtab_destroy(comdatum->permissions.table);
+ }
+ kfree(datum);
+ return 0;
+}
- rc = -ENOMEM;
- key = kstrdup(OBJECT_R, GFP_KERNEL);
- if (!key)
- goto out;
+static void constraint_expr_destroy(struct constraint_expr *expr)
+{
+ if (expr) {
+ ebitmap_destroy(&expr->names);
+ if (expr->type_names) {
+ ebitmap_destroy(&expr->type_names->types);
+ ebitmap_destroy(&expr->type_names->negset);
+ kfree(expr->type_names);
+ }
+ kfree(expr);
+ }
+}
- rc = hashtab_insert(p->p_roles.table, key, role);
- if (rc)
- goto out;
+static int cls_destroy(void *key, void *datum, void *p)
+{
+ struct class_datum *cladatum;
+ struct constraint_node *constraint, *ctemp;
+ struct constraint_expr *e, *etmp;
- return 0;
-out:
kfree(key);
- kfree(role);
- return rc;
+ if (datum) {
+ cladatum = datum;
+ hashtab_map(cladatum->permissions.table, perm_destroy, NULL);
+ hashtab_destroy(cladatum->permissions.table);
+ constraint = cladatum->constraints;
+ while (constraint) {
+ e = constraint->expr;
+ while (e) {
+ etmp = e;
+ e = e->next;
+ constraint_expr_destroy(etmp);
+ }
+ ctemp = constraint;
+ constraint = constraint->next;
+ kfree(ctemp);
+ }
+
+ constraint = cladatum->validatetrans;
+ while (constraint) {
+ e = constraint->expr;
+ while (e) {
+ etmp = e;
+ e = e->next;
+ constraint_expr_destroy(etmp);
+ }
+ ctemp = constraint;
+ constraint = constraint->next;
+ kfree(ctemp);
+ }
+ kfree(cladatum->comkey);
+ }
+ kfree(datum);
+ return 0;
}
-static u32 filenametr_hash(struct hashtab *h, const void *k)
+static int role_destroy(void *key, void *datum, void *p)
{
- const struct filename_trans *ft = k;
- unsigned long hash;
- unsigned int byte_num;
- unsigned char focus;
-
- hash = ft->stype ^ ft->ttype ^ ft->tclass;
+ struct role_datum *role;
- byte_num = 0;
- while ((focus = ft->name[byte_num++]))
- hash = partial_name_hash(focus, hash);
- return hash & (h->size - 1);
+ kfree(key);
+ if (datum) {
+ role = datum;
+ ebitmap_destroy(&role->dominates);
+ ebitmap_destroy(&role->types);
+ }
+ kfree(datum);
+ return 0;
}
-static int filenametr_cmp(struct hashtab *h, const void *k1, const void *k2)
+static int type_destroy(void *key, void *datum, void *p)
{
- const struct filename_trans *ft1 = k1;
- const struct filename_trans *ft2 = k2;
- int v;
-
- v = ft1->stype - ft2->stype;
- if (v)
- return v;
+ kfree(key);
+ kfree(datum);
+ return 0;
+}
- v = ft1->ttype - ft2->ttype;
- if (v)
- return v;
+static int user_destroy(void *key, void *datum, void *p)
+{
+ struct user_datum *usrdatum;
- v = ft1->tclass - ft2->tclass;
- if (v)
- return v;
+ kfree(key);
+ if (datum) {
+ usrdatum = datum;
+ ebitmap_destroy(&usrdatum->roles);
+ ebitmap_destroy(&usrdatum->range.level[0].cat);
+ ebitmap_destroy(&usrdatum->range.level[1].cat);
+ ebitmap_destroy(&usrdatum->dfltlevel.cat);
+ }
+ kfree(datum);
+ return 0;
+}
- return strcmp(ft1->name, ft2->name);
+static int sens_destroy(void *key, void *datum, void *p)
+{
+ struct level_datum *levdatum;
+ kfree(key);
+ if (datum) {
+ levdatum = datum;
+ if (levdatum->level)
+ ebitmap_destroy(&levdatum->level->cat);
+ kfree(levdatum->level);
+ }
+ kfree(datum);
+ return 0;
}
-static u32 rangetr_hash(struct hashtab *h, const void *k)
+static int cat_destroy(void *key, void *datum, void *p)
{
- const struct range_trans *key = k;
- return (key->source_type + (key->target_type << 3) +
- (key->target_class << 5)) & (h->size - 1);
+ kfree(key);
+ kfree(datum);
+ return 0;
}
-static int rangetr_cmp(struct hashtab *h, const void *k1, const void *k2)
+static int (*destroy_f[SYM_NUM]) (void *key, void *datum, void *datap) =
{
- const struct range_trans *key1 = k1, *key2 = k2;
- int v;
+ common_destroy,
+ cls_destroy,
+ role_destroy,
+ type_destroy,
+ user_destroy,
+ cond_destroy_bool,
+ sens_destroy,
+ cat_destroy,
+};
- v = key1->source_type - key2->source_type;
- if (v)
- return v;
+static int filenametr_destroy(void *key, void *datum, void *p)
+{
+ struct filename_trans *ft = key;
+ kfree(ft->name);
+ kfree(key);
+ kfree(datum);
+ cond_resched();
+ return 0;
+}
- v = key1->target_type - key2->target_type;
- if (v)
- return v;
+static int range_tr_destroy(void *key, void *datum, void *p)
+{
+ struct mls_range *rt = datum;
+ kfree(key);
+ ebitmap_destroy(&rt->level[0].cat);
+ ebitmap_destroy(&rt->level[1].cat);
+ kfree(datum);
+ cond_resched();
+ return 0;
+}
- v = key1->target_class - key2->target_class;
+static void ocontext_destroy(struct ocontext *c, int i)
+{
+ if (!c)
+ return;
- return v;
+ context_destroy(&c->context[0]);
+ context_destroy(&c->context[1]);
+ if (i == OCON_ISID || i == OCON_FS ||
+ i == OCON_NETIF || i == OCON_FSUSE)
+ kfree(c->u.name);
+ kfree(c);
}
/*
- * Initialize a policy database structure.
+ * Free any memory allocated by a policy database structure.
*/
-static int policydb_init(struct policydb *p)
+void policydb_destroy(struct policydb *p)
{
- int i, rc;
-
- memset(p, 0, sizeof(*p));
+ struct ocontext *c, *ctmp;
+ struct genfs *g, *gtmp;
+ int i;
+ struct role_allow *ra, *lra = NULL;
+ struct role_trans *tr, *ltr = NULL;
for (i = 0; i < SYM_NUM; i++) {
- rc = symtab_init(&p->symtab[i], symtab_sizes[i]);
- if (rc)
- goto out;
+ cond_resched();
+ hashtab_map(p->symtab[i].table, destroy_f[i], NULL);
+ hashtab_destroy(p->symtab[i].table);
}
- rc = avtab_init(&p->te_avtab);
- if (rc)
- goto out;
+ for (i = 0; i < SYM_NUM; i++)
+ kvfree(p->sym_val_to_name[i]);
- rc = roles_init(p);
- if (rc)
- goto out;
+ kfree(p->class_val_to_struct);
+ kfree(p->role_val_to_struct);
+ kfree(p->user_val_to_struct);
+ kvfree(p->type_val_to_struct_array);
- rc = cond_policydb_init(p);
- if (rc)
- goto out;
+ avtab_destroy(&p->te_avtab);
- p->filename_trans = hashtab_create(filenametr_hash, filenametr_cmp, (1 << 10));
- if (!p->filename_trans) {
- rc = -ENOMEM;
- goto out;
+ for (i = 0; i < OCON_NUM; i++) {
+ cond_resched();
+ c = p->ocontexts[i];
+ while (c) {
+ ctmp = c;
+ c = c->next;
+ ocontext_destroy(ctmp, i);
+ }
+ p->ocontexts[i] = NULL;
}
- p->range_tr = hashtab_create(rangetr_hash, rangetr_cmp, 256);
- if (!p->range_tr) {
- rc = -ENOMEM;
- goto out;
+ g = p->genfs;
+ while (g) {
+ cond_resched();
+ kfree(g->fstype);
+ c = g->head;
+ while (c) {
+ ctmp = c;
+ c = c->next;
+ ocontext_destroy(ctmp, OCON_FSUSE);
+ }
+ gtmp = g;
+ g = g->next;
+ kfree(gtmp);
}
+ p->genfs = NULL;
- ebitmap_init(&p->filename_trans_ttypes);
- ebitmap_init(&p->policycaps);
- ebitmap_init(&p->permissive_map);
-
- return 0;
-out:
- hashtab_destroy(p->filename_trans);
- hashtab_destroy(p->range_tr);
- for (i = 0; i < SYM_NUM; i++)
- hashtab_destroy(p->symtab[i].table);
- return rc;
-}
-
-/*
- * The following *_index functions are used to
- * define the val_to_name and val_to_struct arrays
- * in a policy database structure. The val_to_name
- * arrays are used when converting security context
- * structures into string representations. The
- * val_to_struct arrays are used when the attributes
- * of a class, role, or user are needed.
- */
-
-static int common_index(void *key, void *datum, void *datap)
-{
- struct policydb *p;
- struct common_datum *comdatum;
+ cond_policydb_destroy(p);
- comdatum = datum;
- p = datap;
- if (!comdatum->value || comdatum->value > p->p_commons.nprim)
- return -EINVAL;
+ for (tr = p->role_tr; tr; tr = tr->next) {
+ cond_resched();
+ kfree(ltr);
+ ltr = tr;
+ }
+ kfree(ltr);
- p->sym_val_to_name[SYM_COMMONS][comdatum->value - 1] = key;
+ for (ra = p->role_allow; ra; ra = ra->next) {
+ cond_resched();
+ kfree(lra);
+ lra = ra;
+ }
+ kfree(lra);
- return 0;
-}
+ hashtab_map(p->filename_trans, filenametr_destroy, NULL);
+ hashtab_destroy(p->filename_trans);
-static int class_index(void *key, void *datum, void *datap)
-{
- struct policydb *p;
- struct class_datum *cladatum;
+ hashtab_map(p->range_tr, range_tr_destroy, NULL);
+ hashtab_destroy(p->range_tr);
- cladatum = datum;
- p = datap;
- if (!cladatum->value || cladatum->value > p->p_classes.nprim)
- return -EINVAL;
+ if (p->type_attr_map_array) {
+ for (i = 0; i < p->p_types.nprim; i++)
+ ebitmap_destroy(&p->type_attr_map_array[i]);
+ kvfree(p->type_attr_map_array);
+ }
- p->sym_val_to_name[SYM_CLASSES][cladatum->value - 1] = key;
- p->class_val_to_struct[cladatum->value - 1] = cladatum;
- return 0;
+ ebitmap_destroy(&p->filename_trans_ttypes);
+ ebitmap_destroy(&p->policycaps);
+ ebitmap_destroy(&p->permissive_map);
}
-static int role_index(void *key, void *datum, void *datap)
+/*
+ * Initialize the role table.
+ */
+static int roles_init(struct policydb *p)
{
- struct policydb *p;
+ char *key = NULL;
+ int rc;
struct role_datum *role;
- role = datum;
- p = datap;
- if (!role->value
- || role->value > p->p_roles.nprim
- || role->bounds > p->p_roles.nprim)
- return -EINVAL;
-
- p->sym_val_to_name[SYM_ROLES][role->value - 1] = key;
- p->role_val_to_struct[role->value - 1] = role;
- return 0;
-}
+ role = kzalloc(sizeof(*role), GFP_KERNEL);
+ if (!role)
+ return -ENOMEM;
-static int type_index(void *key, void *datum, void *datap)
-{
- struct policydb *p;
- struct type_datum *typdatum;
+ rc = -EINVAL;
+ role->value = ++p->p_roles.nprim;
+ if (role->value != OBJECT_R_VAL)
+ goto out;
- typdatum = datum;
- p = datap;
+ rc = -ENOMEM;
+ key = kstrdup(OBJECT_R, GFP_KERNEL);
+ if (!key)
+ goto out;
- if (typdatum->primary) {
- if (!typdatum->value
- || typdatum->value > p->p_types.nprim
- || typdatum->bounds > p->p_types.nprim)
- return -EINVAL;
- p->sym_val_to_name[SYM_TYPES][typdatum->value - 1] = key;
- p->type_val_to_struct_array[typdatum->value - 1] = typdatum;
- }
+ rc = hashtab_insert(p->p_roles.table, key, role);
+ if (rc)
+ goto out;
return 0;
+out:
+ kfree(key);
+ kfree(role);
+ return rc;
}
-static int user_index(void *key, void *datum, void *datap)
+static u32 filenametr_hash(struct hashtab *h, const void *k)
{
- struct policydb *p;
- struct user_datum *usrdatum;
+ const struct filename_trans *ft = k;
+ unsigned long hash;
+ unsigned int byte_num;
+ unsigned char focus;
- usrdatum = datum;
- p = datap;
- if (!usrdatum->value
- || usrdatum->value > p->p_users.nprim
- || usrdatum->bounds > p->p_users.nprim)
- return -EINVAL;
+ hash = ft->stype ^ ft->ttype ^ ft->tclass;
- p->sym_val_to_name[SYM_USERS][usrdatum->value - 1] = key;
- p->user_val_to_struct[usrdatum->value - 1] = usrdatum;
- return 0;
+ byte_num = 0;
+ while ((focus = ft->name[byte_num++]))
+ hash = partial_name_hash(focus, hash);
+ return hash & (h->size - 1);
}
-static int sens_index(void *key, void *datum, void *datap)
+static int filenametr_cmp(struct hashtab *h, const void *k1, const void *k2)
{
- struct policydb *p;
- struct level_datum *levdatum;
-
- levdatum = datum;
- p = datap;
-
- if (!levdatum->isalias) {
- if (!levdatum->level->sens ||
- levdatum->level->sens > p->p_levels.nprim)
- return -EINVAL;
-
- p->sym_val_to_name[SYM_LEVELS][levdatum->level->sens - 1] = key;
- }
-
- return 0;
-}
+ const struct filename_trans *ft1 = k1;
+ const struct filename_trans *ft2 = k2;
+ int v;
-static int cat_index(void *key, void *datum, void *datap)
-{
- struct policydb *p;
- struct cat_datum *catdatum;
+ v = ft1->stype - ft2->stype;
+ if (v)
+ return v;
- catdatum = datum;
- p = datap;
+ v = ft1->ttype - ft2->ttype;
+ if (v)
+ return v;
- if (!catdatum->isalias) {
- if (!catdatum->value || catdatum->value > p->p_cats.nprim)
- return -EINVAL;
+ v = ft1->tclass - ft2->tclass;
+ if (v)
+ return v;
- p->sym_val_to_name[SYM_CATS][catdatum->value - 1] = key;
- }
+ return strcmp(ft1->name, ft2->name);
- return 0;
}
-static int (*index_f[SYM_NUM]) (void *key, void *datum, void *datap) =
+static u32 rangetr_hash(struct hashtab *h, const void *k)
{
- common_index,
- class_index,
- role_index,
- type_index,
- user_index,
- cond_index_bool,
- sens_index,
- cat_index,
-};
+ const struct range_trans *key = k;
+ return (key->source_type + (key->target_type << 3) +
+ (key->target_class << 5)) & (h->size - 1);
+}
-#ifdef DEBUG_HASHES
-static void hash_eval(struct hashtab *h, const char *hash_name)
+static int rangetr_cmp(struct hashtab *h, const void *k1, const void *k2)
{
- struct hashtab_info info;
+ const struct range_trans *key1 = k1, *key2 = k2;
+ int v;
- hashtab_stat(h, &info);
- pr_debug("SELinux: %s: %d entries and %d/%d buckets used, "
- "longest chain length %d\n", hash_name, h->nel,
- info.slots_used, h->size, info.max_chain_len);
-}
+ v = key1->source_type - key2->source_type;
+ if (v)
+ return v;
-static void symtab_hash_eval(struct symtab *s)
-{
- int i;
+ v = key1->target_type - key2->target_type;
+ if (v)
+ return v;
- for (i = 0; i < SYM_NUM; i++)
- hash_eval(s[i].table, symtab_name[i]);
-}
+ v = key1->target_class - key2->target_class;
-#else
-static inline void hash_eval(struct hashtab *h, char *hash_name)
-{
+ return v;
}
-#endif
/*
- * Define the other val_to_name and val_to_struct arrays
- * in a policy database structure.
- *
- * Caller must clean up on failure.
+ * Initialize a policy database structure.
*/
-static int policydb_index(struct policydb *p)
+static int policydb_init(struct policydb *p)
{
int i, rc;
- if (p->mls_enabled)
- pr_debug("SELinux: %d users, %d roles, %d types, %d bools, %d sens, %d cats\n",
- p->p_users.nprim, p->p_roles.nprim, p->p_types.nprim,
- p->p_bools.nprim, p->p_levels.nprim, p->p_cats.nprim);
- else
- pr_debug("SELinux: %d users, %d roles, %d types, %d bools\n",
- p->p_users.nprim, p->p_roles.nprim, p->p_types.nprim,
- p->p_bools.nprim);
-
- pr_debug("SELinux: %d classes, %d rules\n",
- p->p_classes.nprim, p->te_avtab.nel);
-
-#ifdef DEBUG_HASHES
- avtab_hash_eval(&p->te_avtab, "rules");
- symtab_hash_eval(p->symtab);
-#endif
-
- p->class_val_to_struct = kcalloc(p->p_classes.nprim,
- sizeof(*p->class_val_to_struct),
- GFP_KERNEL);
- if (!p->class_val_to_struct)
- return -ENOMEM;
+ memset(p, 0, sizeof(*p));
- p->role_val_to_struct = kcalloc(p->p_roles.nprim,
- sizeof(*p->role_val_to_struct),
- GFP_KERNEL);
- if (!p->role_val_to_struct)
- return -ENOMEM;
+ for (i = 0; i < SYM_NUM; i++) {
+ rc = symtab_init(&p->symtab[i], symtab_sizes[i]);
+ if (rc)
+ goto out;
+ }
- p->user_val_to_struct = kcalloc(p->p_users.nprim,
- sizeof(*p->user_val_to_struct),
- GFP_KERNEL);
- if (!p->user_val_to_struct)
- return -ENOMEM;
+ rc = avtab_init(&p->te_avtab);
+ if (rc)
+ goto out;
- p->type_val_to_struct_array = kvcalloc(p->p_types.nprim,
- sizeof(*p->type_val_to_struct_array),
- GFP_KERNEL);
- if (!p->type_val_to_struct_array)
- return -ENOMEM;
+ rc = roles_init(p);
+ if (rc)
+ goto out;
- rc = cond_init_bool_indexes(p);
+ rc = cond_policydb_init(p);
if (rc)
goto out;
- for (i = 0; i < SYM_NUM; i++) {
- p->sym_val_to_name[i] = kvcalloc(p->symtab[i].nprim,
- sizeof(char *),
- GFP_KERNEL);
- if (!p->sym_val_to_name[i])
- return -ENOMEM;
+ p->filename_trans = hashtab_create(filenametr_hash, filenametr_cmp, (1 << 10));
+ if (!p->filename_trans) {
+ rc = -ENOMEM;
+ goto out;
+ }
- rc = hashtab_map(p->symtab[i].table, index_f[i], p);
- if (rc)
- goto out;
+ p->range_tr = hashtab_create(rangetr_hash, rangetr_cmp, 256);
+ if (!p->range_tr) {
+ rc = -ENOMEM;
+ goto out;
}
- rc = 0;
+
+ ebitmap_init(&p->filename_trans_ttypes);
+ ebitmap_init(&p->policycaps);
+ ebitmap_init(&p->permissive_map);
+
+ return 0;
out:
+ hashtab_destroy(p->filename_trans);
+ hashtab_destroy(p->range_tr);
+ for (i = 0; i < SYM_NUM; i++) {
+ hashtab_map(p->symtab[i].table, destroy_f[i], NULL);
+ hashtab_destroy(p->symtab[i].table);
+ }
return rc;
}
/*
- * The following *_destroy functions are used to
- * free any memory allocated for each kind of
- * symbol data in the policy database.
+ * The following *_index functions are used to
+ * define the val_to_name and val_to_struct arrays
+ * in a policy database structure. The val_to_name
+ * arrays are used when converting security context
+ * structures into string representations. The
+ * val_to_struct arrays are used when the attributes
+ * of a class, role, or user are needed.
*/
-static int perm_destroy(void *key, void *datum, void *p)
-{
- kfree(key);
- kfree(datum);
- return 0;
-}
-
-static int common_destroy(void *key, void *datum, void *p)
+static int common_index(void *key, void *datum, void *datap)
{
+ struct policydb *p;
struct common_datum *comdatum;
- kfree(key);
- if (datum) {
- comdatum = datum;
- hashtab_map(comdatum->permissions.table, perm_destroy, NULL);
- hashtab_destroy(comdatum->permissions.table);
- }
- kfree(datum);
+ comdatum = datum;
+ p = datap;
+ if (!comdatum->value || comdatum->value > p->p_commons.nprim)
+ return -EINVAL;
+
+ p->sym_val_to_name[SYM_COMMONS][comdatum->value - 1] = key;
+
return 0;
}
-static void constraint_expr_destroy(struct constraint_expr *expr)
+static int class_index(void *key, void *datum, void *datap)
{
- if (expr) {
- ebitmap_destroy(&expr->names);
- if (expr->type_names) {
- ebitmap_destroy(&expr->type_names->types);
- ebitmap_destroy(&expr->type_names->negset);
- kfree(expr->type_names);
- }
- kfree(expr);
- }
+ struct policydb *p;
+ struct class_datum *cladatum;
+
+ cladatum = datum;
+ p = datap;
+ if (!cladatum->value || cladatum->value > p->p_classes.nprim)
+ return -EINVAL;
+
+ p->sym_val_to_name[SYM_CLASSES][cladatum->value - 1] = key;
+ p->class_val_to_struct[cladatum->value - 1] = cladatum;
+ return 0;
}
-static int cls_destroy(void *key, void *datum, void *p)
+static int role_index(void *key, void *datum, void *datap)
{
- struct class_datum *cladatum;
- struct constraint_node *constraint, *ctemp;
- struct constraint_expr *e, *etmp;
+ struct policydb *p;
+ struct role_datum *role;
- kfree(key);
- if (datum) {
- cladatum = datum;
- hashtab_map(cladatum->permissions.table, perm_destroy, NULL);
- hashtab_destroy(cladatum->permissions.table);
- constraint = cladatum->constraints;
- while (constraint) {
- e = constraint->expr;
- while (e) {
- etmp = e;
- e = e->next;
- constraint_expr_destroy(etmp);
- }
- ctemp = constraint;
- constraint = constraint->next;
- kfree(ctemp);
- }
+ role = datum;
+ p = datap;
+ if (!role->value
+ || role->value > p->p_roles.nprim
+ || role->bounds > p->p_roles.nprim)
+ return -EINVAL;
- constraint = cladatum->validatetrans;
- while (constraint) {
- e = constraint->expr;
- while (e) {
- etmp = e;
- e = e->next;
- constraint_expr_destroy(etmp);
- }
- ctemp = constraint;
- constraint = constraint->next;
- kfree(ctemp);
- }
- kfree(cladatum->comkey);
- }
- kfree(datum);
+ p->sym_val_to_name[SYM_ROLES][role->value - 1] = key;
+ p->role_val_to_struct[role->value - 1] = role;
return 0;
}
-static int role_destroy(void *key, void *datum, void *p)
+static int type_index(void *key, void *datum, void *datap)
{
- struct role_datum *role;
+ struct policydb *p;
+ struct type_datum *typdatum;
- kfree(key);
- if (datum) {
- role = datum;
- ebitmap_destroy(&role->dominates);
- ebitmap_destroy(&role->types);
+ typdatum = datum;
+ p = datap;
+
+ if (typdatum->primary) {
+ if (!typdatum->value
+ || typdatum->value > p->p_types.nprim
+ || typdatum->bounds > p->p_types.nprim)
+ return -EINVAL;
+ p->sym_val_to_name[SYM_TYPES][typdatum->value - 1] = key;
+ p->type_val_to_struct_array[typdatum->value - 1] = typdatum;
}
- kfree(datum);
- return 0;
-}
-static int type_destroy(void *key, void *datum, void *p)
-{
- kfree(key);
- kfree(datum);
return 0;
}
-static int user_destroy(void *key, void *datum, void *p)
+static int user_index(void *key, void *datum, void *datap)
{
+ struct policydb *p;
struct user_datum *usrdatum;
- kfree(key);
- if (datum) {
- usrdatum = datum;
- ebitmap_destroy(&usrdatum->roles);
- ebitmap_destroy(&usrdatum->range.level[0].cat);
- ebitmap_destroy(&usrdatum->range.level[1].cat);
- ebitmap_destroy(&usrdatum->dfltlevel.cat);
- }
- kfree(datum);
+ usrdatum = datum;
+ p = datap;
+ if (!usrdatum->value
+ || usrdatum->value > p->p_users.nprim
+ || usrdatum->bounds > p->p_users.nprim)
+ return -EINVAL;
+
+ p->sym_val_to_name[SYM_USERS][usrdatum->value - 1] = key;
+ p->user_val_to_struct[usrdatum->value - 1] = usrdatum;
return 0;
}
-static int sens_destroy(void *key, void *datum, void *p)
+static int sens_index(void *key, void *datum, void *datap)
{
+ struct policydb *p;
struct level_datum *levdatum;
- kfree(key);
- if (datum) {
- levdatum = datum;
- if (levdatum->level)
- ebitmap_destroy(&levdatum->level->cat);
- kfree(levdatum->level);
+ levdatum = datum;
+ p = datap;
+
+ if (!levdatum->isalias) {
+ if (!levdatum->level->sens ||
+ levdatum->level->sens > p->p_levels.nprim)
+ return -EINVAL;
+
+ p->sym_val_to_name[SYM_LEVELS][levdatum->level->sens - 1] = key;
}
- kfree(datum);
+
return 0;
}
-static int cat_destroy(void *key, void *datum, void *p)
+static int cat_index(void *key, void *datum, void *datap)
{
- kfree(key);
- kfree(datum);
+ struct policydb *p;
+ struct cat_datum *catdatum;
+
+ catdatum = datum;
+ p = datap;
+
+ if (!catdatum->isalias) {
+ if (!catdatum->value || catdatum->value > p->p_cats.nprim)
+ return -EINVAL;
+
+ p->sym_val_to_name[SYM_CATS][catdatum->value - 1] = key;
+ }
+
return 0;
}
-static int (*destroy_f[SYM_NUM]) (void *key, void *datum, void *datap) =
+static int (*index_f[SYM_NUM]) (void *key, void *datum, void *datap) =
{
- common_destroy,
- cls_destroy,
- role_destroy,
- type_destroy,
- user_destroy,
- cond_destroy_bool,
- sens_destroy,
- cat_destroy,
+ common_index,
+ class_index,
+ role_index,
+ type_index,
+ user_index,
+ cond_index_bool,
+ sens_index,
+ cat_index,
};
-static int filenametr_destroy(void *key, void *datum, void *p)
+#ifdef DEBUG_HASHES
+static void hash_eval(struct hashtab *h, const char *hash_name)
{
- struct filename_trans *ft = key;
- kfree(ft->name);
- kfree(key);
- kfree(datum);
- cond_resched();
- return 0;
+ struct hashtab_info info;
+
+ hashtab_stat(h, &info);
+ pr_debug("SELinux: %s: %d entries and %d/%d buckets used, "
+ "longest chain length %d\n", hash_name, h->nel,
+ info.slots_used, h->size, info.max_chain_len);
}
-static int range_tr_destroy(void *key, void *datum, void *p)
+static void symtab_hash_eval(struct symtab *s)
{
- struct mls_range *rt = datum;
- kfree(key);
- ebitmap_destroy(&rt->level[0].cat);
- ebitmap_destroy(&rt->level[1].cat);
- kfree(datum);
- cond_resched();
- return 0;
+ int i;
+
+ for (i = 0; i < SYM_NUM; i++)
+ hash_eval(s[i].table, symtab_name[i]);
}
-static void ocontext_destroy(struct ocontext *c, int i)
+#else
+static inline void hash_eval(struct hashtab *h, char *hash_name)
{
- if (!c)
- return;
-
- context_destroy(&c->context[0]);
- context_destroy(&c->context[1]);
- if (i == OCON_ISID || i == OCON_FS ||
- i == OCON_NETIF || i == OCON_FSUSE)
- kfree(c->u.name);
- kfree(c);
}
+#endif
/*
- * Free any memory allocated by a policy database structure.
+ * Define the other val_to_name and val_to_struct arrays
+ * in a policy database structure.
+ *
+ * Caller must clean up on failure.
*/
-void policydb_destroy(struct policydb *p)
+static int policydb_index(struct policydb *p)
{
- struct ocontext *c, *ctmp;
- struct genfs *g, *gtmp;
- int i;
- struct role_allow *ra, *lra = NULL;
- struct role_trans *tr, *ltr = NULL;
-
- for (i = 0; i < SYM_NUM; i++) {
- cond_resched();
- hashtab_map(p->symtab[i].table, destroy_f[i], NULL);
- hashtab_destroy(p->symtab[i].table);
- }
-
- for (i = 0; i < SYM_NUM; i++)
- kvfree(p->sym_val_to_name[i]);
+ int i, rc;
- kfree(p->class_val_to_struct);
- kfree(p->role_val_to_struct);
- kfree(p->user_val_to_struct);
- kvfree(p->type_val_to_struct_array);
+ if (p->mls_enabled)
+ pr_debug("SELinux: %d users, %d roles, %d types, %d bools, %d sens, %d cats\n",
+ p->p_users.nprim, p->p_roles.nprim, p->p_types.nprim,
+ p->p_bools.nprim, p->p_levels.nprim, p->p_cats.nprim);
+ else
+ pr_debug("SELinux: %d users, %d roles, %d types, %d bools\n",
+ p->p_users.nprim, p->p_roles.nprim, p->p_types.nprim,
+ p->p_bools.nprim);
- avtab_destroy(&p->te_avtab);
+ pr_debug("SELinux: %d classes, %d rules\n",
+ p->p_classes.nprim, p->te_avtab.nel);
- for (i = 0; i < OCON_NUM; i++) {
- cond_resched();
- c = p->ocontexts[i];
- while (c) {
- ctmp = c;
- c = c->next;
- ocontext_destroy(ctmp, i);
- }
- p->ocontexts[i] = NULL;
- }
+#ifdef DEBUG_HASHES
+ avtab_hash_eval(&p->te_avtab, "rules");
+ symtab_hash_eval(p->symtab);
+#endif
- g = p->genfs;
- while (g) {
- cond_resched();
- kfree(g->fstype);
- c = g->head;
- while (c) {
- ctmp = c;
- c = c->next;
- ocontext_destroy(ctmp, OCON_FSUSE);
- }
- gtmp = g;
- g = g->next;
- kfree(gtmp);
- }
- p->genfs = NULL;
+ p->class_val_to_struct = kcalloc(p->p_classes.nprim,
+ sizeof(*p->class_val_to_struct),
+ GFP_KERNEL);
+ if (!p->class_val_to_struct)
+ return -ENOMEM;
- cond_policydb_destroy(p);
+ p->role_val_to_struct = kcalloc(p->p_roles.nprim,
+ sizeof(*p->role_val_to_struct),
+ GFP_KERNEL);
+ if (!p->role_val_to_struct)
+ return -ENOMEM;
- for (tr = p->role_tr; tr; tr = tr->next) {
- cond_resched();
- kfree(ltr);
- ltr = tr;
- }
- kfree(ltr);
+ p->user_val_to_struct = kcalloc(p->p_users.nprim,
+ sizeof(*p->user_val_to_struct),
+ GFP_KERNEL);
+ if (!p->user_val_to_struct)
+ return -ENOMEM;
- for (ra = p->role_allow; ra; ra = ra->next) {
- cond_resched();
- kfree(lra);
- lra = ra;
- }
- kfree(lra);
+ p->type_val_to_struct_array = kvcalloc(p->p_types.nprim,
+ sizeof(*p->type_val_to_struct_array),
+ GFP_KERNEL);
+ if (!p->type_val_to_struct_array)
+ return -ENOMEM;
- hashtab_map(p->filename_trans, filenametr_destroy, NULL);
- hashtab_destroy(p->filename_trans);
+ rc = cond_init_bool_indexes(p);
+ if (rc)
+ goto out;
- hashtab_map(p->range_tr, range_tr_destroy, NULL);
- hashtab_destroy(p->range_tr);
+ for (i = 0; i < SYM_NUM; i++) {
+ p->sym_val_to_name[i] = kvcalloc(p->symtab[i].nprim,
+ sizeof(char *),
+ GFP_KERNEL);
+ if (!p->sym_val_to_name[i])
+ return -ENOMEM;
- if (p->type_attr_map_array) {
- for (i = 0; i < p->p_types.nprim; i++)
- ebitmap_destroy(&p->type_attr_map_array[i]);
- kvfree(p->type_attr_map_array);
+ rc = hashtab_map(p->symtab[i].table, index_f[i], p);
+ if (rc)
+ goto out;
}
-
- ebitmap_destroy(&p->filename_trans_ttypes);
- ebitmap_destroy(&p->policycaps);
- ebitmap_destroy(&p->permissive_map);
+ rc = 0;
+out:
+ return rc;
}
/*
--
2.21.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 2/3] selinux: policydb - fix some checkpatch.pl warnings
2019-07-29 8:41 [PATCH v2 0/3] selinux: policydb - fix memory leak and do some cleanup Ondrej Mosnacek
2019-07-29 8:41 ` [PATCH v2 1/3] selinux: policydb - fix memory leak in policydb_init() Ondrej Mosnacek
@ 2019-07-29 8:41 ` Ondrej Mosnacek
2019-08-05 20:48 ` Paul Moore
2019-07-29 8:41 ` [PATCH v2 3/3] selinux: policydb - rename type_val_to_struct_array Ondrej Mosnacek
2 siblings, 1 reply; 12+ messages in thread
From: Ondrej Mosnacek @ 2019-07-29 8:41 UTC (permalink / raw)
To: selinux, Paul Moore; +Cc: Dmitry Vyukov
Fix most of the code style warnings discovered when moving code around.
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
security/selinux/ss/policydb.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index 451f2bcd2d83..ffeae0e252d2 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -334,6 +334,7 @@ static int (*destroy_f[SYM_NUM]) (void *key, void *datum, void *datap) =
static int filenametr_destroy(void *key, void *datum, void *p)
{
struct filename_trans *ft = key;
+
kfree(ft->name);
kfree(key);
kfree(datum);
@@ -344,6 +345,7 @@ static int filenametr_destroy(void *key, void *datum, void *p)
static int range_tr_destroy(void *key, void *datum, void *p)
{
struct mls_range *rt = datum;
+
kfree(key);
ebitmap_destroy(&rt->level[0].cat);
ebitmap_destroy(&rt->level[1].cat);
@@ -526,6 +528,7 @@ static int filenametr_cmp(struct hashtab *h, const void *k1, const void *k2)
static u32 rangetr_hash(struct hashtab *h, const void *k)
{
const struct range_trans *key = k;
+
return (key->source_type + (key->target_type << 3) +
(key->target_class << 5)) & (h->size - 1);
}
@@ -575,7 +578,8 @@ static int policydb_init(struct policydb *p)
if (rc)
goto out;
- p->filename_trans = hashtab_create(filenametr_hash, filenametr_cmp, (1 << 10));
+ p->filename_trans = hashtab_create(filenametr_hash, filenametr_cmp,
+ (1 << 10));
if (!p->filename_trans) {
rc = -ENOMEM;
goto out;
@@ -751,9 +755,9 @@ static void hash_eval(struct hashtab *h, const char *hash_name)
struct hashtab_info info;
hashtab_stat(h, &info);
- pr_debug("SELinux: %s: %d entries and %d/%d buckets used, "
- "longest chain length %d\n", hash_name, h->nel,
- info.slots_used, h->size, info.max_chain_len);
+ pr_debug("SELinux: %s: %d entries and %d/%d buckets used, longest chain length %d\n",
+ hash_name, h->nel, info.slots_used, h->size,
+ info.max_chain_len);
}
static void symtab_hash_eval(struct symtab *s)
--
2.21.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 3/3] selinux: policydb - rename type_val_to_struct_array
2019-07-29 8:41 [PATCH v2 0/3] selinux: policydb - fix memory leak and do some cleanup Ondrej Mosnacek
2019-07-29 8:41 ` [PATCH v2 1/3] selinux: policydb - fix memory leak in policydb_init() Ondrej Mosnacek
2019-07-29 8:41 ` [PATCH v2 2/3] selinux: policydb - fix some checkpatch.pl warnings Ondrej Mosnacek
@ 2019-07-29 8:41 ` Ondrej Mosnacek
2019-08-05 20:49 ` Paul Moore
2 siblings, 1 reply; 12+ messages in thread
From: Ondrej Mosnacek @ 2019-07-29 8:41 UTC (permalink / raw)
To: selinux, Paul Moore; +Cc: Dmitry Vyukov
The name is overly long and inconsistent with the other *_val_to_struct
members. Dropping the "_array" prefix makes the code easier to read and
gets rid of one line over 80 characters warning.
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
security/selinux/ss/policydb.c | 14 +++++++-------
security/selinux/ss/policydb.h | 2 +-
security/selinux/ss/services.c | 6 +++---
3 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index ffeae0e252d2..4e2f35f11d40 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -390,7 +390,7 @@ void policydb_destroy(struct policydb *p)
kfree(p->class_val_to_struct);
kfree(p->role_val_to_struct);
kfree(p->user_val_to_struct);
- kvfree(p->type_val_to_struct_array);
+ kvfree(p->type_val_to_struct);
avtab_destroy(&p->te_avtab);
@@ -677,7 +677,7 @@ static int type_index(void *key, void *datum, void *datap)
|| typdatum->bounds > p->p_types.nprim)
return -EINVAL;
p->sym_val_to_name[SYM_TYPES][typdatum->value - 1] = key;
- p->type_val_to_struct_array[typdatum->value - 1] = typdatum;
+ p->type_val_to_struct[typdatum->value - 1] = typdatum;
}
return 0;
@@ -819,10 +819,10 @@ static int policydb_index(struct policydb *p)
if (!p->user_val_to_struct)
return -ENOMEM;
- p->type_val_to_struct_array = kvcalloc(p->p_types.nprim,
- sizeof(*p->type_val_to_struct_array),
- GFP_KERNEL);
- if (!p->type_val_to_struct_array)
+ p->type_val_to_struct = kvcalloc(p->p_types.nprim,
+ sizeof(*p->type_val_to_struct),
+ GFP_KERNEL);
+ if (!p->type_val_to_struct)
return -ENOMEM;
rc = cond_init_bool_indexes(p);
@@ -1726,7 +1726,7 @@ static int type_bounds_sanity_check(void *key, void *datum, void *datap)
return -EINVAL;
}
- upper = p->type_val_to_struct_array[upper->bounds - 1];
+ upper = p->type_val_to_struct[upper->bounds - 1];
BUG_ON(!upper);
if (upper->attribute) {
diff --git a/security/selinux/ss/policydb.h b/security/selinux/ss/policydb.h
index 27039149ff0a..05fc672831aa 100644
--- a/security/selinux/ss/policydb.h
+++ b/security/selinux/ss/policydb.h
@@ -255,7 +255,7 @@ struct policydb {
struct class_datum **class_val_to_struct;
struct role_datum **role_val_to_struct;
struct user_datum **user_val_to_struct;
- struct type_datum **type_val_to_struct_array;
+ struct type_datum **type_val_to_struct;
/* type enforcement access vectors and transitions */
struct avtab te_avtab;
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index d3a8f6fbc552..ca56cd045bf9 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -544,13 +544,13 @@ static void type_attribute_bounds_av(struct policydb *policydb,
struct type_datum *target;
u32 masked = 0;
- source = policydb->type_val_to_struct_array[scontext->type - 1];
+ source = policydb->type_val_to_struct[scontext->type - 1];
BUG_ON(!source);
if (!source->bounds)
return;
- target = policydb->type_val_to_struct_array[tcontext->type - 1];
+ target = policydb->type_val_to_struct[tcontext->type - 1];
BUG_ON(!target);
memset(&lo_avd, 0, sizeof(lo_avd));
@@ -893,7 +893,7 @@ int security_bounded_transition(struct selinux_state *state,
index = new_context->type;
while (true) {
- type = policydb->type_val_to_struct_array[index - 1];
+ type = policydb->type_val_to_struct[index - 1];
BUG_ON(!type);
/* not bounded anymore */
--
2.21.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/3] selinux: policydb - fix memory leak in policydb_init()
2019-07-29 8:41 ` [PATCH v2 1/3] selinux: policydb - fix memory leak in policydb_init() Ondrej Mosnacek
@ 2019-07-29 22:48 ` Paul Moore
2019-07-30 12:20 ` Ondrej Mosnacek
0 siblings, 1 reply; 12+ messages in thread
From: Paul Moore @ 2019-07-29 22:48 UTC (permalink / raw)
To: Ondrej Mosnacek; +Cc: selinux, Dmitry Vyukov, syzbot+fee3a14d4cdf92646287
On Mon, Jul 29, 2019 at 4:41 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> Since roles_init() adds some entries to the role hash table, we need to
> destroy also its keys/values on error, otherwise we get a memory leak in
> the error path.
>
> To avoid a forward declaration and maintain a sane layout, move all the
> destroy stuff above policydb_init. No changes are made to the moved code
> in this patch. Note that this triggers some pre-existing checkpatch.pl
> warnings - these will be fixed in follow-up patches.
>
> Reported-by: syzbot+fee3a14d4cdf92646287@syzkaller.appspotmail.com
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
> security/selinux/ss/policydb.c | 976 +++++++++++++++++----------------
> 1 file changed, 489 insertions(+), 487 deletions(-)
Hmmm, that is one ugly patch isn't it? If I saw this diff I'm not
sure I would have suggested what I did, or rather I would have
suggested something slightly different.
When I ran my quick test when I was looking at your v1 patch, I only
moved perm_destroy() through ocontext_destroy(), leaving out
policydb_destroy(), and the diff was much more cleaner[*] (diffstat
below, includes the actual fix too). Could you try that and see if it
cleans up your patch?
security/selinux/ss/policydb.c | 378 +++++++++++++++++-----------------
1 file changed, 190 insertions(+), 188 deletions(-)
[*] In this case "cleaner" simply means that the moved lines were not
interleaved with existing code (just a big block of adds at the top,
the fix in the middle, and a big block of removals at the bottom).
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/3] selinux: policydb - fix memory leak in policydb_init()
2019-07-29 22:48 ` Paul Moore
@ 2019-07-30 12:20 ` Ondrej Mosnacek
2019-07-30 15:10 ` Paul Moore
0 siblings, 1 reply; 12+ messages in thread
From: Ondrej Mosnacek @ 2019-07-30 12:20 UTC (permalink / raw)
To: Paul Moore; +Cc: SElinux list, Dmitry Vyukov, syzbot
On Tue, Jul 30, 2019 at 12:48 AM Paul Moore <paul@paul-moore.com> wrote:
> On Mon, Jul 29, 2019 at 4:41 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> >
> > Since roles_init() adds some entries to the role hash table, we need to
> > destroy also its keys/values on error, otherwise we get a memory leak in
> > the error path.
> >
> > To avoid a forward declaration and maintain a sane layout, move all the
> > destroy stuff above policydb_init. No changes are made to the moved code
> > in this patch. Note that this triggers some pre-existing checkpatch.pl
> > warnings - these will be fixed in follow-up patches.
> >
> > Reported-by: syzbot+fee3a14d4cdf92646287@syzkaller.appspotmail.com
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > ---
> > security/selinux/ss/policydb.c | 976 +++++++++++++++++----------------
> > 1 file changed, 489 insertions(+), 487 deletions(-)
>
> Hmmm, that is one ugly patch isn't it? If I saw this diff I'm not
> sure I would have suggested what I did, or rather I would have
> suggested something slightly different.
>
> When I ran my quick test when I was looking at your v1 patch, I only
> moved perm_destroy() through ocontext_destroy(), leaving out
> policydb_destroy(), and the diff was much more cleaner[*] (diffstat
> below, includes the actual fix too). Could you try that and see if it
> cleans up your patch?
Yeah, excluding policydb_destroy() from the move is what's needed to
get a nice patch... Actually, what do you think about keeping the
bugfix patch as before (with the forward declaration) and then doing
the moving around in a separate patch (removing the forward
declaration)? Then we keep the patch with the actual fix small, but
still get a clean final result. It would also allow moving
policydb_destroy() up closer to the other destroy functions in another
separate patch (I tried it and both patches end up clean when the move
is split up like this). (I don't have a strong preference for this,
let me know what works best for you.)
>
> security/selinux/ss/policydb.c | 378 +++++++++++++++++-----------------
> 1 file changed, 190 insertions(+), 188 deletions(-)
>
> [*] In this case "cleaner" simply means that the moved lines were not
> interleaved with existing code (just a big block of adds at the top,
> the fix in the middle, and a big block of removals at the bottom).
>
> --
> paul moore
> www.paul-moore.com
--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/3] selinux: policydb - fix memory leak in policydb_init()
2019-07-30 12:20 ` Ondrej Mosnacek
@ 2019-07-30 15:10 ` Paul Moore
2019-07-30 16:15 ` Ondrej Mosnacek
0 siblings, 1 reply; 12+ messages in thread
From: Paul Moore @ 2019-07-30 15:10 UTC (permalink / raw)
To: Ondrej Mosnacek; +Cc: SElinux list, Dmitry Vyukov, syzbot
On Tue, Jul 30, 2019 at 8:20 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Tue, Jul 30, 2019 at 12:48 AM Paul Moore <paul@paul-moore.com> wrote:
> > On Mon, Jul 29, 2019 at 4:41 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > >
> > > Since roles_init() adds some entries to the role hash table, we need to
> > > destroy also its keys/values on error, otherwise we get a memory leak in
> > > the error path.
> > >
> > > To avoid a forward declaration and maintain a sane layout, move all the
> > > destroy stuff above policydb_init. No changes are made to the moved code
> > > in this patch. Note that this triggers some pre-existing checkpatch.pl
> > > warnings - these will be fixed in follow-up patches.
> > >
> > > Reported-by: syzbot+fee3a14d4cdf92646287@syzkaller.appspotmail.com
> > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > > ---
> > > security/selinux/ss/policydb.c | 976 +++++++++++++++++----------------
> > > 1 file changed, 489 insertions(+), 487 deletions(-)
> >
> > Hmmm, that is one ugly patch isn't it? If I saw this diff I'm not
> > sure I would have suggested what I did, or rather I would have
> > suggested something slightly different.
> >
> > When I ran my quick test when I was looking at your v1 patch, I only
> > moved perm_destroy() through ocontext_destroy(), leaving out
> > policydb_destroy(), and the diff was much more cleaner[*] (diffstat
> > below, includes the actual fix too). Could you try that and see if it
> > cleans up your patch?
>
> Yeah, excluding policydb_destroy() from the move is what's needed to
> get a nice patch...
Good, let's just do that.
> Actually, what do you think about keeping the
> bugfix patch as before (with the forward declaration) and then doing
> the moving around in a separate patch (removing the forward
> declaration)?
Yes, I thought about that too when looking at your patch yesterday and
trying to sort out why it was such a messy diff.
> Then we keep the patch with the actual fix small, but
> still get a clean final result. It would also allow moving
> policydb_destroy() up closer to the other destroy functions in another
> separate patch (I tried it and both patches end up clean when the move
> is split up like this). (I don't have a strong preference for this,
> let me know what works best for you.)
I'm fine with leaving policydb_destroy() where it is, but I agree that
separating the fix is likely worthwhile. I'll go ahead and merge your
v1 patch into selinux/stable-5.3 (it's borderline -stable material
IMHO, but I'm pretty sure GregKH would pull it into -stable anyway, he
pulls everything with a "Fixes" tag it seems), and then merge the
reorganization patch into selinux/next. Honestly, I can go ahead and
submit the reorg patch, it's basically already sitting in a tree on my
disk anyway, but if you would prefer to do it that's fine too, just
let me know.
I'll may also merge the v1 fix into selinux/next in order to fix the
inevitable merge conflict, but that isn't something you have to worry
about.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/3] selinux: policydb - fix memory leak in policydb_init()
2019-07-30 15:10 ` Paul Moore
@ 2019-07-30 16:15 ` Ondrej Mosnacek
2019-07-30 23:15 ` Paul Moore
2019-07-31 22:07 ` Paul Moore
0 siblings, 2 replies; 12+ messages in thread
From: Ondrej Mosnacek @ 2019-07-30 16:15 UTC (permalink / raw)
To: Paul Moore; +Cc: SElinux list, Dmitry Vyukov, syzbot
On Tue, Jul 30, 2019 at 5:10 PM Paul Moore <paul@paul-moore.com> wrote:
> On Tue, Jul 30, 2019 at 8:20 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > On Tue, Jul 30, 2019 at 12:48 AM Paul Moore <paul@paul-moore.com> wrote:
> > > On Mon, Jul 29, 2019 at 4:41 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > >
> > > > Since roles_init() adds some entries to the role hash table, we need to
> > > > destroy also its keys/values on error, otherwise we get a memory leak in
> > > > the error path.
> > > >
> > > > To avoid a forward declaration and maintain a sane layout, move all the
> > > > destroy stuff above policydb_init. No changes are made to the moved code
> > > > in this patch. Note that this triggers some pre-existing checkpatch.pl
> > > > warnings - these will be fixed in follow-up patches.
> > > >
> > > > Reported-by: syzbot+fee3a14d4cdf92646287@syzkaller.appspotmail.com
> > > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > > > ---
> > > > security/selinux/ss/policydb.c | 976 +++++++++++++++++----------------
> > > > 1 file changed, 489 insertions(+), 487 deletions(-)
> > >
> > > Hmmm, that is one ugly patch isn't it? If I saw this diff I'm not
> > > sure I would have suggested what I did, or rather I would have
> > > suggested something slightly different.
> > >
> > > When I ran my quick test when I was looking at your v1 patch, I only
> > > moved perm_destroy() through ocontext_destroy(), leaving out
> > > policydb_destroy(), and the diff was much more cleaner[*] (diffstat
> > > below, includes the actual fix too). Could you try that and see if it
> > > cleans up your patch?
> >
> > Yeah, excluding policydb_destroy() from the move is what's needed to
> > get a nice patch...
>
> Good, let's just do that.
>
> > Actually, what do you think about keeping the
> > bugfix patch as before (with the forward declaration) and then doing
> > the moving around in a separate patch (removing the forward
> > declaration)?
>
> Yes, I thought about that too when looking at your patch yesterday and
> trying to sort out why it was such a messy diff.
>
> > Then we keep the patch with the actual fix small, but
> > still get a clean final result. It would also allow moving
> > policydb_destroy() up closer to the other destroy functions in another
> > separate patch (I tried it and both patches end up clean when the move
> > is split up like this). (I don't have a strong preference for this,
> > let me know what works best for you.)
>
> I'm fine with leaving policydb_destroy() where it is, but I agree that
> separating the fix is likely worthwhile. I'll go ahead and merge your
> v1 patch into selinux/stable-5.3 (it's borderline -stable material
> IMHO, but I'm pretty sure GregKH would pull it into -stable anyway, he
> pulls everything with a "Fixes" tag it seems), and then merge the
> reorganization patch into selinux/next. Honestly, I can go ahead and
> submit the reorg patch, it's basically already sitting in a tree on my
> disk anyway, but if you would prefer to do it that's fine too, just
> let me know.
Sure, feel free to submit the reorg yourself (I assume you will then
merge the checkpatch fixes 2-3/3 on top, right?)
>
> I'll may also merge the v1 fix into selinux/next in order to fix the
> inevitable merge conflict, but that isn't something you have to worry
> about.
--
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 1/3] selinux: policydb - fix memory leak in policydb_init()
2019-07-30 16:15 ` Ondrej Mosnacek
@ 2019-07-30 23:15 ` Paul Moore
2019-07-31 22:07 ` Paul Moore
1 sibling, 0 replies; 12+ messages in thread
From: Paul Moore @ 2019-07-30 23:15 UTC (permalink / raw)
To: Ondrej Mosnacek; +Cc: SElinux list, Dmitry Vyukov, syzbot
On Tue, Jul 30, 2019 at 12:15 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Tue, Jul 30, 2019 at 5:10 PM Paul Moore <paul@paul-moore.com> wrote:
> > On Tue, Jul 30, 2019 at 8:20 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > On Tue, Jul 30, 2019 at 12:48 AM Paul Moore <paul@paul-moore.com> wrote:
> > > > On Mon, Jul 29, 2019 at 4:41 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > > >
> > > > > Since roles_init() adds some entries to the role hash table, we need to
> > > > > destroy also its keys/values on error, otherwise we get a memory leak in
> > > > > the error path.
> > > > >
> > > > > To avoid a forward declaration and maintain a sane layout, move all the
> > > > > destroy stuff above policydb_init. No changes are made to the moved code
> > > > > in this patch. Note that this triggers some pre-existing checkpatch.pl
> > > > > warnings - these will be fixed in follow-up patches.
> > > > >
> > > > > Reported-by: syzbot+fee3a14d4cdf92646287@syzkaller.appspotmail.com
> > > > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > > > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > > > > ---
> > > > > security/selinux/ss/policydb.c | 976 +++++++++++++++++----------------
> > > > > 1 file changed, 489 insertions(+), 487 deletions(-)
> > > >
> > > > Hmmm, that is one ugly patch isn't it? If I saw this diff I'm not
> > > > sure I would have suggested what I did, or rather I would have
> > > > suggested something slightly different.
> > > >
> > > > When I ran my quick test when I was looking at your v1 patch, I only
> > > > moved perm_destroy() through ocontext_destroy(), leaving out
> > > > policydb_destroy(), and the diff was much more cleaner[*] (diffstat
> > > > below, includes the actual fix too). Could you try that and see if it
> > > > cleans up your patch?
> > >
> > > Yeah, excluding policydb_destroy() from the move is what's needed to
> > > get a nice patch...
> >
> > Good, let's just do that.
> >
> > > Actually, what do you think about keeping the
> > > bugfix patch as before (with the forward declaration) and then doing
> > > the moving around in a separate patch (removing the forward
> > > declaration)?
> >
> > Yes, I thought about that too when looking at your patch yesterday and
> > trying to sort out why it was such a messy diff.
> >
> > > Then we keep the patch with the actual fix small, but
> > > still get a clean final result. It would also allow moving
> > > policydb_destroy() up closer to the other destroy functions in another
> > > separate patch (I tried it and both patches end up clean when the move
> > > is split up like this). (I don't have a strong preference for this,
> > > let me know what works best for you.)
> >
> > I'm fine with leaving policydb_destroy() where it is, but I agree that
> > separating the fix is likely worthwhile. I'll go ahead and merge your
> > v1 patch into selinux/stable-5.3 (it's borderline -stable material
> > IMHO, but I'm pretty sure GregKH would pull it into -stable anyway, he
> > pulls everything with a "Fixes" tag it seems), and then merge the
> > reorganization patch into selinux/next. Honestly, I can go ahead and
> > submit the reorg patch, it's basically already sitting in a tree on my
> > disk anyway, but if you would prefer to do it that's fine too, just
> > let me know.
>
> Sure, feel free to submit the reorg yourself (I assume you will then
> merge the checkpatch fixes 2-3/3 on top, right?)
Yep, that's my plan (I should have mentioned that in the previous email).
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/3] selinux: policydb - fix memory leak in policydb_init()
2019-07-30 16:15 ` Ondrej Mosnacek
2019-07-30 23:15 ` Paul Moore
@ 2019-07-31 22:07 ` Paul Moore
1 sibling, 0 replies; 12+ messages in thread
From: Paul Moore @ 2019-07-31 22:07 UTC (permalink / raw)
To: Ondrej Mosnacek; +Cc: SElinux list, Dmitry Vyukov, syzbot
On Tue, Jul 30, 2019 at 12:15 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Tue, Jul 30, 2019 at 5:10 PM Paul Moore <paul@paul-moore.com> wrote:
> > On Tue, Jul 30, 2019 at 8:20 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > On Tue, Jul 30, 2019 at 12:48 AM Paul Moore <paul@paul-moore.com> wrote:
> > > > On Mon, Jul 29, 2019 at 4:41 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > > >
> > > > > Since roles_init() adds some entries to the role hash table, we need to
> > > > > destroy also its keys/values on error, otherwise we get a memory leak in
> > > > > the error path.
> > > > >
> > > > > To avoid a forward declaration and maintain a sane layout, move all the
> > > > > destroy stuff above policydb_init. No changes are made to the moved code
> > > > > in this patch. Note that this triggers some pre-existing checkpatch.pl
> > > > > warnings - these will be fixed in follow-up patches.
> > > > >
> > > > > Reported-by: syzbot+fee3a14d4cdf92646287@syzkaller.appspotmail.com
> > > > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > > > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > > > > ---
> > > > > security/selinux/ss/policydb.c | 976 +++++++++++++++++----------------
> > > > > 1 file changed, 489 insertions(+), 487 deletions(-)
> > > >
> > > > Hmmm, that is one ugly patch isn't it? If I saw this diff I'm not
> > > > sure I would have suggested what I did, or rather I would have
> > > > suggested something slightly different.
> > > >
> > > > When I ran my quick test when I was looking at your v1 patch, I only
> > > > moved perm_destroy() through ocontext_destroy(), leaving out
> > > > policydb_destroy(), and the diff was much more cleaner[*] (diffstat
> > > > below, includes the actual fix too). Could you try that and see if it
> > > > cleans up your patch?
> > >
> > > Yeah, excluding policydb_destroy() from the move is what's needed to
> > > get a nice patch...
> >
> > Good, let's just do that.
> >
> > > Actually, what do you think about keeping the
> > > bugfix patch as before (with the forward declaration) and then doing
> > > the moving around in a separate patch (removing the forward
> > > declaration)?
> >
> > Yes, I thought about that too when looking at your patch yesterday and
> > trying to sort out why it was such a messy diff.
> >
> > > Then we keep the patch with the actual fix small, but
> > > still get a clean final result. It would also allow moving
> > > policydb_destroy() up closer to the other destroy functions in another
> > > separate patch (I tried it and both patches end up clean when the move
> > > is split up like this). (I don't have a strong preference for this,
> > > let me know what works best for you.)
> >
> > I'm fine with leaving policydb_destroy() where it is, but I agree that
> > separating the fix is likely worthwhile. I'll go ahead and merge your
> > v1 patch into selinux/stable-5.3 (it's borderline -stable material
> > IMHO, but I'm pretty sure GregKH would pull it into -stable anyway, he
> > pulls everything with a "Fixes" tag it seems), and then merge the
> > reorganization patch into selinux/next. Honestly, I can go ahead and
> > submit the reorg patch, it's basically already sitting in a tree on my
> > disk anyway, but if you would prefer to do it that's fine too, just
> > let me know.
>
> Sure, feel free to submit the reorg yourself (I assume you will then
> merge the checkpatch fixes 2-3/3 on top, right?)
>
> > I'll may also merge the v1 fix into selinux/next in order to fix the
> > inevitable merge conflict, but that isn't something you have to worry
> > about.
FYI, since selinux/next is still "empty" I think I'm just going to
base it on selinux/stable-5.3 instead of the usual v5.3-rc1.
Hopefully that shouldn't be a problem, but if it becomes an issue we
can adjust it.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/3] selinux: policydb - fix some checkpatch.pl warnings
2019-07-29 8:41 ` [PATCH v2 2/3] selinux: policydb - fix some checkpatch.pl warnings Ondrej Mosnacek
@ 2019-08-05 20:48 ` Paul Moore
0 siblings, 0 replies; 12+ messages in thread
From: Paul Moore @ 2019-08-05 20:48 UTC (permalink / raw)
To: Ondrej Mosnacek; +Cc: selinux, Dmitry Vyukov
On Mon, Jul 29, 2019 at 4:41 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> Fix most of the code style warnings discovered when moving code around.
>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
> security/selinux/ss/policydb.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
Merged into selinux/next, thanks.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 3/3] selinux: policydb - rename type_val_to_struct_array
2019-07-29 8:41 ` [PATCH v2 3/3] selinux: policydb - rename type_val_to_struct_array Ondrej Mosnacek
@ 2019-08-05 20:49 ` Paul Moore
0 siblings, 0 replies; 12+ messages in thread
From: Paul Moore @ 2019-08-05 20:49 UTC (permalink / raw)
To: Ondrej Mosnacek; +Cc: selinux, Dmitry Vyukov
On Mon, Jul 29, 2019 at 4:41 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> The name is overly long and inconsistent with the other *_val_to_struct
> members. Dropping the "_array" prefix makes the code easier to read and
> gets rid of one line over 80 characters warning.
>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
> security/selinux/ss/policydb.c | 14 +++++++-------
> security/selinux/ss/policydb.h | 2 +-
> security/selinux/ss/services.c | 6 +++---
> 3 files changed, 11 insertions(+), 11 deletions(-)
Merged into selinux/next, thanks.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2019-08-05 20:49 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-29 8:41 [PATCH v2 0/3] selinux: policydb - fix memory leak and do some cleanup Ondrej Mosnacek
2019-07-29 8:41 ` [PATCH v2 1/3] selinux: policydb - fix memory leak in policydb_init() Ondrej Mosnacek
2019-07-29 22:48 ` Paul Moore
2019-07-30 12:20 ` Ondrej Mosnacek
2019-07-30 15:10 ` Paul Moore
2019-07-30 16:15 ` Ondrej Mosnacek
2019-07-30 23:15 ` Paul Moore
2019-07-31 22:07 ` Paul Moore
2019-07-29 8:41 ` [PATCH v2 2/3] selinux: policydb - fix some checkpatch.pl warnings Ondrej Mosnacek
2019-08-05 20:48 ` Paul Moore
2019-07-29 8:41 ` [PATCH v2 3/3] selinux: policydb - rename type_val_to_struct_array Ondrej Mosnacek
2019-08-05 20:49 ` Paul Moore
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).