From 9398c7f794078dc1768cc061b3da8cdd59f179a5 Mon Sep 17 00:00:00 2001 From: Eric Paris Date: Tue, 23 Nov 2010 11:40:08 -0500 Subject: [PATCH 01/12] SELinux: standardize return code handling in policydb.c policydb.c has lots of different standards on how to handle return paths on error. For the most part transition to rc=errno if (failure) goto out; [...] out: cleanup() return rc; Instead of doing cleanup mid function, or having multiple returns or other options. This doesn't do that for every function, but most of the complex functions which have cleanup routines on error. Signed-off-by: Eric Paris --- security/selinux/ss/policydb.c | 553 ++++++++++++++++----------------- 1 file changed, 267 insertions(+), 286 deletions(-) diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c index 94f630d93a5c..6ad73e81da5c 100644 --- a/security/selinux/ss/policydb.c +++ b/security/selinux/ss/policydb.c @@ -148,32 +148,30 @@ static int roles_init(struct policydb *p) int rc; struct role_datum *role; + rc = -ENOMEM; role = kzalloc(sizeof(*role), GFP_KERNEL); - if (!role) { - rc = -ENOMEM; + if (!role) goto out; - } + + rc = -EINVAL; role->value = ++p->p_roles.nprim; - if (role->value != OBJECT_R_VAL) { - rc = -EINVAL; - goto out_free_role; - } + if (role->value != OBJECT_R_VAL) + goto out; + + rc = -ENOMEM; key = kstrdup(OBJECT_R, GFP_KERNEL); - if (!key) { - rc = -ENOMEM; - goto out_free_role; - } + if (!key) + goto out; + rc = hashtab_insert(p->p_roles.table, key, role); if (rc) - goto out_free_key; -out: - return rc; + goto out; -out_free_key: + return 0; +out: kfree(key); -out_free_role: kfree(role); - goto out; + return rc; } static u32 rangetr_hash(struct hashtab *h, const void *k) @@ -213,35 +211,33 @@ static int policydb_init(struct policydb *p) for (i = 0; i < SYM_NUM; i++) { rc = symtab_init(&p->symtab[i], symtab_sizes[i]); if (rc) - goto out_free_symtab; + goto out; } rc = avtab_init(&p->te_avtab); if (rc) - goto out_free_symtab; + goto out; rc = roles_init(p); if (rc) - goto out_free_symtab; + goto out; rc = cond_policydb_init(p); if (rc) - goto out_free_symtab; + goto out; p->range_tr = hashtab_create(rangetr_hash, rangetr_cmp, 256); if (!p->range_tr) - goto out_free_symtab; + goto out; ebitmap_init(&p->policycaps); ebitmap_init(&p->permissive_map); + return 0; out: - return rc; - -out_free_symtab: for (i = 0; i < SYM_NUM; i++) hashtab_destroy(p->symtab[i].table); - goto out; + return rc; } /* @@ -391,30 +387,27 @@ static int policydb_index_classes(struct policydb *p) { int rc; + rc = -ENOMEM; p->p_common_val_to_name = kmalloc(p->p_commons.nprim * sizeof(char *), GFP_KERNEL); - if (!p->p_common_val_to_name) { - rc = -ENOMEM; + if (!p->p_common_val_to_name) goto out; - } rc = hashtab_map(p->p_commons.table, common_index, p); if (rc) goto out; + rc = -ENOMEM; p->class_val_to_struct = kmalloc(p->p_classes.nprim * sizeof(*(p->class_val_to_struct)), GFP_KERNEL); - if (!p->class_val_to_struct) { - rc = -ENOMEM; + if (!p->class_val_to_struct) goto out; - } + rc = -ENOMEM; p->p_class_val_to_name = kmalloc(p->p_classes.nprim * sizeof(char *), GFP_KERNEL); - if (!p->p_class_val_to_name) { - rc = -ENOMEM; + if (!p->p_class_val_to_name) goto out; - } rc = hashtab_map(p->p_classes.table, class_index, p); out: @@ -460,7 +453,7 @@ static inline void rangetr_hash_eval(struct hashtab *h) */ static int policydb_index_others(struct policydb *p) { - int i, rc = 0; + int i, rc; printk(KERN_DEBUG "SELinux: %d users, %d roles, %d types, %d bools", p->p_users.nprim, p->p_roles.nprim, p->p_types.nprim, p->p_bools.nprim); @@ -477,47 +470,42 @@ static int policydb_index_others(struct policydb *p) symtab_hash_eval(p->symtab); #endif + rc = -ENOMEM; p->role_val_to_struct = kmalloc(p->p_roles.nprim * sizeof(*(p->role_val_to_struct)), GFP_KERNEL); - if (!p->role_val_to_struct) { - rc = -ENOMEM; + if (!p->role_val_to_struct) goto out; - } + rc = -ENOMEM; p->user_val_to_struct = kmalloc(p->p_users.nprim * sizeof(*(p->user_val_to_struct)), GFP_KERNEL); - if (!p->user_val_to_struct) { - rc = -ENOMEM; + if (!p->user_val_to_struct) goto out; - } + rc = -ENOMEM; p->type_val_to_struct = kmalloc(p->p_types.nprim * sizeof(*(p->type_val_to_struct)), GFP_KERNEL); - if (!p->type_val_to_struct) { - rc = -ENOMEM; + if (!p->type_val_to_struct) goto out; - } - if (cond_init_bool_indexes(p)) { - rc = -ENOMEM; + rc = -ENOMEM; + if (cond_init_bool_indexes(p)) goto out; - } for (i = SYM_ROLES; i < SYM_NUM; i++) { + rc = -ENOMEM; p->sym_val_to_name[i] = kmalloc(p->symtab[i].nprim * sizeof(char *), GFP_KERNEL); - if (!p->sym_val_to_name[i]) { - rc = -ENOMEM; + if (!p->sym_val_to_name[i]) goto out; - } rc = hashtab_map(p->symtab[i].table, index_f[i], p); if (rc) goto out; } - + rc = 0; out: return rc; } @@ -540,9 +528,11 @@ static int common_destroy(void *key, void *datum, void *p) struct common_datum *comdatum; kfree(key); - comdatum = datum; - hashtab_map(comdatum->permissions.table, perm_destroy, NULL); - hashtab_destroy(comdatum->permissions.table); + if (datum) { + comdatum = datum; + hashtab_map(comdatum->permissions.table, perm_destroy, NULL); + hashtab_destroy(comdatum->permissions.table); + } kfree(datum); return 0; } @@ -554,38 +544,40 @@ static int cls_destroy(void *key, void *datum, void *p) struct constraint_expr *e, *etmp; kfree(key); - 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) { - ebitmap_destroy(&e->names); - etmp = e; - e = e->next; - kfree(etmp); + 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) { + ebitmap_destroy(&e->names); + etmp = e; + e = e->next; + kfree(etmp); + } + ctemp = constraint; + constraint = constraint->next; + kfree(ctemp); } - ctemp = constraint; - constraint = constraint->next; - kfree(ctemp); - } - constraint = cladatum->validatetrans; - while (constraint) { - e = constraint->expr; - while (e) { - ebitmap_destroy(&e->names); - etmp = e; - e = e->next; - kfree(etmp); + constraint = cladatum->validatetrans; + while (constraint) { + e = constraint->expr; + while (e) { + ebitmap_destroy(&e->names); + etmp = e; + e = e->next; + kfree(etmp); + } + ctemp = constraint; + constraint = constraint->next; + kfree(ctemp); } - ctemp = constraint; - constraint = constraint->next; - kfree(ctemp); - } - kfree(cladatum->comkey); + kfree(cladatum->comkey); + } kfree(datum); return 0; } @@ -595,9 +587,11 @@ static int role_destroy(void *key, void *datum, void *p) struct role_datum *role; kfree(key); - role = datum; - ebitmap_destroy(&role->dominates); - ebitmap_destroy(&role->types); + if (datum) { + role = datum; + ebitmap_destroy(&role->dominates); + ebitmap_destroy(&role->types); + } kfree(datum); return 0; } @@ -614,11 +608,13 @@ static int user_destroy(void *key, void *datum, void *p) struct user_datum *usrdatum; kfree(key); - 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); + 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; } @@ -628,9 +624,11 @@ static int sens_destroy(void *key, void *datum, void *p) struct level_datum *levdatum; kfree(key); - levdatum = datum; - ebitmap_destroy(&levdatum->level->cat); - kfree(levdatum->level); + if (datum) { + levdatum = datum; + ebitmap_destroy(&levdatum->level->cat); + kfree(levdatum->level); + } kfree(datum); return 0; } @@ -785,19 +783,21 @@ int policydb_load_isids(struct policydb *p, struct sidtab *s) head = p->ocontexts[OCON_ISID]; for (c = head; c; c = c->next) { + rc = -EINVAL; if (!c->context[0].user) { - printk(KERN_ERR "SELinux: SID %s was never " - "defined.\n", c->u.name); - rc = -EINVAL; + printk(KERN_ERR "SELinux: SID %s was never defined.\n", + c->u.name); goto out; } - if (sidtab_insert(s, c->sid[0], &c->context[0])) { - printk(KERN_ERR "SELinux: unable to load initial " - "SID %s.\n", c->u.name); - rc = -EINVAL; + + rc = sidtab_insert(s, c->sid[0], &c->context[0]); + if (rc) { + printk(KERN_ERR "SELinux: unable to load initial SID %s.\n", + c->u.name); goto out; } } + rc = 0; out: return rc; } @@ -846,8 +846,7 @@ int policydb_context_isvalid(struct policydb *p, struct context *c) * Role must be authorized for the type. */ role = p->role_val_to_struct[c->role - 1]; - if (!ebitmap_get_bit(&role->types, - c->type - 1)) + if (!ebitmap_get_bit(&role->types, c->type - 1)) /* role may not be associated with type */ return 0; @@ -858,8 +857,7 @@ int policydb_context_isvalid(struct policydb *p, struct context *c) if (!usrdatum) return 0; - if (!ebitmap_get_bit(&usrdatum->roles, - c->role - 1)) + if (!ebitmap_get_bit(&usrdatum->roles, c->role - 1)) /* user may not be associated with role */ return 0; } @@ -881,20 +879,22 @@ static int mls_read_range_helper(struct mls_range *r, void *fp) int rc; rc = next_entry(buf, fp, sizeof(u32)); - if (rc < 0) + if (rc) goto out; + rc = -EINVAL; items = le32_to_cpu(buf[0]); if (items > ARRAY_SIZE(buf)) { printk(KERN_ERR "SELinux: mls: range overflow\n"); - rc = -EINVAL; goto out; } + rc = next_entry(buf, fp, sizeof(u32) * items); - if (rc < 0) { + if (rc) { printk(KERN_ERR "SELinux: mls: truncated range\n"); goto out; } + r->level[0].sens = le32_to_cpu(buf[0]); if (items > 1) r->level[1].sens = le32_to_cpu(buf[1]); @@ -903,15 +903,13 @@ static int mls_read_range_helper(struct mls_range *r, void *fp) rc = ebitmap_read(&r->level[0].cat, fp); if (rc) { - printk(KERN_ERR "SELinux: mls: error reading low " - "categories\n"); + printk(KERN_ERR "SELinux: mls: error reading low categories\n"); goto out; } if (items > 1) { rc = ebitmap_read(&r->level[1].cat, fp); if (rc) { - printk(KERN_ERR "SELinux: mls: error reading high " - "categories\n"); + printk(KERN_ERR "SELinux: mls: error reading high categories\n"); goto bad_high; } } else { @@ -922,12 +920,11 @@ static int mls_read_range_helper(struct mls_range *r, void *fp) } } - rc = 0; -out: - return rc; + return 0; bad_high: ebitmap_destroy(&r->level[0].cat); - goto out; +out: + return rc; } /* @@ -942,7 +939,7 @@ static int context_read_and_validate(struct context *c, int rc; rc = next_entry(buf, fp, sizeof buf); - if (rc < 0) { + if (rc) { printk(KERN_ERR "SELinux: context truncated\n"); goto out; } @@ -950,19 +947,20 @@ static int context_read_and_validate(struct context *c, c->role = le32_to_cpu(buf[1]); c->type = le32_to_cpu(buf[2]); if (p->policyvers >= POLICYDB_VERSION_MLS) { - if (mls_read_range_helper(&c->range, fp)) { - printk(KERN_ERR "SELinux: error reading MLS range of " - "context\n"); - rc = -EINVAL; + rc = mls_read_range_helper(&c->range, fp); + if (rc) { + printk(KERN_ERR "SELinux: error reading MLS range of context\n"); goto out; } } + rc = -EINVAL; if (!policydb_context_isvalid(p, c)) { printk(KERN_ERR "SELinux: invalid security context\n"); context_destroy(c); - rc = -EINVAL; + goto out; } + rc = 0; out: return rc; } @@ -981,37 +979,36 @@ static int perm_read(struct policydb *p, struct hashtab *h, void *fp) __le32 buf[2]; u32 len; + rc = -ENOMEM; perdatum = kzalloc(sizeof(*perdatum), GFP_KERNEL); - if (!perdatum) { - rc = -ENOMEM; - goto out; - } + if (!perdatum) + goto bad; rc = next_entry(buf, fp, sizeof buf); - if (rc < 0) + if (rc) goto bad; len = le32_to_cpu(buf[0]); perdatum->value = le32_to_cpu(buf[1]); + rc = -ENOMEM; key = kmalloc(len + 1, GFP_KERNEL); - if (!key) { - rc = -ENOMEM; + if (!key) goto bad; - } + rc = next_entry(key, fp, len); - if (rc < 0) + if (rc) goto bad; key[len] = '\0'; rc = hashtab_insert(h, key, perdatum); if (rc) goto bad; -out: - return rc; + + return 0; bad: perm_destroy(key, perdatum, NULL); - goto out; + return rc; } static int common_read(struct policydb *p, struct hashtab *h, void *fp) @@ -1022,14 +1019,13 @@ static int common_read(struct policydb *p, struct hashtab *h, void *fp) u32 len, nel; int i, rc; + rc = -ENOMEM; comdatum = kzalloc(sizeof(*comdatum), GFP_KERNEL); - if (!comdatum) { - rc = -ENOMEM; - goto out; - } + if (!comdatum) + goto bad; rc = next_entry(buf, fp, sizeof buf); - if (rc < 0) + if (rc) goto bad; len = le32_to_cpu(buf[0]); @@ -1041,13 +1037,13 @@ static int common_read(struct policydb *p, struct hashtab *h, void *fp) comdatum->permissions.nprim = le32_to_cpu(buf[2]); nel = le32_to_cpu(buf[3]); + rc = -ENOMEM; key = kmalloc(len + 1, GFP_KERNEL); - if (!key) { - rc = -ENOMEM; + if (!key) goto bad; - } + rc = next_entry(key, fp, len); - if (rc < 0) + if (rc) goto bad; key[len] = '\0'; @@ -1060,11 +1056,10 @@ static int common_read(struct policydb *p, struct hashtab *h, void *fp) rc = hashtab_insert(h, key, comdatum); if (rc) goto bad; -out: - return rc; + return 0; bad: common_destroy(key, comdatum, NULL); - goto out; + return rc; } static int read_cons_helper(struct constraint_node **nodep, int ncons, @@ -1088,7 +1083,7 @@ static int read_cons_helper(struct constraint_node **nodep, int ncons, *nodep = c; rc = next_entry(buf, fp, (sizeof(u32) * 2)); - if (rc < 0) + if (rc) return rc; c->permissions = le32_to_cpu(buf[0]); nexpr = le32_to_cpu(buf[1]); @@ -1105,7 +1100,7 @@ static int read_cons_helper(struct constraint_node **nodep, int ncons, c->expr = e; rc = next_entry(buf, fp, (sizeof(u32) * 3)); - if (rc < 0) + if (rc) return rc; e->expr_type = le32_to_cpu(buf[0]); e->attr = le32_to_cpu(buf[1]); @@ -1133,8 +1128,9 @@ static int read_cons_helper(struct constraint_node **nodep, int ncons, if (depth == (CEXPR_MAXDEPTH - 1)) return -EINVAL; depth++; - if (ebitmap_read(&e->names, fp)) - return -EINVAL; + rc = ebitmap_read(&e->names, fp); + if (rc) + return rc; break; default: return -EINVAL; @@ -1157,14 +1153,13 @@ static int class_read(struct policydb *p, struct hashtab *h, void *fp) u32 len, len2, ncons, nel; int i, rc; + rc = -ENOMEM; cladatum = kzalloc(sizeof(*cladatum), GFP_KERNEL); - if (!cladatum) { - rc = -ENOMEM; - goto out; - } + if (!cladatum) + goto bad; rc = next_entry(buf, fp, sizeof(u32)*6); - if (rc < 0) + if (rc) goto bad; len = le32_to_cpu(buf[0]); @@ -1179,33 +1174,30 @@ static int class_read(struct policydb *p, struct hashtab *h, void *fp) ncons = le32_to_cpu(buf[5]); + rc = -ENOMEM; key = kmalloc(len + 1, GFP_KERNEL); - if (!key) { - rc = -ENOMEM; + if (!key) goto bad; - } + rc = next_entry(key, fp, len); - if (rc < 0) + if (rc) goto bad; key[len] = '\0'; if (len2) { + rc = -ENOMEM; cladatum->comkey = kmalloc(len2 + 1, GFP_KERNEL); - if (!cladatum->comkey) { - rc = -ENOMEM; + if (!cladatum->comkey) goto bad; - } rc = next_entry(cladatum->comkey, fp, len2); - if (rc < 0) + if (rc) goto bad; cladatum->comkey[len2] = '\0'; - cladatum->comdatum = hashtab_search(p->p_commons.table, - cladatum->comkey); + rc = -EINVAL; + cladatum->comdatum = hashtab_search(p->p_commons.table, cladatum->comkey); if (!cladatum->comdatum) { - printk(KERN_ERR "SELinux: unknown common %s\n", - cladatum->comkey); - rc = -EINVAL; + printk(KERN_ERR "SELinux: unknown common %s\n", cladatum->comkey); goto bad; } } @@ -1222,7 +1214,7 @@ static int class_read(struct policydb *p, struct hashtab *h, void *fp) if (p->policyvers >= POLICYDB_VERSION_VALIDATETRANS) { /* grab the validatetrans rules */ rc = next_entry(buf, fp, sizeof(u32)); - if (rc < 0) + if (rc) goto bad; ncons = le32_to_cpu(buf[0]); rc = read_cons_helper(&cladatum->validatetrans, ncons, 1, fp); @@ -1234,12 +1226,10 @@ static int class_read(struct policydb *p, struct hashtab *h, void *fp) if (rc) goto bad; - rc = 0; -out: - return rc; + return 0; bad: cls_destroy(key, cladatum, NULL); - goto out; + return rc; } static int role_read(struct policydb *p, struct hashtab *h, void *fp) @@ -1250,17 +1240,16 @@ static int role_read(struct policydb *p, struct hashtab *h, void *fp) __le32 buf[3]; u32 len; + rc = -ENOMEM; role = kzalloc(sizeof(*role), GFP_KERNEL); - if (!role) { - rc = -ENOMEM; - goto out; - } + if (!role) + goto bad; if (p->policyvers >= POLICYDB_VERSION_BOUNDARY) to_read = 3; rc = next_entry(buf, fp, sizeof(buf[0]) * to_read); - if (rc < 0) + if (rc) goto bad; len = le32_to_cpu(buf[0]); @@ -1268,13 +1257,13 @@ static int role_read(struct policydb *p, struct hashtab *h, void *fp) if (p->policyvers >= POLICYDB_VERSION_BOUNDARY) role->bounds = le32_to_cpu(buf[2]); + rc = -ENOMEM; key = kmalloc(len + 1, GFP_KERNEL); - if (!key) { - rc = -ENOMEM; + if (!key) goto bad; - } + rc = next_entry(key, fp, len); - if (rc < 0) + if (rc) goto bad; key[len] = '\0'; @@ -1287,10 +1276,10 @@ static int role_read(struct policydb *p, struct hashtab *h, void *fp) goto bad; if (strcmp(key, OBJECT_R) == 0) { + rc = -EINVAL; if (role->value != OBJECT_R_VAL) { printk(KERN_ERR "SELinux: Role %s has wrong value %d\n", OBJECT_R, role->value); - rc = -EINVAL; goto bad; } rc = 0; @@ -1300,11 +1289,10 @@ static int role_read(struct policydb *p, struct hashtab *h, void *fp) rc = hashtab_insert(h, key, role); if (rc) goto bad; -out: - return rc; + return 0; bad: role_destroy(key, role, NULL); - goto out; + return rc; } static int type_read(struct policydb *p, struct hashtab *h, void *fp) @@ -1315,17 +1303,16 @@ static int type_read(struct policydb *p, struct hashtab *h, void *fp) __le32 buf[4]; u32 len; + rc = -ENOMEM; typdatum = kzalloc(sizeof(*typdatum), GFP_KERNEL); - if (!typdatum) { - rc = -ENOMEM; - return rc; - } + if (!typdatum) + goto bad; if (p->policyvers >= POLICYDB_VERSION_BOUNDARY) to_read = 4; rc = next_entry(buf, fp, sizeof(buf[0]) * to_read); - if (rc < 0) + if (rc) goto bad; len = le32_to_cpu(buf[0]); @@ -1343,24 +1330,22 @@ static int type_read(struct policydb *p, struct hashtab *h, void *fp) typdatum->primary = le32_to_cpu(buf[2]); } + rc = -ENOMEM; key = kmalloc(len + 1, GFP_KERNEL); - if (!key) { - rc = -ENOMEM; + if (!key) goto bad; - } rc = next_entry(key, fp, len); - if (rc < 0) + if (rc) goto bad; key[len] = '\0'; rc = hashtab_insert(h, key, typdatum); if (rc) goto bad; -out: - return rc; + return 0; bad: type_destroy(key, typdatum, NULL); - goto out; + return rc; } @@ -1376,22 +1361,18 @@ static int mls_read_level(struct mls_level *lp, void *fp) memset(lp, 0, sizeof(*lp)); rc = next_entry(buf, fp, sizeof buf); - if (rc < 0) { + if (rc) { printk(KERN_ERR "SELinux: mls: truncated level\n"); - goto bad; + return rc; } lp->sens = le32_to_cpu(buf[0]); - if (ebitmap_read(&lp->cat, fp)) { - printk(KERN_ERR "SELinux: mls: error reading level " - "categories\n"); - goto bad; + rc = ebitmap_read(&lp->cat, fp); + if (rc) { + printk(KERN_ERR "SELinux: mls: error reading level categories\n"); + return rc; } - return 0; - -bad: - return -EINVAL; } static int user_read(struct policydb *p, struct hashtab *h, void *fp) @@ -1402,17 +1383,16 @@ static int user_read(struct policydb *p, struct hashtab *h, void *fp) __le32 buf[3]; u32 len; + rc = -ENOMEM; usrdatum = kzalloc(sizeof(*usrdatum), GFP_KERNEL); - if (!usrdatum) { - rc = -ENOMEM; - goto out; - } + if (!usrdatum) + goto bad; if (p->policyvers >= POLICYDB_VERSION_BOUNDARY) to_read = 3; rc = next_entry(buf, fp, sizeof(buf[0]) * to_read); - if (rc < 0) + if (rc) goto bad; len = le32_to_cpu(buf[0]); @@ -1420,13 +1400,12 @@ static int user_read(struct policydb *p, struct hashtab *h, void *fp) if (p->policyvers >= POLICYDB_VERSION_BOUNDARY) usrdatum->bounds = le32_to_cpu(buf[2]); + rc = -ENOMEM; key = kmalloc(len + 1, GFP_KERNEL); - if (!key) { - rc = -ENOMEM; + if (!key) goto bad; - } rc = next_entry(key, fp, len); - if (rc < 0) + if (rc) goto bad; key[len] = '\0'; @@ -1446,11 +1425,10 @@ static int user_read(struct policydb *p, struct hashtab *h, void *fp) rc = hashtab_insert(h, key, usrdatum); if (rc) goto bad; -out: - return rc; + return 0; bad: user_destroy(key, usrdatum, NULL); - goto out; + return rc; } static int sens_read(struct policydb *p, struct hashtab *h, void *fp) @@ -1461,47 +1439,43 @@ static int sens_read(struct policydb *p, struct hashtab *h, void *fp) __le32 buf[2]; u32 len; + rc = -ENOMEM; levdatum = kzalloc(sizeof(*levdatum), GFP_ATOMIC); - if (!levdatum) { - rc = -ENOMEM; - goto out; - } + if (!levdatum) + goto bad; rc = next_entry(buf, fp, sizeof buf); - if (rc < 0) + if (rc) goto bad; len = le32_to_cpu(buf[0]); levdatum->isalias = le32_to_cpu(buf[1]); + rc = -ENOMEM; key = kmalloc(len + 1, GFP_ATOMIC); - if (!key) { - rc = -ENOMEM; + if (!key) goto bad; - } rc = next_entry(key, fp, len); - if (rc < 0) + if (rc) goto bad; key[len] = '\0'; + rc = -ENOMEM; levdatum->level = kmalloc(sizeof(struct mls_level), GFP_ATOMIC); - if (!levdatum->level) { - rc = -ENOMEM; + if (!levdatum->level) goto bad; - } - if (mls_read_level(levdatum->level, fp)) { - rc = -EINVAL; + + rc = mls_read_level(levdatum->level, fp); + if (rc) goto bad; - } rc = hashtab_insert(h, key, levdatum); if (rc) goto bad; -out: - return rc; + return 0; bad: sens_destroy(key, levdatum, NULL); - goto out; + return rc; } static int cat_read(struct policydb *p, struct hashtab *h, void *fp) @@ -1512,39 +1486,35 @@ static int cat_read(struct policydb *p, struct hashtab *h, void *fp) __le32 buf[3]; u32 len; + rc = -ENOMEM; catdatum = kzalloc(sizeof(*catdatum), GFP_ATOMIC); - if (!catdatum) { - rc = -ENOMEM; - goto out; - } + if (!catdatum) + goto bad; rc = next_entry(buf, fp, sizeof buf); - if (rc < 0) + if (rc) goto bad; len = le32_to_cpu(buf[0]); catdatum->value = le32_to_cpu(buf[1]); catdatum->isalias = le32_to_cpu(buf[2]); + rc = -ENOMEM; key = kmalloc(len + 1, GFP_ATOMIC); - if (!key) { - rc = -ENOMEM; + if (!key) goto bad; - } rc = next_entry(key, fp, len); - if (rc < 0) + if (rc) goto bad; key[len] = '\0'; rc = hashtab_insert(h, key, catdatum); if (rc) goto bad; -out: - return rc; - + return 0; bad: cat_destroy(key, catdatum, NULL); - goto out; + return rc; } static int (*read_f[SYM_NUM]) (struct policydb *p, struct hashtab *h, void *fp) = @@ -2066,13 +2036,14 @@ int policydb_read(struct policydb *p, void *fp) rc = policydb_init(p); if (rc) - goto out; + return rc; /* Read the magic number and string length. */ rc = next_entry(buf, fp, sizeof(u32) * 2); - if (rc < 0) + if (rc) goto bad; + rc = -EINVAL; if (le32_to_cpu(buf[0]) != POLICYDB_MAGIC) { printk(KERN_ERR "SELinux: policydb magic number 0x%x does " "not match expected magic number 0x%x\n", @@ -2080,6 +2051,7 @@ int policydb_read(struct policydb *p, void *fp) goto bad; } + rc = -EINVAL; len = le32_to_cpu(buf[1]); if (len != strlen(POLICYDB_STRING)) { printk(KERN_ERR "SELinux: policydb string length %d does not " @@ -2087,19 +2059,23 @@ int policydb_read(struct policydb *p, void *fp) len, strlen(POLICYDB_STRING)); goto bad; } + + rc = -ENOMEM; policydb_str = kmalloc(len + 1, GFP_KERNEL); if (!policydb_str) { printk(KERN_ERR "SELinux: unable to allocate memory for policydb " "string of length %d\n", len); - rc = -ENOMEM; goto bad; } + rc = next_entry(policydb_str, fp, len); - if (rc < 0) { + if (rc) { printk(KERN_ERR "SELinux: truncated policydb string identifier\n"); kfree(policydb_str); goto bad; } + + rc = -EINVAL; policydb_str[len] = '\0'; if (strcmp(policydb_str, POLICYDB_STRING)) { printk(KERN_ERR "SELinux: policydb string %s does not match " @@ -2113,9 +2089,10 @@ int policydb_read(struct policydb *p, void *fp) /* Read the version and table sizes. */ rc = next_entry(buf, fp, sizeof(u32)*4); - if (rc < 0) + if (rc) goto bad; + rc = -EINVAL; p->policyvers = le32_to_cpu(buf[0]); if (p->policyvers < POLICYDB_VERSION_MIN || p->policyvers > POLICYDB_VERSION_MAX) { @@ -2128,6 +2105,7 @@ int policydb_read(struct policydb *p, void *fp) if ((le32_to_cpu(buf[1]) & POLICYDB_CONFIG_MLS)) { p->mls_enabled = 1; + rc = -EINVAL; if (p->policyvers < POLICYDB_VERSION_MLS) { printk(KERN_ERR "SELinux: security policydb version %d " "(MLS) not backwards compatible\n", @@ -2138,14 +2116,19 @@ int policydb_read(struct policydb *p, void *fp) p->reject_unknown = !!(le32_to_cpu(buf[1]) & REJECT_UNKNOWN); p->allow_unknown = !!(le32_to_cpu(buf[1]) & ALLOW_UNKNOWN); - if (p->policyvers >= POLICYDB_VERSION_POLCAP && - ebitmap_read(&p->policycaps, fp) != 0) - goto bad; + if (p->policyvers >= POLICYDB_VERSION_POLCAP) { + rc = ebitmap_read(&p->policycaps, fp); + if (rc) + goto bad; + } - if (p->policyvers >= POLICYDB_VERSION_PERMISSIVE && - ebitmap_read(&p->permissive_map, fp) != 0) - goto bad; + if (p->policyvers >= POLICYDB_VERSION_PERMISSIVE) { + rc = ebitmap_read(&p->permissive_map, fp); + if (rc) + goto bad; + } + rc = -EINVAL; info = policydb_lookup_compat(p->policyvers); if (!info) { printk(KERN_ERR "SELinux: unable to find policy compat info " @@ -2153,6 +2136,7 @@ int policydb_read(struct policydb *p, void *fp) goto bad; } + rc = -EINVAL; if (le32_to_cpu(buf[2]) != info->sym_num || le32_to_cpu(buf[3]) != info->ocon_num) { printk(KERN_ERR "SELinux: policydb table sizes (%d,%d) do " @@ -2164,7 +2148,7 @@ int policydb_read(struct policydb *p, void *fp) for (i = 0; i < info->sym_num; i++) { rc = next_entry(buf, fp, sizeof(u32)*2); - if (rc < 0) + if (rc) goto bad; nprim = le32_to_cpu(buf[0]); nel = le32_to_cpu(buf[1]); @@ -2188,60 +2172,58 @@ int policydb_read(struct policydb *p, void *fp) } rc = next_entry(buf, fp, sizeof(u32)); - if (rc < 0) + if (rc) goto bad; nel = le32_to_cpu(buf[0]); ltr = NULL; for (i = 0; i < nel; i++) { + rc = -ENOMEM; tr = kzalloc(sizeof(*tr), GFP_KERNEL); - if (!tr) { - rc = -ENOMEM; + if (!tr) goto bad; - } if (ltr) ltr->next = tr; else p->role_tr = tr; rc = next_entry(buf, fp, sizeof(u32)*3); - if (rc < 0) + if (rc) goto bad; + + rc = -EINVAL; tr->role = le32_to_cpu(buf[0]); tr->type = le32_to_cpu(buf[1]); tr->new_role = le32_to_cpu(buf[2]); if (!policydb_role_isvalid(p, tr->role) || !policydb_type_isvalid(p, tr->type) || - !policydb_role_isvalid(p, tr->new_role)) { - rc = -EINVAL; + !policydb_role_isvalid(p, tr->new_role)) goto bad; - } ltr = tr; } rc = next_entry(buf, fp, sizeof(u32)); - if (rc < 0) + if (rc) goto bad; nel = le32_to_cpu(buf[0]); lra = NULL; for (i = 0; i < nel; i++) { + rc = -ENOMEM; ra = kzalloc(sizeof(*ra), GFP_KERNEL); - if (!ra) { - rc = -ENOMEM; + if (!ra) goto bad; - } if (lra) lra->next = ra; else p->role_allow = ra; rc = next_entry(buf, fp, sizeof(u32)*2); - if (rc < 0) + if (rc) goto bad; + + rc = -EINVAL; ra->role = le32_to_cpu(buf[0]); ra->new_role = le32_to_cpu(buf[1]); if (!policydb_role_isvalid(p, ra->role) || - !policydb_role_isvalid(p, ra->new_role)) { - rc = -EINVAL; + !policydb_role_isvalid(p, ra->new_role)) goto bad; - } lra = ra; } @@ -2253,13 +2235,14 @@ int policydb_read(struct policydb *p, void *fp) if (rc) goto bad; + rc = -EINVAL; p->process_class = string_to_security_class(p, "process"); if (!p->process_class) goto bad; - p->process_trans_perms = string_to_av_perm(p, p->process_class, - "transition"); - p->process_trans_perms |= string_to_av_perm(p, p->process_class, - "dyntransition"); + + rc = -EINVAL; + p->process_trans_perms = string_to_av_perm(p, p->process_class, "transition"); + p->process_trans_perms |= string_to_av_perm(p, p->process_class, "dyntransition"); if (!p->process_trans_perms) goto bad; @@ -2312,8 +2295,6 @@ int policydb_read(struct policydb *p, void *fp) out: return rc; bad: - if (!rc) - rc = -EINVAL; policydb_destroy(p); goto out; } @@ -3076,7 +3057,7 @@ int policydb_write(struct policydb *p, void *fp) if (!info) { printk(KERN_ERR "SELinux: compatibility lookup failed for policy " "version %d", p->policyvers); - return rc; + return -EINVAL; } buf[0] = cpu_to_le32(p->policyvers); From b77a493b1dc8010245feeac001e5c7ed0988678f Mon Sep 17 00:00:00 2001 From: Eric Paris Date: Tue, 23 Nov 2010 11:40:08 -0500 Subject: [PATCH 02/12] SELinux: standardize return code handling in selinuxfs.c selinuxfs.c has lots of different standards on how to handle return paths on error. For the most part transition to rc=errno if (failure) goto out; [...] out: cleanup() return rc; Instead of doing cleanup mid function, or having multiple returns or other options. This doesn't do that for every function, but most of the complex functions which have cleanup routines on error. Signed-off-by: Eric Paris --- security/selinux/selinuxfs.c | 660 +++++++++++++++++------------------ 1 file changed, 317 insertions(+), 343 deletions(-) diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c index 073fd5b0a53a..8bae68e21af9 100644 --- a/security/selinux/selinuxfs.c +++ b/security/selinux/selinuxfs.c @@ -141,19 +141,24 @@ static ssize_t sel_write_enforce(struct file *file, const char __user *buf, size_t count, loff_t *ppos) { - char *page; + char *page = NULL; ssize_t length; int new_value; + length = -ENOMEM; if (count >= PAGE_SIZE) - return -ENOMEM; - if (*ppos != 0) { - /* No partial writes. */ - return -EINVAL; - } + goto out; + + /* No partial writes. */ + length = EINVAL; + if (*ppos != 0) + goto out; + + length = -ENOMEM; page = (char *)get_zeroed_page(GFP_KERNEL); if (!page) - return -ENOMEM; + goto out; + length = -EFAULT; if (copy_from_user(page, buf, count)) goto out; @@ -268,20 +273,25 @@ static ssize_t sel_write_disable(struct file *file, const char __user *buf, size_t count, loff_t *ppos) { - char *page; + char *page = NULL; ssize_t length; int new_value; extern int selinux_disable(void); + length = -ENOMEM; if (count >= PAGE_SIZE) - return -ENOMEM; - if (*ppos != 0) { - /* No partial writes. */ - return -EINVAL; - } + goto out;; + + /* No partial writes. */ + length = -EINVAL; + if (*ppos != 0) + goto out; + + length = -ENOMEM; page = (char *)get_zeroed_page(GFP_KERNEL); if (!page) - return -ENOMEM; + goto out; + length = -EFAULT; if (copy_from_user(page, buf, count)) goto out; @@ -292,7 +302,7 @@ static ssize_t sel_write_disable(struct file *file, const char __user *buf, if (new_value) { length = selinux_disable(); - if (length < 0) + if (length) goto out; audit_log(current->audit_context, GFP_KERNEL, AUDIT_MAC_STATUS, "selinux=0 auid=%u ses=%u", @@ -493,7 +503,6 @@ static ssize_t sel_write_load(struct file *file, const char __user *buf, size_t count, loff_t *ppos) { - int ret; ssize_t length; void *data = NULL; @@ -503,17 +512,19 @@ static ssize_t sel_write_load(struct file *file, const char __user *buf, if (length) goto out; - if (*ppos != 0) { - /* No partial writes. */ - length = -EINVAL; + /* No partial writes. */ + length = -EINVAL; + if (*ppos != 0) goto out; - } - if ((count > 64 * 1024 * 1024) - || (data = vmalloc(count)) == NULL) { - length = -ENOMEM; + length = -EFBIG; + if (count > 64 * 1024 * 1024) + goto out; + + length = -ENOMEM; + data = vmalloc(count); + if (!data) goto out; - } length = -EFAULT; if (copy_from_user(data, buf, count) != 0) @@ -523,23 +534,19 @@ static ssize_t sel_write_load(struct file *file, const char __user *buf, if (length) goto out; - ret = sel_make_bools(); - if (ret) { - length = ret; + length = sel_make_bools(); + if (length) goto out1; - } - ret = sel_make_classes(); - if (ret) { - length = ret; + length = sel_make_classes(); + if (length) goto out1; - } - ret = sel_make_policycap(); - if (ret) - length = ret; - else - length = count; + length = sel_make_policycap(); + if (length) + goto out1; + + length = count; out1: audit_log(current->audit_context, GFP_KERNEL, AUDIT_MAC_POLICY_LOAD, @@ -559,26 +566,26 @@ static const struct file_operations sel_load_ops = { static ssize_t sel_write_context(struct file *file, char *buf, size_t size) { - char *canon; + char *canon = NULL; u32 sid, len; ssize_t length; length = task_has_security(current, SECURITY__CHECK_CONTEXT); if (length) - return length; + goto out; length = security_context_to_sid(buf, size, &sid); - if (length < 0) - return length; + if (length) + goto out; length = security_sid_to_context(sid, &canon, &len); - if (length < 0) - return length; + if (length) + goto out; + length = -ERANGE; if (len > SIMPLE_TRANSACTION_LIMIT) { printk(KERN_ERR "SELinux: %s: context size (%u) exceeds " "payload max\n", __func__, len); - length = -ERANGE; goto out; } @@ -602,23 +609,28 @@ static ssize_t sel_read_checkreqprot(struct file *filp, char __user *buf, static ssize_t sel_write_checkreqprot(struct file *file, const char __user *buf, size_t count, loff_t *ppos) { - char *page; + char *page = NULL; ssize_t length; unsigned int new_value; length = task_has_security(current, SECURITY__SETCHECKREQPROT); if (length) - return length; + goto out; + length = -ENOMEM; if (count >= PAGE_SIZE) - return -ENOMEM; - if (*ppos != 0) { - /* No partial writes. */ - return -EINVAL; - } + goto out; + + /* No partial writes. */ + length = -EINVAL; + if (*ppos != 0) + goto out; + + length = -ENOMEM; page = (char *)get_zeroed_page(GFP_KERNEL); if (!page) - return -ENOMEM; + goto out; + length = -EFAULT; if (copy_from_user(page, buf, count)) goto out; @@ -693,7 +705,7 @@ static const struct file_operations transaction_ops = { static ssize_t sel_write_access(struct file *file, char *buf, size_t size) { - char *scon, *tcon; + char *scon = NULL, *tcon = NULL; u32 ssid, tsid; u16 tclass; struct av_decision avd; @@ -701,27 +713,29 @@ static ssize_t sel_write_access(struct file *file, char *buf, size_t size) length = task_has_security(current, SECURITY__COMPUTE_AV); if (length) - return length; + goto out; length = -ENOMEM; scon = kzalloc(size + 1, GFP_KERNEL); if (!scon) - return length; + goto out; + length = -ENOMEM; tcon = kzalloc(size + 1, GFP_KERNEL); if (!tcon) goto out; length = -EINVAL; if (sscanf(buf, "%s %s %hu", scon, tcon, &tclass) != 3) - goto out2; + goto out; length = security_context_to_sid(scon, strlen(scon) + 1, &ssid); - if (length < 0) - goto out2; + if (length) + goto out; + length = security_context_to_sid(tcon, strlen(tcon) + 1, &tsid); - if (length < 0) - goto out2; + if (length) + goto out; security_compute_av_user(ssid, tsid, tclass, &avd); @@ -730,133 +744,131 @@ static ssize_t sel_write_access(struct file *file, char *buf, size_t size) avd.allowed, 0xffffffff, avd.auditallow, avd.auditdeny, avd.seqno, avd.flags); -out2: - kfree(tcon); out: + kfree(tcon); kfree(scon); return length; } static ssize_t sel_write_create(struct file *file, char *buf, size_t size) { - char *scon, *tcon; + char *scon = NULL, *tcon = NULL; u32 ssid, tsid, newsid; u16 tclass; ssize_t length; - char *newcon; + char *newcon = NULL; u32 len; length = task_has_security(current, SECURITY__COMPUTE_CREATE); if (length) - return length; + goto out; length = -ENOMEM; scon = kzalloc(size + 1, GFP_KERNEL); if (!scon) - return length; + goto out; + length = -ENOMEM; tcon = kzalloc(size + 1, GFP_KERNEL); if (!tcon) goto out; length = -EINVAL; if (sscanf(buf, "%s %s %hu", scon, tcon, &tclass) != 3) - goto out2; + goto out; length = security_context_to_sid(scon, strlen(scon) + 1, &ssid); - if (length < 0) - goto out2; + if (length) + goto out; + length = security_context_to_sid(tcon, strlen(tcon) + 1, &tsid); - if (length < 0) - goto out2; + if (length) + goto out; length = security_transition_sid_user(ssid, tsid, tclass, &newsid); - if (length < 0) - goto out2; + if (length) + goto out; length = security_sid_to_context(newsid, &newcon, &len); - if (length < 0) - goto out2; + if (length) + goto out; + length = -ERANGE; if (len > SIMPLE_TRANSACTION_LIMIT) { printk(KERN_ERR "SELinux: %s: context size (%u) exceeds " "payload max\n", __func__, len); - length = -ERANGE; - goto out3; + goto out; } memcpy(buf, newcon, len); length = len; -out3: - kfree(newcon); -out2: - kfree(tcon); out: + kfree(newcon); + kfree(tcon); kfree(scon); return length; } static ssize_t sel_write_relabel(struct file *file, char *buf, size_t size) { - char *scon, *tcon; + char *scon = NULL, *tcon = NULL; u32 ssid, tsid, newsid; u16 tclass; ssize_t length; - char *newcon; + char *newcon = NULL; u32 len; length = task_has_security(current, SECURITY__COMPUTE_RELABEL); if (length) - return length; + goto out; length = -ENOMEM; scon = kzalloc(size + 1, GFP_KERNEL); if (!scon) - return length; + goto out; + length = -ENOMEM; tcon = kzalloc(size + 1, GFP_KERNEL); if (!tcon) goto out; length = -EINVAL; if (sscanf(buf, "%s %s %hu", scon, tcon, &tclass) != 3) - goto out2; + goto out; length = security_context_to_sid(scon, strlen(scon) + 1, &ssid); - if (length < 0) - goto out2; + if (length) + goto out; + length = security_context_to_sid(tcon, strlen(tcon) + 1, &tsid); - if (length < 0) - goto out2; + if (length) + goto out; length = security_change_sid(ssid, tsid, tclass, &newsid); - if (length < 0) - goto out2; + if (length) + goto out; length = security_sid_to_context(newsid, &newcon, &len); - if (length < 0) - goto out2; + if (length) + goto out; - if (len > SIMPLE_TRANSACTION_LIMIT) { - length = -ERANGE; - goto out3; - } + length = -ERANGE; + if (len > SIMPLE_TRANSACTION_LIMIT) + goto out; memcpy(buf, newcon, len); length = len; -out3: - kfree(newcon); -out2: - kfree(tcon); out: + kfree(newcon); + kfree(tcon); kfree(scon); return length; } static ssize_t sel_write_user(struct file *file, char *buf, size_t size) { - char *con, *user, *ptr; - u32 sid, *sids; + char *con = NULL, *user = NULL, *ptr; + u32 sid, *sids = NULL; ssize_t length; char *newcon; int i, rc; @@ -864,28 +876,29 @@ static ssize_t sel_write_user(struct file *file, char *buf, size_t size) length = task_has_security(current, SECURITY__COMPUTE_USER); if (length) - return length; + goto out;; length = -ENOMEM; con = kzalloc(size + 1, GFP_KERNEL); if (!con) - return length; + goto out;; + length = -ENOMEM; user = kzalloc(size + 1, GFP_KERNEL); if (!user) goto out; length = -EINVAL; if (sscanf(buf, "%s %s", con, user) != 2) - goto out2; + goto out; length = security_context_to_sid(con, strlen(con) + 1, &sid); - if (length < 0) - goto out2; + if (length) + goto out; length = security_get_user_sids(sid, user, &sids, &nsids); - if (length < 0) - goto out2; + if (length) + goto out; length = sprintf(buf, "%u", nsids) + 1; ptr = buf + length; @@ -893,82 +906,80 @@ static ssize_t sel_write_user(struct file *file, char *buf, size_t size) rc = security_sid_to_context(sids[i], &newcon, &len); if (rc) { length = rc; - goto out3; + goto out; } if ((length + len) >= SIMPLE_TRANSACTION_LIMIT) { kfree(newcon); length = -ERANGE; - goto out3; + goto out; } memcpy(ptr, newcon, len); kfree(newcon); ptr += len; length += len; } -out3: - kfree(sids); -out2: - kfree(user); out: + kfree(sids); + kfree(user); kfree(con); return length; } static ssize_t sel_write_member(struct file *file, char *buf, size_t size) { - char *scon, *tcon; + char *scon = NULL, *tcon = NULL; u32 ssid, tsid, newsid; u16 tclass; ssize_t length; - char *newcon; + char *newcon = NULL; u32 len; length = task_has_security(current, SECURITY__COMPUTE_MEMBER); if (length) - return length; + goto out; length = -ENOMEM; scon = kzalloc(size + 1, GFP_KERNEL); if (!scon) - return length; + goto out;; + length = -ENOMEM; tcon = kzalloc(size + 1, GFP_KERNEL); if (!tcon) goto out; length = -EINVAL; if (sscanf(buf, "%s %s %hu", scon, tcon, &tclass) != 3) - goto out2; + goto out; length = security_context_to_sid(scon, strlen(scon) + 1, &ssid); - if (length < 0) - goto out2; + if (length) + goto out; + length = security_context_to_sid(tcon, strlen(tcon) + 1, &tsid); - if (length < 0) - goto out2; + if (length) + goto out; length = security_member_sid(ssid, tsid, tclass, &newsid); - if (length < 0) - goto out2; + if (length) + goto out; length = security_sid_to_context(newsid, &newcon, &len); - if (length < 0) - goto out2; + if (length) + goto out; + length = -ERANGE; if (len > SIMPLE_TRANSACTION_LIMIT) { printk(KERN_ERR "SELinux: %s: context size (%u) exceeds " "payload max\n", __func__, len); - length = -ERANGE; - goto out3; + goto out; } memcpy(buf, newcon, len); length = len; -out3: - kfree(newcon); -out2: - kfree(tcon); out: + kfree(newcon); + kfree(tcon); kfree(scon); return length; } @@ -998,16 +1009,14 @@ static ssize_t sel_read_bool(struct file *filep, char __user *buf, mutex_lock(&sel_mutex); - if (index >= bool_num || strcmp(name, bool_pending_names[index])) { - ret = -EINVAL; + ret = -EINVAL; + if (index >= bool_num || strcmp(name, bool_pending_names[index])) goto out; - } + ret = -ENOMEM; page = (char *)get_zeroed_page(GFP_KERNEL); - if (!page) { - ret = -ENOMEM; + if (!page) goto out; - } cur_enforcing = security_get_bool_value(index); if (cur_enforcing < 0) { @@ -1019,8 +1028,7 @@ static ssize_t sel_read_bool(struct file *filep, char __user *buf, ret = simple_read_from_buffer(buf, count, ppos, page, length); out: mutex_unlock(&sel_mutex); - if (page) - free_page((unsigned long)page); + free_page((unsigned long)page); return ret; } @@ -1040,26 +1048,23 @@ static ssize_t sel_write_bool(struct file *filep, const char __user *buf, if (length) goto out; - if (index >= bool_num || strcmp(name, bool_pending_names[index])) { - length = -EINVAL; + length = -EINVAL; + if (index >= bool_num || strcmp(name, bool_pending_names[index])) goto out; - } - if (count >= PAGE_SIZE) { - length = -ENOMEM; + length = -ENOMEM; + if (count >= PAGE_SIZE) goto out; - } - if (*ppos != 0) { - /* No partial writes. */ - length = -EINVAL; + /* No partial writes. */ + length = -EINVAL; + if (*ppos != 0) goto out; - } + + length = -ENOMEM; page = (char *)get_zeroed_page(GFP_KERNEL); - if (!page) { - length = -ENOMEM; + if (!page) goto out; - } length = -EFAULT; if (copy_from_user(page, buf, count)) @@ -1077,8 +1082,7 @@ static ssize_t sel_write_bool(struct file *filep, const char __user *buf, out: mutex_unlock(&sel_mutex); - if (page) - free_page((unsigned long) page); + free_page((unsigned long) page); return length; } @@ -1102,19 +1106,19 @@ static ssize_t sel_commit_bools_write(struct file *filep, if (length) goto out; - if (count >= PAGE_SIZE) { - length = -ENOMEM; + length = -ENOMEM; + if (count >= PAGE_SIZE) goto out; - } - if (*ppos != 0) { - /* No partial writes. */ + + /* No partial writes. */ + length = -EINVAL; + if (*ppos != 0) goto out; - } + + length = -ENOMEM; page = (char *)get_zeroed_page(GFP_KERNEL); - if (!page) { - length = -ENOMEM; + if (!page) goto out; - } length = -EFAULT; if (copy_from_user(page, buf, count)) @@ -1124,15 +1128,16 @@ static ssize_t sel_commit_bools_write(struct file *filep, if (sscanf(page, "%d", &new_value) != 1) goto out; + length = 0; if (new_value && bool_pending_values) - security_set_bools(bool_num, bool_pending_values); + length = security_set_bools(bool_num, bool_pending_values); - length = count; + if (!length) + length = count; out: mutex_unlock(&sel_mutex); - if (page) - free_page((unsigned long) page); + free_page((unsigned long) page); return length; } @@ -1169,7 +1174,7 @@ static void sel_remove_entries(struct dentry *de) static int sel_make_bools(void) { - int i, ret = 0; + int i, ret; ssize_t len; struct dentry *dentry = NULL; struct dentry *dir = bool_dir; @@ -1190,38 +1195,40 @@ static int sel_make_bools(void) sel_remove_entries(dir); + ret = -ENOMEM; page = (char *)get_zeroed_page(GFP_KERNEL); if (!page) - return -ENOMEM; + goto out; ret = security_get_bools(&num, &names, &values); - if (ret != 0) + if (ret) goto out; for (i = 0; i < num; i++) { + ret = -ENOMEM; dentry = d_alloc_name(dir, names[i]); - if (!dentry) { - ret = -ENOMEM; - goto err; - } - inode = sel_make_inode(dir->d_sb, S_IFREG | S_IRUGO | S_IWUSR); - if (!inode) { - ret = -ENOMEM; - goto err; - } + if (!dentry) + goto out; + ret = -ENOMEM; + inode = sel_make_inode(dir->d_sb, S_IFREG | S_IRUGO | S_IWUSR); + if (!inode) + goto out; + + ret = -EINVAL; len = snprintf(page, PAGE_SIZE, "/%s/%s", BOOL_DIR_NAME, names[i]); - if (len < 0) { - ret = -EINVAL; - goto err; - } else if (len >= PAGE_SIZE) { - ret = -ENAMETOOLONG; - goto err; - } + if (len < 0) + goto out; + + ret = -ENAMETOOLONG; + if (len >= PAGE_SIZE) + goto out; + isec = (struct inode_security_struct *)inode->i_security; ret = security_genfs_sid("selinuxfs", page, SECCLASS_FILE, &sid); if (ret) - goto err; + goto out; + isec->sid = sid; isec->initialized = 1; inode->i_fop = &sel_bool_ops; @@ -1231,10 +1238,12 @@ static int sel_make_bools(void) bool_num = num; bool_pending_names = names; bool_pending_values = values; + + free_page((unsigned long)page); + return 0; out: free_page((unsigned long)page); - return ret; -err: + if (names) { for (i = 0; i < num; i++) kfree(names[i]); @@ -1242,8 +1251,8 @@ err: } kfree(values); sel_remove_entries(dir); - ret = -ENOMEM; - goto out; + + return ret; } #define NULL_FILE_NAME "null" @@ -1265,47 +1274,41 @@ static ssize_t sel_write_avc_cache_threshold(struct file *file, size_t count, loff_t *ppos) { - char *page; + char *page = NULL; ssize_t ret; int new_value; - if (count >= PAGE_SIZE) { - ret = -ENOMEM; + ret = task_has_security(current, SECURITY__SETSECPARAM); + if (ret) goto out; - } - if (*ppos != 0) { - /* No partial writes. */ - ret = -EINVAL; + ret = -ENOMEM; + if (count >= PAGE_SIZE) goto out; - } + /* No partial writes. */ + ret = -EINVAL; + if (*ppos != 0) + goto out; + + ret = -ENOMEM; page = (char *)get_zeroed_page(GFP_KERNEL); - if (!page) { - ret = -ENOMEM; + if (!page) goto out; - } - if (copy_from_user(page, buf, count)) { - ret = -EFAULT; - goto out_free; - } - - if (sscanf(page, "%u", &new_value) != 1) { - ret = -EINVAL; + ret = -EFAULT; + if (copy_from_user(page, buf, count)) goto out; - } - if (new_value != avc_cache_threshold) { - ret = task_has_security(current, SECURITY__SETSECPARAM); - if (ret) - goto out_free; - avc_cache_threshold = new_value; - } + ret = -EINVAL; + if (sscanf(page, "%u", &new_value) != 1) + goto out; + + avc_cache_threshold = new_value; + ret = count; -out_free: - free_page((unsigned long)page); out: + free_page((unsigned long)page); return ret; } @@ -1313,19 +1316,18 @@ static ssize_t sel_read_avc_hash_stats(struct file *filp, char __user *buf, size_t count, loff_t *ppos) { char *page; - ssize_t ret = 0; + ssize_t length; page = (char *)__get_free_page(GFP_KERNEL); - if (!page) { - ret = -ENOMEM; - goto out; - } - ret = avc_get_hash_stats(page); - if (ret >= 0) - ret = simple_read_from_buffer(buf, count, ppos, page, ret); + if (!page) + return -ENOMEM; + + length = avc_get_hash_stats(page); + if (length >= 0) + length = simple_read_from_buffer(buf, count, ppos, page, length); free_page((unsigned long)page); -out: - return ret; + + return length; } static const struct file_operations sel_avc_cache_threshold_ops = { @@ -1407,7 +1409,7 @@ static const struct file_operations sel_avc_cache_stats_ops = { static int sel_make_avc_files(struct dentry *dir) { - int i, ret = 0; + int i; static struct tree_descr files[] = { { "cache_threshold", &sel_avc_cache_threshold_ops, S_IRUGO|S_IWUSR }, @@ -1422,22 +1424,19 @@ static int sel_make_avc_files(struct dentry *dir) struct dentry *dentry; dentry = d_alloc_name(dir, files[i].name); - if (!dentry) { - ret = -ENOMEM; - goto out; - } + if (!dentry) + return -ENOMEM; inode = sel_make_inode(dir->d_sb, S_IFREG|files[i].mode); - if (!inode) { - ret = -ENOMEM; - goto out; - } + if (!inode) + return -ENOMEM; + inode->i_fop = files[i].ops; inode->i_ino = ++sel_last_ino; d_add(dentry, inode); } -out: - return ret; + + return 0; } static ssize_t sel_read_initcon(struct file *file, char __user *buf, @@ -1451,7 +1450,7 @@ static ssize_t sel_read_initcon(struct file *file, char __user *buf, inode = file->f_path.dentry->d_inode; sid = inode->i_ino&SEL_INO_MASK; ret = security_sid_to_context(sid, &con, &len); - if (ret < 0) + if (ret) return ret; ret = simple_read_from_buffer(buf, count, ppos, con, len); @@ -1466,28 +1465,25 @@ static const struct file_operations sel_initcon_ops = { static int sel_make_initcon_files(struct dentry *dir) { - int i, ret = 0; + int i; for (i = 1; i <= SECINITSID_NUM; i++) { struct inode *inode; struct dentry *dentry; dentry = d_alloc_name(dir, security_get_initial_sid_context(i)); - if (!dentry) { - ret = -ENOMEM; - goto out; - } + if (!dentry) + return -ENOMEM; inode = sel_make_inode(dir->d_sb, S_IFREG|S_IRUGO); - if (!inode) { - ret = -ENOMEM; - goto out; - } + if (!inode) + return -ENOMEM; + inode->i_fop = &sel_initcon_ops; inode->i_ino = i|SEL_INITCON_INO_OFFSET; d_add(dentry, inode); } -out: - return ret; + + return 0; } static inline unsigned int sel_div(unsigned long a, unsigned long b) @@ -1523,15 +1519,13 @@ static ssize_t sel_read_class(struct file *file, char __user *buf, unsigned long ino = file->f_path.dentry->d_inode->i_ino; page = (char *)__get_free_page(GFP_KERNEL); - if (!page) { - rc = -ENOMEM; - goto out; - } + if (!page) + return -ENOMEM; len = snprintf(page, PAGE_SIZE, "%d", sel_ino_to_class(ino)); rc = simple_read_from_buffer(buf, count, ppos, page, len); free_page((unsigned long)page); -out: + return rc; } @@ -1548,15 +1542,13 @@ static ssize_t sel_read_perm(struct file *file, char __user *buf, unsigned long ino = file->f_path.dentry->d_inode->i_ino; page = (char *)__get_free_page(GFP_KERNEL); - if (!page) { - rc = -ENOMEM; - goto out; - } + if (!page) + return -ENOMEM; len = snprintf(page, PAGE_SIZE, "%d", sel_ino_to_perm(ino)); rc = simple_read_from_buffer(buf, count, ppos, page, len); free_page((unsigned long)page); -out: + return rc; } @@ -1587,39 +1579,37 @@ static const struct file_operations sel_policycap_ops = { static int sel_make_perm_files(char *objclass, int classvalue, struct dentry *dir) { - int i, rc = 0, nperms; + int i, rc, nperms; char **perms; rc = security_get_permissions(objclass, &perms, &nperms); if (rc) - goto out; + return rc; for (i = 0; i < nperms; i++) { struct inode *inode; struct dentry *dentry; + rc = -ENOMEM; dentry = d_alloc_name(dir, perms[i]); - if (!dentry) { - rc = -ENOMEM; - goto out1; - } + if (!dentry) + goto out; + rc = -ENOMEM; inode = sel_make_inode(dir->d_sb, S_IFREG|S_IRUGO); - if (!inode) { - rc = -ENOMEM; - goto out1; - } + if (!inode) + goto out; + inode->i_fop = &sel_perm_ops; /* i+1 since perm values are 1-indexed */ inode->i_ino = sel_perm_to_ino(classvalue, i + 1); d_add(dentry, inode); } - -out1: + rc = 0; +out: for (i = 0; i < nperms; i++) kfree(perms[i]); kfree(perms); -out: return rc; } @@ -1631,34 +1621,27 @@ static int sel_make_class_dir_entries(char *classname, int index, int rc; dentry = d_alloc_name(dir, "index"); - if (!dentry) { - rc = -ENOMEM; - goto out; - } + if (!dentry) + return -ENOMEM; inode = sel_make_inode(dir->d_sb, S_IFREG|S_IRUGO); - if (!inode) { - rc = -ENOMEM; - goto out; - } + if (!inode) + return -ENOMEM; inode->i_fop = &sel_class_ops; inode->i_ino = sel_class_to_ino(index); d_add(dentry, inode); dentry = d_alloc_name(dir, "perms"); - if (!dentry) { - rc = -ENOMEM; - goto out; - } + if (!dentry) + return -ENOMEM; rc = sel_make_dir(dir->d_inode, dentry, &last_class_ino); if (rc) - goto out; + return rc; rc = sel_make_perm_files(classname, index, dentry); -out: return rc; } @@ -1688,15 +1671,15 @@ static void sel_remove_classes(void) static int sel_make_classes(void) { - int rc = 0, nclasses, i; + int rc, nclasses, i; char **classes; /* delete any existing entries */ sel_remove_classes(); rc = security_get_classes(&classes, &nclasses); - if (rc < 0) - goto out; + if (rc) + return rc; /* +2 since classes are 1-indexed */ last_class_ino = sel_class_to_ino(nclasses + 2); @@ -1704,29 +1687,27 @@ static int sel_make_classes(void) for (i = 0; i < nclasses; i++) { struct dentry *class_name_dir; + rc = -ENOMEM; class_name_dir = d_alloc_name(class_dir, classes[i]); - if (!class_name_dir) { - rc = -ENOMEM; - goto out1; - } + if (!class_name_dir) + goto out; rc = sel_make_dir(class_dir->d_inode, class_name_dir, &last_class_ino); if (rc) - goto out1; + goto out; /* i+1 since class values are 1-indexed */ rc = sel_make_class_dir_entries(classes[i], i + 1, class_name_dir); if (rc) - goto out1; + goto out; } - -out1: + rc = 0; +out: for (i = 0; i < nclasses; i++) kfree(classes[i]); kfree(classes); -out: return rc; } @@ -1763,14 +1744,12 @@ static int sel_make_policycap(void) static int sel_make_dir(struct inode *dir, struct dentry *dentry, unsigned long *ino) { - int ret = 0; struct inode *inode; inode = sel_make_inode(dir->i_sb, S_IFDIR | S_IRUGO | S_IXUGO); - if (!inode) { - ret = -ENOMEM; - goto out; - } + if (!inode) + return -ENOMEM; + inode->i_op = &simple_dir_inode_operations; inode->i_fop = &simple_dir_operations; inode->i_ino = ++(*ino); @@ -1779,8 +1758,8 @@ static int sel_make_dir(struct inode *dir, struct dentry *dentry, d_add(dentry, inode); /* bump link count on parent directory, too */ inc_nlink(dir); -out: - return ret; + + return 0; } static int sel_fill_super(struct super_block *sb, void *data, int silent) @@ -1816,11 +1795,10 @@ static int sel_fill_super(struct super_block *sb, void *data, int silent) root_inode = sb->s_root->d_inode; + ret = -ENOMEM; dentry = d_alloc_name(sb->s_root, BOOL_DIR_NAME); - if (!dentry) { - ret = -ENOMEM; + if (!dentry) goto err; - } ret = sel_make_dir(root_inode, dentry, &sel_last_ino); if (ret) @@ -1828,17 +1806,16 @@ static int sel_fill_super(struct super_block *sb, void *data, int silent) bool_dir = dentry; + ret = -ENOMEM; dentry = d_alloc_name(sb->s_root, NULL_FILE_NAME); - if (!dentry) { - ret = -ENOMEM; + if (!dentry) goto err; - } + ret = -ENOMEM; inode = sel_make_inode(sb, S_IFCHR | S_IRUGO | S_IWUGO); - if (!inode) { - ret = -ENOMEM; + if (!inode) goto err; - } + inode->i_ino = ++sel_last_ino; isec = (struct inode_security_struct *)inode->i_security; isec->sid = SECINITSID_DEVNULL; @@ -1849,11 +1826,10 @@ static int sel_fill_super(struct super_block *sb, void *data, int silent) d_add(dentry, inode); selinux_null = dentry; + ret = -ENOMEM; dentry = d_alloc_name(sb->s_root, "avc"); - if (!dentry) { - ret = -ENOMEM; + if (!dentry) goto err; - } ret = sel_make_dir(root_inode, dentry, &sel_last_ino); if (ret) @@ -1863,11 +1839,10 @@ static int sel_fill_super(struct super_block *sb, void *data, int silent) if (ret) goto err; + ret = -ENOMEM; dentry = d_alloc_name(sb->s_root, "initial_contexts"); - if (!dentry) { - ret = -ENOMEM; + if (!dentry) goto err; - } ret = sel_make_dir(root_inode, dentry, &sel_last_ino); if (ret) @@ -1877,11 +1852,10 @@ static int sel_fill_super(struct super_block *sb, void *data, int silent) if (ret) goto err; + ret = -ENOMEM; dentry = d_alloc_name(sb->s_root, "class"); - if (!dentry) { - ret = -ENOMEM; + if (!dentry) goto err; - } ret = sel_make_dir(root_inode, dentry, &sel_last_ino); if (ret) @@ -1889,11 +1863,10 @@ static int sel_fill_super(struct super_block *sb, void *data, int silent) class_dir = dentry; + ret = -ENOMEM; dentry = d_alloc_name(sb->s_root, "policy_capabilities"); - if (!dentry) { - ret = -ENOMEM; + if (!dentry) goto err; - } ret = sel_make_dir(root_inode, dentry, &sel_last_ino); if (ret) @@ -1901,12 +1874,11 @@ static int sel_fill_super(struct super_block *sb, void *data, int silent) policycap_dir = dentry; -out: - return ret; + return 0; err: printk(KERN_ERR "SELinux: %s: failed while creating inodes\n", __func__); - goto out; + return ret; } static struct dentry *sel_mount(struct file_system_type *fs_type, @@ -1930,14 +1902,16 @@ static int __init init_sel_fs(void) if (!selinux_enabled) return 0; err = register_filesystem(&sel_fs_type); - if (!err) { - selinuxfs_mount = kern_mount(&sel_fs_type); - if (IS_ERR(selinuxfs_mount)) { - printk(KERN_ERR "selinuxfs: could not mount!\n"); - err = PTR_ERR(selinuxfs_mount); - selinuxfs_mount = NULL; - } + if (err) + return err; + + selinuxfs_mount = kern_mount(&sel_fs_type); + if (IS_ERR(selinuxfs_mount)) { + printk(KERN_ERR "selinuxfs: could not mount!\n"); + err = PTR_ERR(selinuxfs_mount); + selinuxfs_mount = NULL; } + return err; } From 4b02b524487622ce1cf472123899520b583f47dc Mon Sep 17 00:00:00 2001 From: Eric Paris Date: Tue, 23 Nov 2010 11:40:08 -0500 Subject: [PATCH 03/12] SELinux: standardize return code handling in selinuxfs.c selinuxfs.c has lots of different standards on how to handle return paths on error. For the most part transition to rc=errno if (failure) goto out; [...] out: cleanup() return rc; Instead of doing cleanup mid function, or having multiple returns or other options. This doesn't do that for every function, but most of the complex functions which have cleanup routines on error. Signed-off-by: Eric Paris --- security/selinux/ss/services.c | 326 ++++++++++++++++----------------- 1 file changed, 156 insertions(+), 170 deletions(-) diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index 223c1ff6ef23..84e2a98d7cc5 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -701,11 +701,11 @@ static int security_validtrans_handle_fail(struct context *ocontext, char *o = NULL, *n = NULL, *t = NULL; u32 olen, nlen, tlen; - if (context_struct_to_string(ocontext, &o, &olen) < 0) + if (context_struct_to_string(ocontext, &o, &olen)) goto out; - if (context_struct_to_string(ncontext, &n, &nlen) < 0) + if (context_struct_to_string(ncontext, &n, &nlen)) goto out; - if (context_struct_to_string(tcontext, &t, &tlen) < 0) + if (context_struct_to_string(tcontext, &t, &tlen)) goto out; audit_log(current->audit_context, GFP_ATOMIC, AUDIT_SELINUX_ERR, "security_validate_transition: denied for" @@ -801,10 +801,11 @@ int security_bounded_transition(u32 old_sid, u32 new_sid) struct context *old_context, *new_context; struct type_datum *type; int index; - int rc = -EINVAL; + int rc; read_lock(&policy_rwlock); + rc = -EINVAL; old_context = sidtab_search(&sidtab, old_sid); if (!old_context) { printk(KERN_ERR "SELinux: %s: unrecognized SID %u\n", @@ -812,6 +813,7 @@ int security_bounded_transition(u32 old_sid, u32 new_sid) goto out; } + rc = -EINVAL; new_context = sidtab_search(&sidtab, new_sid); if (!new_context) { printk(KERN_ERR "SELinux: %s: unrecognized SID %u\n", @@ -819,11 +821,10 @@ int security_bounded_transition(u32 old_sid, u32 new_sid) goto out; } + rc = 0; /* type/domain unchanged */ - if (old_context->type == new_context->type) { - rc = 0; + if (old_context->type == new_context->type) goto out; - } index = new_context->type; while (true) { @@ -831,16 +832,15 @@ int security_bounded_transition(u32 old_sid, u32 new_sid) BUG_ON(!type); /* not bounded anymore */ - if (!type->bounds) { - rc = -EPERM; + rc = -EPERM; + if (!type->bounds) break; - } /* @newsid is bounded by @oldsid */ - if (type->bounds == old_context->type) { - rc = 0; + rc = 0; + if (type->bounds == old_context->type) break; - } + index = type->bounds; } @@ -1187,16 +1187,13 @@ static int string_to_context_struct(struct policydb *pol, if (rc) goto out; - if ((p - scontext) < scontext_len) { - rc = -EINVAL; + rc = -EINVAL; + if ((p - scontext) < scontext_len) goto out; - } /* Check the validity of the new context. */ - if (!policydb_context_isvalid(pol, ctx)) { - rc = -EINVAL; + if (!policydb_context_isvalid(pol, ctx)) goto out; - } rc = 0; out: if (rc) @@ -1235,27 +1232,26 @@ static int security_context_to_sid_core(const char *scontext, u32 scontext_len, if (force) { /* Save another copy for storing in uninterpreted form */ + rc = -ENOMEM; str = kstrdup(scontext2, gfp_flags); - if (!str) { - kfree(scontext2); - return -ENOMEM; - } + if (!str) + goto out; } read_lock(&policy_rwlock); - rc = string_to_context_struct(&policydb, &sidtab, - scontext2, scontext_len, - &context, def_sid); + rc = string_to_context_struct(&policydb, &sidtab, scontext2, + scontext_len, &context, def_sid); if (rc == -EINVAL && force) { context.str = str; context.len = scontext_len; str = NULL; } else if (rc) - goto out; + goto out_unlock; rc = sidtab_context_to_sid(&sidtab, &context, sid); context_destroy(&context); -out: +out_unlock: read_unlock(&policy_rwlock); +out: kfree(scontext2); kfree(str); return rc; @@ -1319,11 +1315,11 @@ static int compute_sid_handle_invalid_context( char *s = NULL, *t = NULL, *n = NULL; u32 slen, tlen, nlen; - if (context_struct_to_string(scontext, &s, &slen) < 0) + if (context_struct_to_string(scontext, &s, &slen)) goto out; - if (context_struct_to_string(tcontext, &t, &tlen) < 0) + if (context_struct_to_string(tcontext, &t, &tlen)) goto out; - if (context_struct_to_string(newcontext, &n, &nlen) < 0) + if (context_struct_to_string(newcontext, &n, &nlen)) goto out; audit_log(current->audit_context, GFP_ATOMIC, AUDIT_SELINUX_ERR, "security_compute_sid: invalid context %s" @@ -1569,22 +1565,17 @@ static int clone_sid(u32 sid, static inline int convert_context_handle_invalid_context(struct context *context) { - int rc = 0; + char *s; + u32 len; - if (selinux_enforcing) { - rc = -EINVAL; - } else { - char *s; - u32 len; + if (selinux_enforcing) + return -EINVAL; - if (!context_struct_to_string(context, &s, &len)) { - printk(KERN_WARNING - "SELinux: Context %s would be invalid if enforcing\n", - s); - kfree(s); - } + if (!context_struct_to_string(context, &s, &len)) { + printk(KERN_WARNING "SELinux: Context %s would be invalid if enforcing\n", s); + kfree(s); } - return rc; + return 0; } struct convert_context_args { @@ -1621,17 +1612,17 @@ static int convert_context(u32 key, if (c->str) { struct context ctx; + + rc = -ENOMEM; s = kstrdup(c->str, GFP_KERNEL); - if (!s) { - rc = -ENOMEM; + if (!s) goto out; - } + rc = string_to_context_struct(args->newp, NULL, s, c->len, &ctx, SECSID_NULL); kfree(s); if (!rc) { - printk(KERN_INFO - "SELinux: Context %s became valid (mapped).\n", + printk(KERN_INFO "SELinux: Context %s became valid (mapped).\n", c->str); /* Replace string with mapped representation. */ kfree(c->str); @@ -1643,8 +1634,7 @@ static int convert_context(u32 key, goto out; } else { /* Other error condition, e.g. ENOMEM. */ - printk(KERN_ERR - "SELinux: Unable to map context %s, rc = %d.\n", + printk(KERN_ERR "SELinux: Unable to map context %s, rc = %d.\n", c->str, -rc); goto out; } @@ -1654,9 +1644,8 @@ static int convert_context(u32 key, if (rc) goto out; - rc = -EINVAL; - /* Convert the user. */ + rc = -EINVAL; usrdatum = hashtab_search(args->newp->p_users.table, args->oldp->p_user_val_to_name[c->user - 1]); if (!usrdatum) @@ -1664,6 +1653,7 @@ static int convert_context(u32 key, c->user = usrdatum->value; /* Convert the role. */ + rc = -EINVAL; role = hashtab_search(args->newp->p_roles.table, args->oldp->p_role_val_to_name[c->role - 1]); if (!role) @@ -1671,6 +1661,7 @@ static int convert_context(u32 key, c->role = role->value; /* Convert the type. */ + rc = -EINVAL; typdatum = hashtab_search(args->newp->p_types.table, args->oldp->p_type_val_to_name[c->type - 1]); if (!typdatum) @@ -1700,6 +1691,7 @@ static int convert_context(u32 key, oc = args->newp->ocontexts[OCON_ISID]; while (oc && oc->sid[0] != SECINITSID_UNLABELED) oc = oc->next; + rc = -EINVAL; if (!oc) { printk(KERN_ERR "SELinux: unable to look up" " the initial SIDs list\n"); @@ -1719,19 +1711,20 @@ static int convert_context(u32 key, } context_destroy(&oldc); + rc = 0; out: return rc; bad: /* Map old representation to string and save it. */ - if (context_struct_to_string(&oldc, &s, &len)) - return -ENOMEM; + rc = context_struct_to_string(&oldc, &s, &len); + if (rc) + return rc; context_destroy(&oldc); context_destroy(c); c->str = s; c->len = len; - printk(KERN_INFO - "SELinux: Context %s became invalid (unmapped).\n", + printk(KERN_INFO "SELinux: Context %s became invalid (unmapped).\n", c->str); rc = 0; goto out; @@ -2012,7 +2005,7 @@ int security_node_sid(u16 domain, u32 addrlen, u32 *out_sid) { - int rc = 0; + int rc; struct ocontext *c; read_lock(&policy_rwlock); @@ -2021,10 +2014,9 @@ int security_node_sid(u16 domain, case AF_INET: { u32 addr; - if (addrlen != sizeof(u32)) { - rc = -EINVAL; + rc = -EINVAL; + if (addrlen != sizeof(u32)) goto out; - } addr = *((u32 *)addrp); @@ -2038,10 +2030,9 @@ int security_node_sid(u16 domain, } case AF_INET6: - if (addrlen != sizeof(u64) * 2) { - rc = -EINVAL; + rc = -EINVAL; + if (addrlen != sizeof(u64) * 2) goto out; - } c = policydb.ocontexts[OCON_NODE6]; while (c) { if (match_ipv6_addrmask(addrp, c->u.node6.addr, @@ -2052,6 +2043,7 @@ int security_node_sid(u16 domain, break; default: + rc = 0; *out_sid = SECINITSID_NODE; goto out; } @@ -2069,6 +2061,7 @@ int security_node_sid(u16 domain, *out_sid = SECINITSID_NODE; } + rc = 0; out: read_unlock(&policy_rwlock); return rc; @@ -2113,24 +2106,22 @@ int security_get_user_sids(u32 fromsid, context_init(&usercon); + rc = -EINVAL; fromcon = sidtab_search(&sidtab, fromsid); - if (!fromcon) { - rc = -EINVAL; + if (!fromcon) goto out_unlock; - } + rc = -EINVAL; user = hashtab_search(policydb.p_users.table, username); - if (!user) { - rc = -EINVAL; + if (!user) goto out_unlock; - } + usercon.user = user->value; + rc = -ENOMEM; mysids = kcalloc(maxnel, sizeof(*mysids), GFP_ATOMIC); - if (!mysids) { - rc = -ENOMEM; + if (!mysids) goto out_unlock; - } ebitmap_for_each_positive_bit(&user->roles, rnode, i) { role = policydb.role_val_to_struct[i]; @@ -2147,12 +2138,11 @@ int security_get_user_sids(u32 fromsid, if (mynel < maxnel) { mysids[mynel++] = sid; } else { + rc = -ENOMEM; maxnel += SIDS_NEL; mysids2 = kcalloc(maxnel, sizeof(*mysids2), GFP_ATOMIC); - if (!mysids2) { - rc = -ENOMEM; + if (!mysids2) goto out_unlock; - } memcpy(mysids2, mysids, mynel * sizeof(*mysids2)); kfree(mysids); mysids = mysids2; @@ -2160,7 +2150,7 @@ int security_get_user_sids(u32 fromsid, } } } - + rc = 0; out_unlock: read_unlock(&policy_rwlock); if (rc || !mynel) { @@ -2168,9 +2158,9 @@ out_unlock: goto out; } + rc = -ENOMEM; mysids2 = kcalloc(mynel, sizeof(*mysids2), GFP_KERNEL); if (!mysids2) { - rc = -ENOMEM; kfree(mysids); goto out; } @@ -2211,7 +2201,7 @@ int security_genfs_sid(const char *fstype, u16 sclass; struct genfs *genfs; struct ocontext *c; - int rc = 0, cmp = 0; + int rc, cmp = 0; while (path[0] == '/' && path[1] == '/') path++; @@ -2219,6 +2209,7 @@ int security_genfs_sid(const char *fstype, read_lock(&policy_rwlock); sclass = unmap_class(orig_sclass); + *sid = SECINITSID_UNLABELED; for (genfs = policydb.genfs; genfs; genfs = genfs->next) { cmp = strcmp(fstype, genfs->fstype); @@ -2226,11 +2217,9 @@ int security_genfs_sid(const char *fstype, break; } - if (!genfs || cmp) { - *sid = SECINITSID_UNLABELED; - rc = -ENOENT; + rc = -ENOENT; + if (!genfs || cmp) goto out; - } for (c = genfs->head; c; c = c->next) { len = strlen(c->u.name); @@ -2239,21 +2228,18 @@ int security_genfs_sid(const char *fstype, break; } - if (!c) { - *sid = SECINITSID_UNLABELED; - rc = -ENOENT; + rc = -ENOENT; + if (!c) goto out; - } if (!c->sid[0]) { - rc = sidtab_context_to_sid(&sidtab, - &c->context[0], - &c->sid[0]); + rc = sidtab_context_to_sid(&sidtab, &c->context[0], &c->sid[0]); if (rc) goto out; } *sid = c->sid[0]; + rc = 0; out: read_unlock(&policy_rwlock); return rc; @@ -2285,8 +2271,7 @@ int security_fs_use( if (c) { *behavior = c->v.behavior; if (!c->sid[0]) { - rc = sidtab_context_to_sid(&sidtab, - &c->context[0], + rc = sidtab_context_to_sid(&sidtab, &c->context[0], &c->sid[0]); if (rc) goto out; @@ -2309,33 +2294,38 @@ out: int security_get_bools(int *len, char ***names, int **values) { - int i, rc = -ENOMEM; + int i, rc; read_lock(&policy_rwlock); *names = NULL; *values = NULL; + rc = 0; *len = policydb.p_bools.nprim; - if (!*len) { - rc = 0; + if (!*len) goto out; - } - *names = kcalloc(*len, sizeof(char *), GFP_ATOMIC); + rc = -ENOMEM; + *names = kcalloc(*len, sizeof(char *), GFP_ATOMIC); if (!*names) goto err; - *values = kcalloc(*len, sizeof(int), GFP_ATOMIC); + rc = -ENOMEM; + *values = kcalloc(*len, sizeof(int), GFP_ATOMIC); if (!*values) goto err; for (i = 0; i < *len; i++) { size_t name_len; + (*values)[i] = policydb.bool_val_to_struct[i]->state; name_len = strlen(policydb.p_bool_val_to_name[i]) + 1; - (*names)[i] = kmalloc(sizeof(char) * name_len, GFP_ATOMIC); + + rc = -ENOMEM; + (*names)[i] = kmalloc(sizeof(char) * name_len, GFP_ATOMIC); if (!(*names)[i]) goto err; + strncpy((*names)[i], policydb.p_bool_val_to_name[i], name_len); (*names)[i][name_len - 1] = 0; } @@ -2355,17 +2345,16 @@ err: int security_set_bools(int len, int *values) { - int i, rc = 0; + int i, rc; int lenp, seqno = 0; struct cond_node *cur; write_lock_irq(&policy_rwlock); + rc = -EFAULT; lenp = policydb.p_bools.nprim; - if (len != lenp) { - rc = -EFAULT; + if (len != lenp) goto out; - } for (i = 0; i < len; i++) { if (!!values[i] != policydb.bool_val_to_struct[i]->state) { @@ -2391,7 +2380,7 @@ int security_set_bools(int len, int *values) } seqno = ++latest_granting; - + rc = 0; out: write_unlock_irq(&policy_rwlock); if (!rc) { @@ -2405,16 +2394,15 @@ out: int security_get_bool_value(int bool) { - int rc = 0; + int rc; int len; read_lock(&policy_rwlock); + rc = -EFAULT; len = policydb.p_bools.nprim; - if (bool >= len) { - rc = -EFAULT; + if (bool >= len) goto out; - } rc = policydb.bool_val_to_struct[bool]->state; out: @@ -2464,8 +2452,9 @@ int security_sid_mls_copy(u32 sid, u32 mls_sid, u32 *new_sid) struct context newcon; char *s; u32 len; - int rc = 0; + int rc; + rc = 0; if (!ss_initialized || !policydb.mls_enabled) { *new_sid = sid; goto out; @@ -2474,19 +2463,20 @@ int security_sid_mls_copy(u32 sid, u32 mls_sid, u32 *new_sid) context_init(&newcon); read_lock(&policy_rwlock); + + rc = -EINVAL; context1 = sidtab_search(&sidtab, sid); if (!context1) { printk(KERN_ERR "SELinux: %s: unrecognized SID %d\n", __func__, sid); - rc = -EINVAL; goto out_unlock; } + rc = -EINVAL; context2 = sidtab_search(&sidtab, mls_sid); if (!context2) { printk(KERN_ERR "SELinux: %s: unrecognized SID %d\n", __func__, mls_sid); - rc = -EINVAL; goto out_unlock; } @@ -2500,20 +2490,17 @@ int security_sid_mls_copy(u32 sid, u32 mls_sid, u32 *new_sid) /* Check the validity of the new context. */ if (!policydb_context_isvalid(&policydb, &newcon)) { rc = convert_context_handle_invalid_context(&newcon); - if (rc) - goto bad; + if (rc) { + if (!context_struct_to_string(&newcon, &s, &len)) { + audit_log(current->audit_context, GFP_ATOMIC, AUDIT_SELINUX_ERR, + "security_sid_mls_copy: invalid context %s", s); + kfree(s); + } + goto out_unlock; + } } rc = sidtab_context_to_sid(&sidtab, &newcon, new_sid); - goto out_unlock; - -bad: - if (!context_struct_to_string(&newcon, &s, &len)) { - audit_log(current->audit_context, GFP_ATOMIC, AUDIT_SELINUX_ERR, - "security_sid_mls_copy: invalid context %s", s); - kfree(s); - } - out_unlock: read_unlock(&policy_rwlock); context_destroy(&newcon); @@ -2549,6 +2536,8 @@ int security_net_peersid_resolve(u32 nlbl_sid, u32 nlbl_type, struct context *nlbl_ctx; struct context *xfrm_ctx; + *peer_sid = SECSID_NULL; + /* handle the common (which also happens to be the set of easy) cases * right away, these two if statements catch everything involving a * single or absent peer SID/label */ @@ -2567,40 +2556,37 @@ int security_net_peersid_resolve(u32 nlbl_sid, u32 nlbl_type, /* we don't need to check ss_initialized here since the only way both * nlbl_sid and xfrm_sid are not equal to SECSID_NULL would be if the * security server was initialized and ss_initialized was true */ - if (!policydb.mls_enabled) { - *peer_sid = SECSID_NULL; + if (!policydb.mls_enabled) return 0; - } read_lock(&policy_rwlock); + rc = -EINVAL; nlbl_ctx = sidtab_search(&sidtab, nlbl_sid); if (!nlbl_ctx) { printk(KERN_ERR "SELinux: %s: unrecognized SID %d\n", __func__, nlbl_sid); - rc = -EINVAL; - goto out_slowpath; + goto out; } + rc = -EINVAL; xfrm_ctx = sidtab_search(&sidtab, xfrm_sid); if (!xfrm_ctx) { printk(KERN_ERR "SELinux: %s: unrecognized SID %d\n", __func__, xfrm_sid); - rc = -EINVAL; - goto out_slowpath; + goto out; } rc = (mls_context_cmp(nlbl_ctx, xfrm_ctx) ? 0 : -EACCES); + if (rc) + goto out; -out_slowpath: + /* at present NetLabel SIDs/labels really only carry MLS + * information so if the MLS portion of the NetLabel SID + * matches the MLS portion of the labeled XFRM SID/label + * then pass along the XFRM SID as it is the most + * expressive */ + *peer_sid = xfrm_sid; +out: read_unlock(&policy_rwlock); - if (rc == 0) - /* at present NetLabel SIDs/labels really only carry MLS - * information so if the MLS portion of the NetLabel SID - * matches the MLS portion of the labeled XFRM SID/label - * then pass along the XFRM SID as it is the most - * expressive */ - *peer_sid = xfrm_sid; - else - *peer_sid = SECSID_NULL; return rc; } @@ -2619,10 +2605,11 @@ static int get_classes_callback(void *k, void *d, void *args) int security_get_classes(char ***classes, int *nclasses) { - int rc = -ENOMEM; + int rc; read_lock(&policy_rwlock); + rc = -ENOMEM; *nclasses = policydb.p_classes.nprim; *classes = kcalloc(*nclasses, sizeof(**classes), GFP_ATOMIC); if (!*classes) @@ -2630,7 +2617,7 @@ int security_get_classes(char ***classes, int *nclasses) rc = hashtab_map(policydb.p_classes.table, get_classes_callback, *classes); - if (rc < 0) { + if (rc) { int i; for (i = 0; i < *nclasses; i++) kfree((*classes)[i]); @@ -2657,19 +2644,20 @@ static int get_permissions_callback(void *k, void *d, void *args) int security_get_permissions(char *class, char ***perms, int *nperms) { - int rc = -ENOMEM, i; + int rc, i; struct class_datum *match; read_lock(&policy_rwlock); + rc = -EINVAL; match = hashtab_search(policydb.p_classes.table, class); if (!match) { printk(KERN_ERR "SELinux: %s: unrecognized class %s\n", __func__, class); - rc = -EINVAL; goto out; } + rc = -ENOMEM; *nperms = match->permissions.nprim; *perms = kcalloc(*nperms, sizeof(**perms), GFP_ATOMIC); if (!*perms) @@ -2678,13 +2666,13 @@ int security_get_permissions(char *class, char ***perms, int *nperms) if (match->comdatum) { rc = hashtab_map(match->comdatum->permissions.table, get_permissions_callback, *perms); - if (rc < 0) + if (rc) goto err; } rc = hashtab_map(match->permissions.table, get_permissions_callback, *perms); - if (rc < 0) + if (rc) goto err; out: @@ -2796,36 +2784,39 @@ int selinux_audit_rule_init(u32 field, u32 op, char *rulestr, void **vrule) switch (field) { case AUDIT_SUBJ_USER: case AUDIT_OBJ_USER: + rc = -EINVAL; userdatum = hashtab_search(policydb.p_users.table, rulestr); if (!userdatum) - rc = -EINVAL; - else - tmprule->au_ctxt.user = userdatum->value; + goto out; + tmprule->au_ctxt.user = userdatum->value; break; case AUDIT_SUBJ_ROLE: case AUDIT_OBJ_ROLE: + rc = -EINVAL; roledatum = hashtab_search(policydb.p_roles.table, rulestr); if (!roledatum) - rc = -EINVAL; - else - tmprule->au_ctxt.role = roledatum->value; + goto out; + tmprule->au_ctxt.role = roledatum->value; break; case AUDIT_SUBJ_TYPE: case AUDIT_OBJ_TYPE: + rc = -EINVAL; typedatum = hashtab_search(policydb.p_types.table, rulestr); if (!typedatum) - rc = -EINVAL; - else - tmprule->au_ctxt.type = typedatum->value; + goto out; + tmprule->au_ctxt.type = typedatum->value; break; case AUDIT_SUBJ_SEN: case AUDIT_SUBJ_CLR: case AUDIT_OBJ_LEV_LOW: case AUDIT_OBJ_LEV_HIGH: rc = mls_from_string(rulestr, &tmprule->au_ctxt, GFP_ATOMIC); + if (rc) + goto out; break; } - + rc = 0; +out: read_unlock(&policy_rwlock); if (rc) { @@ -3127,28 +3118,23 @@ int security_netlbl_sid_to_secattr(u32 sid, struct netlbl_lsm_secattr *secattr) return 0; read_lock(&policy_rwlock); + + rc = -ENOENT; ctx = sidtab_search(&sidtab, sid); - if (ctx == NULL) { - rc = -ENOENT; - goto netlbl_sid_to_secattr_failure; - } + if (ctx == NULL) + goto out; + + rc = -ENOMEM; secattr->domain = kstrdup(policydb.p_type_val_to_name[ctx->type - 1], GFP_ATOMIC); - if (secattr->domain == NULL) { - rc = -ENOMEM; - goto netlbl_sid_to_secattr_failure; - } + if (secattr->domain == NULL) + goto out; + secattr->attr.secid = sid; secattr->flags |= NETLBL_SECATTR_DOMAIN_CPY | NETLBL_SECATTR_SECID; mls_export_netlbl_lvl(ctx, secattr); rc = mls_export_netlbl_cat(ctx, secattr); - if (rc != 0) - goto netlbl_sid_to_secattr_failure; - read_unlock(&policy_rwlock); - - return 0; - -netlbl_sid_to_secattr_failure: +out: read_unlock(&policy_rwlock); return rc; } From 7ae9f23cbd3ef9daff7f768da4bfd4c56b19300d Mon Sep 17 00:00:00 2001 From: Eric Paris Date: Tue, 23 Nov 2010 11:40:09 -0500 Subject: [PATCH 04/12] selinux: rework security_netlbl_secattr_to_sid security_netlbl_secattr_to_sid is difficult to follow, especially the return codes. Try to make the function obvious. Signed-off-by: Eric Paris --- security/selinux/ss/services.c | 42 +++++++++++++++++----------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index 84e2a98d7cc5..ab6dbce5fd2a 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -3041,7 +3041,7 @@ static void security_netlbl_cache_add(struct netlbl_lsm_secattr *secattr, int security_netlbl_secattr_to_sid(struct netlbl_lsm_secattr *secattr, u32 *sid) { - int rc = -EIDRM; + int rc; struct context *ctx; struct context ctx_new; @@ -3052,16 +3052,15 @@ int security_netlbl_secattr_to_sid(struct netlbl_lsm_secattr *secattr, read_lock(&policy_rwlock); - if (secattr->flags & NETLBL_SECATTR_CACHE) { + if (secattr->flags & NETLBL_SECATTR_CACHE) *sid = *(u32 *)secattr->cache->data; - rc = 0; - } else if (secattr->flags & NETLBL_SECATTR_SECID) { + else if (secattr->flags & NETLBL_SECATTR_SECID) *sid = secattr->attr.secid; - rc = 0; - } else if (secattr->flags & NETLBL_SECATTR_MLS_LVL) { + else if (secattr->flags & NETLBL_SECATTR_MLS_LVL) { + rc = -EIDRM; ctx = sidtab_search(&sidtab, SECINITSID_NETMSG); if (ctx == NULL) - goto netlbl_secattr_to_sid_return; + goto out; context_init(&ctx_new); ctx_new.user = ctx->user; @@ -3069,34 +3068,35 @@ int security_netlbl_secattr_to_sid(struct netlbl_lsm_secattr *secattr, ctx_new.type = ctx->type; mls_import_netlbl_lvl(&ctx_new, secattr); if (secattr->flags & NETLBL_SECATTR_MLS_CAT) { - if (ebitmap_netlbl_import(&ctx_new.range.level[0].cat, - secattr->attr.mls.cat) != 0) - goto netlbl_secattr_to_sid_return; + rc = ebitmap_netlbl_import(&ctx_new.range.level[0].cat, + secattr->attr.mls.cat); + if (rc) + goto out; memcpy(&ctx_new.range.level[1].cat, &ctx_new.range.level[0].cat, sizeof(ctx_new.range.level[0].cat)); } - if (mls_context_isvalid(&policydb, &ctx_new) != 1) - goto netlbl_secattr_to_sid_return_cleanup; + rc = -EIDRM; + if (!mls_context_isvalid(&policydb, &ctx_new)) + goto out_free; rc = sidtab_context_to_sid(&sidtab, &ctx_new, sid); - if (rc != 0) - goto netlbl_secattr_to_sid_return_cleanup; + if (rc) + goto out_free; security_netlbl_cache_add(secattr, *sid); ebitmap_destroy(&ctx_new.range.level[0].cat); - } else { + } else *sid = SECSID_NULL; - rc = 0; - } -netlbl_secattr_to_sid_return: + read_unlock(&policy_rwlock); + return 0; +out_free: + ebitmap_destroy(&ctx_new.range.level[0].cat); +out: read_unlock(&policy_rwlock); return rc; -netlbl_secattr_to_sid_return_cleanup: - ebitmap_destroy(&ctx_new.range.level[0].cat); - goto netlbl_secattr_to_sid_return; } /** From c9e86a9b95f198d7df49b25fcd808ee39cba218f Mon Sep 17 00:00:00 2001 From: Eric Paris Date: Mon, 29 Nov 2010 15:46:39 -0500 Subject: [PATCH 05/12] SELinux: do not set automatic i_ino in selinuxfs selinuxfs carefully uses i_ino to figure out what the inode refers to. The VFS used to generically set this value and we would reset it to something useable. After 85fe4025c616 each filesystem sets this value to a default if needed. Since selinuxfs doesn't use the default value and it can only lead to problems (I'd rather have 2 inodes with i_ino == 0 than one pointing to the wrong data) lets just stop setting a default. Signed-off-by: Eric Paris Acked-by: James Morris --- security/selinux/selinuxfs.c | 1 - 1 file changed, 1 deletion(-) diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c index 8bae68e21af9..45d35e629fc6 100644 --- a/security/selinux/selinuxfs.c +++ b/security/selinux/selinuxfs.c @@ -989,7 +989,6 @@ static struct inode *sel_make_inode(struct super_block *sb, int mode) struct inode *ret = new_inode(sb); if (ret) { - ret->i_ino = get_next_ino(); ret->i_mode = mode; ret->i_atime = ret->i_mtime = ret->i_ctime = CURRENT_TIME; } From c41ab6a1b9028de33e74101cb0aae13098a56fdb Mon Sep 17 00:00:00 2001 From: Eric Paris Date: Mon, 29 Nov 2010 15:47:09 -0500 Subject: [PATCH 06/12] flex_array: fix flex_array_put_ptr macro to be valid C MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Using flex_array_put_ptr() results in a compile error "error: lvalue required as unary ‘&’ operand" fix the casting order to fix this. Signed-off-by: Eric Paris --- include/linux/flex_array.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/linux/flex_array.h b/include/linux/flex_array.h index 631b77f2ac70..70e4efabe0fb 100644 --- a/include/linux/flex_array.h +++ b/include/linux/flex_array.h @@ -71,7 +71,7 @@ void *flex_array_get(struct flex_array *fa, unsigned int element_nr); int flex_array_shrink(struct flex_array *fa); #define flex_array_put_ptr(fa, nr, src, gfp) \ - flex_array_put(fa, nr, &(void *)(src), gfp) + flex_array_put(fa, nr, (void *)&(src), gfp) void *flex_array_get_ptr(struct flex_array *fa, unsigned int element_nr); From 23bdecb000c806cf4ec52764499a600f7200d7a9 Mon Sep 17 00:00:00 2001 From: Eric Paris Date: Mon, 29 Nov 2010 15:47:09 -0500 Subject: [PATCH 07/12] selinux: convert type_val_to_struct to flex_array In rawhide type_val_to_struct will allocate 26848 bytes, an order 3 allocations. While this hasn't been seen to fail it isn't outside the realm of possibiliy on systems with severe memory fragmentation. Convert to flex_array so no allocation will ever be bigger than PAGE_SIZE. Signed-off-by: Eric Paris --- security/selinux/ss/policydb.c | 28 +++++++++++++++++++++------- security/selinux/ss/policydb.h | 2 +- security/selinux/ss/services.c | 17 ++++++++++++----- 3 files changed, 34 insertions(+), 13 deletions(-) diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c index 6ad73e81da5c..af41fdfe1a71 100644 --- a/security/selinux/ss/policydb.c +++ b/security/selinux/ss/policydb.c @@ -307,7 +307,11 @@ static int type_index(void *key, void *datum, void *datap) || typdatum->bounds > p->p_types.nprim) return -EINVAL; p->p_type_val_to_name[typdatum->value - 1] = key; - p->type_val_to_struct[typdatum->value - 1] = typdatum; + /* this flex array was all preallocated, this cannot fail */ + if (flex_array_put_ptr(p->type_val_to_struct_array, + typdatum->value - 1, typdatum, + GFP_KERNEL | __GFP_ZERO)) + BUG(); } return 0; @@ -484,11 +488,17 @@ static int policydb_index_others(struct policydb *p) if (!p->user_val_to_struct) goto out; + /* Yes, I want the sizeof the pointer, not the structure */ rc = -ENOMEM; - p->type_val_to_struct = - kmalloc(p->p_types.nprim * sizeof(*(p->type_val_to_struct)), - GFP_KERNEL); - if (!p->type_val_to_struct) + p->type_val_to_struct_array = flex_array_alloc(sizeof(struct type_datum *), + p->p_types.nprim, + GFP_KERNEL | __GFP_ZERO); + if (!p->type_val_to_struct_array) + goto out; + + rc = flex_array_prealloc(p->type_val_to_struct_array, 0, + p->p_types.nprim - 1, GFP_KERNEL | __GFP_ZERO); + if (rc) goto out; rc = -ENOMEM; @@ -699,7 +709,8 @@ void policydb_destroy(struct policydb *p) kfree(p->class_val_to_struct); kfree(p->role_val_to_struct); kfree(p->user_val_to_struct); - kfree(p->type_val_to_struct); + if (p->type_val_to_struct_array) + flex_array_free(p->type_val_to_struct_array); avtab_destroy(&p->te_avtab); @@ -1618,7 +1629,10 @@ static int type_bounds_sanity_check(void *key, void *datum, void *datap) return -EINVAL; } - upper = p->type_val_to_struct[upper->bounds - 1]; + upper = flex_array_get_ptr(p->type_val_to_struct_array, + upper->bounds - 1); + BUG_ON(!upper); + if (upper->attribute) { printk(KERN_ERR "SELinux: type %s: " "bounded by attribute %s", diff --git a/security/selinux/ss/policydb.h b/security/selinux/ss/policydb.h index 95d3d7de361e..9826a92a6b0c 100644 --- a/security/selinux/ss/policydb.h +++ b/security/selinux/ss/policydb.h @@ -217,7 +217,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; + struct flex_array *type_val_to_struct_array; /* 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 ab6dbce5fd2a..afcbc19817f7 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -530,12 +530,18 @@ static void type_attribute_bounds_av(struct context *scontext, struct context lo_scontext; struct context lo_tcontext; struct av_decision lo_avd; - struct type_datum *source - = policydb.type_val_to_struct[scontext->type - 1]; - struct type_datum *target - = policydb.type_val_to_struct[tcontext->type - 1]; + struct type_datum *source; + struct type_datum *target; u32 masked = 0; + source = flex_array_get_ptr(policydb.type_val_to_struct_array, + scontext->type - 1); + BUG_ON(!source); + + target = flex_array_get_ptr(policydb.type_val_to_struct_array, + tcontext->type - 1); + BUG_ON(!target); + if (source->bounds) { memset(&lo_avd, 0, sizeof(lo_avd)); @@ -828,7 +834,8 @@ int security_bounded_transition(u32 old_sid, u32 new_sid) index = new_context->type; while (true) { - type = policydb.type_val_to_struct[index - 1]; + type = flex_array_get_ptr(policydb.type_val_to_struct_array, + index - 1); BUG_ON(!type); /* not bounded anymore */ From ac76c05becb6beedbb458d0827d3deaa6f479a72 Mon Sep 17 00:00:00 2001 From: Eric Paris Date: Mon, 29 Nov 2010 15:47:09 -0500 Subject: [PATCH 08/12] selinux: convert part of the sym_val_to_name array to use flex_array The sym_val_to_name type array can be quite large as it grows linearly with the number of types. With known policies having over 5k types these allocations are growing large enough that they are likely to fail. Convert those to flex_array so no allocation is larger than PAGE_SIZE Signed-off-by: Eric Paris --- security/selinux/ss/conditional.c | 6 +- security/selinux/ss/mls.c | 25 +++---- security/selinux/ss/policydb.c | 109 ++++++++++++++++++++++-------- security/selinux/ss/policydb.h | 17 +++-- security/selinux/ss/services.c | 38 +++++------ 5 files changed, 127 insertions(+), 68 deletions(-) diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c index 655fe1c6cc69..c3f845cbcd48 100644 --- a/security/selinux/ss/conditional.c +++ b/security/selinux/ss/conditional.c @@ -193,6 +193,7 @@ int cond_index_bool(void *key, void *datum, void *datap) { struct policydb *p; struct cond_bool_datum *booldatum; + struct flex_array *fa; booldatum = datum; p = datap; @@ -200,7 +201,10 @@ int cond_index_bool(void *key, void *datum, void *datap) if (!booldatum->value || booldatum->value > p->p_bools.nprim) return -EINVAL; - p->p_bool_val_to_name[booldatum->value - 1] = key; + fa = p->sym_val_to_name[SYM_BOOLS]; + if (flex_array_put_ptr(fa, booldatum->value - 1, key, + GFP_KERNEL | __GFP_ZERO)) + BUG(); p->bool_val_to_struct[booldatum->value - 1] = booldatum; return 0; diff --git a/security/selinux/ss/mls.c b/security/selinux/ss/mls.c index b4eff7a60c50..1ef8e4e89880 100644 --- a/security/selinux/ss/mls.c +++ b/security/selinux/ss/mls.c @@ -45,7 +45,7 @@ int mls_compute_context_len(struct context *context) len = 1; /* for the beginning ":" */ for (l = 0; l < 2; l++) { int index_sens = context->range.level[l].sens; - len += strlen(policydb.p_sens_val_to_name[index_sens - 1]); + len += strlen(sym_name(&policydb, SYM_LEVELS, index_sens - 1)); /* categories */ head = -2; @@ -55,17 +55,17 @@ int mls_compute_context_len(struct context *context) if (i - prev > 1) { /* one or more negative bits are skipped */ if (head != prev) { - nm = policydb.p_cat_val_to_name[prev]; + nm = sym_name(&policydb, SYM_CATS, prev); len += strlen(nm) + 1; } - nm = policydb.p_cat_val_to_name[i]; + nm = sym_name(&policydb, SYM_CATS, i); len += strlen(nm) + 1; head = i; } prev = i; } if (prev != head) { - nm = policydb.p_cat_val_to_name[prev]; + nm = sym_name(&policydb, SYM_CATS, prev); len += strlen(nm) + 1; } if (l == 0) { @@ -102,8 +102,8 @@ void mls_sid_to_context(struct context *context, scontextp++; for (l = 0; l < 2; l++) { - strcpy(scontextp, - policydb.p_sens_val_to_name[context->range.level[l].sens - 1]); + strcpy(scontextp, sym_name(&policydb, SYM_LEVELS, + context->range.level[l].sens - 1)); scontextp += strlen(scontextp); /* categories */ @@ -118,7 +118,7 @@ void mls_sid_to_context(struct context *context, *scontextp++ = '.'; else *scontextp++ = ','; - nm = policydb.p_cat_val_to_name[prev]; + nm = sym_name(&policydb, SYM_CATS, prev); strcpy(scontextp, nm); scontextp += strlen(nm); } @@ -126,7 +126,7 @@ void mls_sid_to_context(struct context *context, *scontextp++ = ':'; else *scontextp++ = ','; - nm = policydb.p_cat_val_to_name[i]; + nm = sym_name(&policydb, SYM_CATS, i); strcpy(scontextp, nm); scontextp += strlen(nm); head = i; @@ -139,7 +139,7 @@ void mls_sid_to_context(struct context *context, *scontextp++ = '.'; else *scontextp++ = ','; - nm = policydb.p_cat_val_to_name[prev]; + nm = sym_name(&policydb, SYM_CATS, prev); strcpy(scontextp, nm); scontextp += strlen(nm); } @@ -166,7 +166,7 @@ int mls_level_isvalid(struct policydb *p, struct mls_level *l) if (!l->sens || l->sens > p->p_levels.nprim) return 0; levdatum = hashtab_search(p->p_levels.table, - p->p_sens_val_to_name[l->sens - 1]); + sym_name(p, SYM_LEVELS, l->sens - 1)); if (!levdatum) return 0; @@ -482,7 +482,8 @@ int mls_convert_context(struct policydb *oldp, for (l = 0; l < 2; l++) { levdatum = hashtab_search(newp->p_levels.table, - oldp->p_sens_val_to_name[c->range.level[l].sens - 1]); + sym_name(oldp, SYM_LEVELS, + c->range.level[l].sens - 1)); if (!levdatum) return -EINVAL; @@ -493,7 +494,7 @@ int mls_convert_context(struct policydb *oldp, int rc; catdatum = hashtab_search(newp->p_cats.table, - oldp->p_cat_val_to_name[i]); + sym_name(oldp, SYM_CATS, i)); if (!catdatum) return -EINVAL; rc = ebitmap_set_bit(&bitmap, catdatum->value - 1, 1); diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c index af41fdfe1a71..5adca670e5af 100644 --- a/security/selinux/ss/policydb.c +++ b/security/selinux/ss/policydb.c @@ -254,12 +254,17 @@ static int common_index(void *key, void *datum, void *datap) { struct policydb *p; struct common_datum *comdatum; + struct flex_array *fa; comdatum = datum; p = datap; if (!comdatum->value || comdatum->value > p->p_commons.nprim) return -EINVAL; - p->p_common_val_to_name[comdatum->value - 1] = key; + + fa = p->sym_val_to_name[SYM_COMMONS]; + if (flex_array_put_ptr(fa, comdatum->value - 1, key, + GFP_KERNEL | __GFP_ZERO)) + BUG(); return 0; } @@ -267,12 +272,16 @@ static int class_index(void *key, void *datum, void *datap) { struct policydb *p; struct class_datum *cladatum; + struct flex_array *fa; cladatum = datum; p = datap; if (!cladatum->value || cladatum->value > p->p_classes.nprim) return -EINVAL; - p->p_class_val_to_name[cladatum->value - 1] = key; + fa = p->sym_val_to_name[SYM_CLASSES]; + if (flex_array_put_ptr(fa, cladatum->value - 1, key, + GFP_KERNEL | __GFP_ZERO)) + BUG(); p->class_val_to_struct[cladatum->value - 1] = cladatum; return 0; } @@ -281,6 +290,7 @@ static int role_index(void *key, void *datum, void *datap) { struct policydb *p; struct role_datum *role; + struct flex_array *fa; role = datum; p = datap; @@ -288,7 +298,11 @@ static int role_index(void *key, void *datum, void *datap) || role->value > p->p_roles.nprim || role->bounds > p->p_roles.nprim) return -EINVAL; - p->p_role_val_to_name[role->value - 1] = key; + + fa = p->sym_val_to_name[SYM_ROLES]; + if (flex_array_put_ptr(fa, role->value - 1, key, + GFP_KERNEL | __GFP_ZERO)) + BUG(); p->role_val_to_struct[role->value - 1] = role; return 0; } @@ -297,6 +311,7 @@ static int type_index(void *key, void *datum, void *datap) { struct policydb *p; struct type_datum *typdatum; + struct flex_array *fa; typdatum = datum; p = datap; @@ -306,10 +321,13 @@ static int type_index(void *key, void *datum, void *datap) || typdatum->value > p->p_types.nprim || typdatum->bounds > p->p_types.nprim) return -EINVAL; - p->p_type_val_to_name[typdatum->value - 1] = key; - /* this flex array was all preallocated, this cannot fail */ - if (flex_array_put_ptr(p->type_val_to_struct_array, - typdatum->value - 1, typdatum, + fa = p->sym_val_to_name[SYM_TYPES]; + if (flex_array_put_ptr(fa, typdatum->value - 1, key, + GFP_KERNEL | __GFP_ZERO)) + BUG(); + + fa = p->type_val_to_struct_array; + if (flex_array_put_ptr(fa, typdatum->value - 1, typdatum, GFP_KERNEL | __GFP_ZERO)) BUG(); } @@ -321,6 +339,7 @@ static int user_index(void *key, void *datum, void *datap) { struct policydb *p; struct user_datum *usrdatum; + struct flex_array *fa; usrdatum = datum; p = datap; @@ -328,7 +347,11 @@ static int user_index(void *key, void *datum, void *datap) || usrdatum->value > p->p_users.nprim || usrdatum->bounds > p->p_users.nprim) return -EINVAL; - p->p_user_val_to_name[usrdatum->value - 1] = key; + + fa = p->sym_val_to_name[SYM_USERS]; + if (flex_array_put_ptr(fa, usrdatum->value - 1, key, + GFP_KERNEL | __GFP_ZERO)) + BUG(); p->user_val_to_struct[usrdatum->value - 1] = usrdatum; return 0; } @@ -337,6 +360,7 @@ static int sens_index(void *key, void *datum, void *datap) { struct policydb *p; struct level_datum *levdatum; + struct flex_array *fa; levdatum = datum; p = datap; @@ -345,7 +369,10 @@ static int sens_index(void *key, void *datum, void *datap) if (!levdatum->level->sens || levdatum->level->sens > p->p_levels.nprim) return -EINVAL; - p->p_sens_val_to_name[levdatum->level->sens - 1] = key; + fa = p->sym_val_to_name[SYM_LEVELS]; + if (flex_array_put_ptr(fa, levdatum->level->sens - 1, key, + GFP_KERNEL | __GFP_ZERO)) + BUG(); } return 0; @@ -355,6 +382,7 @@ static int cat_index(void *key, void *datum, void *datap) { struct policydb *p; struct cat_datum *catdatum; + struct flex_array *fa; catdatum = datum; p = datap; @@ -362,7 +390,10 @@ static int cat_index(void *key, void *datum, void *datap) if (!catdatum->isalias) { if (!catdatum->value || catdatum->value > p->p_cats.nprim) return -EINVAL; - p->p_cat_val_to_name[catdatum->value - 1] = key; + fa = p->sym_val_to_name[SYM_CATS]; + if (flex_array_put_ptr(fa, catdatum->value - 1, key, + GFP_KERNEL | __GFP_ZERO)) + BUG(); } return 0; @@ -392,9 +423,16 @@ static int policydb_index_classes(struct policydb *p) int rc; rc = -ENOMEM; - p->p_common_val_to_name = - kmalloc(p->p_commons.nprim * sizeof(char *), GFP_KERNEL); - if (!p->p_common_val_to_name) + p->sym_val_to_name[SYM_COMMONS] = flex_array_alloc(sizeof(char *), + p->p_commons.nprim, + GFP_KERNEL | __GFP_ZERO); + if (!p->sym_val_to_name[SYM_COMMONS]) + goto out; + + rc = flex_array_prealloc(p->sym_val_to_name[SYM_COMMONS], + 0, p->p_commons.nprim - 1, + GFP_KERNEL | __GFP_ZERO); + if (rc) goto out; rc = hashtab_map(p->p_commons.table, common_index, p); @@ -408,9 +446,16 @@ static int policydb_index_classes(struct policydb *p) goto out; rc = -ENOMEM; - p->p_class_val_to_name = - kmalloc(p->p_classes.nprim * sizeof(char *), GFP_KERNEL); - if (!p->p_class_val_to_name) + p->sym_val_to_name[SYM_CLASSES] = flex_array_alloc(sizeof(char *), + p->p_classes.nprim, + GFP_KERNEL | __GFP_ZERO); + if (!p->sym_val_to_name[SYM_CLASSES]) + goto out; + + rc = flex_array_prealloc(p->sym_val_to_name[SYM_CLASSES], + 0, p->p_classes.nprim - 1, + GFP_KERNEL | __GFP_ZERO); + if (rc) goto out; rc = hashtab_map(p->p_classes.table, class_index, p); @@ -507,10 +552,18 @@ static int policydb_index_others(struct policydb *p) for (i = SYM_ROLES; i < SYM_NUM; i++) { rc = -ENOMEM; - p->sym_val_to_name[i] = - kmalloc(p->symtab[i].nprim * sizeof(char *), GFP_KERNEL); + p->sym_val_to_name[i] = flex_array_alloc(sizeof(char *), + p->symtab[i].nprim, + GFP_KERNEL | __GFP_ZERO); if (!p->sym_val_to_name[i]) goto out; + + rc = flex_array_prealloc(p->sym_val_to_name[i], + 0, p->symtab[i].nprim - 1, + GFP_KERNEL | __GFP_ZERO); + if (rc) + goto out; + rc = hashtab_map(p->symtab[i].table, index_f[i], p); if (rc) goto out; @@ -703,8 +756,10 @@ void policydb_destroy(struct policydb *p) hashtab_destroy(p->symtab[i].table); } - for (i = 0; i < SYM_NUM; i++) - kfree(p->sym_val_to_name[i]); + for (i = 0; i < SYM_NUM; i++) { + if (p->sym_val_to_name[i]) + flex_array_free(p->sym_val_to_name[i]); + } kfree(p->class_val_to_struct); kfree(p->role_val_to_struct); @@ -1566,9 +1621,9 @@ static int user_bounds_sanity_check(void *key, void *datum, void *datap) printk(KERN_ERR "SELinux: boundary violated policy: " "user=%s role=%s bounds=%s\n", - p->p_user_val_to_name[user->value - 1], - p->p_role_val_to_name[bit], - p->p_user_val_to_name[upper->value - 1]); + sym_name(p, SYM_USERS, user->value - 1), + sym_name(p, SYM_ROLES, bit), + sym_name(p, SYM_USERS, upper->value - 1)); return -EINVAL; } @@ -1603,9 +1658,9 @@ static int role_bounds_sanity_check(void *key, void *datum, void *datap) printk(KERN_ERR "SELinux: boundary violated policy: " "role=%s type=%s bounds=%s\n", - p->p_role_val_to_name[role->value - 1], - p->p_type_val_to_name[bit], - p->p_role_val_to_name[upper->value - 1]); + sym_name(p, SYM_ROLES, role->value - 1), + sym_name(p, SYM_TYPES, bit), + sym_name(p, SYM_ROLES, upper->value - 1)); return -EINVAL; } @@ -1637,7 +1692,7 @@ static int type_bounds_sanity_check(void *key, void *datum, void *datap) printk(KERN_ERR "SELinux: type %s: " "bounded by attribute %s", (char *) key, - p->p_type_val_to_name[upper->value - 1]); + sym_name(p, SYM_TYPES, upper->value - 1)); return -EINVAL; } } diff --git a/security/selinux/ss/policydb.h b/security/selinux/ss/policydb.h index 9826a92a6b0c..4e3ab9d0b315 100644 --- a/security/selinux/ss/policydb.h +++ b/security/selinux/ss/policydb.h @@ -203,15 +203,7 @@ struct policydb { #define p_cats symtab[SYM_CATS] /* symbol names indexed by (value - 1) */ - char **sym_val_to_name[SYM_NUM]; -#define p_common_val_to_name sym_val_to_name[SYM_COMMONS] -#define p_class_val_to_name sym_val_to_name[SYM_CLASSES] -#define p_role_val_to_name sym_val_to_name[SYM_ROLES] -#define p_type_val_to_name sym_val_to_name[SYM_TYPES] -#define p_user_val_to_name sym_val_to_name[SYM_USERS] -#define p_bool_val_to_name sym_val_to_name[SYM_BOOLS] -#define p_sens_val_to_name sym_val_to_name[SYM_LEVELS] -#define p_cat_val_to_name sym_val_to_name[SYM_CATS] + struct flex_array *sym_val_to_name[SYM_NUM]; /* class, role, and user attributes indexed by (value - 1) */ struct class_datum **class_val_to_struct; @@ -321,6 +313,13 @@ static inline int put_entry(void *buf, size_t bytes, int num, struct policy_file return 0; } +static inline char *sym_name(struct policydb *p, unsigned int sym_num, unsigned int element_nr) +{ + struct flex_array *fa = p->sym_val_to_name[sym_num]; + + return flex_array_get_ptr(fa, element_nr); +} + extern u16 string_to_security_class(struct policydb *p, const char *name); extern u32 string_to_av_perm(struct policydb *p, u16 tclass, const char *name); diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index afcbc19817f7..a03cfaf0ee07 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -464,7 +464,7 @@ static void security_dump_masked_av(struct context *scontext, if (!permissions) return; - tclass_name = policydb.p_class_val_to_name[tclass - 1]; + tclass_name = sym_name(&policydb, SYM_CLASSES, tclass - 1); tclass_dat = policydb.class_val_to_struct[tclass - 1]; common_dat = tclass_dat->comdatum; @@ -716,7 +716,7 @@ static int security_validtrans_handle_fail(struct context *ocontext, audit_log(current->audit_context, GFP_ATOMIC, AUDIT_SELINUX_ERR, "security_validate_transition: denied for" " oldcontext=%s newcontext=%s taskcontext=%s tclass=%s", - o, n, t, policydb.p_class_val_to_name[tclass-1]); + o, n, t, sym_name(&policydb, SYM_CLASSES, tclass-1)); out: kfree(o); kfree(n); @@ -1012,9 +1012,9 @@ static int context_struct_to_string(struct context *context, char **scontext, u3 } /* Compute the size of the context. */ - *scontext_len += strlen(policydb.p_user_val_to_name[context->user - 1]) + 1; - *scontext_len += strlen(policydb.p_role_val_to_name[context->role - 1]) + 1; - *scontext_len += strlen(policydb.p_type_val_to_name[context->type - 1]) + 1; + *scontext_len += strlen(sym_name(&policydb, SYM_USERS, context->user - 1)) + 1; + *scontext_len += strlen(sym_name(&policydb, SYM_ROLES, context->role - 1)) + 1; + *scontext_len += strlen(sym_name(&policydb, SYM_TYPES, context->type - 1)) + 1; *scontext_len += mls_compute_context_len(context); if (!scontext) @@ -1030,12 +1030,12 @@ static int context_struct_to_string(struct context *context, char **scontext, u3 * Copy the user name, role name and type name into the context. */ sprintf(scontextp, "%s:%s:%s", - policydb.p_user_val_to_name[context->user - 1], - policydb.p_role_val_to_name[context->role - 1], - policydb.p_type_val_to_name[context->type - 1]); - scontextp += strlen(policydb.p_user_val_to_name[context->user - 1]) + - 1 + strlen(policydb.p_role_val_to_name[context->role - 1]) + - 1 + strlen(policydb.p_type_val_to_name[context->type - 1]); + sym_name(&policydb, SYM_USERS, context->user - 1), + sym_name(&policydb, SYM_ROLES, context->role - 1), + sym_name(&policydb, SYM_TYPES, context->type - 1)); + scontextp += strlen(sym_name(&policydb, SYM_USERS, context->user - 1)) + + 1 + strlen(sym_name(&policydb, SYM_ROLES, context->role - 1)) + + 1 + strlen(sym_name(&policydb, SYM_TYPES, context->type - 1)); mls_sid_to_context(context, &scontextp); @@ -1333,7 +1333,7 @@ static int compute_sid_handle_invalid_context( " for scontext=%s" " tcontext=%s" " tclass=%s", - n, s, t, policydb.p_class_val_to_name[tclass-1]); + n, s, t, sym_name(&policydb, SYM_CLASSES, tclass-1)); out: kfree(s); kfree(t); @@ -1654,7 +1654,7 @@ static int convert_context(u32 key, /* Convert the user. */ rc = -EINVAL; usrdatum = hashtab_search(args->newp->p_users.table, - args->oldp->p_user_val_to_name[c->user - 1]); + sym_name(args->oldp, SYM_USERS, c->user - 1)); if (!usrdatum) goto bad; c->user = usrdatum->value; @@ -1662,7 +1662,7 @@ static int convert_context(u32 key, /* Convert the role. */ rc = -EINVAL; role = hashtab_search(args->newp->p_roles.table, - args->oldp->p_role_val_to_name[c->role - 1]); + sym_name(args->oldp, SYM_ROLES, c->role - 1)); if (!role) goto bad; c->role = role->value; @@ -1670,7 +1670,7 @@ static int convert_context(u32 key, /* Convert the type. */ rc = -EINVAL; typdatum = hashtab_search(args->newp->p_types.table, - args->oldp->p_type_val_to_name[c->type - 1]); + sym_name(args->oldp, SYM_TYPES, c->type - 1)); if (!typdatum) goto bad; c->type = typdatum->value; @@ -2326,14 +2326,14 @@ int security_get_bools(int *len, char ***names, int **values) size_t name_len; (*values)[i] = policydb.bool_val_to_struct[i]->state; - name_len = strlen(policydb.p_bool_val_to_name[i]) + 1; + name_len = strlen(sym_name(&policydb, SYM_BOOLS, i)) + 1; rc = -ENOMEM; (*names)[i] = kmalloc(sizeof(char) * name_len, GFP_ATOMIC); if (!(*names)[i]) goto err; - strncpy((*names)[i], policydb.p_bool_val_to_name[i], name_len); + strncpy((*names)[i], sym_name(&policydb, SYM_BOOLS, i), name_len); (*names)[i][name_len - 1] = 0; } rc = 0; @@ -2368,7 +2368,7 @@ int security_set_bools(int len, int *values) audit_log(current->audit_context, GFP_ATOMIC, AUDIT_MAC_CONFIG_CHANGE, "bool=%s val=%d old_val=%d auid=%u ses=%u", - policydb.p_bool_val_to_name[i], + sym_name(&policydb, SYM_BOOLS, i), !!values[i], policydb.bool_val_to_struct[i]->state, audit_get_loginuid(current), @@ -3132,7 +3132,7 @@ int security_netlbl_sid_to_secattr(u32 sid, struct netlbl_lsm_secattr *secattr) goto out; rc = -ENOMEM; - secattr->domain = kstrdup(policydb.p_type_val_to_name[ctx->type - 1], + secattr->domain = kstrdup(sym_name(&policydb, SYM_TYPES, ctx->type - 1), GFP_ATOMIC); if (secattr->domain == NULL) goto out; From 1d9bc6dc5b6b9cc9299739f0245ce4841f066b92 Mon Sep 17 00:00:00 2001 From: Eric Paris Date: Mon, 29 Nov 2010 15:47:09 -0500 Subject: [PATCH 09/12] SELinux: merge policydb_index_classes and policydb_index_others We duplicate functionality in policydb_index_classes() and policydb_index_others(). This patch merges those functions just to make it clear there is nothing special happening here. Signed-off-by: Eric Paris --- security/selinux/ss/policydb.c | 69 +++++----------------------------- 1 file changed, 10 insertions(+), 59 deletions(-) diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c index 5adca670e5af..be9de3872837 100644 --- a/security/selinux/ss/policydb.c +++ b/security/selinux/ss/policydb.c @@ -411,58 +411,6 @@ static int (*index_f[SYM_NUM]) (void *key, void *datum, void *datap) = cat_index, }; -/* - * Define the common val_to_name array and the class - * val_to_name and val_to_struct arrays in a policy - * database structure. - * - * Caller must clean up upon failure. - */ -static int policydb_index_classes(struct policydb *p) -{ - int rc; - - rc = -ENOMEM; - p->sym_val_to_name[SYM_COMMONS] = flex_array_alloc(sizeof(char *), - p->p_commons.nprim, - GFP_KERNEL | __GFP_ZERO); - if (!p->sym_val_to_name[SYM_COMMONS]) - goto out; - - rc = flex_array_prealloc(p->sym_val_to_name[SYM_COMMONS], - 0, p->p_commons.nprim - 1, - GFP_KERNEL | __GFP_ZERO); - if (rc) - goto out; - - rc = hashtab_map(p->p_commons.table, common_index, p); - if (rc) - goto out; - - rc = -ENOMEM; - p->class_val_to_struct = - kmalloc(p->p_classes.nprim * sizeof(*(p->class_val_to_struct)), GFP_KERNEL); - if (!p->class_val_to_struct) - goto out; - - rc = -ENOMEM; - p->sym_val_to_name[SYM_CLASSES] = flex_array_alloc(sizeof(char *), - p->p_classes.nprim, - GFP_KERNEL | __GFP_ZERO); - if (!p->sym_val_to_name[SYM_CLASSES]) - goto out; - - rc = flex_array_prealloc(p->sym_val_to_name[SYM_CLASSES], - 0, p->p_classes.nprim - 1, - GFP_KERNEL | __GFP_ZERO); - if (rc) - goto out; - - rc = hashtab_map(p->p_classes.table, class_index, p); -out: - return rc; -} - #ifdef DEBUG_HASHES static void symtab_hash_eval(struct symtab *s) { @@ -500,7 +448,7 @@ static inline void rangetr_hash_eval(struct hashtab *h) * * Caller must clean up on failure. */ -static int policydb_index_others(struct policydb *p) +static int policydb_index(struct policydb *p) { int i, rc; @@ -519,6 +467,13 @@ static int policydb_index_others(struct policydb *p) symtab_hash_eval(p->symtab); #endif + rc = -ENOMEM; + p->class_val_to_struct = + kmalloc(p->p_classes.nprim * sizeof(*(p->class_val_to_struct)), + GFP_KERNEL); + if (!p->class_val_to_struct) + goto out; + rc = -ENOMEM; p->role_val_to_struct = kmalloc(p->p_roles.nprim * sizeof(*(p->role_val_to_struct)), @@ -550,7 +505,7 @@ static int policydb_index_others(struct policydb *p) if (cond_init_bool_indexes(p)) goto out; - for (i = SYM_ROLES; i < SYM_NUM; i++) { + for (i = 0; i < SYM_NUM; i++) { rc = -ENOMEM; p->sym_val_to_name[i] = flex_array_alloc(sizeof(char *), p->symtab[i].nprim, @@ -2296,11 +2251,7 @@ int policydb_read(struct policydb *p, void *fp) lra = ra; } - rc = policydb_index_classes(p); - if (rc) - goto bad; - - rc = policydb_index_others(p); + rc = policydb_index(p); if (rc) goto bad; From 415103f9932d45f7927f4b17e3a9a13834cdb9a1 Mon Sep 17 00:00:00 2001 From: Eric Paris Date: Thu, 2 Dec 2010 16:13:40 -0500 Subject: [PATCH 10/12] SELinux: do not compute transition labels on mountpoint labeled filesystems selinux_inode_init_security computes transitions sids even for filesystems that use mount point labeling. It shouldn't do that. It should just use the mount point label always and no matter what. This causes 2 problems. 1) it makes file creation slower than it needs to be since we calculate the transition sid and 2) it allows files to be created with a different label than the mount point! # id -Z staff_u:sysadm_r:sysadm_t:s0-s0:c0.c1023 # sesearch --type --class file --source sysadm_t --target tmp_t Found 1 semantic te rules: type_transition sysadm_t tmp_t : file user_tmp_t; # mount -o loop,context="system_u:object_r:tmp_t:s0" /tmp/fs /mnt/tmp # ls -lZ /mnt/tmp drwx------. root root system_u:object_r:tmp_t:s0 lost+found # touch /mnt/tmp/file1 # ls -lZ /mnt/tmp -rw-r--r--. root root staff_u:object_r:user_tmp_t:s0 file1 drwx------. root root system_u:object_r:tmp_t:s0 lost+found Whoops, we have a mount point labeled filesystem tmp_t with a user_tmp_t labeled file! Signed-off-by: Eric Paris Reviewed-by: Reviewed-by: James Morris --- security/selinux/hooks.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 65fa8bf596f5..cda18fd8ca0f 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -2525,7 +2525,10 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir, sid = tsec->sid; newsid = tsec->create_sid; - if (!newsid || !(sbsec->flags & SE_SBLABELSUPP)) { + if ((sbsec->flags & SE_SBINITIALIZED) && + (sbsec->behavior == SECURITY_FS_USE_MNTPOINT)) + newsid = sbsec->mntpoint_sid; + else if (!newsid || !(sbsec->flags & SE_SBLABELSUPP)) { rc = security_transition_sid(sid, dsec->sid, inode_mode_to_security_class(inode->i_mode), &newsid); From 73ff5fc0a86b28b77e02a6963b388d1dbfa0a263 Mon Sep 17 00:00:00 2001 From: Eric Paris Date: Tue, 7 Dec 2010 16:17:28 -0500 Subject: [PATCH 11/12] selinux: cache sidtab_context_to_sid results sidtab_context_to_sid takes up a large share of time when creating large numbers of new inodes (~30-40% in oprofile runs). This patch implements a cache of 3 entries which is checked before we do a full context_to_sid lookup. On one system this showed over a x3 improvement in the number of inodes that could be created per second and around a 20% improvement on another system. Any time we look up the same context string sucessivly (imagine ls -lZ) we should hit this cache hot. A cache miss should have a relatively minor affect on performance next to doing the full table search. All operations on the cache are done COMPLETELY lockless. We know that all struct sidtab_node objects created will never be deleted until a new policy is loaded thus we never have to worry about a pointer being dereferenced. Since we also know that pointer assignment is atomic we know that the cache will always have valid pointers. Given this information we implement a FIFO cache in an array of 3 pointers. Every result (whether a cache hit or table lookup) will be places in the 0 spot of the cache and the rest of the entries moved down one spot. The 3rd entry will be lost. Races are possible and are even likely to happen. Lets assume that 4 tasks are hitting sidtab_context_to_sid. The first task checks against the first entry in the cache and it is a miss. Now lets assume a second task updates the cache with a new entry. This will push the first entry back to the second spot. Now the first task might check against the second entry (which it already checked) and will miss again. Now say some third task updates the cache and push the second entry to the third spot. The first task my check the third entry (for the third time!) and again have a miss. At which point it will just do a full table lookup. No big deal! Signed-off-by: Eric Paris --- security/selinux/ss/sidtab.c | 39 ++++++++++++++++++++++++++++++++++-- security/selinux/ss/sidtab.h | 2 ++ 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/security/selinux/ss/sidtab.c b/security/selinux/ss/sidtab.c index e817989764cd..5840a35155fc 100644 --- a/security/selinux/ss/sidtab.c +++ b/security/selinux/ss/sidtab.c @@ -147,6 +147,17 @@ out: return rc; } +static void sidtab_update_cache(struct sidtab *s, struct sidtab_node *n, int loc) +{ + BUG_ON(loc >= SIDTAB_CACHE_LEN); + + while (loc > 0) { + s->cache[loc] = s->cache[loc - 1]; + loc--; + } + s->cache[0] = n; +} + static inline u32 sidtab_search_context(struct sidtab *s, struct context *context) { @@ -156,14 +167,33 @@ static inline u32 sidtab_search_context(struct sidtab *s, for (i = 0; i < SIDTAB_SIZE; i++) { cur = s->htable[i]; while (cur) { - if (context_cmp(&cur->context, context)) + if (context_cmp(&cur->context, context)) { + sidtab_update_cache(s, cur, SIDTAB_CACHE_LEN - 1); return cur->sid; + } cur = cur->next; } } return 0; } +static inline u32 sidtab_search_cache(struct sidtab *s, struct context *context) +{ + int i; + struct sidtab_node *node; + + for (i = 0; i < SIDTAB_CACHE_LEN; i++) { + node = s->cache[i]; + if (unlikely(!node)) + return 0; + if (context_cmp(&node->context, context)) { + sidtab_update_cache(s, node, i); + return node->sid; + } + } + return 0; +} + int sidtab_context_to_sid(struct sidtab *s, struct context *context, u32 *out_sid) @@ -174,7 +204,9 @@ int sidtab_context_to_sid(struct sidtab *s, *out_sid = SECSID_NULL; - sid = sidtab_search_context(s, context); + sid = sidtab_search_cache(s, context); + if (!sid) + sid = sidtab_search_context(s, context); if (!sid) { spin_lock_irqsave(&s->lock, flags); /* Rescan now that we hold the lock. */ @@ -259,12 +291,15 @@ void sidtab_destroy(struct sidtab *s) void sidtab_set(struct sidtab *dst, struct sidtab *src) { unsigned long flags; + int i; spin_lock_irqsave(&src->lock, flags); dst->htable = src->htable; dst->nel = src->nel; dst->next_sid = src->next_sid; dst->shutdown = 0; + for (i = 0; i < SIDTAB_CACHE_LEN; i++) + dst->cache[i] = NULL; spin_unlock_irqrestore(&src->lock, flags); } diff --git a/security/selinux/ss/sidtab.h b/security/selinux/ss/sidtab.h index 64ea5b1cdea4..84dc154d9389 100644 --- a/security/selinux/ss/sidtab.h +++ b/security/selinux/ss/sidtab.h @@ -26,6 +26,8 @@ struct sidtab { unsigned int nel; /* number of elements */ unsigned int next_sid; /* next SID to allocate */ unsigned char shutdown; +#define SIDTAB_CACHE_LEN 3 + struct sidtab_node *cache[SIDTAB_CACHE_LEN]; spinlock_t lock; }; From 350e4f31e0eaf56dfc3b328d24a11bdf42a41fb8 Mon Sep 17 00:00:00 2001 From: Eric Paris Date: Thu, 16 Dec 2010 11:46:51 -0500 Subject: [PATCH 12/12] SELinux: define permissions for DCB netlink messages Commit 2f90b865 added two new netlink message types to the netlink route socket. SELinux has hooks to define if netlink messages are allowed to be sent or received, but it did not know about these two new message types. By default we allow such actions so noone likely noticed. This patch adds the proper definitions and thus proper permissions enforcement. Signed-off-by: Eric Paris --- security/selinux/nlmsgtab.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/security/selinux/nlmsgtab.c b/security/selinux/nlmsgtab.c index 75ec0c6ebacd..8b02b2137da2 100644 --- a/security/selinux/nlmsgtab.c +++ b/security/selinux/nlmsgtab.c @@ -65,6 +65,8 @@ static struct nlmsg_perm nlmsg_route_perms[] = { RTM_NEWADDRLABEL, NETLINK_ROUTE_SOCKET__NLMSG_WRITE }, { RTM_DELADDRLABEL, NETLINK_ROUTE_SOCKET__NLMSG_WRITE }, { RTM_GETADDRLABEL, NETLINK_ROUTE_SOCKET__NLMSG_READ }, + { RTM_GETDCB, NETLINK_ROUTE_SOCKET__NLMSG_READ }, + { RTM_SETDCB, NETLINK_ROUTE_SOCKET__NLMSG_WRITE }, }; static struct nlmsg_perm nlmsg_firewall_perms[] =