selinux.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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).