[RFC PATCH 01/22] selinux: supply missing field initializers

[RFC PATCH 01/22] selinux: supply missing field initializers

[Christian Göttsche]

From: Christian Göttsche <cgzones@googlemail.com>

Please clang by supplying the missing field initializes:

    security/selinux/selinuxfs.c:2004:21: warning: missing field 'ops' initializer [-Wmissing-field-initializers]
     2004 |                 /* last one */ {""}
          |                                   ^
    ./security/selinux/include/classmap.h:182:9: warning: missing field 'perms' initializer [-Wmissing-field-initializers]
      182 |         { NULL }
          |                ^

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 security/selinux/include/classmap.h | 2 +-
 security/selinux/selinuxfs.c        | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
index 2bc20135324a..03e82477dce9 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -179,7 +179,7 @@ const struct security_class_mapping secclass_map[] = {
 	{ "anon_inode", { COMMON_FILE_PERMS, NULL } },
 	{ "io_uring", { "override_creds", "sqpoll", "cmd", NULL } },
 	{ "user_namespace", { "create", NULL } },
-	{ NULL }
+	/* last one */ { NULL, {} }
 };
 
 #ifdef __KERNEL__ /* avoid this check when building host programs */
diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index 234f4789b787..c3e9627f1283 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -2001,7 +2001,7 @@ static int sel_fill_super(struct super_block *sb, struct fs_context *fc)
 		[SEL_POLICY] = {"policy", &sel_policy_ops, S_IRUGO},
 		[SEL_VALIDATE_TRANS] = {"validatetrans", &sel_transition_ops,
 					S_IWUGO},
-		/* last one */ {""}
+		/* last one */ {"", NULL, 0}
 	};
 
 	ret = selinux_fs_info_create(sb);
-- 
2.45.2

[RFC PATCH 02/22] selinux: avoid using types indicating user space interaction

[Christian Göttsche]

From: Christian Göttsche <cgzones@googlemail.com>

Integer types starting with a double underscore, like __u32, are
intended for usage of variables interacting with user-space.

Just use the plain variant.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 security/selinux/hooks.c       | 2 +-
 security/selinux/ss/policydb.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index ad3abd48eed1..637a180e6b9b 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3135,7 +3135,7 @@ static int selinux_inode_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
 	const struct cred *cred = current_cred();
 	struct inode *inode = d_backing_inode(dentry);
 	unsigned int ia_valid = iattr->ia_valid;
-	__u32 av = FILE__WRITE;
+	u32 av = FILE__WRITE;
 
 	/* ATTR_FORCE is just used for ATTR_KILL_S[UG]ID. */
 	if (ia_valid & ATTR_FORCE) {
diff --git a/security/selinux/ss/policydb.h b/security/selinux/ss/policydb.h
index 4bba386264a3..5c11069121d3 100644
--- a/security/selinux/ss/policydb.h
+++ b/security/selinux/ss/policydb.h
@@ -144,7 +144,7 @@ struct range_trans {
 
 /* Boolean data type */
 struct cond_bool_datum {
-	__u32 value; /* internal type value */
+	u32 value; /* internal type value */
 	int state;
 };
 
-- 
2.45.2

[RFC PATCH 03/22] selinux: align and constify functions

[Christian Göttsche]

From: Christian Göttsche <cgzones@googlemail.com>

Align the parameter names between declarations and definitions, and
constify read-only parameters.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 security/selinux/include/conditional.h | 2 +-
 security/selinux/include/security.h    | 4 ++--
 security/selinux/ss/avtab.h            | 2 +-
 security/selinux/ss/services.c         | 4 ++--
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/security/selinux/include/conditional.h b/security/selinux/include/conditional.h
index 5910bb7c2eca..060833e2dba2 100644
--- a/security/selinux/include/conditional.h
+++ b/security/selinux/include/conditional.h
@@ -16,7 +16,7 @@
 int security_get_bools(struct selinux_policy *policy, u32 *len, char ***names,
 		       int **values);
 
-int security_set_bools(u32 len, int *values);
+int security_set_bools(u32 len, const int *values);
 
 int security_get_bool_value(u32 index);
 
diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
index c7f2731abd03..576fec17c0e0 100644
--- a/security/selinux/include/security.h
+++ b/security/selinux/include/security.h
@@ -289,7 +289,7 @@ int security_context_to_sid_default(const char *scontext, u32 scontext_len,
 int security_context_to_sid_force(const char *scontext, u32 scontext_len,
 				  u32 *sid);
 
-int security_get_user_sids(u32 callsid, char *username, u32 **sids, u32 *nel);
+int security_get_user_sids(u32 fromsid, const char *username, u32 **sids, u32 *nel);
 
 int security_port_sid(u8 protocol, u16 port, u32 *out_sid);
 
@@ -307,7 +307,7 @@ int security_validate_transition(u32 oldsid, u32 newsid, u32 tasksid,
 int security_validate_transition_user(u32 oldsid, u32 newsid, u32 tasksid,
 				      u16 tclass);
 
-int security_bounded_transition(u32 oldsid, u32 newsid);
+int security_bounded_transition(u32 old_sid, u32 new_sid);
 
 int security_sid_mls_copy(u32 sid, u32 mls_sid, u32 *new_sid);
 
diff --git a/security/selinux/ss/avtab.h b/security/selinux/ss/avtab.h
index f4407185401c..e10b78e61611 100644
--- a/security/selinux/ss/avtab.h
+++ b/security/selinux/ss/avtab.h
@@ -89,7 +89,7 @@ struct avtab {
 };
 
 void avtab_init(struct avtab *h);
-int avtab_alloc(struct avtab *, u32);
+int avtab_alloc(struct avtab *h, u32 nrules);
 int avtab_alloc_dup(struct avtab *new, const struct avtab *orig);
 void avtab_destroy(struct avtab *h);
 
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 9652aec400cb..b7ef8ab06185 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -2708,7 +2708,7 @@ int security_node_sid(u16 domain,
  */
 
 int security_get_user_sids(u32 fromsid,
-			   char *username,
+			   const char *username,
 			   u32 **sids,
 			   u32 *nel)
 {
@@ -3030,7 +3030,7 @@ int security_get_bools(struct selinux_policy *policy,
 }
 
 
-int security_set_bools(u32 len, int *values)
+int security_set_bools(u32 len, const int *values)
 {
 	struct selinux_state *state = &selinux_state;
 	struct selinux_policy *newpolicy, *oldpolicy;
-- 
2.45.2

[RFC PATCH 04/22] selinux: rework match_ipv6_addrmask()

[Christian Göttsche]

From: Christian Göttsche <cgzones@googlemail.com>

Constify parameters, add size hints, and simplify control flow.

According to godbolt the same assembly is generated.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 security/selinux/ss/services.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index b7ef8ab06185..261a512528d5 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -2593,17 +2593,15 @@ int security_netif_sid(char *name, u32 *if_sid)
 	return rc;
 }
 
-static int match_ipv6_addrmask(u32 *input, u32 *addr, u32 *mask)
+static bool match_ipv6_addrmask(const u32 input[4], const u32 addr[4], const u32 mask[4])
 {
-	int i, fail = 0;
+	int i;
 
 	for (i = 0; i < 4; i++)
-		if (addr[i] != (input[i] & mask[i])) {
-			fail = 1;
-			break;
-		}
+		if (addr[i] != (input[i] & mask[i]))
+			return false;
 
-	return !fail;
+	return true;
 }
 
 /**
-- 
2.45.2

[RFC PATCH 05/22] selinux: avoid nontransitive comparison

[Christian Göttsche]

From: Christian Göttsche <cgzones@googlemail.com>

Avoid using nontransitive comparison to prevent unexpected sorting
results due to (well-defined) overflows.
See https://www.qualys.com/2024/01/30/qsort.txt for a related issue in
glibc's qsort(3).

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 security/selinux/ss/policydb.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index 383f3ae82a73..d04d9ada3835 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -37,6 +37,8 @@
 #include "mls.h"
 #include "services.h"
 
+#define spaceship_cmp(a, b) (((a) > (b)) - ((a) < (b)))
+
 #ifdef CONFIG_SECURITY_SELINUX_DEBUG
 /* clang-format off */
 static const char *const symtab_name[SYM_NUM] = {
@@ -421,11 +423,11 @@ static int filenametr_cmp(const void *k1, const void *k2)
 	const struct filename_trans_key *ft2 = k2;
 	int v;
 
-	v = ft1->ttype - ft2->ttype;
+	v = spaceship_cmp(ft1->ttype, ft2->ttype);
 	if (v)
 		return v;
 
-	v = ft1->tclass - ft2->tclass;
+	v = spaceship_cmp(ft1->tclass, ft2->tclass);
 	if (v)
 		return v;
 
@@ -456,15 +458,15 @@ static int rangetr_cmp(const void *k1, const void *k2)
 	const struct range_trans *key1 = k1, *key2 = k2;
 	int v;
 
-	v = key1->source_type - key2->source_type;
+	v = spaceship_cmp(key1->source_type, key2->source_type);
 	if (v)
 		return v;
 
-	v = key1->target_type - key2->target_type;
+	v = spaceship_cmp(key1->target_type, key2->target_type);
 	if (v)
 		return v;
 
-	v = key1->target_class - key2->target_class;
+	v = spaceship_cmp(key1->target_class, key2->target_class);
 
 	return v;
 }
@@ -493,15 +495,15 @@ static int role_trans_cmp(const void *k1, const void *k2)
 	const struct role_trans_key *key1 = k1, *key2 = k2;
 	int v;
 
-	v = key1->role - key2->role;
+	v = spaceship_cmp(key1->role, key2->role);
 	if (v)
 		return v;
 
-	v = key1->type - key2->type;
+	v = spaceship_cmp(key1->type, key2->type);
 	if (v)
 		return v;
 
-	return key1->tclass - key2->tclass;
+	return spaceship_cmp(key1->tclass, key2->tclass);
 }
 
 static const struct hashtab_key_params roletr_key_params = {
-- 
2.45.2

[RFC PATCH 06/22] selinux: rename comparison functions for clarity

[Christian Göttsche]

From: Christian Göttsche <cgzones@googlemail.com>

The functions context_cmp() and mls_context_cmp() are not traditional
C style compare functions returning -1, 0, and 1 for less than, equal,
and greater than; they only return whether their arguments are equal.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 security/selinux/ss/context.c  |  2 +-
 security/selinux/ss/context.h  | 10 +++++-----
 security/selinux/ss/services.c |  2 +-
 security/selinux/ss/sidtab.c   |  2 +-
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/security/selinux/ss/context.c b/security/selinux/ss/context.c
index e39990f494dd..a528b7f76280 100644
--- a/security/selinux/ss/context.c
+++ b/security/selinux/ss/context.c
@@ -20,7 +20,7 @@ u32 context_compute_hash(const struct context *c)
 	 * context struct with only the len & str set (and vice versa)
 	 * under a given policy. Since context structs from different
 	 * policies should never meet, it is safe to hash valid and
-	 * invalid contexts differently. The context_cmp() function
+	 * invalid contexts differently. The context_equal() function
 	 * already operates under the same assumption.
 	 */
 	if (c->len)
diff --git a/security/selinux/ss/context.h b/security/selinux/ss/context.h
index 7ccab2e6965f..e1307f6f7f50 100644
--- a/security/selinux/ss/context.h
+++ b/security/selinux/ss/context.h
@@ -132,8 +132,8 @@ static inline int mls_context_glblub(struct context *dst,
 	return rc;
 }
 
-static inline int mls_context_cmp(const struct context *c1,
-				  const struct context *c2)
+static inline bool mls_context_equal(const struct context *c1,
+				     const struct context *c2)
 {
 	return ((c1->range.level[0].sens == c2->range.level[0].sens) &&
 		ebitmap_cmp(&c1->range.level[0].cat, &c2->range.level[0].cat) &&
@@ -188,15 +188,15 @@ static inline void context_destroy(struct context *c)
 	mls_context_destroy(c);
 }
 
-static inline int context_cmp(const struct context *c1,
-			      const struct context *c2)
+static inline bool context_equal(const struct context *c1,
+				 const struct context *c2)
 {
 	if (c1->len && c2->len)
 		return (c1->len == c2->len && !strcmp(c1->str, c2->str));
 	if (c1->len || c2->len)
 		return 0;
 	return ((c1->user == c2->user) && (c1->role == c2->role) &&
-		(c1->type == c2->type) && mls_context_cmp(c1, c2));
+		(c1->type == c2->type) && mls_context_equal(c1, c2));
 }
 
 u32 context_compute_hash(const struct context *c);
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 261a512528d5..2b155f22a0f4 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -3327,7 +3327,7 @@ int security_net_peersid_resolve(u32 nlbl_sid, u32 nlbl_type,
 		       __func__, xfrm_sid);
 		goto out;
 	}
-	rc = (mls_context_cmp(nlbl_ctx, xfrm_ctx) ? 0 : -EACCES);
+	rc = (mls_context_equal(nlbl_ctx, xfrm_ctx) ? 0 : -EACCES);
 	if (rc)
 		goto out;
 
diff --git a/security/selinux/ss/sidtab.c b/security/selinux/ss/sidtab.c
index c8848cbba81f..c74353672dcf 100644
--- a/security/selinux/ss/sidtab.c
+++ b/security/selinux/ss/sidtab.c
@@ -66,7 +66,7 @@ static u32 context_to_sid(struct sidtab *s, struct context *context, u32 hash)
 	hash_for_each_possible_rcu(s->context_to_sid, entry, list, hash) {
 		if (entry->hash != hash)
 			continue;
-		if (context_cmp(&entry->context, context)) {
+		if (context_equal(&entry->context, context)) {
 			sid = entry->sid;
 			break;
 		}
-- 
2.45.2

[RFC PATCH 07/22] selinux: use known type instead of void pointer

[Christian Göttsche]

From: Christian Göttsche <cgzones@googlemail.com>

Improve type safety and readability by using the known type.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 security/selinux/ss/avtab.c       |  8 +--
 security/selinux/ss/avtab.h       |  9 ++--
 security/selinux/ss/conditional.c | 12 ++---
 security/selinux/ss/conditional.h |  6 +--
 security/selinux/ss/ebitmap.c     |  4 +-
 security/selinux/ss/ebitmap.h     |  5 +-
 security/selinux/ss/policydb.c    | 88 +++++++++++++++----------------
 security/selinux/ss/policydb.h    | 17 +++---
 8 files changed, 76 insertions(+), 73 deletions(-)

diff --git a/security/selinux/ss/avtab.c b/security/selinux/ss/avtab.c
index 8e400dd736b7..23210faaa046 100644
--- a/security/selinux/ss/avtab.c
+++ b/security/selinux/ss/avtab.c
@@ -336,7 +336,7 @@ static const uint16_t spec_order[] = {
 };
 /* clang-format on */
 
-int avtab_read_item(struct avtab *a, void *fp, struct policydb *pol,
+int avtab_read_item(struct avtab *a, struct policy_file *fp, struct policydb *pol,
 		    int (*insertf)(struct avtab *a, const struct avtab_key *k,
 				   const struct avtab_datum *d, void *p),
 		    void *p)
@@ -500,7 +500,7 @@ static int avtab_insertf(struct avtab *a, const struct avtab_key *k,
 	return avtab_insert(a, k, d);
 }
 
-int avtab_read(struct avtab *a, void *fp, struct policydb *pol)
+int avtab_read(struct avtab *a, struct policy_file *fp, struct policydb *pol)
 {
 	int rc;
 	__le32 buf[1];
@@ -543,7 +543,7 @@ int avtab_read(struct avtab *a, void *fp, struct policydb *pol)
 	goto out;
 }
 
-int avtab_write_item(struct policydb *p, const struct avtab_node *cur, void *fp)
+int avtab_write_item(struct policydb *p, const struct avtab_node *cur, struct policy_file *fp)
 {
 	__le16 buf16[4];
 	__le32 buf32[ARRAY_SIZE(cur->datum.u.xperms->perms.p)];
@@ -579,7 +579,7 @@ int avtab_write_item(struct policydb *p, const struct avtab_node *cur, void *fp)
 	return 0;
 }
 
-int avtab_write(struct policydb *p, struct avtab *a, void *fp)
+int avtab_write(struct policydb *p, struct avtab *a, struct policy_file *fp)
 {
 	u32 i;
 	int rc = 0;
diff --git a/security/selinux/ss/avtab.h b/security/selinux/ss/avtab.h
index e10b78e61611..c671b6e909d4 100644
--- a/security/selinux/ss/avtab.h
+++ b/security/selinux/ss/avtab.h
@@ -105,15 +105,16 @@ static inline void avtab_hash_eval(struct avtab *h, const char *tag)
 #endif
 
 struct policydb;
-int avtab_read_item(struct avtab *a, void *fp, struct policydb *pol,
+struct policy_file;
+int avtab_read_item(struct avtab *a, struct policy_file *fp, struct policydb *pol,
 		    int (*insert)(struct avtab *a, const struct avtab_key *k,
 				  const struct avtab_datum *d, void *p),
 		    void *p);
 
-int avtab_read(struct avtab *a, void *fp, struct policydb *pol);
+int avtab_read(struct avtab *a, struct policy_file *fp, struct policydb *pol);
 int avtab_write_item(struct policydb *p, const struct avtab_node *cur,
-		     void *fp);
-int avtab_write(struct policydb *p, struct avtab *a, void *fp);
+		     struct policy_file *fp);
+int avtab_write(struct policydb *p, struct avtab *a, struct policy_file *fp);
 
 struct avtab_node *avtab_insert_nonunique(struct avtab *h,
 					  const struct avtab_key *key,
diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
index 64ba95e40a6f..244dc8279113 100644
--- a/security/selinux/ss/conditional.c
+++ b/security/selinux/ss/conditional.c
@@ -206,7 +206,7 @@ static int bool_isvalid(struct cond_bool_datum *b)
 	return 1;
 }
 
-int cond_read_bool(struct policydb *p, struct symtab *s, void *fp)
+int cond_read_bool(struct policydb *p, struct symtab *s, struct policy_file *fp)
 {
 	char *key = NULL;
 	struct cond_bool_datum *booldatum;
@@ -323,7 +323,7 @@ static int cond_insertf(struct avtab *a, const struct avtab_key *k,
 	return 0;
 }
 
-static int cond_read_av_list(struct policydb *p, void *fp,
+static int cond_read_av_list(struct policydb *p, struct policy_file *fp,
 			     struct cond_av_list *list,
 			     struct cond_av_list *other)
 {
@@ -375,7 +375,7 @@ static int expr_node_isvalid(struct policydb *p, struct cond_expr_node *expr)
 	return 1;
 }
 
-static int cond_read_node(struct policydb *p, struct cond_node *node, void *fp)
+static int cond_read_node(struct policydb *p, struct cond_node *node, struct policy_file *fp)
 {
 	__le32 buf[2];
 	u32 i, len;
@@ -415,7 +415,7 @@ static int cond_read_node(struct policydb *p, struct cond_node *node, void *fp)
 	return cond_read_av_list(p, fp, &node->false_list, &node->true_list);
 }
 
-int cond_read_list(struct policydb *p, void *fp)
+int cond_read_list(struct policydb *p, struct policy_file *fp)
 {
 	__le32 buf[1];
 	u32 i, len;
@@ -453,7 +453,7 @@ int cond_write_bool(void *vkey, void *datum, void *ptr)
 	char *key = vkey;
 	struct cond_bool_datum *booldatum = datum;
 	struct policy_data *pd = ptr;
-	void *fp = pd->fp;
+	struct policy_file *fp = pd->fp;
 	__le32 buf[3];
 	u32 len;
 	int rc;
@@ -536,7 +536,7 @@ static int cond_write_node(struct policydb *p, struct cond_node *node,
 	return 0;
 }
 
-int cond_write_list(struct policydb *p, void *fp)
+int cond_write_list(struct policydb *p, struct policy_file *fp)
 {
 	u32 i;
 	__le32 buf[1];
diff --git a/security/selinux/ss/conditional.h b/security/selinux/ss/conditional.h
index 8827715bad75..468e98ad3ea1 100644
--- a/security/selinux/ss/conditional.h
+++ b/security/selinux/ss/conditional.h
@@ -68,10 +68,10 @@ int cond_destroy_bool(void *key, void *datum, void *p);
 
 int cond_index_bool(void *key, void *datum, void *datap);
 
-int cond_read_bool(struct policydb *p, struct symtab *s, void *fp);
-int cond_read_list(struct policydb *p, void *fp);
+int cond_read_bool(struct policydb *p, struct symtab *s, struct policy_file *fp);
+int cond_read_list(struct policydb *p, struct policy_file *fp);
 int cond_write_bool(void *key, void *datum, void *ptr);
-int cond_write_list(struct policydb *p, void *fp);
+int cond_write_list(struct policydb *p, struct policy_file *fp);
 
 void cond_compute_av(struct avtab *ctab, struct avtab_key *key,
 		     struct av_decision *avd, struct extended_perms *xperms);
diff --git a/security/selinux/ss/ebitmap.c b/security/selinux/ss/ebitmap.c
index 99c01be15115..cd84357db2c4 100644
--- a/security/selinux/ss/ebitmap.c
+++ b/security/selinux/ss/ebitmap.c
@@ -360,7 +360,7 @@ void ebitmap_destroy(struct ebitmap *e)
 	e->node = NULL;
 }
 
-int ebitmap_read(struct ebitmap *e, void *fp)
+int ebitmap_read(struct ebitmap *e, struct policy_file *fp)
 {
 	struct ebitmap_node *n = NULL;
 	u32 mapunit, count, startbit, index, i;
@@ -478,7 +478,7 @@ int ebitmap_read(struct ebitmap *e, void *fp)
 	goto out;
 }
 
-int ebitmap_write(const struct ebitmap *e, void *fp)
+int ebitmap_write(const struct ebitmap *e, struct policy_file *fp)
 {
 	struct ebitmap_node *n;
 	u32 bit, count, last_bit, last_startbit;
diff --git a/security/selinux/ss/ebitmap.h b/security/selinux/ss/ebitmap.h
index ba2ac3da1153..e0150695566c 100644
--- a/security/selinux/ss/ebitmap.h
+++ b/security/selinux/ss/ebitmap.h
@@ -129,8 +129,9 @@ int ebitmap_contains(const struct ebitmap *e1, const struct ebitmap *e2,
 int ebitmap_get_bit(const struct ebitmap *e, u32 bit);
 int ebitmap_set_bit(struct ebitmap *e, u32 bit, int value);
 void ebitmap_destroy(struct ebitmap *e);
-int ebitmap_read(struct ebitmap *e, void *fp);
-int ebitmap_write(const struct ebitmap *e, void *fp);
+struct policy_file;
+int ebitmap_read(struct ebitmap *e, struct policy_file *fp);
+int ebitmap_write(const struct ebitmap *e, struct policy_file *fp);
 u32 ebitmap_hash(const struct ebitmap *e, u32 hash);
 
 #ifdef CONFIG_NETLABEL
diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index d04d9ada3835..e94ecb81c6d3 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -994,7 +994,7 @@ int policydb_context_isvalid(struct policydb *p, struct context *c)
  * Read a MLS range structure from a policydb binary
  * representation file.
  */
-static int mls_read_range_helper(struct mls_range *r, void *fp)
+static int mls_read_range_helper(struct mls_range *r, struct policy_file *fp)
 {
 	__le32 buf[2];
 	u32 items;
@@ -1054,7 +1054,7 @@ static int mls_read_range_helper(struct mls_range *r, void *fp)
  * from a policydb binary representation file.
  */
 static int context_read_and_validate(struct context *c, struct policydb *p,
-				     void *fp)
+				     struct policy_file *fp)
 {
 	__le32 buf[3];
 	int rc;
@@ -1092,7 +1092,7 @@ static int context_read_and_validate(struct context *c, struct policydb *p,
  * binary representation file.
  */
 
-static int str_read(char **strp, gfp_t flags, void *fp, u32 len)
+static int str_read(char **strp, gfp_t flags, struct policy_file *fp, u32 len)
 {
 	int rc;
 	char *str;
@@ -1115,7 +1115,7 @@ static int str_read(char **strp, gfp_t flags, void *fp, u32 len)
 	return 0;
 }
 
-static int perm_read(struct policydb *p, struct symtab *s, void *fp)
+static int perm_read(struct policydb *p, struct symtab *s, struct policy_file *fp)
 {
 	char *key = NULL;
 	struct perm_datum *perdatum;
@@ -1148,7 +1148,7 @@ static int perm_read(struct policydb *p, struct symtab *s, void *fp)
 	return rc;
 }
 
-static int common_read(struct policydb *p, struct symtab *s, void *fp)
+static int common_read(struct policydb *p, struct symtab *s, struct policy_file *fp)
 {
 	char *key = NULL;
 	struct common_datum *comdatum;
@@ -1200,7 +1200,7 @@ static void type_set_init(struct type_set *t)
 	ebitmap_init(&t->negset);
 }
 
-static int type_set_read(struct type_set *t, void *fp)
+static int type_set_read(struct type_set *t, struct policy_file *fp)
 {
 	__le32 buf[1];
 	int rc;
@@ -1219,7 +1219,7 @@ static int type_set_read(struct type_set *t, void *fp)
 }
 
 static int read_cons_helper(struct policydb *p, struct constraint_node **nodep,
-			    u32 ncons, int allowxtarget, void *fp)
+			    u32 ncons, int allowxtarget, struct policy_file *fp)
 {
 	struct constraint_node *c, *lc;
 	struct constraint_expr *e, *le;
@@ -1313,7 +1313,7 @@ static int read_cons_helper(struct policydb *p, struct constraint_node **nodep,
 	return 0;
 }
 
-static int class_read(struct policydb *p, struct symtab *s, void *fp)
+static int class_read(struct policydb *p, struct symtab *s, struct policy_file *fp)
 {
 	char *key = NULL;
 	struct class_datum *cladatum;
@@ -1410,7 +1410,7 @@ static int class_read(struct policydb *p, struct symtab *s, void *fp)
 	return rc;
 }
 
-static int role_read(struct policydb *p, struct symtab *s, void *fp)
+static int role_read(struct policydb *p, struct symtab *s, struct policy_file *fp)
 {
 	char *key = NULL;
 	struct role_datum *role;
@@ -1467,7 +1467,7 @@ static int role_read(struct policydb *p, struct symtab *s, void *fp)
 	return rc;
 }
 
-static int type_read(struct policydb *p, struct symtab *s, void *fp)
+static int type_read(struct policydb *p, struct symtab *s, struct policy_file *fp)
 {
 	char *key = NULL;
 	struct type_datum *typdatum;
@@ -1519,7 +1519,7 @@ static int type_read(struct policydb *p, struct symtab *s, void *fp)
  * Read a MLS level structure from a policydb binary
  * representation file.
  */
-static int mls_read_level(struct mls_level *lp, void *fp)
+static int mls_read_level(struct mls_level *lp, struct policy_file *fp)
 {
 	__le32 buf[1];
 	int rc;
@@ -1541,7 +1541,7 @@ static int mls_read_level(struct mls_level *lp, void *fp)
 	return 0;
 }
 
-static int user_read(struct policydb *p, struct symtab *s, void *fp)
+static int user_read(struct policydb *p, struct symtab *s, struct policy_file *fp)
 {
 	char *key = NULL;
 	struct user_datum *usrdatum;
@@ -1592,7 +1592,7 @@ static int user_read(struct policydb *p, struct symtab *s, void *fp)
 	return rc;
 }
 
-static int sens_read(struct policydb *p, struct symtab *s, void *fp)
+static int sens_read(struct policydb *p, struct symtab *s, struct policy_file *fp)
 {
 	char *key = NULL;
 	struct level_datum *levdatum;
@@ -1633,7 +1633,7 @@ static int sens_read(struct policydb *p, struct symtab *s, void *fp)
 	return rc;
 }
 
-static int cat_read(struct policydb *p, struct symtab *s, void *fp)
+static int cat_read(struct policydb *p, struct symtab *s, struct policy_file *fp)
 {
 	char *key = NULL;
 	struct cat_datum *catdatum;
@@ -1668,7 +1668,7 @@ static int cat_read(struct policydb *p, struct symtab *s, void *fp)
 
 /* clang-format off */
 static int (*const read_f[SYM_NUM])(struct policydb *p, struct symtab *s,
-				    void *fp) = {
+				    struct policy_file *fp) = {
 	common_read,
 	class_read,
 	role_read,
@@ -1838,7 +1838,7 @@ u32 string_to_av_perm(struct policydb *p, u16 tclass, const char *name)
 	return 1U << (perdatum->value - 1);
 }
 
-static int range_read(struct policydb *p, void *fp)
+static int range_read(struct policydb *p, struct policy_file *fp)
 {
 	struct range_trans *rt = NULL;
 	struct mls_range *r = NULL;
@@ -1915,7 +1915,7 @@ static int range_read(struct policydb *p, void *fp)
 	return rc;
 }
 
-static int filename_trans_read_helper_compat(struct policydb *p, void *fp)
+static int filename_trans_read_helper_compat(struct policydb *p, struct policy_file *fp)
 {
 	struct filename_trans_key key, *ft = NULL;
 	struct filename_trans_datum *last, *datum = NULL;
@@ -2000,7 +2000,7 @@ static int filename_trans_read_helper_compat(struct policydb *p, void *fp)
 	return rc;
 }
 
-static int filename_trans_read_helper(struct policydb *p, void *fp)
+static int filename_trans_read_helper(struct policydb *p, struct policy_file *fp)
 {
 	struct filename_trans_key *ft = NULL;
 	struct filename_trans_datum **dst, *datum, *first = NULL;
@@ -2089,7 +2089,7 @@ static int filename_trans_read_helper(struct policydb *p, void *fp)
 	return rc;
 }
 
-static int filename_trans_read(struct policydb *p, void *fp)
+static int filename_trans_read(struct policydb *p, struct policy_file *fp)
 {
 	u32 nel, i;
 	__le32 buf[1];
@@ -2130,7 +2130,7 @@ static int filename_trans_read(struct policydb *p, void *fp)
 	return 0;
 }
 
-static int genfs_read(struct policydb *p, void *fp)
+static int genfs_read(struct policydb *p, struct policy_file *fp)
 {
 	int rc;
 	u32 i, j, nel, nel2, len, len2;
@@ -2244,7 +2244,7 @@ static int genfs_read(struct policydb *p, void *fp)
 }
 
 static int ocontext_read(struct policydb *p,
-			 const struct policydb_compat_info *info, void *fp)
+			 const struct policydb_compat_info *info, struct policy_file *fp)
 {
 	int rc;
 	unsigned int i;
@@ -2441,7 +2441,7 @@ static int ocontext_read(struct policydb *p,
  * Read the configuration data from a policy database binary
  * representation file into a policy database structure.
  */
-int policydb_read(struct policydb *p, void *fp)
+int policydb_read(struct policydb *p, struct policy_file *fp)
 {
 	struct role_allow *ra, *lra;
 	struct role_trans_key *rtk = NULL;
@@ -2764,7 +2764,7 @@ int policydb_read(struct policydb *p, void *fp)
  * Write a MLS level structure to a policydb binary
  * representation file.
  */
-static int mls_write_level(struct mls_level *l, void *fp)
+static int mls_write_level(struct mls_level *l, struct policy_file *fp)
 {
 	__le32 buf[1];
 	int rc;
@@ -2785,7 +2785,7 @@ static int mls_write_level(struct mls_level *l, void *fp)
  * Write a MLS range structure to a policydb binary
  * representation file.
  */
-static int mls_write_range_helper(struct mls_range *r, void *fp)
+static int mls_write_range_helper(struct mls_range *r, struct policy_file *fp)
 {
 	__le32 buf[3];
 	size_t items;
@@ -2825,7 +2825,7 @@ static int sens_write(void *vkey, void *datum, void *ptr)
 	char *key = vkey;
 	struct level_datum *levdatum = datum;
 	struct policy_data *pd = ptr;
-	void *fp = pd->fp;
+	struct policy_file *fp = pd->fp;
 	__le32 buf[2];
 	size_t len;
 	int rc;
@@ -2853,7 +2853,7 @@ static int cat_write(void *vkey, void *datum, void *ptr)
 	char *key = vkey;
 	struct cat_datum *catdatum = datum;
 	struct policy_data *pd = ptr;
-	void *fp = pd->fp;
+	struct policy_file *fp = pd->fp;
 	__le32 buf[3];
 	size_t len;
 	int rc;
@@ -2878,7 +2878,7 @@ static int role_trans_write_one(void *key, void *datum, void *ptr)
 	struct role_trans_key *rtk = key;
 	struct role_trans_datum *rtd = datum;
 	struct policy_data *pd = ptr;
-	void *fp = pd->fp;
+	struct policy_file *fp = pd->fp;
 	struct policydb *p = pd->p;
 	__le32 buf[3];
 	int rc;
@@ -2898,7 +2898,7 @@ static int role_trans_write_one(void *key, void *datum, void *ptr)
 	return 0;
 }
 
-static int role_trans_write(struct policydb *p, void *fp)
+static int role_trans_write(struct policydb *p, struct policy_file *fp)
 {
 	struct policy_data pd = { .p = p, .fp = fp };
 	__le32 buf[1];
@@ -2912,7 +2912,7 @@ static int role_trans_write(struct policydb *p, void *fp)
 	return hashtab_map(&p->role_tr, role_trans_write_one, &pd);
 }
 
-static int role_allow_write(struct role_allow *r, void *fp)
+static int role_allow_write(struct role_allow *r, struct policy_file *fp)
 {
 	struct role_allow *ra;
 	__le32 buf[2];
@@ -2940,7 +2940,7 @@ static int role_allow_write(struct role_allow *r, void *fp)
  * Write a security context structure
  * to a policydb binary representation file.
  */
-static int context_write(struct policydb *p, struct context *c, void *fp)
+static int context_write(struct policydb *p, struct context *c, struct policy_file *fp)
 {
 	int rc;
 	__le32 buf[3];
@@ -2993,7 +2993,7 @@ static int common_write(void *vkey, void *datum, void *ptr)
 	char *key = vkey;
 	struct common_datum *comdatum = datum;
 	struct policy_data *pd = ptr;
-	void *fp = pd->fp;
+	struct policy_file *fp = pd->fp;
 	__le32 buf[4];
 	size_t len;
 	int rc;
@@ -3018,7 +3018,7 @@ static int common_write(void *vkey, void *datum, void *ptr)
 	return 0;
 }
 
-static int type_set_write(struct type_set *t, void *fp)
+static int type_set_write(struct type_set *t, struct policy_file *fp)
 {
 	int rc;
 	__le32 buf[1];
@@ -3037,7 +3037,7 @@ static int type_set_write(struct type_set *t, void *fp)
 }
 
 static int write_cons_helper(struct policydb *p, struct constraint_node *node,
-			     void *fp)
+			     struct policy_file *fp)
 {
 	struct constraint_node *c;
 	struct constraint_expr *e;
@@ -3088,7 +3088,7 @@ static int class_write(void *vkey, void *datum, void *ptr)
 	char *key = vkey;
 	struct class_datum *cladatum = datum;
 	struct policy_data *pd = ptr;
-	void *fp = pd->fp;
+	struct policy_file *fp = pd->fp;
 	struct policydb *p = pd->p;
 	struct constraint_node *c;
 	__le32 buf[6];
@@ -3173,7 +3173,7 @@ static int role_write(void *vkey, void *datum, void *ptr)
 	char *key = vkey;
 	struct role_datum *role = datum;
 	struct policy_data *pd = ptr;
-	void *fp = pd->fp;
+	struct policy_file *fp = pd->fp;
 	struct policydb *p = pd->p;
 	__le32 buf[3];
 	size_t items, len;
@@ -3213,7 +3213,7 @@ static int type_write(void *vkey, void *datum, void *ptr)
 	struct type_datum *typdatum = datum;
 	struct policy_data *pd = ptr;
 	struct policydb *p = pd->p;
-	void *fp = pd->fp;
+	struct policy_file *fp = pd->fp;
 	__le32 buf[4];
 	int rc;
 	size_t items, len;
@@ -3254,7 +3254,7 @@ static int user_write(void *vkey, void *datum, void *ptr)
 	struct user_datum *usrdatum = datum;
 	struct policy_data *pd = ptr;
 	struct policydb *p = pd->p;
-	void *fp = pd->fp;
+	struct policy_file *fp = pd->fp;
 	__le32 buf[3];
 	size_t items, len;
 	int rc;
@@ -3439,7 +3439,7 @@ static int ocontext_write(struct policydb *p,
 	return 0;
 }
 
-static int genfs_write(struct policydb *p, void *fp)
+static int genfs_write(struct policydb *p, struct policy_file *fp)
 {
 	struct genfs *genfs;
 	struct ocontext *c;
@@ -3497,7 +3497,7 @@ static int range_write_helper(void *key, void *data, void *ptr)
 	struct range_trans *rt = key;
 	struct mls_range *r = data;
 	struct policy_data *pd = ptr;
-	void *fp = pd->fp;
+	struct policy_file *fp = pd->fp;
 	struct policydb *p = pd->p;
 	int rc;
 
@@ -3519,7 +3519,7 @@ static int range_write_helper(void *key, void *data, void *ptr)
 	return 0;
 }
 
-static int range_write(struct policydb *p, void *fp)
+static int range_write(struct policydb *p, struct policy_file *fp)
 {
 	__le32 buf[1];
 	int rc;
@@ -3546,7 +3546,7 @@ static int filename_write_helper_compat(void *key, void *data, void *ptr)
 	struct filename_trans_key *ft = key;
 	struct filename_trans_datum *datum = data;
 	struct ebitmap_node *node;
-	void *fp = ptr;
+	struct policy_file *fp = ptr;
 	__le32 buf[4];
 	int rc;
 	u32 bit, len = strlen(ft->name);
@@ -3583,7 +3583,7 @@ static int filename_write_helper(void *key, void *data, void *ptr)
 {
 	struct filename_trans_key *ft = key;
 	struct filename_trans_datum *datum;
-	void *fp = ptr;
+	struct policy_file *fp = ptr;
 	__le32 buf[3];
 	int rc;
 	u32 ndatum, len = strlen(ft->name);
@@ -3628,7 +3628,7 @@ static int filename_write_helper(void *key, void *data, void *ptr)
 	return 0;
 }
 
-static int filename_trans_write(struct policydb *p, void *fp)
+static int filename_trans_write(struct policydb *p, struct policy_file *fp)
 {
 	__le32 buf[1];
 	int rc;
@@ -3660,7 +3660,7 @@ static int filename_trans_write(struct policydb *p, void *fp)
  * structure to a policy database binary representation
  * file.
  */
-int policydb_write(struct policydb *p, void *fp)
+int policydb_write(struct policydb *p, struct policy_file *fp)
 {
 	unsigned int num_syms;
 	int rc;
diff --git a/security/selinux/ss/policydb.h b/security/selinux/ss/policydb.h
index 5c11069121d3..985f319e2266 100644
--- a/security/selinux/ss/policydb.h
+++ b/security/selinux/ss/policydb.h
@@ -318,8 +318,14 @@ extern int policydb_context_isvalid(struct policydb *p, struct context *c);
 extern int policydb_class_isvalid(struct policydb *p, unsigned int class);
 extern int policydb_type_isvalid(struct policydb *p, unsigned int type);
 extern int policydb_role_isvalid(struct policydb *p, unsigned int role);
-extern int policydb_read(struct policydb *p, void *fp);
-extern int policydb_write(struct policydb *p, void *fp);
+
+struct policy_file {
+	char *data;
+	size_t len;
+};
+
+extern int policydb_read(struct policydb *p, struct policy_file *fp);
+extern int policydb_write(struct policydb *p, struct policy_file *fp);
 
 extern struct filename_trans_datum *
 policydb_filenametr_search(struct policydb *p, struct filename_trans_key *key);
@@ -342,14 +348,9 @@ policydb_roletr_search(struct policydb *p, struct role_trans_key *key);
 #define POLICYDB_MAGIC	SELINUX_MAGIC
 #define POLICYDB_STRING "SE Linux"
 
-struct policy_file {
-	char *data;
-	size_t len;
-};
-
 struct policy_data {
 	struct policydb *p;
-	void *fp;
+	struct policy_file *fp;
 };
 
 static inline int next_entry(void *buf, struct policy_file *fp, size_t bytes)
-- 
2.45.2

[RFC PATCH 08/22] selinux: avoid unnecessary indirection in struct level_datum

[Christian Göttsche]

From: Christian Göttsche <cgzones@googlemail.com>

Store the owned member of type struct mls_level directly in the parent
struct instead of an extra heap allocation.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 security/selinux/ss/mls.c      |  6 +++---
 security/selinux/ss/policydb.c | 22 ++++++++--------------
 security/selinux/ss/policydb.h |  2 +-
 3 files changed, 12 insertions(+), 18 deletions(-)

diff --git a/security/selinux/ss/mls.c b/security/selinux/ss/mls.c
index 989c809d310d..a6e49269f535 100644
--- a/security/selinux/ss/mls.c
+++ b/security/selinux/ss/mls.c
@@ -171,7 +171,7 @@ int mls_level_isvalid(struct policydb *p, struct mls_level *l)
 	 * levdatum->level->cat and no bit in l->cat is larger than
 	 * p->p_cats.nprim.
 	 */
-	return ebitmap_contains(&levdatum->level->cat, &l->cat,
+	return ebitmap_contains(&levdatum->level.cat, &l->cat,
 				p->p_cats.nprim);
 }
 
@@ -289,7 +289,7 @@ int mls_context_to_sid(struct policydb *pol, char oldc, char *scontext,
 		levdatum = symtab_search(&pol->p_levels, sensitivity);
 		if (!levdatum)
 			return -EINVAL;
-		context->range.level[l].sens = levdatum->level->sens;
+		context->range.level[l].sens = levdatum->level.sens;
 
 		/* Extract category set. */
 		while (next_cat != NULL) {
@@ -456,7 +456,7 @@ int mls_convert_context(struct policydb *oldp, struct policydb *newp,
 
 		if (!levdatum)
 			return -EINVAL;
-		newc->range.level[l].sens = levdatum->level->sens;
+		newc->range.level[l].sens = levdatum->level.sens;
 
 		ebitmap_for_each_positive_bit(&oldc->range.level[l].cat, node,
 					      i)
diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index e94ecb81c6d3..5e9a54645b80 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -298,9 +298,7 @@ static int sens_destroy(void *key, void *datum, void *p)
 	kfree(key);
 	if (datum) {
 		levdatum = datum;
-		if (levdatum->level)
-			ebitmap_destroy(&levdatum->level->cat);
-		kfree(levdatum->level);
+		ebitmap_destroy(&levdatum->level.cat);
 	}
 	kfree(datum);
 	return 0;
@@ -632,11 +630,11 @@ static int sens_index(void *key, void *datum, void *datap)
 	p = datap;
 
 	if (!levdatum->isalias) {
-		if (!levdatum->level->sens ||
-		    levdatum->level->sens > p->p_levels.nprim)
+		if (!levdatum->level.sens ||
+		    levdatum->level.sens > p->p_levels.nprim)
 			return -EINVAL;
 
-		p->sym_val_to_name[SYM_LEVELS][levdatum->level->sens - 1] = key;
+		p->sym_val_to_name[SYM_LEVELS][levdatum->level.sens - 1] = key;
 	}
 
 	return 0;
@@ -1615,12 +1613,7 @@ static int sens_read(struct policydb *p, struct symtab *s, struct policy_file *f
 	if (rc)
 		goto bad;
 
-	rc = -ENOMEM;
-	levdatum->level = kmalloc(sizeof(*levdatum->level), GFP_KERNEL);
-	if (!levdatum->level)
-		goto bad;
-
-	rc = mls_read_level(levdatum->level, fp);
+	rc = mls_read_level(&levdatum->level, fp);
 	if (rc)
 		goto bad;
 
@@ -2841,7 +2834,7 @@ static int sens_write(void *vkey, void *datum, void *ptr)
 	if (rc)
 		return rc;
 
-	rc = mls_write_level(levdatum->level, fp);
+	rc = mls_write_level(&levdatum->level, fp);
 	if (rc)
 		return rc;
 
@@ -3303,7 +3296,8 @@ static int (*const write_f[SYM_NUM])(void *key, void *datum, void *datap) = {
 /* clang-format on */
 
 static int ocontext_write(struct policydb *p,
-			  const struct policydb_compat_info *info, void *fp)
+			  const struct policydb_compat_info *info,
+			  struct policy_file *fp)
 {
 	unsigned int i, j;
 	int rc;
diff --git a/security/selinux/ss/policydb.h b/security/selinux/ss/policydb.h
index 985f319e2266..aa37f4eb6244 100644
--- a/security/selinux/ss/policydb.h
+++ b/security/selinux/ss/policydb.h
@@ -126,7 +126,7 @@ struct user_datum {
 
 /* Sensitivity attributes */
 struct level_datum {
-	struct mls_level *level; /* sensitivity and associated categories */
+	struct mls_level level; /* sensitivity and associated categories */
 	unsigned char isalias; /* is this sensitivity an alias for another? */
 };
 
-- 
2.45.2

[RFC PATCH 09/22] selinux: make use of str_read()

[Christian Göttsche]

From: Christian Göttsche <cgzones@googlemail.com>

Simplify the call sites, and enable future string validation in a single
place.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 security/selinux/ss/conditional.c | 10 ++--------
 security/selinux/ss/policydb.c    | 22 ++++++++--------------
 security/selinux/ss/policydb.h    |  2 ++
 3 files changed, 12 insertions(+), 22 deletions(-)

diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
index 244dc8279113..f8a21d10ff92 100644
--- a/security/selinux/ss/conditional.c
+++ b/security/selinux/ss/conditional.c
@@ -230,17 +230,11 @@ int cond_read_bool(struct policydb *p, struct symtab *s, struct policy_file *fp)
 		goto err;
 
 	len = le32_to_cpu(buf[2]);
-	if (((len == 0) || (len == (u32)-1)))
-		goto err;
 
-	rc = -ENOMEM;
-	key = kmalloc(len + 1, GFP_KERNEL);
-	if (!key)
-		goto err;
-	rc = next_entry(key, fp, len);
+	rc = str_read(&key, GFP_KERNEL, fp, len);
 	if (rc)
 		goto err;
-	key[len] = '\0';
+
 	rc = symtab_insert(s, key, booldatum);
 	if (rc)
 		goto err;
diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index 5e9a54645b80..1de48cccd61a 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -1090,7 +1090,7 @@ static int context_read_and_validate(struct context *c, struct policydb *p,
  * binary representation file.
  */
 
-static int str_read(char **strp, gfp_t flags, struct policy_file *fp, u32 len)
+int str_read(char **strp, gfp_t flags, struct policy_file *fp, u32 len)
 {
 	int rc;
 	char *str;
@@ -2470,24 +2470,18 @@ int policydb_read(struct policydb *p, struct policy_file *fp)
 		goto bad;
 	}
 
-	rc = -ENOMEM;
-	policydb_str = kmalloc(len + 1, GFP_KERNEL);
-	if (!policydb_str) {
-		pr_err("SELinux:  unable to allocate memory for policydb "
-		       "string of length %d\n",
-		       len);
-		goto bad;
-	}
-
-	rc = next_entry(policydb_str, fp, len);
+	rc = str_read(&policydb_str, GFP_KERNEL, fp, len);
 	if (rc) {
-		pr_err("SELinux:  truncated policydb string identifier\n");
-		kfree(policydb_str);
+		if (rc == -ENOMEM) {
+			pr_err("SELinux:  unable to allocate memory for policydb string of length %d\n",
+			       len);
+		} else {
+			pr_err("SELinux:  truncated policydb string identifier\n");
+		}
 		goto bad;
 	}
 
 	rc = -EINVAL;
-	policydb_str[len] = '\0';
 	if (strcmp(policydb_str, POLICYDB_STRING)) {
 		pr_err("SELinux:  policydb string %s does not match "
 		       "my string %s\n",
diff --git a/security/selinux/ss/policydb.h b/security/selinux/ss/policydb.h
index aa37f4eb6244..9e7ee53c996b 100644
--- a/security/selinux/ss/policydb.h
+++ b/security/selinux/ss/policydb.h
@@ -387,6 +387,8 @@ static inline char *sym_name(struct policydb *p, unsigned int sym_num,
 	return p->sym_val_to_name[sym_num][element_nr];
 }
 
+extern int str_read(char **strp, gfp_t flags, struct policy_file *fp, u32 len);
+
 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);
 
-- 
2.45.2

[RFC PATCH 10/22] selinux: use u16 for security classes

[Christian Göttsche]

From: Christian Göttsche <cgzones@googlemail.com>

Security class identifiers are limited to 2^16, thus use the appropriate
type u16 consistently.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 security/selinux/ss/policydb.c | 52 +++++++++++++++++++++++++---------
 security/selinux/ss/policydb.h | 10 +++----
 security/selinux/ss/services.c |  2 +-
 3 files changed, 45 insertions(+), 19 deletions(-)

diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index 1de48cccd61a..493969c4e4fd 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -922,7 +922,7 @@ int policydb_load_isids(struct policydb *p, struct sidtab *s)
 	return 0;
 }
 
-int policydb_class_isvalid(struct policydb *p, unsigned int class)
+int policydb_class_isvalid(struct policydb *p, u16 class)
 {
 	if (!class || class > p->p_classes.nprim)
 		return 0;
@@ -1316,7 +1316,7 @@ static int class_read(struct policydb *p, struct symtab *s, struct policy_file *
 	char *key = NULL;
 	struct class_datum *cladatum;
 	__le32 buf[6];
-	u32 i, len, len2, ncons, nel;
+	u32 i, len, len2, ncons, nel, val;
 	int rc;
 
 	cladatum = kzalloc(sizeof(*cladatum), GFP_KERNEL);
@@ -1329,9 +1329,14 @@ static int class_read(struct policydb *p, struct symtab *s, struct policy_file *
 
 	len = le32_to_cpu(buf[0]);
 	len2 = le32_to_cpu(buf[1]);
-	cladatum->value = le32_to_cpu(buf[2]);
 	nel = le32_to_cpu(buf[4]);
 
+	val = le32_to_cpu(buf[2]);
+	rc = -EINVAL;
+	if (val >= U16_MAX)
+		goto bad;
+	cladatum->value = val;
+
 	rc = symtab_init(&cladatum->permissions, nel);
 	if (rc)
 		goto bad;
@@ -1837,7 +1842,7 @@ static int range_read(struct policydb *p, struct policy_file *fp)
 	struct mls_range *r = NULL;
 	int rc;
 	__le32 buf[2];
-	u32 i, nel;
+	u32 i, nel, val;
 
 	if (p->policyvers < POLICYDB_VERSION_MLS)
 		return 0;
@@ -1868,7 +1873,11 @@ static int range_read(struct policydb *p, struct policy_file *fp)
 			rc = next_entry(buf, fp, sizeof(u32));
 			if (rc)
 				goto out;
-			rt->target_class = le32_to_cpu(buf[0]);
+			rc = -EINVAL;
+			val = le32_to_cpu(buf[0]);
+			if (val >= U16_MAX)
+				goto out;
+			rt->target_class = val;
 		} else
 			rt->target_class = p->process_class;
 
@@ -1913,7 +1922,7 @@ static int filename_trans_read_helper_compat(struct policydb *p, struct policy_f
 	struct filename_trans_key key, *ft = NULL;
 	struct filename_trans_datum *last, *datum = NULL;
 	char *name = NULL;
-	u32 len, stype, otype;
+	u32 len, stype, otype, val;
 	__le32 buf[4];
 	int rc;
 
@@ -1934,7 +1943,11 @@ static int filename_trans_read_helper_compat(struct policydb *p, struct policy_f
 
 	stype = le32_to_cpu(buf[0]);
 	key.ttype = le32_to_cpu(buf[1]);
-	key.tclass = le32_to_cpu(buf[2]);
+	val = le32_to_cpu(buf[2]);
+	rc = -EINVAL;
+	if (val > U16_MAX || !policydb_class_isvalid(p, val))
+		goto out;
+	key.tclass = val;
 	key.name = name;
 
 	otype = le32_to_cpu(buf[3]);
@@ -1998,7 +2011,8 @@ static int filename_trans_read_helper(struct policydb *p, struct policy_file *fp
 	struct filename_trans_key *ft = NULL;
 	struct filename_trans_datum **dst, *datum, *first = NULL;
 	char *name = NULL;
-	u32 len, ttype, tclass, ndatum, i;
+	u32 len, ttype, ndatum, i, val;
+	u16 tclass;
 	__le32 buf[3];
 	int rc;
 
@@ -2018,7 +2032,11 @@ static int filename_trans_read_helper(struct policydb *p, struct policy_file *fp
 		goto out;
 
 	ttype = le32_to_cpu(buf[0]);
-	tclass = le32_to_cpu(buf[1]);
+	val = le32_to_cpu(buf[1]);
+	rc = -EINVAL;
+	if (val > U16_MAX || !policydb_class_isvalid(p, val))
+		goto out;
+	tclass = val;
 
 	ndatum = le32_to_cpu(buf[2]);
 	if (ndatum == 0) {
@@ -2126,7 +2144,7 @@ static int filename_trans_read(struct policydb *p, struct policy_file *fp)
 static int genfs_read(struct policydb *p, struct policy_file *fp)
 {
 	int rc;
-	u32 i, j, nel, nel2, len, len2;
+	u32 i, j, nel, nel2, len, len2, val;
 	__le32 buf[1];
 	struct ocontext *l, *c;
 	struct ocontext *newc = NULL;
@@ -2196,7 +2214,11 @@ static int genfs_read(struct policydb *p, struct policy_file *fp)
 			if (rc)
 				goto out;
 
-			newc->v.sclass = le32_to_cpu(buf[0]);
+			rc = -EINVAL;
+			val = le32_to_cpu(buf[0]);
+			if (val >= U16_MAX)
+				goto out;
+			newc->v.sclass = val;
 			rc = context_read_and_validate(&newc->context[0], p,
 						       fp);
 			if (rc)
@@ -2441,7 +2463,7 @@ int policydb_read(struct policydb *p, struct policy_file *fp)
 	struct role_trans_datum *rtd = NULL;
 	int rc;
 	__le32 buf[4];
-	u32 i, j, len, nprim, nel, perm;
+	u32 i, j, len, nprim, nel, perm, val;
 
 	char *policydb_str;
 	const struct policydb_compat_info *info;
@@ -2627,7 +2649,11 @@ int policydb_read(struct policydb *p, struct policy_file *fp)
 			rc = next_entry(buf, fp, sizeof(u32));
 			if (rc)
 				goto bad;
-			rtk->tclass = le32_to_cpu(buf[0]);
+			rc = -EINVAL;
+			val = le32_to_cpu(buf[0]);
+			if (val >= U16_MAX)
+				goto bad;
+			rtk->tclass = val;
 		} else
 			rtk->tclass = p->process_class;
 
diff --git a/security/selinux/ss/policydb.h b/security/selinux/ss/policydb.h
index 9e7ee53c996b..6d685a3fc080 100644
--- a/security/selinux/ss/policydb.h
+++ b/security/selinux/ss/policydb.h
@@ -48,7 +48,7 @@ struct common_datum {
 
 /* Class attributes */
 struct class_datum {
-	u32 value; /* class value */
+	u16 value; /* class value */
 	char *comkey; /* common name */
 	struct common_datum *comdatum; /* common datum */
 	struct symtab permissions; /* class-specific permission symbol table */
@@ -82,7 +82,7 @@ struct role_datum {
 struct role_trans_key {
 	u32 role; /* current role */
 	u32 type; /* program executable type, or new object type */
-	u32 tclass; /* process class, or new object class */
+	u16 tclass; /* process class, or new object class */
 };
 
 struct role_trans_datum {
@@ -139,7 +139,7 @@ struct cat_datum {
 struct range_trans {
 	u32 source_type;
 	u32 target_type;
-	u32 target_class;
+	u16 target_class;
 };
 
 /* Boolean data type */
@@ -195,7 +195,7 @@ struct ocontext {
 		} ibendport;
 	} u;
 	union {
-		u32 sclass; /* security class for genfs */
+		u16 sclass; /* security class for genfs */
 		u32 behavior; /* labeling behavior for fs_use */
 	} v;
 	struct context context[2]; /* security context(s) */
@@ -315,7 +315,7 @@ struct policydb {
 extern void policydb_destroy(struct policydb *p);
 extern int policydb_load_isids(struct policydb *p, struct sidtab *s);
 extern int policydb_context_isvalid(struct policydb *p, struct context *c);
-extern int policydb_class_isvalid(struct policydb *p, unsigned int class);
+extern int policydb_class_isvalid(struct policydb *p, u16 class);
 extern int policydb_type_isvalid(struct policydb *p, unsigned int type);
 extern int policydb_role_isvalid(struct policydb *p, unsigned int role);
 
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 2b155f22a0f4..835b2ac49562 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -3346,7 +3346,7 @@ static int get_classes_callback(void *k, void *d, void *args)
 {
 	struct class_datum *datum = d;
 	char *name = k, **classes = args;
-	u32 value = datum->value - 1;
+	u16 value = datum->value - 1;
 
 	classes[value] = kstrdup(name, GFP_ATOMIC);
 	if (!classes[value])
-- 
2.45.2

[RFC PATCH 11/22] selinux: more strict policy parsing

[Christian Göttsche]

From: Christian Göttsche <cgzones@googlemail.com>

Be more strict during parsing of policies and reject invalid values.

Add some error messages in the case of policy parse failures, to
enhance debugging, either on a malformed policy or a too strict check.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 security/selinux/ss/avtab.c       |  46 +++++++--
 security/selinux/ss/avtab.h       |   1 +
 security/selinux/ss/conditional.c |  22 ++---
 security/selinux/ss/constraint.h  |   2 +-
 security/selinux/ss/policydb.c    | 157 ++++++++++++++++++++++++------
 security/selinux/ss/policydb.h    |  10 +-
 security/selinux/ss/services.c    |   2 -
 7 files changed, 184 insertions(+), 56 deletions(-)

diff --git a/security/selinux/ss/avtab.c b/security/selinux/ss/avtab.c
index 23210faaa046..6122c1c7e951 100644
--- a/security/selinux/ss/avtab.c
+++ b/security/selinux/ss/avtab.c
@@ -337,6 +337,7 @@ static const uint16_t spec_order[] = {
 /* clang-format on */
 
 int avtab_read_item(struct avtab *a, struct policy_file *fp, struct policydb *pol,
+		    bool conditional,
 		    int (*insertf)(struct avtab *a, const struct avtab_key *k,
 				   const struct avtab_datum *d, void *p),
 		    void *p)
@@ -349,7 +350,7 @@ int avtab_read_item(struct avtab *a, struct policy_file *fp, struct policydb *po
 	struct avtab_extended_perms xperms;
 	__le32 buf32[ARRAY_SIZE(xperms.perms.p)];
 	int rc;
-	unsigned int set, vers = pol->policyvers;
+	unsigned int vers = pol->policyvers;
 
 	memset(&key, 0, sizeof(struct avtab_key));
 	memset(&datum, 0, sizeof(struct avtab_datum));
@@ -361,8 +362,8 @@ int avtab_read_item(struct avtab *a, struct policy_file *fp, struct policydb *po
 			return rc;
 		}
 		items2 = le32_to_cpu(buf32[0]);
-		if (items2 > ARRAY_SIZE(buf32)) {
-			pr_err("SELinux: avtab: entry overflow\n");
+		if (items2 < 5 || items2 > ARRAY_SIZE(buf32)) {
+			pr_err("SELinux: avtab: invalid item count\n");
 			return -EINVAL;
 		}
 		rc = next_entry(buf32, fp, sizeof(u32) * items2);
@@ -391,6 +392,13 @@ int avtab_read_item(struct avtab *a, struct policy_file *fp, struct policydb *po
 			return -EINVAL;
 		}
 
+		if (!policydb_type_isvalid(pol, key.source_type) ||
+		    !policydb_type_isvalid(pol, key.target_type) ||
+		    !policydb_class_isvalid(pol, key.target_class)) {
+			pr_err("SELinux: avtab: invalid type or class\n");
+			return -EINVAL;
+		}
+
 		val = le32_to_cpu(buf32[items++]);
 		enabled = (val & AVTAB_ENABLED_OLD) ? AVTAB_ENABLED : 0;
 
@@ -409,6 +417,11 @@ int avtab_read_item(struct avtab *a, struct policy_file *fp, struct policydb *po
 
 		for (i = 0; i < ARRAY_SIZE(spec_order); i++) {
 			if (val & spec_order[i]) {
+				if (items >= items2) {
+					pr_err("SELinux: avtab: entry has too many items (%d/%d)\n",
+					       items + 1, items2);
+					return -EINVAL;
+				}
 				key.specified = spec_order[i] | enabled;
 				datum.u.data = le32_to_cpu(buf32[items++]);
 				rc = insertf(a, &key, &datum, p);
@@ -444,9 +457,13 @@ int avtab_read_item(struct avtab *a, struct policy_file *fp, struct policydb *po
 		return -EINVAL;
 	}
 
-	set = hweight16(key.specified & (AVTAB_XPERMS | AVTAB_TYPE | AVTAB_AV));
-	if (!set || set > 1) {
-		pr_err("SELinux:  avtab:  more than one specifier\n");
+	if (hweight16(key.specified & ~AVTAB_ENABLED) != 1) {
+		pr_err("SELinux:  avtab:  not exactly one specifier\n");
+		return -EINVAL;
+	}
+
+	if (key.specified & ~(AVTAB_AV | AVTAB_TYPE | AVTAB_XPERMS | AVTAB_ENABLED)) {
+		pr_err("SELinux:  avtab:  invalid specifier\n");
 		return -EINVAL;
 	}
 
@@ -457,6 +474,12 @@ int avtab_read_item(struct avtab *a, struct policy_file *fp, struct policydb *po
 		       "was specified\n",
 		       vers);
 		return -EINVAL;
+	} else if ((key.specified & AVTAB_XPERMS) && conditional) {
+		pr_err("SELinux:  avtab:  policy version %u does not "
+		       "support extended permissions rules in conditional "
+		       "policies and one was specified\n",
+		       vers);
+		return -EINVAL;
 	} else if (key.specified & AVTAB_XPERMS) {
 		memset(&xperms, 0, sizeof(struct avtab_extended_perms));
 		rc = next_entry(&xperms.specified, fp, sizeof(u8));
@@ -464,6 +487,15 @@ int avtab_read_item(struct avtab *a, struct policy_file *fp, struct policydb *po
 			pr_err("SELinux: avtab: truncated entry\n");
 			return rc;
 		}
+		switch (xperms.specified) {
+		case AVTAB_XPERMS_IOCTLFUNCTION:
+		case AVTAB_XPERMS_IOCTLDRIVER:
+		case AVTAB_XPERMS_NLMSG:
+			break;
+		default:
+			pr_err("SELinux: avtab: invalid xperm specifier %#x\n", xperms.specified);
+			return -EINVAL;
+		}
 		rc = next_entry(&xperms.driver, fp, sizeof(u8));
 		if (rc) {
 			pr_err("SELinux: avtab: truncated entry\n");
@@ -523,7 +555,7 @@ int avtab_read(struct avtab *a, struct policy_file *fp, struct policydb *pol)
 		goto bad;
 
 	for (i = 0; i < nel; i++) {
-		rc = avtab_read_item(a, fp, pol, avtab_insertf, NULL);
+		rc = avtab_read_item(a, fp, pol, false, avtab_insertf, NULL);
 		if (rc) {
 			if (rc == -ENOMEM)
 				pr_err("SELinux: avtab: out of memory\n");
diff --git a/security/selinux/ss/avtab.h b/security/selinux/ss/avtab.h
index c671b6e909d4..b5ce072d0f1c 100644
--- a/security/selinux/ss/avtab.h
+++ b/security/selinux/ss/avtab.h
@@ -107,6 +107,7 @@ static inline void avtab_hash_eval(struct avtab *h, const char *tag)
 struct policydb;
 struct policy_file;
 int avtab_read_item(struct avtab *a, struct policy_file *fp, struct policydb *pol,
+		    bool conditional,
 		    int (*insert)(struct avtab *a, const struct avtab_key *k,
 				  const struct avtab_datum *d, void *p),
 		    void *p);
diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
index f8a21d10ff92..f85d22952d5a 100644
--- a/security/selinux/ss/conditional.c
+++ b/security/selinux/ss/conditional.c
@@ -199,19 +199,12 @@ int cond_index_bool(void *key, void *datum, void *datap)
 	return 0;
 }
 
-static int bool_isvalid(struct cond_bool_datum *b)
-{
-	if (!(b->state == 0 || b->state == 1))
-		return 0;
-	return 1;
-}
-
 int cond_read_bool(struct policydb *p, struct symtab *s, struct policy_file *fp)
 {
 	char *key = NULL;
 	struct cond_bool_datum *booldatum;
 	__le32 buf[3];
-	u32 len;
+	u32 len, val;
 	int rc;
 
 	booldatum = kzalloc(sizeof(*booldatum), GFP_KERNEL);
@@ -223,11 +216,12 @@ int cond_read_bool(struct policydb *p, struct symtab *s, struct policy_file *fp)
 		goto err;
 
 	booldatum->value = le32_to_cpu(buf[0]);
-	booldatum->state = le32_to_cpu(buf[1]);
+	val = le32_to_cpu(buf[1]);
 
 	rc = -EINVAL;
-	if (!bool_isvalid(booldatum))
+	if (val != 0 && val != 1)
 		goto err;
+	booldatum->state = (int)val;
 
 	len = le32_to_cpu(buf[2]);
 
@@ -241,6 +235,7 @@ int cond_read_bool(struct policydb *p, struct symtab *s, struct policy_file *fp)
 
 	return 0;
 err:
+	pr_err("SELinux: conditional: failed to read boolean\n");
 	cond_destroy_bool(key, booldatum, NULL);
 	return rc;
 }
@@ -342,8 +337,8 @@ static int cond_read_av_list(struct policydb *p, struct policy_file *fp,
 	data.other = other;
 	for (i = 0; i < len; i++) {
 		data.dst = &list->nodes[i];
-		rc = avtab_read_item(&p->te_cond_avtab, fp, p, cond_insertf,
-				     &data);
+		rc = avtab_read_item(&p->te_cond_avtab, fp, p, true,
+				     cond_insertf, &data);
 		if (rc) {
 			kfree(list->nodes);
 			list->nodes = NULL;
@@ -362,7 +357,8 @@ static int expr_node_isvalid(struct policydb *p, struct cond_expr_node *expr)
 		return 0;
 	}
 
-	if (expr->boolean > p->p_bools.nprim) {
+	if (expr->expr_type == COND_BOOL &&
+	    (expr->boolean == 0 || expr->boolean > p->p_bools.nprim)) {
 		pr_err("SELinux: conditional expressions uses unknown bool.\n");
 		return 0;
 	}
diff --git a/security/selinux/ss/constraint.h b/security/selinux/ss/constraint.h
index 203033cfad67..26ffdb8c1c29 100644
--- a/security/selinux/ss/constraint.h
+++ b/security/selinux/ss/constraint.h
@@ -50,7 +50,7 @@ struct constraint_expr {
 	u32 op; /* operator */
 
 	struct ebitmap names; /* names */
-	struct type_set *type_names;
+	struct type_set *type_names; /* (unused) */
 
 	struct constraint_expr *next; /* next expression */
 };
diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index 493969c4e4fd..1157e66b4664 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -629,13 +629,11 @@ static int sens_index(void *key, void *datum, void *datap)
 	levdatum = datum;
 	p = datap;
 
-	if (!levdatum->isalias) {
-		if (!levdatum->level.sens ||
-		    levdatum->level.sens > p->p_levels.nprim)
-			return -EINVAL;
+	if (!levdatum->level.sens || levdatum->level.sens > p->p_levels.nprim)
+		return -EINVAL;
 
+	if (!levdatum->isalias)
 		p->sym_val_to_name[SYM_LEVELS][levdatum->level.sens - 1] = key;
-	}
 
 	return 0;
 }
@@ -648,12 +646,11 @@ static int cat_index(void *key, void *datum, void *datap)
 	catdatum = datum;
 	p = datap;
 
-	if (!catdatum->isalias) {
-		if (!catdatum->value || catdatum->value > p->p_cats.nprim)
-			return -EINVAL;
+	if (!catdatum->value || catdatum->value > p->p_cats.nprim)
+		return -EINVAL;
 
+	if (!catdatum->isalias)
 		p->sym_val_to_name[SYM_CATS][catdatum->value - 1] = key;
-	}
 
 	return 0;
 }
@@ -1131,6 +1128,9 @@ static int perm_read(struct policydb *p, struct symtab *s, struct policy_file *f
 
 	len = le32_to_cpu(buf[0]);
 	perdatum->value = le32_to_cpu(buf[1]);
+	rc = -EINVAL;
+	if (perdatum->value < 1 || perdatum->value > 32)
+		goto bad;
 
 	rc = str_read(&key, GFP_KERNEL, fp, len);
 	if (rc)
@@ -1165,6 +1165,9 @@ static int common_read(struct policydb *p, struct symtab *s, struct policy_file
 	len = le32_to_cpu(buf[0]);
 	comdatum->value = le32_to_cpu(buf[1]);
 	nel = le32_to_cpu(buf[3]);
+	rc = -EINVAL;
+	if (nel > 32)
+		goto bad;
 
 	rc = symtab_init(&comdatum->permissions, nel);
 	if (rc)
@@ -1330,6 +1333,9 @@ static int class_read(struct policydb *p, struct symtab *s, struct policy_file *
 	len = le32_to_cpu(buf[0]);
 	len2 = le32_to_cpu(buf[1]);
 	nel = le32_to_cpu(buf[4]);
+	rc = -EINVAL;
+	if (nel > 32)
+		goto bad;
 
 	val = le32_to_cpu(buf[2]);
 	rc = -EINVAL;
@@ -1391,16 +1397,59 @@ static int class_read(struct policydb *p, struct symtab *s, struct policy_file *
 		if (rc)
 			goto bad;
 
-		cladatum->default_user = le32_to_cpu(buf[0]);
-		cladatum->default_role = le32_to_cpu(buf[1]);
-		cladatum->default_range = le32_to_cpu(buf[2]);
+		rc = -EINVAL;
+		val = le32_to_cpu(buf[0]);
+		switch (val) {
+		case 0:
+		case DEFAULT_SOURCE:
+		case DEFAULT_TARGET:
+			cladatum->default_user = val;
+			break;
+		default:
+			goto bad;
+		}
+		val = le32_to_cpu(buf[1]);
+		switch (val) {
+		case 0:
+		case DEFAULT_SOURCE:
+		case DEFAULT_TARGET:
+			cladatum->default_role = val;
+			break;
+		default:
+			goto bad;
+		}
+		val = le32_to_cpu(buf[2]);
+		switch (val) {
+		case 0:
+		case DEFAULT_SOURCE_LOW:
+		case DEFAULT_SOURCE_HIGH:
+		case DEFAULT_SOURCE_LOW_HIGH:
+		case DEFAULT_TARGET_LOW:
+		case DEFAULT_TARGET_HIGH:
+		case DEFAULT_TARGET_LOW_HIGH:
+		case DEFAULT_GLBLUB:
+			cladatum->default_range = val;
+			break;
+		default:
+			goto bad;
+		}
 	}
 
 	if (p->policyvers >= POLICYDB_VERSION_DEFAULT_TYPE) {
 		rc = next_entry(buf, fp, sizeof(u32) * 1);
 		if (rc)
 			goto bad;
-		cladatum->default_type = le32_to_cpu(buf[0]);
+		rc = -EINVAL;
+		val = le32_to_cpu(buf[0]);
+		switch (val) {
+		case 0:
+		case DEFAULT_TARGET:
+		case DEFAULT_SOURCE:
+			cladatum->default_type = val;
+			break;
+		default:
+			goto bad;
+		}
 	}
 
 	rc = symtab_insert(s, key, cladatum);
@@ -1410,6 +1459,8 @@ static int class_read(struct policydb *p, struct symtab *s, struct policy_file *
 	return 0;
 bad:
 	cls_destroy(key, cladatum, NULL);
+	if (rc)
+		pr_err("SELinux:  invalid class\n");
 	return rc;
 }
 
@@ -1522,7 +1573,7 @@ static int type_read(struct policydb *p, struct symtab *s, struct policy_file *f
  * Read a MLS level structure from a policydb binary
  * representation file.
  */
-static int mls_read_level(struct mls_level *lp, struct policy_file *fp)
+static int mls_read_level(const struct policydb *p, struct mls_level *lp, struct policy_file *fp)
 {
 	__le32 buf[1];
 	int rc;
@@ -1581,7 +1632,7 @@ static int user_read(struct policydb *p, struct symtab *s, struct policy_file *f
 		rc = mls_read_range_helper(&usrdatum->range, fp);
 		if (rc)
 			goto bad;
-		rc = mls_read_level(&usrdatum->dfltlevel, fp);
+		rc = mls_read_level(p, &usrdatum->dfltlevel, fp);
 		if (rc)
 			goto bad;
 	}
@@ -1601,7 +1652,7 @@ static int sens_read(struct policydb *p, struct symtab *s, struct policy_file *f
 	struct level_datum *levdatum;
 	int rc;
 	__le32 buf[2];
-	u32 len;
+	u32 len, val;
 
 	levdatum = kzalloc(sizeof(*levdatum), GFP_KERNEL);
 	if (!levdatum)
@@ -1612,13 +1663,17 @@ static int sens_read(struct policydb *p, struct symtab *s, struct policy_file *f
 		goto bad;
 
 	len = le32_to_cpu(buf[0]);
-	levdatum->isalias = le32_to_cpu(buf[1]);
+	val = le32_to_cpu(buf[1]);
+	rc = -EINVAL;
+	if (val != 0 && val != 1)
+		goto bad;
+	levdatum->isalias = val;
 
 	rc = str_read(&key, GFP_KERNEL, fp, len);
 	if (rc)
 		goto bad;
 
-	rc = mls_read_level(&levdatum->level, fp);
+	rc = mls_read_level(p, &levdatum->level, fp);
 	if (rc)
 		goto bad;
 
@@ -1628,6 +1683,8 @@ static int sens_read(struct policydb *p, struct symtab *s, struct policy_file *f
 	return 0;
 bad:
 	sens_destroy(key, levdatum, NULL);
+	if (rc)
+		pr_err("SELinux:  invalid sensitivity\n");
 	return rc;
 }
 
@@ -1637,7 +1694,7 @@ static int cat_read(struct policydb *p, struct symtab *s, struct policy_file *fp
 	struct cat_datum *catdatum;
 	int rc;
 	__le32 buf[3];
-	u32 len;
+	u32 len, val;
 
 	catdatum = kzalloc(sizeof(*catdatum), GFP_KERNEL);
 	if (!catdatum)
@@ -1649,7 +1706,11 @@ static int cat_read(struct policydb *p, struct symtab *s, struct policy_file *fp
 
 	len = le32_to_cpu(buf[0]);
 	catdatum->value = le32_to_cpu(buf[1]);
-	catdatum->isalias = le32_to_cpu(buf[2]);
+	val = le32_to_cpu(buf[2]);
+	rc = -EINVAL;
+	if (val != 0 && val != 1)
+		goto bad;
+	catdatum->isalias = val;
 
 	rc = str_read(&key, GFP_KERNEL, fp, len);
 	if (rc)
@@ -1661,6 +1722,8 @@ static int cat_read(struct policydb *p, struct symtab *s, struct policy_file *fp
 	return 0;
 bad:
 	cat_destroy(key, catdatum, NULL);
+	if (rc)
+		pr_err("SELinux:  invalid category\n");
 	return rc;
 }
 
@@ -1914,6 +1977,8 @@ static int range_read(struct policydb *p, struct policy_file *fp)
 out:
 	kfree(rt);
 	kfree(r);
+	if (rc)
+		pr_err("SELinux:  invalid range\n");
 	return rc;
 }
 
@@ -1941,10 +2006,14 @@ static int filename_trans_read_helper_compat(struct policydb *p, struct policy_f
 	if (rc)
 		goto out;
 
+	rc = -EINVAL;
 	stype = le32_to_cpu(buf[0]);
+	if (!policydb_type_isvalid(p, stype))
+		goto out;
 	key.ttype = le32_to_cpu(buf[1]);
+	if (!policydb_type_isvalid(p, key.ttype))
+		goto out;
 	val = le32_to_cpu(buf[2]);
-	rc = -EINVAL;
 	if (val > U16_MAX || !policydb_class_isvalid(p, val))
 		goto out;
 	key.tclass = val;
@@ -2003,6 +2072,9 @@ static int filename_trans_read_helper_compat(struct policydb *p, struct policy_f
 	kfree(ft);
 	kfree(name);
 	kfree(datum);
+
+	if (rc)
+		pr_err("SELinux:  invalid compat filename transition\n");
 	return rc;
 }
 
@@ -2031,7 +2103,10 @@ static int filename_trans_read_helper(struct policydb *p, struct policy_file *fp
 	if (rc)
 		goto out;
 
+	rc = -EINVAL;
 	ttype = le32_to_cpu(buf[0]);
+	if (!policydb_type_isvalid(p, ttype))
+		goto out;
 	val = le32_to_cpu(buf[1]);
 	rc = -EINVAL;
 	if (val > U16_MAX || !policydb_class_isvalid(p, val))
@@ -2066,6 +2141,10 @@ static int filename_trans_read_helper(struct policydb *p, struct policy_file *fp
 
 		datum->otype = le32_to_cpu(buf[0]);
 
+		rc = -EINVAL;
+		if (!policydb_type_isvalid(p, datum->otype))
+			goto out;
+
 		dst = &datum->next;
 	}
 
@@ -2097,6 +2176,9 @@ static int filename_trans_read_helper(struct policydb *p, struct policy_file *fp
 		ebitmap_destroy(&datum->stypes);
 		kfree(datum);
 	}
+
+	if (rc)
+		pr_err("SELinux:  invalid filename transition\n");
 	return rc;
 }
 
@@ -2216,7 +2298,7 @@ static int genfs_read(struct policydb *p, struct policy_file *fp)
 
 			rc = -EINVAL;
 			val = le32_to_cpu(buf[0]);
-			if (val >= U16_MAX)
+			if (val >= U16_MAX || (val != 0 && !policydb_class_isvalid(p, val)))
 				goto out;
 			newc->v.sclass = val;
 			rc = context_read_and_validate(&newc->context[0], p,
@@ -2255,6 +2337,9 @@ static int genfs_read(struct policydb *p, struct policy_file *fp)
 	}
 	ocontext_destroy(newc, OCON_FSUSE);
 
+	if (rc)
+		pr_err("SELinux:  invalid genfs\n");
+
 	return rc;
 }
 
@@ -2263,7 +2348,7 @@ static int ocontext_read(struct policydb *p,
 {
 	int rc;
 	unsigned int i;
-	u32 j, nel, len;
+	u32 j, nel, len, val;
 	__be64 prefixbuf[1];
 	__le32 buf[3];
 	struct ocontext *l, *c;
@@ -2327,11 +2412,25 @@ static int ocontext_read(struct policydb *p,
 				rc = next_entry(buf, fp, sizeof(u32) * 3);
 				if (rc)
 					goto out;
-				c->u.port.protocol = le32_to_cpu(buf[0]);
-				c->u.port.low_port = le32_to_cpu(buf[1]);
-				c->u.port.high_port = le32_to_cpu(buf[2]);
-				rc = context_read_and_validate(&c->context[0],
-							       p, fp);
+
+				rc = -EINVAL;
+				val = le32_to_cpu(buf[0]);
+				if (val > U8_MAX)
+					goto out;
+				c->u.port.protocol = val;
+				val = le32_to_cpu(buf[1]);
+				if (val > U16_MAX)
+					goto out;
+				c->u.port.low_port = val;
+				val = le32_to_cpu(buf[2]);
+				if (val > U16_MAX)
+					goto out;
+				c->u.port.high_port = val;
+				if (c->u.port.low_port == 0 ||
+				    c->u.port.low_port > c->u.port.high_port)
+					goto out;
+
+				rc = context_read_and_validate(&c->context[0], p, fp);
 				if (rc)
 					goto out;
 				break;
@@ -2449,6 +2548,8 @@ static int ocontext_read(struct policydb *p,
 	}
 	rc = 0;
 out:
+	if (rc)
+		pr_err("SELinux:  invalid ocon\n");
 	return rc;
 }
 
diff --git a/security/selinux/ss/policydb.h b/security/selinux/ss/policydb.h
index 6d685a3fc080..4d278583c172 100644
--- a/security/selinux/ss/policydb.h
+++ b/security/selinux/ss/policydb.h
@@ -74,7 +74,7 @@ struct class_datum {
 /* Role attributes */
 struct role_datum {
 	u32 value; /* internal role value */
-	u32 bounds; /* boundary of role */
+	u32 bounds; /* boundary of role, 0 for none */
 	struct ebitmap dominates; /* set of roles dominated by this role */
 	struct ebitmap types; /* set of authorized types for role */
 };
@@ -110,15 +110,15 @@ struct role_allow {
 /* Type attributes */
 struct type_datum {
 	u32 value; /* internal type value */
-	u32 bounds; /* boundary of type */
-	unsigned char primary; /* primary name? */
+	u32 bounds; /* boundary of type, 0 for none */
+	unsigned char primary; /* primary name? (unused) */
 	unsigned char attribute; /* attribute ?*/
 };
 
 /* User attributes */
 struct user_datum {
 	u32 value; /* internal user value */
-	u32 bounds; /* bounds of user */
+	u32 bounds; /* bounds of user, 0 for none */
 	struct ebitmap roles; /* set of authorized roles for user */
 	struct mls_range range; /* MLS range (min - max) for user */
 	struct mls_level dfltlevel; /* default login MLS level for user */
@@ -195,7 +195,7 @@ struct ocontext {
 		} ibendport;
 	} u;
 	union {
-		u16 sclass; /* security class for genfs */
+		u16 sclass; /* security class for genfs (can be 0 for wildcard) */
 		u32 behavior; /* labeling behavior for fs_use */
 	} v;
 	struct context context[2]; /* security context(s) */
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 835b2ac49562..d8b5fb98adb9 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -445,8 +445,6 @@ static int dump_masked_av_helper(void *k, void *d, void *args)
 	struct perm_datum *pdatum = d;
 	char **permission_names = args;
 
-	BUG_ON(pdatum->value < 1 || pdatum->value > 32);
-
 	permission_names[pdatum->value - 1] = (char *)k;
 
 	return 0;
-- 
2.45.2

[RFC PATCH 12/22] selinux: check length fields in policies

[Christian Göttsche]

From: Christian Göttsche <cgzones@googlemail.com>

In multiple places the binary policy announces how many items of some
kind are to be expected next.  Before reading them the kernel already
allocates enough memory for that announced size.  Validate that the
remaining input size can actually fit the announced items, to avoid OOM
issues on malformed binary policies.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 security/selinux/ss/avtab.c       |  4 ++++
 security/selinux/ss/conditional.c | 14 ++++++++++++++
 security/selinux/ss/policydb.c    | 23 +++++++++++++++++++++++
 security/selinux/ss/policydb.h    | 13 +++++++++++++
 4 files changed, 54 insertions(+)

diff --git a/security/selinux/ss/avtab.c b/security/selinux/ss/avtab.c
index 6122c1c7e951..002853dd3443 100644
--- a/security/selinux/ss/avtab.c
+++ b/security/selinux/ss/avtab.c
@@ -550,6 +550,10 @@ int avtab_read(struct avtab *a, struct policy_file *fp, struct policydb *pol)
 		goto bad;
 	}
 
+	rc = oom_check(2 * sizeof(u32), nel, fp);
+	if (rc)
+		goto bad;
+
 	rc = avtab_alloc(a, nel);
 	if (rc)
 		goto bad;
diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
index f85d22952d5a..07008ea081ba 100644
--- a/security/selinux/ss/conditional.c
+++ b/security/selinux/ss/conditional.c
@@ -12,6 +12,7 @@
 
 #include "security.h"
 #include "conditional.h"
+#include "policydb.h"
 #include "services.h"
 
 /*
@@ -329,6 +330,10 @@ static int cond_read_av_list(struct policydb *p, struct policy_file *fp,
 	if (len == 0)
 		return 0;
 
+	rc = oom_check(2 * sizeof(u32), len, fp);
+	if (rc)
+		return rc;
+
 	list->nodes = kcalloc(len, sizeof(*list->nodes), GFP_KERNEL);
 	if (!list->nodes)
 		return -ENOMEM;
@@ -379,6 +384,11 @@ static int cond_read_node(struct policydb *p, struct cond_node *node, struct pol
 
 	/* expr */
 	len = le32_to_cpu(buf[1]);
+
+	rc = oom_check(2 * sizeof(u32), len, fp);
+	if (rc)
+		return rc;
+
 	node->expr.nodes = kcalloc(len, sizeof(*node->expr.nodes), GFP_KERNEL);
 	if (!node->expr.nodes)
 		return -ENOMEM;
@@ -417,6 +427,10 @@ int cond_read_list(struct policydb *p, struct policy_file *fp)
 
 	len = le32_to_cpu(buf[0]);
 
+	rc = oom_check(2 * sizeof(u32), len, fp);
+	if (rc)
+		goto err;
+
 	p->cond_list = kcalloc(len, sizeof(*p->cond_list), GFP_KERNEL);
 	if (!p->cond_list)
 		return -ENOMEM;
diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index 1157e66b4664..5ba1e8d483fb 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -1095,6 +1095,9 @@ int str_read(char **strp, gfp_t flags, struct policy_file *fp, u32 len)
 	if ((len == 0) || (len == (u32)-1))
 		return -EINVAL;
 
+	if (oom_check(sizeof(char), len, fp))
+		return -EINVAL;
+
 	str = kmalloc(len + 1, flags | __GFP_NOWARN);
 	if (!str)
 		return -ENOMEM;
@@ -1169,6 +1172,10 @@ static int common_read(struct policydb *p, struct symtab *s, struct policy_file
 	if (nel > 32)
 		goto bad;
 
+	rc = oom_check(/*guaranteed read by perm_read()*/2 * sizeof(u32), nel, fp);
+	if (rc)
+		goto bad;
+
 	rc = symtab_init(&comdatum->permissions, nel);
 	if (rc)
 		goto bad;
@@ -1343,6 +1350,10 @@ static int class_read(struct policydb *p, struct symtab *s, struct policy_file *
 		goto bad;
 	cladatum->value = val;
 
+	rc = oom_check(/*guaranteed read by perm_read()*/2 * sizeof(u32), nel, fp);
+	if (rc)
+		goto bad;
+
 	rc = symtab_init(&cladatum->permissions, nel);
 	if (rc)
 		goto bad;
@@ -1916,6 +1927,10 @@ static int range_read(struct policydb *p, struct policy_file *fp)
 
 	nel = le32_to_cpu(buf[0]);
 
+	rc = oom_check(sizeof(u32) * 2, nel, fp);
+	if (rc)
+		return rc;
+
 	rc = hashtab_init(&p->range_tr, nel);
 	if (rc)
 		return rc;
@@ -2684,6 +2699,10 @@ int policydb_read(struct policydb *p, struct policy_file *fp)
 		nprim = le32_to_cpu(buf[0]);
 		nel = le32_to_cpu(buf[1]);
 
+		rc = oom_check(/*guaranteed read by read_f()*/ 4 * sizeof(u32), nel, fp);
+		if (rc)
+			goto out;
+
 		rc = symtab_init(&p->symtab[i], nel);
 		if (rc)
 			goto out;
@@ -2725,6 +2744,10 @@ int policydb_read(struct policydb *p, struct policy_file *fp)
 		goto bad;
 	nel = le32_to_cpu(buf[0]);
 
+	rc = oom_check(3 * sizeof(u32), nel, fp);
+	if (rc)
+		goto bad;
+
 	rc = hashtab_init(&p->role_tr, nel);
 	if (rc)
 		goto bad;
diff --git a/security/selinux/ss/policydb.h b/security/selinux/ss/policydb.h
index 4d278583c172..fee9132b0d42 100644
--- a/security/selinux/ss/policydb.h
+++ b/security/selinux/ss/policydb.h
@@ -353,6 +353,19 @@ struct policy_data {
 	struct policy_file *fp;
 };
 
+static inline int oom_check(size_t bytes, size_t num, const struct policy_file *fp)
+{
+	size_t len;
+
+	if (unlikely(check_mul_overflow(bytes, num, &len)))
+		return -EINVAL;
+
+	if (unlikely(len > fp->len))
+		return -EINVAL;
+
+	return 0;
+}
+
 static inline int next_entry(void *buf, struct policy_file *fp, size_t bytes)
 {
 	if (bytes > fp->len)
-- 
2.45.2

[RFC PATCH 13/22] selinux: validate constraints

[Christian Göttsche]

From: Christian Göttsche <cgzones@googlemail.com>

Validate constraint expressions during reading the policy.
Avoid the usage of BUG() on constraint evaluation, to mitigate malformed
policies halting the system.

Closes: https://github.com/SELinuxProject/selinux-testsuite/issues/76

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 security/selinux/ss/policydb.c | 61 ++++++++++++++++++++++++++++++++--
 security/selinux/ss/services.c | 55 +++++++++++++++---------------
 2 files changed, 88 insertions(+), 28 deletions(-)

diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index 5ba1e8d483fb..5d99e1498b55 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -1251,6 +1251,8 @@ static int read_cons_helper(struct policydb *p, struct constraint_node **nodep,
 			return rc;
 		c->permissions = le32_to_cpu(buf[0]);
 		nexpr = le32_to_cpu(buf[1]);
+		if (nexpr == 0)
+			return -EINVAL;
 		le = NULL;
 		depth = -1;
 		for (j = 0; j < nexpr; j++) {
@@ -1282,15 +1284,70 @@ static int read_cons_helper(struct policydb *p, struct constraint_node **nodep,
 				depth--;
 				break;
 			case CEXPR_ATTR:
-				if (depth == (CEXPR_MAXDEPTH - 1))
+				if (depth >= (CEXPR_MAXDEPTH - 1))
 					return -EINVAL;
 				depth++;
 				break;
+
+				switch (e->op) {
+				case CEXPR_EQ:
+				case CEXPR_NEQ:
+					break;
+				case CEXPR_DOM:
+				case CEXPR_DOMBY:
+				case CEXPR_INCOMP:
+					if ((e->attr & CEXPR_USER) || (e->attr & CEXPR_TYPE))
+						return -EINVAL;
+					break;
+				default:
+					return -EINVAL;
+				}
+
+				switch (e->attr) {
+				case CEXPR_USER:
+				case CEXPR_ROLE:
+				case CEXPR_TYPE:
+				case CEXPR_L1L2:
+				case CEXPR_L1H2:
+				case CEXPR_H1L2:
+				case CEXPR_H1H2:
+				case CEXPR_L1H1:
+				case CEXPR_L2H2:
+					break;
+				default:
+					return -EINVAL;
+				}
+
+				break;
 			case CEXPR_NAMES:
 				if (!allowxtarget && (e->attr & CEXPR_XTARGET))
 					return -EINVAL;
-				if (depth == (CEXPR_MAXDEPTH - 1))
+				if (depth >= (CEXPR_MAXDEPTH - 1))
+					return -EINVAL;
+
+				switch (e->op) {
+				case CEXPR_EQ:
+				case CEXPR_NEQ:
+					break;
+				default:
+					return -EINVAL;
+				}
+
+				switch (e->attr) {
+				case CEXPR_USER:
+				case CEXPR_USER | CEXPR_TARGET:
+				case CEXPR_USER | CEXPR_XTARGET:
+				case CEXPR_ROLE:
+				case CEXPR_ROLE | CEXPR_TARGET:
+				case CEXPR_ROLE | CEXPR_XTARGET:
+				case CEXPR_TYPE:
+				case CEXPR_TYPE | CEXPR_TARGET:
+				case CEXPR_TYPE | CEXPR_XTARGET:
+					break;
+				default:
 					return -EINVAL;
+				}
+
 				depth++;
 				rc = ebitmap_read(&e->names, fp);
 				if (rc)
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index d8b5fb98adb9..b1b90f802328 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -278,22 +278,25 @@ static int constraint_expr_eval(struct policydb *policydb,
 	for (e = cexpr; e; e = e->next) {
 		switch (e->expr_type) {
 		case CEXPR_NOT:
-			BUG_ON(sp < 0);
+			if (unlikely(sp < 0))
+				goto invalid;
 			s[sp] = !s[sp];
 			break;
 		case CEXPR_AND:
-			BUG_ON(sp < 1);
+			if (unlikely(sp < 1))
+				goto invalid;
 			sp--;
 			s[sp] &= s[sp + 1];
 			break;
 		case CEXPR_OR:
-			BUG_ON(sp < 1);
+			if (unlikely(sp < 1))
+				goto invalid;
 			sp--;
 			s[sp] |= s[sp + 1];
 			break;
 		case CEXPR_ATTR:
-			if (sp == (CEXPR_MAXDEPTH - 1))
-				return 0;
+			if (unlikely(sp >= (CEXPR_MAXDEPTH - 1)))
+				goto invalid;
 			switch (e->attr) {
 			case CEXPR_USER:
 				val1 = scontext->user;
@@ -369,13 +372,11 @@ static int constraint_expr_eval(struct policydb *policydb,
 					s[++sp] = mls_level_incomp(l2, l1);
 					continue;
 				default:
-					BUG();
-					return 0;
+					goto invalid;
 				}
 				break;
 			default:
-				BUG();
-				return 0;
+				goto invalid;
 			}
 
 			switch (e->op) {
@@ -386,22 +387,19 @@ static int constraint_expr_eval(struct policydb *policydb,
 				s[++sp] = (val1 != val2);
 				break;
 			default:
-				BUG();
-				return 0;
+				goto invalid;
 			}
 			break;
 		case CEXPR_NAMES:
-			if (sp == (CEXPR_MAXDEPTH-1))
-				return 0;
+			if (unlikely(sp >= (CEXPR_MAXDEPTH-1)))
+				goto invalid;
 			c = scontext;
 			if (e->attr & CEXPR_TARGET)
 				c = tcontext;
 			else if (e->attr & CEXPR_XTARGET) {
 				c = xcontext;
-				if (!c) {
-					BUG();
-					return 0;
-				}
+				if (unlikely(!c))
+					goto invalid;
 			}
 			if (e->attr & CEXPR_USER)
 				val1 = c->user;
@@ -409,10 +407,8 @@ static int constraint_expr_eval(struct policydb *policydb,
 				val1 = c->role;
 			else if (e->attr & CEXPR_TYPE)
 				val1 = c->type;
-			else {
-				BUG();
-				return 0;
-			}
+			else
+				goto invalid;
 
 			switch (e->op) {
 			case CEXPR_EQ:
@@ -422,18 +418,25 @@ static int constraint_expr_eval(struct policydb *policydb,
 				s[++sp] = !ebitmap_get_bit(&e->names, val1 - 1);
 				break;
 			default:
-				BUG();
-				return 0;
+				goto invalid;
 			}
 			break;
 		default:
-			BUG();
-			return 0;
+			goto invalid;
 		}
 	}
 
-	BUG_ON(sp != 0);
+	if (unlikely(sp != 0))
+		goto invalid;
+
 	return s[0];
+
+invalid:
+	/* Should *never* be reached, cause malformed constraints should
+	 * have been filtered by read_cons_helper().
+	 */
+	WARN_ONCE(true, "SELinux: invalid constraint passed validation\n");
+	return 0;
 }
 
 /*
-- 
2.45.2

[RFC PATCH 14/22] selinux: pre-validate conditional expressions

[Christian Göttsche]

From: Christian Göttsche <cgzones@googlemail.com>

Validate conditional expressions while reading the policy, to avoid
unexpected access decisions on malformed policies.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 security/selinux/ss/conditional.c | 116 ++++++++++++++++++++----------
 security/selinux/ss/policydb.c    |   7 ++
 security/selinux/ss/policydb.h    |   1 +
 3 files changed, 88 insertions(+), 36 deletions(-)

diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
index 07008ea081ba..d37b4bdf6ba9 100644
--- a/security/selinux/ss/conditional.c
+++ b/security/selinux/ss/conditional.c
@@ -21,65 +21,119 @@
  * or undefined (-1). Undefined occurs when the expression
  * exceeds the stack depth of COND_EXPR_MAXDEPTH.
  */
-static int cond_evaluate_expr(struct policydb *p, struct cond_expr *expr)
+static int cond_evaluate_expr(const struct policydb *p, const struct cond_expr *expr)
 {
 	u32 i;
 	int s[COND_EXPR_MAXDEPTH];
 	int sp = -1;
 
-	if (expr->len == 0)
-		return -1;
+	if (unlikely(expr->len == 0))
+		goto invalid;
 
 	for (i = 0; i < expr->len; i++) {
-		struct cond_expr_node *node = &expr->nodes[i];
+		const struct cond_expr_node *node = &expr->nodes[i];
 
 		switch (node->expr_type) {
 		case COND_BOOL:
-			if (sp == (COND_EXPR_MAXDEPTH - 1))
-				return -1;
+			if (unlikely(sp >= (COND_EXPR_MAXDEPTH - 1)))
+				goto invalid;
 			sp++;
 			s[sp] = p->bool_val_to_struct[node->boolean - 1]->state;
 			break;
 		case COND_NOT:
-			if (sp < 0)
-				return -1;
+			if (unlikely(sp < 0))
+				goto invalid;
 			s[sp] = !s[sp];
 			break;
 		case COND_OR:
-			if (sp < 1)
-				return -1;
+			if (unlikely(sp < 1))
+				goto invalid;
 			sp--;
 			s[sp] |= s[sp + 1];
 			break;
 		case COND_AND:
-			if (sp < 1)
-				return -1;
+			if (unlikely(sp < 1))
+				goto invalid;
 			sp--;
 			s[sp] &= s[sp + 1];
 			break;
 		case COND_XOR:
-			if (sp < 1)
-				return -1;
+			if (unlikely(sp < 1))
+				goto invalid;
 			sp--;
 			s[sp] ^= s[sp + 1];
 			break;
 		case COND_EQ:
-			if (sp < 1)
-				return -1;
+			if (unlikely(sp < 1))
+				goto invalid;
 			sp--;
 			s[sp] = (s[sp] == s[sp + 1]);
 			break;
 		case COND_NEQ:
-			if (sp < 1)
-				return -1;
+			if (unlikely(sp < 1))
+				goto invalid;
 			sp--;
 			s[sp] = (s[sp] != s[sp + 1]);
 			break;
 		default:
-			return -1;
+			goto invalid;
 		}
 	}
+
+	if (unlikely(sp != 0))
+		goto invalid;
+
 	return s[0];
+
+invalid:
+	/* Should *never* be reached, cause malformed expressions should
+	 * have been filtered by cond_validate_expr().
+	 */
+	WARN_ONCE(true, "SELinux: invalid conditional expression passed validation\n");
+	return -1;
+}
+
+static int cond_validate_expr(const struct policydb *p, const struct cond_expr *expr)
+{
+	u32 i;
+	int depth = -1;
+
+	if (expr->len == 0)
+		return -EINVAL;
+
+	for (i = 0; i < expr->len; i++) {
+		const struct cond_expr_node *node = &expr->nodes[i];
+
+		switch (node->expr_type) {
+		case COND_BOOL:
+			if (depth >= (COND_EXPR_MAXDEPTH - 1))
+				return -EINVAL;
+			depth++;
+			if (!policydb_boolean_isvalid(p, node->boolean))
+				return -EINVAL;
+			break;
+		case COND_NOT:
+			if (depth < 0)
+				return -EINVAL;
+			break;
+		case COND_OR:
+		case COND_AND:
+		case COND_XOR:
+		case COND_EQ:
+		case COND_NEQ:
+			if (depth < 1)
+				return -EINVAL;
+			depth--;
+			break;
+		default:
+			return -EINVAL;
+		}
+	}
+
+	if (depth != 0)
+		return -EINVAL;
+
+	return 0;
 }
 
 /*
@@ -355,21 +409,6 @@ static int cond_read_av_list(struct policydb *p, struct policy_file *fp,
 	return 0;
 }
 
-static int expr_node_isvalid(struct policydb *p, struct cond_expr_node *expr)
-{
-	if (expr->expr_type <= 0 || expr->expr_type > COND_LAST) {
-		pr_err("SELinux: conditional expressions uses unknown operator.\n");
-		return 0;
-	}
-
-	if (expr->expr_type == COND_BOOL &&
-	    (expr->boolean == 0 || expr->boolean > p->p_bools.nprim)) {
-		pr_err("SELinux: conditional expressions uses unknown bool.\n");
-		return 0;
-	}
-	return 1;
-}
-
 static int cond_read_node(struct policydb *p, struct cond_node *node, struct policy_file *fp)
 {
 	__le32 buf[2];
@@ -384,6 +423,8 @@ static int cond_read_node(struct policydb *p, struct cond_node *node, struct pol
 
 	/* expr */
 	len = le32_to_cpu(buf[1]);
+	if (len == 0)
+		return -EINVAL;
 
 	rc = oom_check(2 * sizeof(u32), len, fp);
 	if (rc)
@@ -404,9 +445,12 @@ static int cond_read_node(struct policydb *p, struct cond_node *node, struct pol
 
 		expr->expr_type = le32_to_cpu(buf[0]);
 		expr->boolean = le32_to_cpu(buf[1]);
+	}
 
-		if (!expr_node_isvalid(p, expr))
-			return -EINVAL;
+	rc = cond_validate_expr(p, &node->expr);
+	if (rc) {
+		pr_err("SELinux: invalid conditional expression\n");
+		return rc;
 	}
 
 	rc = cond_read_av_list(p, fp, &node->true_list, NULL);
diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index 5d99e1498b55..1768ac4ecc2c 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -940,6 +940,13 @@ int policydb_type_isvalid(struct policydb *p, unsigned int type)
 	return 1;
 }
 
+int policydb_boolean_isvalid(const struct policydb *p, u32 boolean)
+{
+	if (!boolean || boolean > p->p_bools.nprim)
+		return 0;
+	return 1;
+}
+
 /*
  * Return 1 if the fields in the security context
  * structure `c' are valid.  Return 0 otherwise.
diff --git a/security/selinux/ss/policydb.h b/security/selinux/ss/policydb.h
index fee9132b0d42..c94253a1ddbc 100644
--- a/security/selinux/ss/policydb.h
+++ b/security/selinux/ss/policydb.h
@@ -318,6 +318,7 @@ extern int policydb_context_isvalid(struct policydb *p, struct context *c);
 extern int policydb_class_isvalid(struct policydb *p, u16 class);
 extern int policydb_type_isvalid(struct policydb *p, unsigned int type);
 extern int policydb_role_isvalid(struct policydb *p, unsigned int role);
+extern int policydb_boolean_isvalid(const struct policydb *p, u32 boolean);
 
 struct policy_file {
 	char *data;
-- 
2.45.2

[RFC PATCH 15/22] selinux: introduce ebitmap_highest_set_bit()

[Christian Göttsche]

From: Christian Göttsche <cgzones@googlemail.com>

Introduce an ebitmap function to calculate the highest set bit.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 security/selinux/ss/ebitmap.c | 27 +++++++++++++++++++++++++++
 security/selinux/ss/ebitmap.h |  1 +
 2 files changed, 28 insertions(+)

diff --git a/security/selinux/ss/ebitmap.c b/security/selinux/ss/ebitmap.c
index cd84357db2c4..74686b8a4c35 100644
--- a/security/selinux/ss/ebitmap.c
+++ b/security/selinux/ss/ebitmap.c
@@ -257,6 +257,33 @@ int ebitmap_contains(const struct ebitmap *e1, const struct ebitmap *e2,
 	return 1;
 }
 
+u32 ebitmap_highest_set_bit(const struct ebitmap *e)
+{
+	const struct ebitmap_node *n;
+	unsigned long unit;
+	u32 pos = 0;
+
+	n = e->node;
+	if (!n)
+		return 0;
+
+	while (n->next)
+		n = n->next;
+
+	for (unsigned int i = EBITMAP_UNIT_NUMS; i > 0; i--) {
+		unit = n->maps[i - 1];
+		if (unit == 0)
+			continue;
+
+		pos = (i - 1) * EBITMAP_UNIT_SIZE;
+		while (unit >>= 1)
+			pos++;
+		break;
+	}
+
+	return n->startbit + pos;
+}
+
 int ebitmap_get_bit(const struct ebitmap *e, u32 bit)
 {
 	const struct ebitmap_node *n;
diff --git a/security/selinux/ss/ebitmap.h b/security/selinux/ss/ebitmap.h
index e0150695566c..99a21d83a0b3 100644
--- a/security/selinux/ss/ebitmap.h
+++ b/security/selinux/ss/ebitmap.h
@@ -126,6 +126,7 @@ int ebitmap_and(struct ebitmap *dst, const struct ebitmap *e1,
 		const struct ebitmap *e2);
 int ebitmap_contains(const struct ebitmap *e1, const struct ebitmap *e2,
 		     u32 last_e2bit);
+u32 ebitmap_highest_set_bit(const struct ebitmap *e);
 int ebitmap_get_bit(const struct ebitmap *e, u32 bit);
 int ebitmap_set_bit(struct ebitmap *e, u32 bit, int value);
 void ebitmap_destroy(struct ebitmap *e);
-- 
2.45.2

[RFC PATCH 16/22] selinux: check type attr map overflows

[Christian Göttsche]

From: Christian Göttsche <cgzones@googlemail.com>

Validate that no types with an invalid too high ID are present in the
attribute map.  Gaps are still not checked.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 security/selinux/ss/policydb.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index 1768ac4ecc2c..9b2eae70e2dc 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -2941,6 +2941,11 @@ int policydb_read(struct policydb *p, struct policy_file *fp)
 			if (rc)
 				goto bad;
 		}
+
+		rc = -EINVAL;
+		if (ebitmap_highest_set_bit(e) >= p->p_types.nprim)
+			goto bad;
+
 		/* add the type itself as the degenerate case */
 		rc = ebitmap_set_bit(e, i, 1);
 		if (rc)
-- 
2.45.2

[RFC PATCH 17/22] selinux: reorder policydb_index()

[Christian Göttsche]

From: Christian Göttsche <cgzones@googlemail.com>

Index as soon as possible to enable isvalid() checks to fail on gaps.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 security/selinux/ss/policydb.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index 9b2eae70e2dc..2fe9a68b16f0 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -723,7 +723,6 @@ static int policydb_index(struct policydb *p)
 	pr_debug("SELinux:  %d classes, %d rules\n", p->p_classes.nprim,
 		 p->te_avtab.nel);
 
-	avtab_hash_eval(&p->te_avtab, "rules");
 	symtab_hash_eval(p->symtab);
 
 	p->class_val_to_struct = kcalloc(p->p_classes.nprim,
@@ -2786,6 +2785,10 @@ int policydb_read(struct policydb *p, struct policy_file *fp)
 		p->symtab[i].nprim = nprim;
 	}
 
+	rc = policydb_index(p);
+	if (rc)
+		goto bad;
+
 	rc = -EINVAL;
 	p->process_class = string_to_security_class(p, "process");
 	if (!p->process_class) {
@@ -2797,6 +2800,8 @@ int policydb_read(struct policydb *p, struct policy_file *fp)
 	if (rc)
 		goto bad;
 
+	avtab_hash_eval(&p->te_avtab, "rules");
+
 	if (p->policyvers >= POLICYDB_VERSION_BOOL) {
 		rc = cond_read_list(p, fp);
 		if (rc)
@@ -2893,10 +2898,6 @@ int policydb_read(struct policydb *p, struct policy_file *fp)
 	if (rc)
 		goto bad;
 
-	rc = policydb_index(p);
-	if (rc)
-		goto bad;
-
 	rc = -EINVAL;
 	perm = string_to_av_perm(p, p->process_class, "transition");
 	if (!perm) {
-- 
2.45.2

[RFC PATCH 18/22] selinux: beef up isvalid checks

[Christian Göttsche]

From: Christian Göttsche <cgzones@googlemail.com>

Check that an ID does not refer to a gap in the global array of
definitions.

Constify parameters of isvalid() function and change return type to
bool.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 security/selinux/ss/hashtab.h  |  4 +--
 security/selinux/ss/mls.c      | 66 +++++++++++++++++++++-------------
 security/selinux/ss/mls.h      |  6 ++--
 security/selinux/ss/policydb.c | 56 ++++++++++++++++-------------
 security/selinux/ss/policydb.h | 12 +++----
 security/selinux/ss/services.c |  2 +-
 security/selinux/ss/symtab.c   |  2 +-
 security/selinux/ss/symtab.h   |  2 +-
 8 files changed, 88 insertions(+), 62 deletions(-)

diff --git a/security/selinux/ss/hashtab.h b/security/selinux/ss/hashtab.h
index deba82d78c3a..c641fb12916b 100644
--- a/security/selinux/ss/hashtab.h
+++ b/security/selinux/ss/hashtab.h
@@ -94,11 +94,11 @@ static inline int hashtab_insert(struct hashtab *h, void *key, void *datum,
  * Returns NULL if no entry has the specified key or
  * the datum of the entry otherwise.
  */
-static inline void *hashtab_search(struct hashtab *h, const void *key,
+static inline void *hashtab_search(const struct hashtab *h, const void *key,
 				   struct hashtab_key_params key_params)
 {
 	u32 hvalue;
-	struct hashtab_node *cur;
+	const struct hashtab_node *cur;
 
 	if (!h->size)
 		return NULL;
diff --git a/security/selinux/ss/mls.c b/security/selinux/ss/mls.c
index a6e49269f535..3cd36e2015fa 100644
--- a/security/selinux/ss/mls.c
+++ b/security/selinux/ss/mls.c
@@ -32,7 +32,7 @@
 int mls_compute_context_len(struct policydb *p, struct context *context)
 {
 	int i, l, len, head, prev;
-	char *nm;
+	const char *nm;
 	struct ebitmap *e;
 	struct ebitmap_node *node;
 
@@ -86,7 +86,8 @@ int mls_compute_context_len(struct policydb *p, struct context *context)
 void mls_sid_to_context(struct policydb *p, struct context *context,
 			char **scontext)
 {
-	char *scontextp, *nm;
+	const char *nm;
+	char *scontextp;
 	int i, l, head, prev;
 	struct ebitmap *e;
 	struct ebitmap_node *node;
@@ -155,27 +156,44 @@ void mls_sid_to_context(struct policydb *p, struct context *context,
 	*scontext = scontextp;
 }
 
-int mls_level_isvalid(struct policydb *p, struct mls_level *l)
+bool mls_level_isvalid(const struct policydb *p, const struct mls_level *l)
 {
-	struct level_datum *levdatum;
+	const char *name;
+	const struct level_datum *levdatum;
+	struct ebitmap_node *node;
+	u32 bit;
+	int rc;
 
 	if (!l->sens || l->sens > p->p_levels.nprim)
-		return 0;
-	levdatum = symtab_search(&p->p_levels,
-				 sym_name(p, SYM_LEVELS, l->sens - 1));
+		return false;
+
+	name = sym_name(p, SYM_LEVELS, l->sens - 1);
+	if (!name)
+		return false;
+
+	levdatum = symtab_search(&p->p_levels, name);
 	if (!levdatum)
-		return 0;
+		return false;
 
 	/*
-	 * Return 1 iff all the bits set in l->cat are also be set in
+	 * Validate that all bits set in l->cat are also be set in
 	 * levdatum->level->cat and no bit in l->cat is larger than
 	 * p->p_cats.nprim.
 	 */
-	return ebitmap_contains(&levdatum->level.cat, &l->cat,
-				p->p_cats.nprim);
+	rc = ebitmap_contains(&levdatum->level.cat, &l->cat,
+			      p->p_cats.nprim);
+	if (!rc)
+		return false;
+
+	ebitmap_for_each_positive_bit(&levdatum->level.cat, node, bit) {
+		if (!sym_name(p, SYM_CATS, bit))
+			return false;
+	}
+
+	return true;
 }
 
-int mls_range_isvalid(struct policydb *p, struct mls_range *r)
+bool mls_range_isvalid(const struct policydb *p, const struct mls_range *r)
 {
 	return (mls_level_isvalid(p, &r->level[0]) &&
 		mls_level_isvalid(p, &r->level[1]) &&
@@ -183,32 +201,32 @@ int mls_range_isvalid(struct policydb *p, struct mls_range *r)
 }
 
 /*
- * Return 1 if the MLS fields in the security context
+ * Return true if the MLS fields in the security context
  * structure `c' are valid.  Return 0 otherwise.
  */
-int mls_context_isvalid(struct policydb *p, struct context *c)
+bool mls_context_isvalid(const struct policydb *p, const struct context *c)
 {
-	struct user_datum *usrdatum;
+	const struct user_datum *usrdatum;
 
 	if (!p->mls_enabled)
-		return 1;
+		return true;
 
 	if (!mls_range_isvalid(p, &c->range))
-		return 0;
+		return false;
 
 	if (c->role == OBJECT_R_VAL)
-		return 1;
+		return true;
 
 	/*
 	 * User must be authorized for the MLS range.
 	 */
 	if (!c->user || c->user > p->p_users.nprim)
-		return 0;
+		return false;
 	usrdatum = p->user_val_to_struct[c->user - 1];
-	if (!mls_range_contains(usrdatum->range, c->range))
-		return 0; /* user may not be associated with range */
+	if (!usrdatum || !mls_range_contains(usrdatum->range, c->range))
+		return false; /* user may not be associated with range */
 
-	return 1;
+	return true;
 }
 
 /*
@@ -449,8 +467,8 @@ int mls_convert_context(struct policydb *oldp, struct policydb *newp,
 		return 0;
 
 	for (l = 0; l < 2; l++) {
-		char *name = sym_name(oldp, SYM_LEVELS,
-				      oldc->range.level[l].sens - 1);
+		const char *name = sym_name(oldp, SYM_LEVELS,
+					    oldc->range.level[l].sens - 1);
 
 		levdatum = symtab_search(&newp->p_levels, name);
 
diff --git a/security/selinux/ss/mls.h b/security/selinux/ss/mls.h
index 07980636751f..93cde1b22992 100644
--- a/security/selinux/ss/mls.h
+++ b/security/selinux/ss/mls.h
@@ -27,9 +27,9 @@
 int mls_compute_context_len(struct policydb *p, struct context *context);
 void mls_sid_to_context(struct policydb *p, struct context *context,
 			char **scontext);
-int mls_context_isvalid(struct policydb *p, struct context *c);
-int mls_range_isvalid(struct policydb *p, struct mls_range *r);
-int mls_level_isvalid(struct policydb *p, struct mls_level *l);
+bool mls_context_isvalid(const struct policydb *p, const struct context *c);
+bool mls_range_isvalid(const struct policydb *p, const struct mls_range *r);
+bool mls_level_isvalid(const struct policydb *p, const struct mls_level *l);
 
 int mls_context_to_sid(struct policydb *p, char oldc, char *scontext,
 		       struct context *context, struct sidtab *s, u32 def_sid);
diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index 2fe9a68b16f0..191d4f2de93f 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -918,51 +918,59 @@ int policydb_load_isids(struct policydb *p, struct sidtab *s)
 	return 0;
 }
 
-int policydb_class_isvalid(struct policydb *p, u16 class)
+bool policydb_class_isvalid(const struct policydb *p, u16 class)
 {
 	if (!class || class > p->p_classes.nprim)
-		return 0;
-	return 1;
+		return false;
+	if (!p->sym_val_to_name[SYM_CLASSES][class - 1])
+		return false;
+	return true;
 }
 
-int policydb_role_isvalid(struct policydb *p, unsigned int role)
+bool policydb_role_isvalid(const struct policydb *p, u32 role)
 {
 	if (!role || role > p->p_roles.nprim)
-		return 0;
-	return 1;
+		return false;
+	if (!p->sym_val_to_name[SYM_ROLES][role - 1])
+		return false;
+	return true;
 }
 
-int policydb_type_isvalid(struct policydb *p, unsigned int type)
+bool policydb_type_isvalid(const struct policydb *p, u32 type)
 {
 	if (!type || type > p->p_types.nprim)
-		return 0;
-	return 1;
+		return false;
+	if (!p->sym_val_to_name[SYM_TYPES][type - 1])
+		return false;
+	return true;
 }
 
-int policydb_boolean_isvalid(const struct policydb *p, u32 boolean)
+bool policydb_boolean_isvalid(const struct policydb *p, u32 boolean)
 {
 	if (!boolean || boolean > p->p_bools.nprim)
-		return 0;
-	return 1;
+		return false;
+	if (!p->sym_val_to_name[SYM_BOOLS][boolean - 1])
+		return false;
+	return true;
 }
 
 /*
- * Return 1 if the fields in the security context
+ * Return true if the fields in the security context
  * structure `c' are valid.  Return 0 otherwise.
  */
-int policydb_context_isvalid(struct policydb *p, struct context *c)
+bool policydb_context_isvalid(const struct policydb *p, const struct context *c)
 {
-	struct role_datum *role;
-	struct user_datum *usrdatum;
+	const struct role_datum *role;
+	const struct user_datum *usrdatum;
 
 	if (!c->role || c->role > p->p_roles.nprim)
-		return 0;
+		return false;
 
 	if (!c->user || c->user > p->p_users.nprim)
-		return 0;
+		return false;
 
 	if (!c->type || c->type > p->p_types.nprim)
-		return 0;
+		return false;
 
 	if (c->role != OBJECT_R_VAL) {
 		/*
@@ -971,24 +979,24 @@ int policydb_context_isvalid(struct policydb *p, struct context *c)
 		role = p->role_val_to_struct[c->role - 1];
 		if (!role || !ebitmap_get_bit(&role->types, c->type - 1))
 			/* role may not be associated with type */
-			return 0;
+			return false;
 
 		/*
 		 * User must be authorized for the role.
 		 */
 		usrdatum = p->user_val_to_struct[c->user - 1];
 		if (!usrdatum)
-			return 0;
+			return false;
 
 		if (!ebitmap_get_bit(&usrdatum->roles, c->role - 1))
 			/* user may not be associated with role */
-			return 0;
+			return false;
 	}
 
 	if (!mls_context_isvalid(p, c))
-		return 0;
+		return false;
 
-	return 1;
+	return true;
 }
 
 /*
diff --git a/security/selinux/ss/policydb.h b/security/selinux/ss/policydb.h
index c94253a1ddbc..2126f2b39628 100644
--- a/security/selinux/ss/policydb.h
+++ b/security/selinux/ss/policydb.h
@@ -314,11 +314,11 @@ struct policydb {
 
 extern void policydb_destroy(struct policydb *p);
 extern int policydb_load_isids(struct policydb *p, struct sidtab *s);
-extern int policydb_context_isvalid(struct policydb *p, struct context *c);
-extern int policydb_class_isvalid(struct policydb *p, u16 class);
-extern int policydb_type_isvalid(struct policydb *p, unsigned int type);
-extern int policydb_role_isvalid(struct policydb *p, unsigned int role);
-extern int policydb_boolean_isvalid(const struct policydb *p, u32 boolean);
+extern bool policydb_context_isvalid(const struct policydb *p, const struct context *c);
+extern bool policydb_class_isvalid(const struct policydb *p, u16 class);
+extern bool policydb_type_isvalid(const struct policydb *p, u32 type);
+extern bool policydb_role_isvalid(const struct policydb *p, u32 role);
+extern bool policydb_boolean_isvalid(const struct policydb *p, u32 boolean);
 
 struct policy_file {
 	char *data;
@@ -395,7 +395,7 @@ static inline int put_entry(const void *buf, size_t bytes, size_t num,
 	return 0;
 }
 
-static inline char *sym_name(struct policydb *p, unsigned int sym_num,
+static inline const char *sym_name(const struct policydb *p, unsigned int sym_num,
 			     unsigned int element_nr)
 {
 	return p->sym_val_to_name[sym_num][element_nr];
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index b1b90f802328..07d194357601 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -463,7 +463,7 @@ static void security_dump_masked_av(struct policydb *policydb,
 	struct common_datum *common_dat;
 	struct class_datum *tclass_dat;
 	struct audit_buffer *ab;
-	char *tclass_name;
+	const char *tclass_name;
 	char *scontext_name = NULL;
 	char *tcontext_name = NULL;
 	char *permission_names[32];
diff --git a/security/selinux/ss/symtab.c b/security/selinux/ss/symtab.c
index 832660fd84a9..a756554e7f1d 100644
--- a/security/selinux/ss/symtab.c
+++ b/security/selinux/ss/symtab.c
@@ -50,7 +50,7 @@ int symtab_insert(struct symtab *s, char *name, void *datum)
 	return hashtab_insert(&s->table, name, datum, symtab_key_params);
 }
 
-void *symtab_search(struct symtab *s, const char *name)
+void *symtab_search(const struct symtab *s, const char *name)
 {
 	return hashtab_search(&s->table, name, symtab_key_params);
 }
diff --git a/security/selinux/ss/symtab.h b/security/selinux/ss/symtab.h
index 8e667cdbf38f..7cfa3b44953a 100644
--- a/security/selinux/ss/symtab.h
+++ b/security/selinux/ss/symtab.h
@@ -21,6 +21,6 @@ struct symtab {
 int symtab_init(struct symtab *s, u32 size);
 
 int symtab_insert(struct symtab *s, char *name, void *datum);
-void *symtab_search(struct symtab *s, const char *name);
+void *symtab_search(const struct symtab *s, const char *name);
 
 #endif /* _SS_SYMTAB_H_ */
-- 
2.45.2

[RFC PATCH 19/22] selinux: validate symbols

[Christian Göttsche]

From: Christian Göttsche <cgzones@googlemail.com>

Some symbol tables need to be validated after indexing, since during
indexing their referenced entries might not yet have been indexed.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 security/selinux/ss/policydb.c | 94 ++++++++++++++++++++++++++++++++++
 1 file changed, 94 insertions(+)

diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index 191d4f2de93f..691c2c5caeb3 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -668,6 +668,84 @@ static int (*const index_f[SYM_NUM])(void *key, void *datum, void *datap) = {
 };
 /* clang-format on */
 
+static int role_validate(void *key, void *datum, void *datap)
+{
+	const struct policydb *p = datap;
+	const struct role_datum *role = datum;
+	struct ebitmap_node *node;
+	u32 i;
+
+	ebitmap_for_each_positive_bit(&role->dominates, node, i) {
+		if (!policydb_role_isvalid(p, i))
+			goto bad;
+	}
+
+	ebitmap_for_each_positive_bit(&role->types, node, i) {
+		if (!policydb_type_isvalid(p, i + 1))
+			goto bad;
+	}
+
+	return 0;
+
+bad:
+	pr_err("SELinux:  invalid role %s\n", sym_name(p, SYM_ROLES, role->value - 1));
+	return -EINVAL;
+}
+
+static int user_validate(void *key, void *datum, void *datap)
+{
+	const struct policydb *p = datap;
+	const struct user_datum *usrdatum = datum;
+	struct ebitmap_node *node;
+	u32 i;
+
+	ebitmap_for_each_positive_bit(&usrdatum->roles, node, i) {
+		if (!policydb_role_isvalid(p, i))
+			goto bad;
+	}
+
+	if (!mls_range_isvalid(p, &usrdatum->range))
+		goto bad;
+
+	if (!mls_level_isvalid(p, &usrdatum->dfltlevel))
+		goto bad;
+
+	return 0;
+
+bad:
+	pr_err("SELinux:  invalid user %s\n", sym_name(p, SYM_USERS, usrdatum->value - 1));
+	return -EINVAL;
+}
+
+static int sens_validate(void *key, void *datum, void *datap)
+{
+	const struct policydb *p = datap;
+	const struct level_datum *levdatum = datum;
+
+	if (!mls_level_isvalid(p, &levdatum->level))
+		goto bad;
+
+	return 0;
+
+bad:
+	pr_err("SELinux:  invalid sensitivity\n");
+	return -EINVAL;
+}
+
+
+/* clang-format off */
+static int (*const validate_f[SYM_NUM])(void *key, void *datum, void *datap) = {
+	NULL, /* Everything validated in common_read() and common_index() */
+	NULL, /* Everything validated in class_read() and class_index() */
+	role_validate,
+	NULL, /* Everything validated in type_read(), type_index() and type_bounds_sanity_check() */
+	user_validate,
+	NULL, /* Everything validated in cond_read_bool() and cond_index_bool() */
+	sens_validate,
+	NULL, /* Everything validated in cat_read() and cat_index() */
+};
+/* clang-format on */
+
 #ifdef CONFIG_SECURITY_SELINUX_DEBUG
 static void hash_eval(struct hashtab *h, const char *hash_name,
 		      const char *hash_details)
@@ -760,6 +838,16 @@ static int policydb_index(struct policydb *p)
 		if (rc)
 			goto out;
 	}
+
+	for (i = 0; i < SYM_NUM; i++) {
+		if (!validate_f[i])
+			continue;
+
+		rc = hashtab_map(&p->symtab[i].table, validate_f[i], p);
+		if (rc)
+			goto out;
+	}
+
 	rc = 0;
 out:
 	return rc;
@@ -1082,6 +1170,12 @@ static int context_read_and_validate(struct context *c, struct policydb *p,
 			pr_err("SELinux: error reading MLS range of context\n");
 			goto out;
 		}
+
+		rc = -EINVAL;
+		if (!mls_range_isvalid(p, &c->range)) {
+			pr_warn("SELinux: invalid range in security context\n");
+			goto out;
+		}
 	}
 
 	rc = -EINVAL;
-- 
2.45.2

[RFC PATCH 20/22] selinux: more strict bounds check

[Christian Göttsche]

From: Christian Göttsche <cgzones@googlemail.com>

Validate the types used in bounds checks.
Replace the usage of BUG(), to avoid halting the system on malformed
polices.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 security/selinux/ss/policydb.c | 29 +++++++++++++++++++++++++++--
 security/selinux/ss/policydb.h |  1 +
 security/selinux/ss/services.c |  3 +++
 3 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index 691c2c5caeb3..17fbd145ba1b 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -1015,6 +1015,15 @@ bool policydb_class_isvalid(const struct policydb *p, u16 class)
 	return true;
 }
 
+bool policydb_user_isvalid(const struct policydb *p, u32 user)
+{
+	if (!user || user > p->p_roles.nprim)
+		return false;
+	if (!p->sym_val_to_name[SYM_USERS][user - 1])
+		return false;
+	return true;
+}
+
 bool policydb_role_isvalid(const struct policydb *p, u32 role)
 {
 	if (!role || role > p->p_roles.nprim)
@@ -1935,6 +1944,12 @@ static int user_bounds_sanity_check(void *key, void *datum, void *datap)
 			return -EINVAL;
 		}
 
+		if (!policydb_user_isvalid(p, upper->bounds)) {
+			pr_err("SELinux: user %s: invalid boundary id %d\n",
+			       (char *) key, upper->bounds);
+			return -EINVAL;
+		}
+
 		upper = p->user_val_to_struct[upper->bounds - 1];
 		ebitmap_for_each_positive_bit(&user->roles, node, bit)
 		{
@@ -1972,6 +1987,12 @@ static int role_bounds_sanity_check(void *key, void *datum, void *datap)
 			return -EINVAL;
 		}
 
+		if (!policydb_role_isvalid(p, upper->bounds)) {
+			pr_err("SELinux: role %s: invalid boundary id %d\n",
+			       (char *) key, upper->bounds);
+			return -EINVAL;
+		}
+
 		upper = p->role_val_to_struct[upper->bounds - 1];
 		ebitmap_for_each_positive_bit(&role->types, node, bit)
 		{
@@ -2006,9 +2027,13 @@ static int type_bounds_sanity_check(void *key, void *datum, void *datap)
 			return -EINVAL;
 		}
 
-		upper = p->type_val_to_struct[upper->bounds - 1];
-		BUG_ON(!upper);
+		if (!policydb_type_isvalid(p, upper->bounds)) {
+			pr_err("SELinux: type %s: invalid boundary id %d\n",
+			       (char *) key, upper->bounds);
+			return -EINVAL;
+		}
 
+		upper = p->type_val_to_struct[upper->bounds - 1];
 		if (upper->attribute) {
 			pr_err("SELinux: type %s: "
 			       "bounded by attribute %s\n",
diff --git a/security/selinux/ss/policydb.h b/security/selinux/ss/policydb.h
index 2126f2b39628..28dd91a7427f 100644
--- a/security/selinux/ss/policydb.h
+++ b/security/selinux/ss/policydb.h
@@ -318,6 +318,7 @@ extern bool policydb_context_isvalid(const struct policydb *p, const struct cont
 extern bool policydb_class_isvalid(const struct policydb *p, u16 class);
 extern bool policydb_type_isvalid(const struct policydb *p, u32 type);
 extern bool policydb_role_isvalid(const struct policydb *p, u32 role);
+extern bool policydb_user_isvalid(const struct policydb *p, u32 user);
 extern bool policydb_boolean_isvalid(const struct policydb *p, u32 boolean);
 
 struct policy_file {
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 07d194357601..49805e81d6ce 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -711,6 +711,9 @@ static void context_struct_compute_av(struct policydb *policydb,
 	 * If the given source and target types have boundary
 	 * constraint, lazy checks have to mask any violated
 	 * permission and notice it to userspace via audit.
+	 *
+	 * Infinite recursion is avoided via a depth pre-check in
+	 * type_bounds_sanity_check().
 	 */
 	type_attribute_bounds_av(policydb, scontext, tcontext,
 				 tclass, avd);
-- 
2.45.2

[RFC PATCH 21/22] selinux: check for simple types

[Christian Göttsche]

From: Christian Göttsche <cgzones@googlemail.com>

Validate that the target of AVTAB_TYPE rules and file transitions are
simple types and not attributes.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 security/selinux/ss/avtab.c    |  9 ++++++++-
 security/selinux/ss/policydb.c | 23 +++++++++++++++++++++--
 security/selinux/ss/policydb.h |  1 +
 3 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/security/selinux/ss/avtab.c b/security/selinux/ss/avtab.c
index 002853dd3443..2aa16aa75aac 100644
--- a/security/selinux/ss/avtab.c
+++ b/security/selinux/ss/avtab.c
@@ -424,6 +424,13 @@ int avtab_read_item(struct avtab *a, struct policy_file *fp, struct policydb *po
 				}
 				key.specified = spec_order[i] | enabled;
 				datum.u.data = le32_to_cpu(buf32[items++]);
+
+				if ((key.specified & AVTAB_TYPE) &&
+				    !policydb_simpletype_isvalid(pol, datum.u.data)) {
+					pr_err("SELinux: avtab: invalid type\n");
+					return -EINVAL;
+				}
+
 				rc = insertf(a, &key, &datum, p);
 				if (rc)
 					return rc;
@@ -519,7 +526,7 @@ int avtab_read_item(struct avtab *a, struct policy_file *fp, struct policydb *po
 		datum.u.data = le32_to_cpu(*buf32);
 	}
 	if ((key.specified & AVTAB_TYPE) &&
-	    !policydb_type_isvalid(pol, datum.u.data)) {
+	    !policydb_simpletype_isvalid(pol, datum.u.data)) {
 		pr_err("SELinux: avtab: invalid type\n");
 		return -EINVAL;
 	}
diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index 17fbd145ba1b..917b468c5144 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -681,7 +681,7 @@ static int role_validate(void *key, void *datum, void *datap)
 	}
 
 	ebitmap_for_each_positive_bit(&role->types, node, i) {
-		if (!policydb_type_isvalid(p, i + 1))
+		if (!policydb_simpletype_isvalid(p, i + 1))
 			goto bad;
 	}
 
@@ -1042,6 +1042,23 @@ bool policydb_type_isvalid(const struct policydb *p, u32 type)
 	return true;
 }
 
+bool policydb_simpletype_isvalid(const struct policydb *p, u32 type)
+{
+	const struct type_datum *datum;
+
+	if (!type || type > p->p_types.nprim)
+		return false;
+
+	datum = p->type_val_to_struct[type - 1];
+	if (!datum)
+		return false;
+
+	if (datum->attribute)
+		return false;
+
+	return true;
+}
+
 bool policydb_boolean_isvalid(const struct policydb *p, u32 boolean)
 {
 	if (!boolean || boolean > p->p_bools.nprim)
@@ -2225,6 +2242,8 @@ static int filename_trans_read_helper_compat(struct policydb *p, struct policy_f
 	key.name = name;
 
 	otype = le32_to_cpu(buf[3]);
+	if (!policydb_simpletype_isvalid(p, otype))
+		goto out;
 
 	last = NULL;
 	datum = policydb_filenametr_search(p, &key);
@@ -2347,7 +2366,7 @@ static int filename_trans_read_helper(struct policydb *p, struct policy_file *fp
 		datum->otype = le32_to_cpu(buf[0]);
 
 		rc = -EINVAL;
-		if (!policydb_type_isvalid(p, datum->otype))
+		if (!policydb_simpletype_isvalid(p, datum->otype))
 			goto out;
 
 		dst = &datum->next;
diff --git a/security/selinux/ss/policydb.h b/security/selinux/ss/policydb.h
index 28dd91a7427f..73a0b793354e 100644
--- a/security/selinux/ss/policydb.h
+++ b/security/selinux/ss/policydb.h
@@ -317,6 +317,7 @@ extern int policydb_load_isids(struct policydb *p, struct sidtab *s);
 extern bool policydb_context_isvalid(const struct policydb *p, const struct context *c);
 extern bool policydb_class_isvalid(const struct policydb *p, u16 class);
 extern bool policydb_type_isvalid(const struct policydb *p, u32 type);
+extern bool policydb_simpletype_isvalid(const struct policydb *p, u32 type);
 extern bool policydb_role_isvalid(const struct policydb *p, u32 role);
 extern bool policydb_user_isvalid(const struct policydb *p, u32 user);
 extern bool policydb_boolean_isvalid(const struct policydb *p, u32 boolean);
-- 
2.45.2

[RFC PATCH 22/22] selinux: restrict policy strings

[Christian Göttsche]

From: Christian Göttsche <cgzones@googlemail.com>

Validate the characters and the lengths of strings parsed from binary
policies.

  * Disallow control characters
  * Limit characters of identifiers to alphanumeric, underscore, dash,
    and dot
  * Limit identifiers in length to 128, expect types to 1024 and
    categories to 32, characters (excluding NUL-terminator)

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 security/selinux/ss/conditional.c |  2 +-
 security/selinux/ss/policydb.c    | 60 ++++++++++++++++++++-----------
 security/selinux/ss/policydb.h    |  5 ++-
 3 files changed, 44 insertions(+), 23 deletions(-)

diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
index d37b4bdf6ba9..346102417cbf 100644
--- a/security/selinux/ss/conditional.c
+++ b/security/selinux/ss/conditional.c
@@ -280,7 +280,7 @@ int cond_read_bool(struct policydb *p, struct symtab *s, struct policy_file *fp)
 
 	len = le32_to_cpu(buf[2]);
 
-	rc = str_read(&key, GFP_KERNEL, fp, len);
+	rc = str_read(&key, GFP_KERNEL, fp, len, STR_IDENTIFIER, 128);
 	if (rc)
 		goto err;
 
diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index 917b468c5144..d98dfa6c3f30 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -1221,8 +1221,9 @@ static int context_read_and_validate(struct context *c, struct policydb *p,
  * binary representation file.
  */
 
-int str_read(char **strp, gfp_t flags, struct policy_file *fp, u32 len)
+int str_read(char **strp, gfp_t flags, struct policy_file *fp, u32 len, int kind, u32 max_len)
 {
+	u32 i;
 	int rc;
 	char *str;
 
@@ -1232,19 +1233,35 @@ int str_read(char **strp, gfp_t flags, struct policy_file *fp, u32 len)
 	if (oom_check(sizeof(char), len, fp))
 		return -EINVAL;
 
+	if (max_len != 0 && len > max_len)
+		return -EINVAL;
+
 	str = kmalloc(len + 1, flags | __GFP_NOWARN);
 	if (!str)
 		return -ENOMEM;
 
 	rc = next_entry(str, fp, len);
-	if (rc) {
-		kfree(str);
-		return rc;
+	if (rc)
+		goto bad_str;
+
+	rc = -EINVAL;
+	for (i = 0; i < len; i++) {
+		if (iscntrl(str[i]))
+			goto bad_str;
+
+		if (kind == STR_IDENTIFIER &&
+		    !(isalnum(str[i]) || str[i] == '_' || str[i] == '-' || str[i] == '.'))
+			goto bad_str;
+
 	}
 
 	str[len] = '\0';
 	*strp = str;
 	return 0;
+
+bad_str:
+	kfree(str);
+	return rc;
 }
 
 static int perm_read(struct policydb *p, struct symtab *s, struct policy_file *fp)
@@ -1269,7 +1286,7 @@ static int perm_read(struct policydb *p, struct symtab *s, struct policy_file *f
 	if (perdatum->value < 1 || perdatum->value > 32)
 		goto bad;
 
-	rc = str_read(&key, GFP_KERNEL, fp, len);
+	rc = str_read(&key, GFP_KERNEL, fp, len, STR_IDENTIFIER, 128);
 	if (rc)
 		goto bad;
 
@@ -1315,7 +1332,7 @@ static int common_read(struct policydb *p, struct symtab *s, struct policy_file
 		goto bad;
 	comdatum->permissions.nprim = le32_to_cpu(buf[2]);
 
-	rc = str_read(&key, GFP_KERNEL, fp, len);
+	rc = str_read(&key, GFP_KERNEL, fp, len, STR_IDENTIFIER, 128);
 	if (rc)
 		goto bad;
 
@@ -1552,12 +1569,12 @@ static int class_read(struct policydb *p, struct symtab *s, struct policy_file *
 
 	ncons = le32_to_cpu(buf[5]);
 
-	rc = str_read(&key, GFP_KERNEL, fp, len);
+	rc = str_read(&key, GFP_KERNEL, fp, len, STR_IDENTIFIER, 128);
 	if (rc)
 		goto bad;
 
 	if (len2) {
-		rc = str_read(&cladatum->comkey, GFP_KERNEL, fp, len2);
+		rc = str_read(&cladatum->comkey, GFP_KERNEL, fp, len2, STR_IDENTIFIER, 128);
 		if (rc)
 			goto bad;
 
@@ -1691,7 +1708,7 @@ static int role_read(struct policydb *p, struct symtab *s, struct policy_file *f
 	if (p->policyvers >= POLICYDB_VERSION_BOUNDARY)
 		role->bounds = le32_to_cpu(buf[2]);
 
-	rc = str_read(&key, GFP_KERNEL, fp, len);
+	rc = str_read(&key, GFP_KERNEL, fp, len, STR_IDENTIFIER, 128);
 	if (rc)
 		goto bad;
 
@@ -1758,7 +1775,7 @@ static int type_read(struct policydb *p, struct symtab *s, struct policy_file *f
 		typdatum->primary = le32_to_cpu(buf[2]);
 	}
 
-	rc = str_read(&key, GFP_KERNEL, fp, len);
+	rc = str_read(&key, GFP_KERNEL, fp, len, STR_IDENTIFIER, 1024);
 	if (rc)
 		goto bad;
 
@@ -1822,7 +1839,7 @@ static int user_read(struct policydb *p, struct symtab *s, struct policy_file *f
 	if (p->policyvers >= POLICYDB_VERSION_BOUNDARY)
 		usrdatum->bounds = le32_to_cpu(buf[2]);
 
-	rc = str_read(&key, GFP_KERNEL, fp, len);
+	rc = str_read(&key, GFP_KERNEL, fp, len, STR_IDENTIFIER, 128);
 	if (rc)
 		goto bad;
 
@@ -1871,7 +1888,7 @@ static int sens_read(struct policydb *p, struct symtab *s, struct policy_file *f
 		goto bad;
 	levdatum->isalias = val;
 
-	rc = str_read(&key, GFP_KERNEL, fp, len);
+	rc = str_read(&key, GFP_KERNEL, fp, len, STR_IDENTIFIER, 128);
 	if (rc)
 		goto bad;
 
@@ -1914,7 +1931,7 @@ static int cat_read(struct policydb *p, struct symtab *s, struct policy_file *fp
 		goto bad;
 	catdatum->isalias = val;
 
-	rc = str_read(&key, GFP_KERNEL, fp, len);
+	rc = str_read(&key, GFP_KERNEL, fp, len, STR_IDENTIFIER, 32);
 	if (rc)
 		goto bad;
 
@@ -2220,7 +2237,7 @@ static int filename_trans_read_helper_compat(struct policydb *p, struct policy_f
 	len = le32_to_cpu(buf[0]);
 
 	/* path component string */
-	rc = str_read(&name, GFP_KERNEL, fp, len);
+	rc = str_read(&name, GFP_KERNEL, fp, len, STR_UNCONSTRAINT, 0);
 	if (rc)
 		return rc;
 
@@ -2319,7 +2336,7 @@ static int filename_trans_read_helper(struct policydb *p, struct policy_file *fp
 	len = le32_to_cpu(buf[0]);
 
 	/* path component string */
-	rc = str_read(&name, GFP_KERNEL, fp, len);
+	rc = str_read(&name, GFP_KERNEL, fp, len, STR_UNCONSTRAINT, 0);
 	if (rc)
 		return rc;
 
@@ -2473,7 +2490,7 @@ static int genfs_read(struct policydb *p, struct policy_file *fp)
 		if (!newgenfs)
 			goto out;
 
-		rc = str_read(&newgenfs->fstype, GFP_KERNEL, fp, len);
+		rc = str_read(&newgenfs->fstype, GFP_KERNEL, fp, len, STR_IDENTIFIER, 128);
 		if (rc)
 			goto out;
 
@@ -2512,7 +2529,7 @@ static int genfs_read(struct policydb *p, struct policy_file *fp)
 			if (!newc)
 				goto out;
 
-			rc = str_read(&newc->u.name, GFP_KERNEL, fp, len);
+			rc = str_read(&newc->u.name, GFP_KERNEL, fp, len, STR_UNCONSTRAINT, 0);
 			if (rc)
 				goto out;
 
@@ -2615,7 +2632,7 @@ static int ocontext_read(struct policydb *p,
 					goto out;
 				len = le32_to_cpu(buf[0]);
 
-				rc = str_read(&c->u.name, GFP_KERNEL, fp, len);
+				rc = str_read(&c->u.name, GFP_KERNEL, fp, len, STR_IDENTIFIER, 128);
 				if (rc)
 					goto out;
 
@@ -2683,7 +2700,7 @@ static int ocontext_read(struct policydb *p,
 					goto out;
 
 				len = le32_to_cpu(buf[1]);
-				rc = str_read(&c->u.name, GFP_KERNEL, fp, len);
+				rc = str_read(&c->u.name, GFP_KERNEL, fp, len, STR_IDENTIFIER, 128);
 				if (rc)
 					goto out;
 
@@ -2749,7 +2766,7 @@ static int ocontext_read(struct policydb *p,
 				len = le32_to_cpu(buf[0]);
 
 				rc = str_read(&c->u.ibendport.dev_name,
-					      GFP_KERNEL, fp, len);
+					      GFP_KERNEL, fp, len, STR_IDENTIFIER, 128);
 				if (rc)
 					goto out;
 
@@ -2817,7 +2834,8 @@ int policydb_read(struct policydb *p, struct policy_file *fp)
 		goto bad;
 	}
 
-	rc = str_read(&policydb_str, GFP_KERNEL, fp, len);
+	rc = str_read(&policydb_str, GFP_KERNEL, fp, len,
+		      STR_UNCONSTRAINT, strlen(POLICYDB_STRING));
 	if (rc) {
 		if (rc == -ENOMEM) {
 			pr_err("SELinux:  unable to allocate memory for policydb string of length %d\n",
diff --git a/security/selinux/ss/policydb.h b/security/selinux/ss/policydb.h
index 73a0b793354e..fb05174b1b16 100644
--- a/security/selinux/ss/policydb.h
+++ b/security/selinux/ss/policydb.h
@@ -403,7 +403,10 @@ static inline const char *sym_name(const struct policydb *p, unsigned int sym_nu
 	return p->sym_val_to_name[sym_num][element_nr];
 }
 
-extern int str_read(char **strp, gfp_t flags, struct policy_file *fp, u32 len);
+#define STR_UNCONSTRAINT 0
+#define STR_IDENTIFIER 1
+extern int str_read(char **strp, gfp_t flags, struct policy_file *fp, u32 len,
+		    int kind, u32 max_len);
 
 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);
-- 
2.45.2

Re: [RFC PATCH 11/22] selinux: more strict policy parsing

[Thiébaud Weksteen]

On Sat, Nov 16, 2024 at 12:37 AM Christian Göttsche
<cgoettsche@seltendoof.de> wrote:
>
> From: Christian Göttsche <cgzones@googlemail.com>
>
> Be more strict during parsing of policies and reject invalid values.
>
> Add some error messages in the case of policy parse failures, to
> enhance debugging, either on a malformed policy or a too strict check.
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---

Thanks for the patch.

> +               switch (xperms.specified) {
> +               case AVTAB_XPERMS_IOCTLFUNCTION:
> +               case AVTAB_XPERMS_IOCTLDRIVER:
> +               case AVTAB_XPERMS_NLMSG:
> +                       break;
> +               default:
> +                       pr_err("SELinux: avtab: invalid xperm specifier %#x\n", xperms.specified);
> +                       return -EINVAL;
> +               }
>                 rc = next_entry(&xperms.driver, fp, sizeof(u8));

I think this is too restrictive. We should be able to add extended
permissions in a future policy and this should be gracefully handled
by the kernel. You could use a pr_info instead, similarly to what is
done in selinux_set_mapping for unknown permissions.

Re: [RFC PATCH 22/22] selinux: restrict policy strings

[Daniel Burgener]

On 11/15/2024 8:35 AM, Christian Göttsche wrote:
> From: Christian Göttsche <cgzones@googlemail.com>
> 
> Validate the characters and the lengths of strings parsed from binary
> policies.
> 
>    * Disallow control characters
>    * Limit characters of identifiers to alphanumeric, underscore, dash,
>      and dot
>    * Limit identifiers in length to 128, expect types to 1024 and
>      categories to 32, characters (excluding NUL-terminator)
> 
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
>   security/selinux/ss/conditional.c |  2 +-
>   security/selinux/ss/policydb.c    | 60 ++++++++++++++++++++-----------
>   security/selinux/ss/policydb.h    |  5 ++-
>   3 files changed, 44 insertions(+), 23 deletions(-)
> 
> diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
> index d37b4bdf6ba9..346102417cbf 100644
> --- a/security/selinux/ss/conditional.c
> +++ b/security/selinux/ss/conditional.c
> @@ -280,7 +280,7 @@ int cond_read_bool(struct policydb *p, struct symtab *s, struct policy_file *fp)
>   
>   	len = le32_to_cpu(buf[2]);
>   
> -	rc = str_read(&key, GFP_KERNEL, fp, len);
> +	rc = str_read(&key, GFP_KERNEL, fp, len, STR_IDENTIFIER, 128);
>   	if (rc)
>   		goto err;
>

It would be nice if these limits were named constants instead of magic 
numbers.  Right now it's hard to tell if all the "128"s are essentially 
the same limit referenced in different places, or if they could (in 
theory) be changed independently.

> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 917b468c5144..d98dfa6c3f30 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -1221,8 +1221,9 @@ static int context_read_and_validate(struct context *c, struct policydb *p,
>    * binary representation file.
>    */
>   
> -int str_read(char **strp, gfp_t flags, struct policy_file *fp, u32 len)
> +int str_read(char **strp, gfp_t flags, struct policy_file *fp, u32 len, int kind, u32 max_len)
>   {
> +	u32 i;
>   	int rc;
>   	char *str;
>   
> @@ -1232,19 +1233,35 @@ int str_read(char **strp, gfp_t flags, struct policy_file *fp, u32 len)
>   	if (oom_check(sizeof(char), len, fp))
>   		return -EINVAL;
>   
> +	if (max_len != 0 && len > max_len)
> +		return -EINVAL;
> +
>   	str = kmalloc(len + 1, flags | __GFP_NOWARN);
>   	if (!str)
>   		return -ENOMEM;
>   
>   	rc = next_entry(str, fp, len);
> -	if (rc) {
> -		kfree(str);
> -		return rc;
> +	if (rc)
> +		goto bad_str;
> +
> +	rc = -EINVAL;
> +	for (i = 0; i < len; i++) {
> +		if (iscntrl(str[i]))
> +			goto bad_str;
> +
> +		if (kind == STR_IDENTIFIER &&
> +		    !(isalnum(str[i]) || str[i] == '_' || str[i] == '-' || str[i] == '.'))
> +			goto bad_str;
> +
>   	}
>   
>   	str[len] = '\0';
>   	*strp = str;
>   	return 0;
> +
> +bad_str:
> +	kfree(str);
> +	return rc;
>   }
>   
>   static int perm_read(struct policydb *p, struct symtab *s, struct policy_file *fp)
> @@ -1269,7 +1286,7 @@ static int perm_read(struct policydb *p, struct symtab *s, struct policy_file *f
>   	if (perdatum->value < 1 || perdatum->value > 32)
>   		goto bad;
>   
> -	rc = str_read(&key, GFP_KERNEL, fp, len);
> +	rc = str_read(&key, GFP_KERNEL, fp, len, STR_IDENTIFIER, 128);
>   	if (rc)
>   		goto bad;
>   
> @@ -1315,7 +1332,7 @@ static int common_read(struct policydb *p, struct symtab *s, struct policy_file
>   		goto bad;
>   	comdatum->permissions.nprim = le32_to_cpu(buf[2]);
>   
> -	rc = str_read(&key, GFP_KERNEL, fp, len);
> +	rc = str_read(&key, GFP_KERNEL, fp, len, STR_IDENTIFIER, 128);
>   	if (rc)
>   		goto bad;
>   
> @@ -1552,12 +1569,12 @@ static int class_read(struct policydb *p, struct symtab *s, struct policy_file *
>   
>   	ncons = le32_to_cpu(buf[5]);
>   
> -	rc = str_read(&key, GFP_KERNEL, fp, len);
> +	rc = str_read(&key, GFP_KERNEL, fp, len, STR_IDENTIFIER, 128);
>   	if (rc)
>   		goto bad;
>   
>   	if (len2) {
> -		rc = str_read(&cladatum->comkey, GFP_KERNEL, fp, len2);
> +		rc = str_read(&cladatum->comkey, GFP_KERNEL, fp, len2, STR_IDENTIFIER, 128);
>   		if (rc)
>   			goto bad;
>   
> @@ -1691,7 +1708,7 @@ static int role_read(struct policydb *p, struct symtab *s, struct policy_file *f
>   	if (p->policyvers >= POLICYDB_VERSION_BOUNDARY)
>   		role->bounds = le32_to_cpu(buf[2]);
>   
> -	rc = str_read(&key, GFP_KERNEL, fp, len);
> +	rc = str_read(&key, GFP_KERNEL, fp, len, STR_IDENTIFIER, 128);
>   	if (rc)
>   		goto bad;
>   
> @@ -1758,7 +1775,7 @@ static int type_read(struct policydb *p, struct symtab *s, struct policy_file *f
>   		typdatum->primary = le32_to_cpu(buf[2]);
>   	}
>   
> -	rc = str_read(&key, GFP_KERNEL, fp, len);
> +	rc = str_read(&key, GFP_KERNEL, fp, len, STR_IDENTIFIER, 1024);
>   	if (rc)
>   		goto bad;
>   
> @@ -1822,7 +1839,7 @@ static int user_read(struct policydb *p, struct symtab *s, struct policy_file *f
>   	if (p->policyvers >= POLICYDB_VERSION_BOUNDARY)
>   		usrdatum->bounds = le32_to_cpu(buf[2]);
>   
> -	rc = str_read(&key, GFP_KERNEL, fp, len);
> +	rc = str_read(&key, GFP_KERNEL, fp, len, STR_IDENTIFIER, 128);
>   	if (rc)
>   		goto bad;
>   
> @@ -1871,7 +1888,7 @@ static int sens_read(struct policydb *p, struct symtab *s, struct policy_file *f
>   		goto bad;
>   	levdatum->isalias = val;
>   
> -	rc = str_read(&key, GFP_KERNEL, fp, len);
> +	rc = str_read(&key, GFP_KERNEL, fp, len, STR_IDENTIFIER, 128);
>   	if (rc)
>   		goto bad;
>   
> @@ -1914,7 +1931,7 @@ static int cat_read(struct policydb *p, struct symtab *s, struct policy_file *fp
>   		goto bad;
>   	catdatum->isalias = val;
>   
> -	rc = str_read(&key, GFP_KERNEL, fp, len);
> +	rc = str_read(&key, GFP_KERNEL, fp, len, STR_IDENTIFIER, 32);
>   	if (rc)
>   		goto bad;

The category restriction is more tight than the sensitivity one because 
a context may have many categories?  I guess that makes sense, but it 
feels counterintuitive from a user perspective, because I feel like 
users tend to think of categories and sensitivities as essentially the 
same thing.  Would dropping the sensitivity limit to 32 to match the 
category limit make sense?

Is there a more strict limit on the number of categories a context can 
have than the U32_MAX from symtab.nprim?  Because that will allow 
exceeding the page size using too many categories regardless of length 
distinctions, which is a concern if the motivation here is about 
potential future untrusted policy loaders in a namespaced environment.

-Daniel

Re: [RFC PATCH 06/22] selinux: rename comparison functions for clarity

[Daniel Burgener]

On 11/15/2024 8:35 AM, Christian Göttsche wrote:
> From: Christian Göttsche <cgzones@googlemail.com>
> 
> The functions context_cmp() and mls_context_cmp() are not traditional
> C style compare functions returning -1, 0, and 1 for less than, equal,
> and greater than; they only return whether their arguments are equal.
> 
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
>   security/selinux/ss/context.c  |  2 +-
>   security/selinux/ss/context.h  | 10 +++++-----
>   security/selinux/ss/services.c |  2 +-
>   security/selinux/ss/sidtab.c   |  2 +-
>   4 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/security/selinux/ss/context.c b/security/selinux/ss/context.c
> index e39990f494dd..a528b7f76280 100644
> --- a/security/selinux/ss/context.c
> +++ b/security/selinux/ss/context.c
> @@ -20,7 +20,7 @@ u32 context_compute_hash(const struct context *c)
>   	 * context struct with only the len & str set (and vice versa)
>   	 * under a given policy. Since context structs from different
>   	 * policies should never meet, it is safe to hash valid and
> -	 * invalid contexts differently. The context_cmp() function
> +	 * invalid contexts differently. The context_equal() function
>   	 * already operates under the same assumption.
>   	 */
>   	if (c->len)
> diff --git a/security/selinux/ss/context.h b/security/selinux/ss/context.h
> index 7ccab2e6965f..e1307f6f7f50 100644
> --- a/security/selinux/ss/context.h
> +++ b/security/selinux/ss/context.h
> @@ -132,8 +132,8 @@ static inline int mls_context_glblub(struct context *dst,
>   	return rc;
>   }
>   
> -static inline int mls_context_cmp(const struct context *c1,
> -				  const struct context *c2)
> +static inline bool mls_context_equal(const struct context *c1,
> +				     const struct context *c2)
>   {
>   	return ((c1->range.level[0].sens == c2->range.level[0].sens) &&
>   		ebitmap_cmp(&c1->range.level[0].cat, &c2->range.level[0].cat) &&

Should the same logic in this patch be applied to ebitmap_cmp as well?

-Daniel

Re: [RFC PATCH 07/22] selinux: use known type instead of void pointer

[Daniel Burgener]

On 11/15/2024 8:35 AM, Christian Göttsche wrote:
> From: Christian Göttsche <cgzones@googlemail.com>
> 
> Improve type safety and readability by using the known type.
> 
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
>   security/selinux/ss/avtab.c       |  8 +--
>   security/selinux/ss/avtab.h       |  9 ++--
>   security/selinux/ss/conditional.c | 12 ++---
>   security/selinux/ss/conditional.h |  6 +--
>   security/selinux/ss/ebitmap.c     |  4 +-
>   security/selinux/ss/ebitmap.h     |  5 +-
>   security/selinux/ss/policydb.c    | 88 +++++++++++++++----------------
>   security/selinux/ss/policydb.h    | 17 +++---
>   8 files changed, 76 insertions(+), 73 deletions(-)
> 
> diff --git a/security/selinux/ss/avtab.c b/security/selinux/ss/avtab.c
> index 8e400dd736b7..23210faaa046 100644
> --- a/security/selinux/ss/avtab.c
> +++ b/security/selinux/ss/avtab.c
> @@ -336,7 +336,7 @@ static const uint16_t spec_order[] = {
>   };
>   /* clang-format on */
>   
> -int avtab_read_item(struct avtab *a, void *fp, struct policydb *pol,
> +int avtab_read_item(struct avtab *a, struct policy_file *fp, struct policydb *pol,
>   		    int (*insertf)(struct avtab *a, const struct avtab_key *k,
>   				   const struct avtab_datum *d, void *p),
>   		    void *p)
> @@ -500,7 +500,7 @@ static int avtab_insertf(struct avtab *a, const struct avtab_key *k,
>   	return avtab_insert(a, k, d);
>   }
>   
> -int avtab_read(struct avtab *a, void *fp, struct policydb *pol)
> +int avtab_read(struct avtab *a, struct policy_file *fp, struct policydb *pol)
>   {
>   	int rc;
>   	__le32 buf[1];
> @@ -543,7 +543,7 @@ int avtab_read(struct avtab *a, void *fp, struct policydb *pol)
>   	goto out;
>   }
>   
> -int avtab_write_item(struct policydb *p, const struct avtab_node *cur, void *fp)
> +int avtab_write_item(struct policydb *p, const struct avtab_node *cur, struct policy_file *fp)
>   {
>   	__le16 buf16[4];
>   	__le32 buf32[ARRAY_SIZE(cur->datum.u.xperms->perms.p)];
> @@ -579,7 +579,7 @@ int avtab_write_item(struct policydb *p, const struct avtab_node *cur, void *fp)
>   	return 0;
>   }
>   
> -int avtab_write(struct policydb *p, struct avtab *a, void *fp)
> +int avtab_write(struct policydb *p, struct avtab *a, struct policy_file *fp)
>   {
>   	u32 i;
>   	int rc = 0;
> diff --git a/security/selinux/ss/avtab.h b/security/selinux/ss/avtab.h
> index e10b78e61611..c671b6e909d4 100644
> --- a/security/selinux/ss/avtab.h
> +++ b/security/selinux/ss/avtab.h
> @@ -105,15 +105,16 @@ static inline void avtab_hash_eval(struct avtab *h, const char *tag)
>   #endif
>   
>   struct policydb;
> -int avtab_read_item(struct avtab *a, void *fp, struct policydb *pol,
> +struct policy_file;
> +int avtab_read_item(struct avtab *a, struct policy_file *fp, struct policydb *pol,
>   		    int (*insert)(struct avtab *a, const struct avtab_key *k,
>   				  const struct avtab_datum *d, void *p),
>   		    void *p);
>   
> -int avtab_read(struct avtab *a, void *fp, struct policydb *pol);
> +int avtab_read(struct avtab *a, struct policy_file *fp, struct policydb *pol);
>   int avtab_write_item(struct policydb *p, const struct avtab_node *cur,
> -		     void *fp);
> -int avtab_write(struct policydb *p, struct avtab *a, void *fp);
> +		     struct policy_file *fp);
> +int avtab_write(struct policydb *p, struct avtab *a, struct policy_file *fp);
>   
>   struct avtab_node *avtab_insert_nonunique(struct avtab *h,
>   					  const struct avtab_key *key,
> diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
> index 64ba95e40a6f..244dc8279113 100644
> --- a/security/selinux/ss/conditional.c
> +++ b/security/selinux/ss/conditional.c
> @@ -206,7 +206,7 @@ static int bool_isvalid(struct cond_bool_datum *b)
>   	return 1;
>   }
>   
> -int cond_read_bool(struct policydb *p, struct symtab *s, void *fp)
> +int cond_read_bool(struct policydb *p, struct symtab *s, struct policy_file *fp)
>   {
>   	char *key = NULL;
>   	struct cond_bool_datum *booldatum;
> @@ -323,7 +323,7 @@ static int cond_insertf(struct avtab *a, const struct avtab_key *k,
>   	return 0;
>   }
>   
> -static int cond_read_av_list(struct policydb *p, void *fp,
> +static int cond_read_av_list(struct policydb *p, struct policy_file *fp,
>   			     struct cond_av_list *list,
>   			     struct cond_av_list *other)
>   {
> @@ -375,7 +375,7 @@ static int expr_node_isvalid(struct policydb *p, struct cond_expr_node *expr)
>   	return 1;
>   }
>   
> -static int cond_read_node(struct policydb *p, struct cond_node *node, void *fp)
> +static int cond_read_node(struct policydb *p, struct cond_node *node, struct policy_file *fp)
>   {
>   	__le32 buf[2];
>   	u32 i, len;
> @@ -415,7 +415,7 @@ static int cond_read_node(struct policydb *p, struct cond_node *node, void *fp)
>   	return cond_read_av_list(p, fp, &node->false_list, &node->true_list);
>   }
>   
> -int cond_read_list(struct policydb *p, void *fp)
> +int cond_read_list(struct policydb *p, struct policy_file *fp)
>   {
>   	__le32 buf[1];
>   	u32 i, len;
> @@ -453,7 +453,7 @@ int cond_write_bool(void *vkey, void *datum, void *ptr)
>   	char *key = vkey;
>   	struct cond_bool_datum *booldatum = datum;
>   	struct policy_data *pd = ptr;
> -	void *fp = pd->fp;
> +	struct policy_file *fp = pd->fp;
>   	__le32 buf[3];
>   	u32 len;
>   	int rc;
> @@ -536,7 +536,7 @@ static int cond_write_node(struct policydb *p, struct cond_node *node,
>   	return 0;
>   }
>   
> -int cond_write_list(struct policydb *p, void *fp)
> +int cond_write_list(struct policydb *p, struct policy_file *fp)
>   {
>   	u32 i;
>   	__le32 buf[1];
> diff --git a/security/selinux/ss/conditional.h b/security/selinux/ss/conditional.h
> index 8827715bad75..468e98ad3ea1 100644
> --- a/security/selinux/ss/conditional.h
> +++ b/security/selinux/ss/conditional.h
> @@ -68,10 +68,10 @@ int cond_destroy_bool(void *key, void *datum, void *p);
>   
>   int cond_index_bool(void *key, void *datum, void *datap);
>   
> -int cond_read_bool(struct policydb *p, struct symtab *s, void *fp);
> -int cond_read_list(struct policydb *p, void *fp);
> +int cond_read_bool(struct policydb *p, struct symtab *s, struct policy_file *fp);
> +int cond_read_list(struct policydb *p, struct policy_file *fp);
>   int cond_write_bool(void *key, void *datum, void *ptr);
> -int cond_write_list(struct policydb *p, void *fp);
> +int cond_write_list(struct policydb *p, struct policy_file *fp);
>   
>   void cond_compute_av(struct avtab *ctab, struct avtab_key *key,
>   		     struct av_decision *avd, struct extended_perms *xperms);
> diff --git a/security/selinux/ss/ebitmap.c b/security/selinux/ss/ebitmap.c
> index 99c01be15115..cd84357db2c4 100644
> --- a/security/selinux/ss/ebitmap.c
> +++ b/security/selinux/ss/ebitmap.c
> @@ -360,7 +360,7 @@ void ebitmap_destroy(struct ebitmap *e)
>   	e->node = NULL;
>   }
>   
> -int ebitmap_read(struct ebitmap *e, void *fp)
> +int ebitmap_read(struct ebitmap *e, struct policy_file *fp)
>   {
>   	struct ebitmap_node *n = NULL;
>   	u32 mapunit, count, startbit, index, i;
> @@ -478,7 +478,7 @@ int ebitmap_read(struct ebitmap *e, void *fp)
>   	goto out;
>   }
>   
> -int ebitmap_write(const struct ebitmap *e, void *fp)
> +int ebitmap_write(const struct ebitmap *e, struct policy_file *fp)
>   {
>   	struct ebitmap_node *n;
>   	u32 bit, count, last_bit, last_startbit;
> diff --git a/security/selinux/ss/ebitmap.h b/security/selinux/ss/ebitmap.h
> index ba2ac3da1153..e0150695566c 100644
> --- a/security/selinux/ss/ebitmap.h
> +++ b/security/selinux/ss/ebitmap.h
> @@ -129,8 +129,9 @@ int ebitmap_contains(const struct ebitmap *e1, const struct ebitmap *e2,
>   int ebitmap_get_bit(const struct ebitmap *e, u32 bit);
>   int ebitmap_set_bit(struct ebitmap *e, u32 bit, int value);
>   void ebitmap_destroy(struct ebitmap *e);
> -int ebitmap_read(struct ebitmap *e, void *fp);
> -int ebitmap_write(const struct ebitmap *e, void *fp);
> +struct policy_file;
> +int ebitmap_read(struct ebitmap *e, struct policy_file *fp);
> +int ebitmap_write(const struct ebitmap *e, struct policy_file *fp);
>   u32 ebitmap_hash(const struct ebitmap *e, u32 hash);
>   
>   #ifdef CONFIG_NETLABEL
> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index d04d9ada3835..e94ecb81c6d3 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -994,7 +994,7 @@ int policydb_context_isvalid(struct policydb *p, struct context *c)
>    * Read a MLS range structure from a policydb binary
>    * representation file.
>    */
> -static int mls_read_range_helper(struct mls_range *r, void *fp)
> +static int mls_read_range_helper(struct mls_range *r, struct policy_file *fp)
>   {
>   	__le32 buf[2];
>   	u32 items;
> @@ -1054,7 +1054,7 @@ static int mls_read_range_helper(struct mls_range *r, void *fp)
>    * from a policydb binary representation file.
>    */
>   static int context_read_and_validate(struct context *c, struct policydb *p,
> -				     void *fp)
> +				     struct policy_file *fp)
>   {
>   	__le32 buf[3];
>   	int rc;
> @@ -1092,7 +1092,7 @@ static int context_read_and_validate(struct context *c, struct policydb *p,
>    * binary representation file.
>    */
>   
> -static int str_read(char **strp, gfp_t flags, void *fp, u32 len)
> +static int str_read(char **strp, gfp_t flags, struct policy_file *fp, u32 len)
>   {
>   	int rc;
>   	char *str;
> @@ -1115,7 +1115,7 @@ static int str_read(char **strp, gfp_t flags, void *fp, u32 len)
>   	return 0;
>   }
>   
> -static int perm_read(struct policydb *p, struct symtab *s, void *fp)
> +static int perm_read(struct policydb *p, struct symtab *s, struct policy_file *fp)
>   {
>   	char *key = NULL;
>   	struct perm_datum *perdatum;
> @@ -1148,7 +1148,7 @@ static int perm_read(struct policydb *p, struct symtab *s, void *fp)
>   	return rc;
>   }
>   
> -static int common_read(struct policydb *p, struct symtab *s, void *fp)
> +static int common_read(struct policydb *p, struct symtab *s, struct policy_file *fp)
>   {
>   	char *key = NULL;
>   	struct common_datum *comdatum;
> @@ -1200,7 +1200,7 @@ static void type_set_init(struct type_set *t)
>   	ebitmap_init(&t->negset);
>   }
>   
> -static int type_set_read(struct type_set *t, void *fp)
> +static int type_set_read(struct type_set *t, struct policy_file *fp)
>   {
>   	__le32 buf[1];
>   	int rc;
> @@ -1219,7 +1219,7 @@ static int type_set_read(struct type_set *t, void *fp)
>   }
>   
>   static int read_cons_helper(struct policydb *p, struct constraint_node **nodep,
> -			    u32 ncons, int allowxtarget, void *fp)
> +			    u32 ncons, int allowxtarget, struct policy_file *fp)
>   {
>   	struct constraint_node *c, *lc;
>   	struct constraint_expr *e, *le;
> @@ -1313,7 +1313,7 @@ static int read_cons_helper(struct policydb *p, struct constraint_node **nodep,
>   	return 0;
>   }
>   
> -static int class_read(struct policydb *p, struct symtab *s, void *fp)
> +static int class_read(struct policydb *p, struct symtab *s, struct policy_file *fp)
>   {
>   	char *key = NULL;
>   	struct class_datum *cladatum;
> @@ -1410,7 +1410,7 @@ static int class_read(struct policydb *p, struct symtab *s, void *fp)
>   	return rc;
>   }
>   
> -static int role_read(struct policydb *p, struct symtab *s, void *fp)
> +static int role_read(struct policydb *p, struct symtab *s, struct policy_file *fp)
>   {
>   	char *key = NULL;
>   	struct role_datum *role;
> @@ -1467,7 +1467,7 @@ static int role_read(struct policydb *p, struct symtab *s, void *fp)
>   	return rc;
>   }
>   
> -static int type_read(struct policydb *p, struct symtab *s, void *fp)
> +static int type_read(struct policydb *p, struct symtab *s, struct policy_file *fp)
>   {
>   	char *key = NULL;
>   	struct type_datum *typdatum;
> @@ -1519,7 +1519,7 @@ static int type_read(struct policydb *p, struct symtab *s, void *fp)
>    * Read a MLS level structure from a policydb binary
>    * representation file.
>    */
> -static int mls_read_level(struct mls_level *lp, void *fp)
> +static int mls_read_level(struct mls_level *lp, struct policy_file *fp)
>   {
>   	__le32 buf[1];
>   	int rc;
> @@ -1541,7 +1541,7 @@ static int mls_read_level(struct mls_level *lp, void *fp)
>   	return 0;
>   }
>   
> -static int user_read(struct policydb *p, struct symtab *s, void *fp)
> +static int user_read(struct policydb *p, struct symtab *s, struct policy_file *fp)
>   {
>   	char *key = NULL;
>   	struct user_datum *usrdatum;
> @@ -1592,7 +1592,7 @@ static int user_read(struct policydb *p, struct symtab *s, void *fp)
>   	return rc;
>   }
>   
> -static int sens_read(struct policydb *p, struct symtab *s, void *fp)
> +static int sens_read(struct policydb *p, struct symtab *s, struct policy_file *fp)
>   {
>   	char *key = NULL;
>   	struct level_datum *levdatum;
> @@ -1633,7 +1633,7 @@ static int sens_read(struct policydb *p, struct symtab *s, void *fp)
>   	return rc;
>   }
>   
> -static int cat_read(struct policydb *p, struct symtab *s, void *fp)
> +static int cat_read(struct policydb *p, struct symtab *s, struct policy_file *fp)
>   {
>   	char *key = NULL;
>   	struct cat_datum *catdatum;
> @@ -1668,7 +1668,7 @@ static int cat_read(struct policydb *p, struct symtab *s, void *fp)
>   
>   /* clang-format off */
>   static int (*const read_f[SYM_NUM])(struct policydb *p, struct symtab *s,
> -				    void *fp) = {
> +				    struct policy_file *fp) = {
>   	common_read,
>   	class_read,
>   	role_read,
> @@ -1838,7 +1838,7 @@ u32 string_to_av_perm(struct policydb *p, u16 tclass, const char *name)
>   	return 1U << (perdatum->value - 1);
>   }
>   
> -static int range_read(struct policydb *p, void *fp)
> +static int range_read(struct policydb *p, struct policy_file *fp)
>   {
>   	struct range_trans *rt = NULL;
>   	struct mls_range *r = NULL;
> @@ -1915,7 +1915,7 @@ static int range_read(struct policydb *p, void *fp)
>   	return rc;
>   }
>   
> -static int filename_trans_read_helper_compat(struct policydb *p, void *fp)
> +static int filename_trans_read_helper_compat(struct policydb *p, struct policy_file *fp)
>   {
>   	struct filename_trans_key key, *ft = NULL;
>   	struct filename_trans_datum *last, *datum = NULL;
> @@ -2000,7 +2000,7 @@ static int filename_trans_read_helper_compat(struct policydb *p, void *fp)
>   	return rc;
>   }
>   
> -static int filename_trans_read_helper(struct policydb *p, void *fp)
> +static int filename_trans_read_helper(struct policydb *p, struct policy_file *fp)
>   {
>   	struct filename_trans_key *ft = NULL;
>   	struct filename_trans_datum **dst, *datum, *first = NULL;
> @@ -2089,7 +2089,7 @@ static int filename_trans_read_helper(struct policydb *p, void *fp)
>   	return rc;
>   }
>   
> -static int filename_trans_read(struct policydb *p, void *fp)
> +static int filename_trans_read(struct policydb *p, struct policy_file *fp)
>   {
>   	u32 nel, i;
>   	__le32 buf[1];
> @@ -2130,7 +2130,7 @@ static int filename_trans_read(struct policydb *p, void *fp)
>   	return 0;
>   }
>   
> -static int genfs_read(struct policydb *p, void *fp)
> +static int genfs_read(struct policydb *p, struct policy_file *fp)
>   {
>   	int rc;
>   	u32 i, j, nel, nel2, len, len2;
> @@ -2244,7 +2244,7 @@ static int genfs_read(struct policydb *p, void *fp)
>   }
>   
>   static int ocontext_read(struct policydb *p,
> -			 const struct policydb_compat_info *info, void *fp)
> +			 const struct policydb_compat_info *info, struct policy_file *fp)
>   {
>   	int rc;
>   	unsigned int i;
> @@ -2441,7 +2441,7 @@ static int ocontext_read(struct policydb *p,
>    * Read the configuration data from a policy database binary
>    * representation file into a policy database structure.
>    */
> -int policydb_read(struct policydb *p, void *fp)
> +int policydb_read(struct policydb *p, struct policy_file *fp)
>   {
>   	struct role_allow *ra, *lra;
>   	struct role_trans_key *rtk = NULL;
> @@ -2764,7 +2764,7 @@ int policydb_read(struct policydb *p, void *fp)
>    * Write a MLS level structure to a policydb binary
>    * representation file.
>    */
> -static int mls_write_level(struct mls_level *l, void *fp)
> +static int mls_write_level(struct mls_level *l, struct policy_file *fp)
>   {
>   	__le32 buf[1];
>   	int rc;
> @@ -2785,7 +2785,7 @@ static int mls_write_level(struct mls_level *l, void *fp)
>    * Write a MLS range structure to a policydb binary
>    * representation file.
>    */
> -static int mls_write_range_helper(struct mls_range *r, void *fp)
> +static int mls_write_range_helper(struct mls_range *r, struct policy_file *fp)
>   {
>   	__le32 buf[3];
>   	size_t items;
> @@ -2825,7 +2825,7 @@ static int sens_write(void *vkey, void *datum, void *ptr)
>   	char *key = vkey;
>   	struct level_datum *levdatum = datum;
>   	struct policy_data *pd = ptr;
> -	void *fp = pd->fp;
> +	struct policy_file *fp = pd->fp;
>   	__le32 buf[2];
>   	size_t len;
>   	int rc;
> @@ -2853,7 +2853,7 @@ static int cat_write(void *vkey, void *datum, void *ptr)
>   	char *key = vkey;
>   	struct cat_datum *catdatum = datum;
>   	struct policy_data *pd = ptr;
> -	void *fp = pd->fp;
> +	struct policy_file *fp = pd->fp;
>   	__le32 buf[3];
>   	size_t len;
>   	int rc;
> @@ -2878,7 +2878,7 @@ static int role_trans_write_one(void *key, void *datum, void *ptr)
>   	struct role_trans_key *rtk = key;
>   	struct role_trans_datum *rtd = datum;
>   	struct policy_data *pd = ptr;
> -	void *fp = pd->fp;
> +	struct policy_file *fp = pd->fp;
>   	struct policydb *p = pd->p;
>   	__le32 buf[3];
>   	int rc;
> @@ -2898,7 +2898,7 @@ static int role_trans_write_one(void *key, void *datum, void *ptr)
>   	return 0;
>   }
>   
> -static int role_trans_write(struct policydb *p, void *fp)
> +static int role_trans_write(struct policydb *p, struct policy_file *fp)
>   {
>   	struct policy_data pd = { .p = p, .fp = fp };
>   	__le32 buf[1];
> @@ -2912,7 +2912,7 @@ static int role_trans_write(struct policydb *p, void *fp)
>   	return hashtab_map(&p->role_tr, role_trans_write_one, &pd);
>   }
>   
> -static int role_allow_write(struct role_allow *r, void *fp)
> +static int role_allow_write(struct role_allow *r, struct policy_file *fp)
>   {
>   	struct role_allow *ra;
>   	__le32 buf[2];
> @@ -2940,7 +2940,7 @@ static int role_allow_write(struct role_allow *r, void *fp)
>    * Write a security context structure
>    * to a policydb binary representation file.
>    */
> -static int context_write(struct policydb *p, struct context *c, void *fp)
> +static int context_write(struct policydb *p, struct context *c, struct policy_file *fp)
>   {
>   	int rc;
>   	__le32 buf[3];
> @@ -2993,7 +2993,7 @@ static int common_write(void *vkey, void *datum, void *ptr)
>   	char *key = vkey;
>   	struct common_datum *comdatum = datum;
>   	struct policy_data *pd = ptr;
> -	void *fp = pd->fp;
> +	struct policy_file *fp = pd->fp;
>   	__le32 buf[4];
>   	size_t len;
>   	int rc;
> @@ -3018,7 +3018,7 @@ static int common_write(void *vkey, void *datum, void *ptr)
>   	return 0;
>   }
>   
> -static int type_set_write(struct type_set *t, void *fp)
> +static int type_set_write(struct type_set *t, struct policy_file *fp)
>   {
>   	int rc;
>   	__le32 buf[1];
> @@ -3037,7 +3037,7 @@ static int type_set_write(struct type_set *t, void *fp)
>   }
>   
>   static int write_cons_helper(struct policydb *p, struct constraint_node *node,
> -			     void *fp)
> +			     struct policy_file *fp)
>   {
>   	struct constraint_node *c;
>   	struct constraint_expr *e;
> @@ -3088,7 +3088,7 @@ static int class_write(void *vkey, void *datum, void *ptr)
>   	char *key = vkey;
>   	struct class_datum *cladatum = datum;
>   	struct policy_data *pd = ptr;
> -	void *fp = pd->fp;
> +	struct policy_file *fp = pd->fp;
>   	struct policydb *p = pd->p;
>   	struct constraint_node *c;
>   	__le32 buf[6];
> @@ -3173,7 +3173,7 @@ static int role_write(void *vkey, void *datum, void *ptr)
>   	char *key = vkey;
>   	struct role_datum *role = datum;
>   	struct policy_data *pd = ptr;
> -	void *fp = pd->fp;
> +	struct policy_file *fp = pd->fp;
>   	struct policydb *p = pd->p;
>   	__le32 buf[3];
>   	size_t items, len;
> @@ -3213,7 +3213,7 @@ static int type_write(void *vkey, void *datum, void *ptr)
>   	struct type_datum *typdatum = datum;
>   	struct policy_data *pd = ptr;
>   	struct policydb *p = pd->p;
> -	void *fp = pd->fp;
> +	struct policy_file *fp = pd->fp;
>   	__le32 buf[4];
>   	int rc;
>   	size_t items, len;
> @@ -3254,7 +3254,7 @@ static int user_write(void *vkey, void *datum, void *ptr)
>   	struct user_datum *usrdatum = datum;
>   	struct policy_data *pd = ptr;
>   	struct policydb *p = pd->p;
> -	void *fp = pd->fp;
> +	struct policy_file *fp = pd->fp;
>   	__le32 buf[3];
>   	size_t items, len;
>   	int rc;
> @@ -3439,7 +3439,7 @@ static int ocontext_write(struct policydb *p,
>   	return 0;
>   }
>   
> -static int genfs_write(struct policydb *p, void *fp)
> +static int genfs_write(struct policydb *p, struct policy_file *fp)
>   {
>   	struct genfs *genfs;
>   	struct ocontext *c;
> @@ -3497,7 +3497,7 @@ static int range_write_helper(void *key, void *data, void *ptr)
>   	struct range_trans *rt = key;
>   	struct mls_range *r = data;
>   	struct policy_data *pd = ptr;
> -	void *fp = pd->fp;
> +	struct policy_file *fp = pd->fp;
>   	struct policydb *p = pd->p;
>   	int rc;
>   
> @@ -3519,7 +3519,7 @@ static int range_write_helper(void *key, void *data, void *ptr)
>   	return 0;
>   }
>   
> -static int range_write(struct policydb *p, void *fp)
> +static int range_write(struct policydb *p, struct policy_file *fp)
>   {
>   	__le32 buf[1];
>   	int rc;
> @@ -3546,7 +3546,7 @@ static int filename_write_helper_compat(void *key, void *data, void *ptr)
>   	struct filename_trans_key *ft = key;
>   	struct filename_trans_datum *datum = data;
>   	struct ebitmap_node *node;
> -	void *fp = ptr;
> +	struct policy_file *fp = ptr;
>   	__le32 buf[4];
>   	int rc;
>   	u32 bit, len = strlen(ft->name);
> @@ -3583,7 +3583,7 @@ static int filename_write_helper(void *key, void *data, void *ptr)
>   {
>   	struct filename_trans_key *ft = key;
>   	struct filename_trans_datum *datum;
> -	void *fp = ptr;
> +	struct policy_file *fp = ptr;
>   	__le32 buf[3];
>   	int rc;
>   	u32 ndatum, len = strlen(ft->name);
> @@ -3628,7 +3628,7 @@ static int filename_write_helper(void *key, void *data, void *ptr)
>   	return 0;
>   }
>   
> -static int filename_trans_write(struct policydb *p, void *fp)
> +static int filename_trans_write(struct policydb *p, struct policy_file *fp)
>   {
>   	__le32 buf[1];
>   	int rc;
> @@ -3660,7 +3660,7 @@ static int filename_trans_write(struct policydb *p, void *fp)
>    * structure to a policy database binary representation
>    * file.
>    */
> -int policydb_write(struct policydb *p, void *fp)
> +int policydb_write(struct policydb *p, struct policy_file *fp)
>   {
>   	unsigned int num_syms;
>   	int rc;
> diff --git a/security/selinux/ss/policydb.h b/security/selinux/ss/policydb.h
> index 5c11069121d3..985f319e2266 100644
> --- a/security/selinux/ss/policydb.h
> +++ b/security/selinux/ss/policydb.h
> @@ -318,8 +318,14 @@ extern int policydb_context_isvalid(struct policydb *p, struct context *c);
>   extern int policydb_class_isvalid(struct policydb *p, unsigned int class);
>   extern int policydb_type_isvalid(struct policydb *p, unsigned int type);
>   extern int policydb_role_isvalid(struct policydb *p, unsigned int role);
> -extern int policydb_read(struct policydb *p, void *fp);
> -extern int policydb_write(struct policydb *p, void *fp);
> +
> +struct policy_file {
> +	char *data;
> +	size_t len;
> +};
> +
> +extern int policydb_read(struct policydb *p, struct policy_file *fp);
> +extern int policydb_write(struct policydb *p, struct policy_file *fp);

nit: It feels weird to me to have the struct definition break up the 
policydb_*() declarations like this.  I feel as though the file scans 
more cleanly if the policy_file struct definition is directly below the 
policydb struct definition, keeping the policydb_*() functions all together.

>   
>   extern struct filename_trans_datum *
>   policydb_filenametr_search(struct policydb *p, struct filename_trans_key *key);
> @@ -342,14 +348,9 @@ policydb_roletr_search(struct policydb *p, struct role_trans_key *key);
>   #define POLICYDB_MAGIC	SELINUX_MAGIC
>   #define POLICYDB_STRING "SE Linux"
>   
> -struct policy_file {
> -	char *data;
> -	size_t len;
> -};
> -
>   struct policy_data {
>   	struct policydb *p;
> -	void *fp;
> +	struct policy_file *fp;
>   };
>   
>   static inline int next_entry(void *buf, struct policy_file *fp, size_t bytes)

Re: [RFC PATCH 22/22] selinux: restrict policy strings

[Christian Göttsche]

On Fri, 13 Dec 2024 at 23:14, Daniel Burgener
<dburgener@linux.microsoft.com> wrote:
>
> On 11/15/2024 8:35 AM, Christian Göttsche wrote:
> > From: Christian Göttsche <cgzones@googlemail.com>
> >
> > Validate the characters and the lengths of strings parsed from binary
> > policies.
> >
> >    * Disallow control characters
> >    * Limit characters of identifiers to alphanumeric, underscore, dash,
> >      and dot
> >    * Limit identifiers in length to 128, expect types to 1024 and
> >      categories to 32, characters (excluding NUL-terminator)
> >
> > Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> > ---
> >   security/selinux/ss/conditional.c |  2 +-
> >   security/selinux/ss/policydb.c    | 60 ++++++++++++++++++++-----------
> >   security/selinux/ss/policydb.h    |  5 ++-
> >   3 files changed, 44 insertions(+), 23 deletions(-)
> >
> > diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
> > index d37b4bdf6ba9..346102417cbf 100644
> > --- a/security/selinux/ss/conditional.c
> > +++ b/security/selinux/ss/conditional.c
> > @@ -280,7 +280,7 @@ int cond_read_bool(struct policydb *p, struct symtab *s, struct policy_file *fp)
> >
> >       len = le32_to_cpu(buf[2]);
> >
> > -     rc = str_read(&key, GFP_KERNEL, fp, len);
> > +     rc = str_read(&key, GFP_KERNEL, fp, len, STR_IDENTIFIER, 128);
> >       if (rc)
> >               goto err;
> >
>
> It would be nice if these limits were named constants instead of magic
> numbers.  Right now it's hard to tell if all the "128"s are essentially
> the same limit referenced in different places, or if they could (in
> theory) be changed independently.
>
> > diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> > index 917b468c5144..d98dfa6c3f30 100644
> > --- a/security/selinux/ss/policydb.c
> > +++ b/security/selinux/ss/policydb.c
> > @@ -1221,8 +1221,9 @@ static int context_read_and_validate(struct context *c, struct policydb *p,
> >    * binary representation file.
> >    */
> >
> > -int str_read(char **strp, gfp_t flags, struct policy_file *fp, u32 len)
> > +int str_read(char **strp, gfp_t flags, struct policy_file *fp, u32 len, int kind, u32 max_len)
> >   {
> > +     u32 i;
> >       int rc;
> >       char *str;
> >
> > @@ -1232,19 +1233,35 @@ int str_read(char **strp, gfp_t flags, struct policy_file *fp, u32 len)
> >       if (oom_check(sizeof(char), len, fp))
> >               return -EINVAL;
> >
> > +     if (max_len != 0 && len > max_len)
> > +             return -EINVAL;
> > +
> >       str = kmalloc(len + 1, flags | __GFP_NOWARN);
> >       if (!str)
> >               return -ENOMEM;
> >
> >       rc = next_entry(str, fp, len);
> > -     if (rc) {
> > -             kfree(str);
> > -             return rc;
> > +     if (rc)
> > +             goto bad_str;
> > +
> > +     rc = -EINVAL;
> > +     for (i = 0; i < len; i++) {
> > +             if (iscntrl(str[i]))
> > +                     goto bad_str;
> > +
> > +             if (kind == STR_IDENTIFIER &&
> > +                 !(isalnum(str[i]) || str[i] == '_' || str[i] == '-' || str[i] == '.'))
> > +                     goto bad_str;
> > +
> >       }
> >
> >       str[len] = '\0';
> >       *strp = str;
> >       return 0;
> > +
> > +bad_str:
> > +     kfree(str);
> > +     return rc;
> >   }
> >
> >   static int perm_read(struct policydb *p, struct symtab *s, struct policy_file *fp)
> > @@ -1269,7 +1286,7 @@ static int perm_read(struct policydb *p, struct symtab *s, struct policy_file *f
> >       if (perdatum->value < 1 || perdatum->value > 32)
> >               goto bad;
> >
> > -     rc = str_read(&key, GFP_KERNEL, fp, len);
> > +     rc = str_read(&key, GFP_KERNEL, fp, len, STR_IDENTIFIER, 128);
> >       if (rc)
> >               goto bad;
> >
> > @@ -1315,7 +1332,7 @@ static int common_read(struct policydb *p, struct symtab *s, struct policy_file
> >               goto bad;
> >       comdatum->permissions.nprim = le32_to_cpu(buf[2]);
> >
> > -     rc = str_read(&key, GFP_KERNEL, fp, len);
> > +     rc = str_read(&key, GFP_KERNEL, fp, len, STR_IDENTIFIER, 128);
> >       if (rc)
> >               goto bad;
> >
> > @@ -1552,12 +1569,12 @@ static int class_read(struct policydb *p, struct symtab *s, struct policy_file *
> >
> >       ncons = le32_to_cpu(buf[5]);
> >
> > -     rc = str_read(&key, GFP_KERNEL, fp, len);
> > +     rc = str_read(&key, GFP_KERNEL, fp, len, STR_IDENTIFIER, 128);
> >       if (rc)
> >               goto bad;
> >
> >       if (len2) {
> > -             rc = str_read(&cladatum->comkey, GFP_KERNEL, fp, len2);
> > +             rc = str_read(&cladatum->comkey, GFP_KERNEL, fp, len2, STR_IDENTIFIER, 128);
> >               if (rc)
> >                       goto bad;
> >
> > @@ -1691,7 +1708,7 @@ static int role_read(struct policydb *p, struct symtab *s, struct policy_file *f
> >       if (p->policyvers >= POLICYDB_VERSION_BOUNDARY)
> >               role->bounds = le32_to_cpu(buf[2]);
> >
> > -     rc = str_read(&key, GFP_KERNEL, fp, len);
> > +     rc = str_read(&key, GFP_KERNEL, fp, len, STR_IDENTIFIER, 128);
> >       if (rc)
> >               goto bad;
> >
> > @@ -1758,7 +1775,7 @@ static int type_read(struct policydb *p, struct symtab *s, struct policy_file *f
> >               typdatum->primary = le32_to_cpu(buf[2]);
> >       }
> >
> > -     rc = str_read(&key, GFP_KERNEL, fp, len);
> > +     rc = str_read(&key, GFP_KERNEL, fp, len, STR_IDENTIFIER, 1024);
> >       if (rc)
> >               goto bad;
> >
> > @@ -1822,7 +1839,7 @@ static int user_read(struct policydb *p, struct symtab *s, struct policy_file *f
> >       if (p->policyvers >= POLICYDB_VERSION_BOUNDARY)
> >               usrdatum->bounds = le32_to_cpu(buf[2]);
> >
> > -     rc = str_read(&key, GFP_KERNEL, fp, len);
> > +     rc = str_read(&key, GFP_KERNEL, fp, len, STR_IDENTIFIER, 128);
> >       if (rc)
> >               goto bad;
> >
> > @@ -1871,7 +1888,7 @@ static int sens_read(struct policydb *p, struct symtab *s, struct policy_file *f
> >               goto bad;
> >       levdatum->isalias = val;
> >
> > -     rc = str_read(&key, GFP_KERNEL, fp, len);
> > +     rc = str_read(&key, GFP_KERNEL, fp, len, STR_IDENTIFIER, 128);
> >       if (rc)
> >               goto bad;
> >
> > @@ -1914,7 +1931,7 @@ static int cat_read(struct policydb *p, struct symtab *s, struct policy_file *fp
> >               goto bad;
> >       catdatum->isalias = val;
> >
> > -     rc = str_read(&key, GFP_KERNEL, fp, len);
> > +     rc = str_read(&key, GFP_KERNEL, fp, len, STR_IDENTIFIER, 32);
> >       if (rc)
> >               goto bad;
>
> The category restriction is more tight than the sensitivity one because
> a context may have many categories?  I guess that makes sense, but it
> feels counterintuitive from a user perspective, because I feel like
> users tend to think of categories and sensitivities as essentially the
> same thing.  Would dropping the sensitivity limit to 32 to match the
> category limit make sense?

Yes, I'll change the limit for sensitivities to 32 in v2.

> Is there a more strict limit on the number of categories a context can
> have than the U32_MAX from symtab.nprim?  Because that will allow
> exceeding the page size using too many categories regardless of length
> distinctions, which is a concern if the motivation here is about
> potential future untrusted policy loaders in a namespaced environment.

It seems the limit of categories a context can have in the total
number of categories defined in the policy, which seems to be U32_MAX
(or one or two less).
But even today in the common policies on can reach this limit, e.g. via

    $ for ((j=1; j<1000; j++)); do echo "limit $j";
ctx=system_u:system_r:init_t:s0:c0; for ((i=1; i<j; i++)); do
ctx+=,c$i; done; echo -n $ctx > /sys/fs/selinux/context || j=1000;
done;

For me with more than 834 categories (which have a maximum identifier
length of 4 (c833)) the context hits the page size limit.
So the maximum length of 32 is an attempt to minimize the practical likelihood.

> -Daniel
>