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

[RFC PATCH v2 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 1efb9a57d181..47480eb2189b 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 v2 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 617f54abb640..7b2e2c60f0f4 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

Re: [PATCH RFC v2 2/22] selinux: avoid using types indicating user space interaction

[Paul Moore]

On Dec 16, 2024 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgoettsche@seltendoof.de> wrote:
> 
> 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(-)

Merged into selinux/dev, thanks.

--
paul-moore.com

[RFC PATCH v2 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 10949df22fa4..1d47850fff45 100644
--- a/security/selinux/include/security.h
+++ b/security/selinux/include/security.h
@@ -290,7 +290,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);
 
@@ -308,7 +308,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 a7cbb80a11eb..32b8335cf93e 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 55fdc7ca232b..1c4ac392df2a 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -2712,7 +2712,7 @@ int security_node_sid(u16 domain,
  */
 
 int security_get_user_sids(u32 fromsid,
-			   char *username,
+			   const char *username,
 			   u32 **sids,
 			   u32 *nel)
 {
@@ -3034,7 +3034,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

Re: [PATCH RFC v2 3/22] selinux: align and constify functions

[Paul Moore]

On Dec 16, 2024 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgoettsche@seltendoof.de> wrote:
> 
> 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(-)

Merged into selinux/dev, thanks.

--
paul-moore.com

[RFC PATCH v2 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 1c4ac392df2a..9bd14256a154 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -2597,17 +2597,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

Re: [PATCH RFC v2 4/22] selinux: rework match_ipv6_addrmask()

[Paul Moore]

On Dec 16, 2024 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgoettsche@seltendoof.de> wrote:
> 
> 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(-)

Merged into selinux/dev, thanks.

--
paul-moore.com

[RFC PATCH v2 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 3ba5506a3fff..eb944582d7a6 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] = {
@@ -426,11 +428,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;
 
@@ -461,15 +463,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;
 }
@@ -498,15 +500,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

Re: [PATCH RFC v2 5/22] selinux: avoid nontransitive comparison

[Paul Moore]

On Dec 16, 2024 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgoettsche@seltendoof.de> wrote:
> 
> 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 3ba5506a3fff..eb944582d7a6 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)))

I'll admit that it took me a while to figure out why you decided to
name this macro "spaceship_cmp", and then I had a little laugh when
I realized why it was called the "spaceship" operator :)

Anyway, while the spaceship operator is likely familiar to people who
have a Perl background, the kernel is still mostly a C project so I
don't think we can expect a base understanding of Perl, especially
these days as Perl isn't as popular as in the past.  Can we rename
this to something else that makes more sense in the context of C?

--
paul-moore.com

Re: [PATCH RFC v2 5/22] selinux: avoid nontransitive comparison

[Christian Göttsche]

On Wed, 8 Jan 2025 at 04:00, Paul Moore <paul@paul-moore.com> wrote:
>
> On Dec 16, 2024 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgoettsche@seltendoof.de> wrote:
> >
> > 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 3ba5506a3fff..eb944582d7a6 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)))
>
> I'll admit that it took me a while to figure out why you decided to
> name this macro "spaceship_cmp", and then I had a little laugh when
> I realized why it was called the "spaceship" operator :)
>
> Anyway, while the spaceship operator is likely familiar to people who
> have a Perl background, the kernel is still mostly a C project so I
> don't think we can expect a base understanding of Perl, especially
> these days as Perl isn't as popular as in the past.  Can we rename
> this to something else that makes more sense in the context of C?

There seem to be 4 similar macros already in the kernel, 3 named
cmp_int() and one instance named cmp_ptr().
So I am going with cmp_int() in v3 for now, but I am open for other suggestions.

>
> --
> paul-moore.com

Re: [PATCH RFC v2 5/22] selinux: avoid nontransitive comparison

[Christian Göttsche]

On Wed, 8 Jan 2025 at 14:17, Christian Göttsche <cgzones@googlemail.com> wrote:
>
> On Wed, 8 Jan 2025 at 04:00, Paul Moore <paul@paul-moore.com> wrote:
> >
> > On Dec 16, 2024 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgoettsche@seltendoof.de> wrote:
> > >
> > > 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 3ba5506a3fff..eb944582d7a6 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)))
> >
> > I'll admit that it took me a while to figure out why you decided to
> > name this macro "spaceship_cmp", and then I had a little laugh when
> > I realized why it was called the "spaceship" operator :)
> >
> > Anyway, while the spaceship operator is likely familiar to people who
> > have a Perl background, the kernel is still mostly a C project so I
> > don't think we can expect a base understanding of Perl, especially
> > these days as Perl isn't as popular as in the past.  Can we rename
> > this to something else that makes more sense in the context of C?
>
> There seem to be 4 similar macros already in the kernel, 3 named
> cmp_int() and one instance named cmp_ptr().
> So I am going with cmp_int() in v3 for now, but I am open for other suggestions.

p.s.: C++ also had takeoff:
https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0515r3.pdf

> >
> > --
> > paul-moore.com

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

[Christian Göttsche]

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

The functions context_cmp(), mls_context_cmp() and ebitmap_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>
---
v2: also convert ebitmap_cmp() as suggested by Daniel
---
 security/selinux/ss/context.c   |  2 +-
 security/selinux/ss/context.h   | 14 +++++++-------
 security/selinux/ss/ebitmap.c   |  8 ++++----
 security/selinux/ss/ebitmap.h   |  2 +-
 security/selinux/ss/mls_types.h |  2 +-
 security/selinux/ss/services.c  |  2 +-
 security/selinux/ss/sidtab.c    |  2 +-
 7 files changed, 16 insertions(+), 16 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..dd3b9b5b588e 100644
--- a/security/selinux/ss/context.h
+++ b/security/selinux/ss/context.h
@@ -132,13 +132,13 @@ 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) &&
+		ebitmap_equal(&c1->range.level[0].cat, &c2->range.level[0].cat) &&
 		(c1->range.level[1].sens == c2->range.level[1].sens) &&
-		ebitmap_cmp(&c1->range.level[1].cat, &c2->range.level[1].cat));
+		ebitmap_equal(&c1->range.level[1].cat, &c2->range.level[1].cat));
 }
 
 static inline void mls_context_destroy(struct context *c)
@@ -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/ebitmap.c b/security/selinux/ss/ebitmap.c
index 99c01be15115..1cc1e7e711e3 100644
--- a/security/selinux/ss/ebitmap.c
+++ b/security/selinux/ss/ebitmap.c
@@ -25,12 +25,12 @@
 
 static struct kmem_cache *ebitmap_node_cachep __ro_after_init;
 
-int ebitmap_cmp(const struct ebitmap *e1, const struct ebitmap *e2)
+bool ebitmap_equal(const struct ebitmap *e1, const struct ebitmap *e2)
 {
 	const struct ebitmap_node *n1, *n2;
 
 	if (e1->highbit != e2->highbit)
-		return 0;
+		return false;
 
 	n1 = e1->node;
 	n2 = e2->node;
@@ -41,9 +41,9 @@ int ebitmap_cmp(const struct ebitmap *e1, const struct ebitmap *e2)
 	}
 
 	if (n1 || n2)
-		return 0;
+		return false;
 
-	return 1;
+	return true;
 }
 
 int ebitmap_cpy(struct ebitmap *dst, const struct ebitmap *src)
diff --git a/security/selinux/ss/ebitmap.h b/security/selinux/ss/ebitmap.h
index ba2ac3da1153..49eb33de87e0 100644
--- a/security/selinux/ss/ebitmap.h
+++ b/security/selinux/ss/ebitmap.h
@@ -120,7 +120,7 @@ static inline void ebitmap_node_clr_bit(struct ebitmap_node *n, u32 bit)
 	     (bit) < ebitmap_length(e);               \
 	     (bit) = ebitmap_next_positive(e, &(n), bit))
 
-int ebitmap_cmp(const struct ebitmap *e1, const struct ebitmap *e2);
+bool ebitmap_equal(const struct ebitmap *e1, const struct ebitmap *e2);
 int ebitmap_cpy(struct ebitmap *dst, const struct ebitmap *src);
 int ebitmap_and(struct ebitmap *dst, const struct ebitmap *e1,
 		const struct ebitmap *e2);
diff --git a/security/selinux/ss/mls_types.h b/security/selinux/ss/mls_types.h
index 7ef6e8cb0cf4..51df2ebd1211 100644
--- a/security/selinux/ss/mls_types.h
+++ b/security/selinux/ss/mls_types.h
@@ -29,7 +29,7 @@ struct mls_range {
 static inline int mls_level_eq(const struct mls_level *l1,
 			       const struct mls_level *l2)
 {
-	return ((l1->sens == l2->sens) && ebitmap_cmp(&l1->cat, &l2->cat));
+	return ((l1->sens == l2->sens) && ebitmap_equal(&l1->cat, &l2->cat));
 }
 
 static inline int mls_level_dom(const struct mls_level *l1,
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 9bd14256a154..e5c9b62e59c1 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -3331,7 +3331,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 cb7125cc7f8e..59f8c09158ef 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

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

[Paul Moore]

On Dec 16, 2024 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgoettsche@seltendoof.de> wrote:
> 
> The functions context_cmp(), mls_context_cmp() and ebitmap_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>
> ---
> v2: also convert ebitmap_cmp() as suggested by Daniel
> ---
>  security/selinux/ss/context.c   |  2 +-
>  security/selinux/ss/context.h   | 14 +++++++-------
>  security/selinux/ss/ebitmap.c   |  8 ++++----
>  security/selinux/ss/ebitmap.h   |  2 +-
>  security/selinux/ss/mls_types.h |  2 +-
>  security/selinux/ss/services.c  |  2 +-
>  security/selinux/ss/sidtab.c    |  2 +-
>  7 files changed, 16 insertions(+), 16 deletions(-)

Merged into selinux/dev, thanks.

--
paul-moore.com

[RFC PATCH v2 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>
---
v2:
 - move one little diff from subsequent patch to this one
 - reorder struct definition in policydb.h as suggested by Daniel
---
 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    | 91 ++++++++++++++++---------------
 security/selinux/ss/policydb.h    | 16 +++---
 8 files changed, 77 insertions(+), 74 deletions(-)

diff --git a/security/selinux/ss/avtab.c b/security/selinux/ss/avtab.c
index 83add633f92a..c2c31521cace 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, bool conditional)
@@ -507,7 +507,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];
@@ -550,7 +550,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)];
@@ -586,7 +586,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 32b8335cf93e..850b3453f259 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, bool conditional);
 
-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 c9a3060f08a4..d8dcaf2ca88f 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 1cc1e7e711e3..43bc19e21960 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 49eb33de87e0..c9569998f287 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 eb944582d7a6..b57f7db4cd89 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -999,7 +999,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;
@@ -1059,7 +1059,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;
@@ -1097,7 +1097,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;
@@ -1120,7 +1120,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;
@@ -1153,7 +1153,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;
@@ -1205,7 +1205,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;
@@ -1224,7 +1224,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;
@@ -1318,7 +1318,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;
@@ -1415,7 +1415,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;
@@ -1472,7 +1472,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;
@@ -1524,7 +1524,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;
@@ -1546,7 +1546,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;
@@ -1597,7 +1597,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;
@@ -1638,7 +1638,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;
@@ -1673,7 +1673,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,
@@ -1843,7 +1843,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;
@@ -1920,7 +1920,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;
@@ -2005,7 +2005,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;
@@ -2094,7 +2094,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];
@@ -2135,7 +2135,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;
@@ -2249,7 +2249,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;
@@ -2446,7 +2446,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;
@@ -2769,7 +2769,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;
@@ -2790,7 +2790,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;
@@ -2830,7 +2830,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;
@@ -2858,7 +2858,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;
@@ -2883,7 +2883,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;
@@ -2903,7 +2903,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];
@@ -2917,7 +2917,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];
@@ -2945,7 +2945,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];
@@ -2998,7 +2998,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;
@@ -3023,7 +3023,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];
@@ -3042,7 +3042,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;
@@ -3093,7 +3093,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];
@@ -3178,7 +3178,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;
@@ -3218,7 +3218,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;
@@ -3259,7 +3259,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;
@@ -3308,7 +3308,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;
@@ -3444,7 +3445,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;
@@ -3502,7 +3503,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;
 
@@ -3524,7 +3525,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;
@@ -3551,7 +3552,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);
@@ -3588,7 +3589,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);
@@ -3633,7 +3634,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;
@@ -3665,7 +3666,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..c699fa52f59a 100644
--- a/security/selinux/ss/policydb.h
+++ b/security/selinux/ss/policydb.h
@@ -312,14 +312,19 @@ struct policydb {
 	u32 process_trans_perms;
 } __randomize_layout;
 
+struct policy_file {
+	char *data;
+	size_t len;
+};
+
 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_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);
+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 +347,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

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

[Paul Moore]

On Dec 16, 2024 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgoettsche@seltendoof.de> wrote:
> 
> Improve type safety and readability by using the known type.
> 
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
> v2:
>  - move one little diff from subsequent patch to this one
>  - reorder struct definition in policydb.h as suggested by Daniel
> ---
>  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    | 91 ++++++++++++++++---------------
>  security/selinux/ss/policydb.h    | 16 +++---
>  8 files changed, 77 insertions(+), 74 deletions(-)

Merged into selinux/dev, thanks.

--
paul-moore.com

[RFC PATCH v2 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 | 19 ++++++-------------
 security/selinux/ss/policydb.h |  2 +-
 3 files changed, 10 insertions(+), 17 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 b57f7db4cd89..f7d0867428f5 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -303,9 +303,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;
@@ -637,11 +635,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;
@@ -1620,12 +1618,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;
 
@@ -2846,7 +2839,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;
 
diff --git a/security/selinux/ss/policydb.h b/security/selinux/ss/policydb.h
index c699fa52f59a..80d1fa7e4995 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

Re: [PATCH RFC v2 8/22] selinux: avoid unnecessary indirection in struct level_datum

[Paul Moore]

On Dec 16, 2024 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgoettsche@seltendoof.de> wrote:
> 
> 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 | 19 ++++++-------------
>  security/selinux/ss/policydb.h |  2 +-
>  3 files changed, 10 insertions(+), 17 deletions(-)

Merged into selinux/dev, thanks.

--
paul-moore.com

[RFC PATCH v2 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 d8dcaf2ca88f..1bebfcb9c6a1 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 f7d0867428f5..2408c3e8ae39 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -1095,7 +1095,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;
@@ -2475,24 +2475,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 80d1fa7e4995..25650224b6e7 100644
--- a/security/selinux/ss/policydb.h
+++ b/security/selinux/ss/policydb.h
@@ -386,6 +386,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

Re: [PATCH RFC v2 9/22] selinux: make use of str_read()

[Paul Moore]

On Dec 16, 2024 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgoettsche@seltendoof.de> wrote:
> 
> 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(-)

Merged into selinux/dev, thanks.

--
paul-moore.com

[RFC PATCH v2 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 2408c3e8ae39..eeca470cc90c 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -927,7 +927,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;
@@ -1321,7 +1321,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);
@@ -1334,9 +1334,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;
@@ -1842,7 +1847,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;
@@ -1873,7 +1878,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;
 
@@ -1918,7 +1927,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;
 
@@ -1939,7 +1948,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]);
@@ -2003,7 +2016,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;
 
@@ -2023,7 +2037,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) {
@@ -2131,7 +2149,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;
@@ -2201,7 +2219,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)
@@ -2446,7 +2468,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;
@@ -2632,7 +2654,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 25650224b6e7..0c423ad77fd9 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) */
@@ -320,7 +320,7 @@ struct policy_file {
 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);
 extern int policydb_read(struct policydb *p, struct policy_file *fp);
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index e5c9b62e59c1..28c0bdf9fc9d 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -3350,7 +3350,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

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

[Paul Moore]

On Dec 16, 2024 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgoettsche@seltendoof.de> wrote:
> 
> 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 2408c3e8ae39..eeca470cc90c 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -927,7 +927,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;
> @@ -1321,7 +1321,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);
> @@ -1334,9 +1334,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;

While this is a major issue, isn't U16_MAX technically still valid?  In
other words should this be: '(val > U16_MAX)'?

> @@ -1939,7 +1948,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;

We should split out the class validity check into a separate patch and
keep this just as the subject states: consolidate the class type to u16.

As an aside, I'm going to do a quick review pass on the rest of the
patches in this series, but I'm not going to merge them as I keep
hitting a number of merge failures due to this patch not being applied
and I'd rather not have to fix them all up :)

--
paul-moore.com

[RFC PATCH v2 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>
---
v2:
  accept unknown xperm specifiers to support backwards compatibility for
  future ones, suggested by Thiébaud
---
 security/selinux/ss/avtab.c       |  37 +++++--
 security/selinux/ss/conditional.c |  18 ++--
 security/selinux/ss/constraint.h  |   2 +-
 security/selinux/ss/policydb.c    | 157 ++++++++++++++++++++++++------
 security/selinux/ss/policydb.h    |  19 +++-
 security/selinux/ss/services.c    |   2 -
 6 files changed, 182 insertions(+), 53 deletions(-)

diff --git a/security/selinux/ss/avtab.c b/security/selinux/ss/avtab.c
index c2c31521cace..3bd949a200ef 100644
--- a/security/selinux/ss/avtab.c
+++ b/security/selinux/ss/avtab.c
@@ -349,7 +349,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 +361,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 +391,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 +416,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 +456,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;
 	}
 
@@ -471,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_warn_once_policyload(pol, "SELinux: avtab: unsupported xperm specifier %#x\n",
+						xperms.specified);
+		}
 		rc = next_entry(&xperms.driver, fp, sizeof(u8));
 		if (rc) {
 			pr_err("SELinux: avtab: truncated entry\n");
diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
index 1bebfcb9c6a1..35442f4ceedf 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;
 }
@@ -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 eeca470cc90c..1275fd7d9148 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -634,13 +634,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;
 }
@@ -653,12 +651,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;
 }
@@ -1136,6 +1133,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)
@@ -1170,6 +1170,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)
@@ -1335,6 +1338,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;
@@ -1396,16 +1402,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);
@@ -1415,6 +1464,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;
 }
 
@@ -1527,7 +1578,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;
@@ -1586,7 +1637,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;
 	}
@@ -1606,7 +1657,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)
@@ -1617,13 +1668,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;
 
@@ -1633,6 +1688,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;
 }
 
@@ -1642,7 +1699,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)
@@ -1654,7 +1711,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)
@@ -1666,6 +1727,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;
 }
 
@@ -1919,6 +1982,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;
 }
 
@@ -1946,10 +2011,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;
@@ -2008,6 +2077,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;
 }
 
@@ -2036,7 +2108,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))
@@ -2071,6 +2146,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;
 	}
 
@@ -2102,6 +2181,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;
 }
 
@@ -2221,7 +2303,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,
@@ -2260,6 +2342,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;
 }
 
@@ -2268,7 +2353,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;
@@ -2332,11 +2417,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;
@@ -2454,6 +2553,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 0c423ad77fd9..690dc4a00cf3 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) */
@@ -391,4 +391,13 @@ 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);
 
+#define pr_warn_once_policyload(policy, fmt, ...)                        \
+	do {                                                             \
+		static void *prev_policy__;                              \
+		if (policy != prev_policy__) {                           \
+			printk(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__); \
+			policy = prev_policy__;                          \
+		}                                                        \
+	} while (0)
+
 #endif /* _SS_POLICYDB_H_ */
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 28c0bdf9fc9d..d5725c768d59 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

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

[Paul Moore]

On Dec 16, 2024 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgoettsche@seltendoof.de> wrote:
> 
> 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>
> ---
> v2:
>   accept unknown xperm specifiers to support backwards compatibility for
>   future ones, suggested by Thiébaud
> ---
>  security/selinux/ss/avtab.c       |  37 +++++--
>  security/selinux/ss/conditional.c |  18 ++--
>  security/selinux/ss/constraint.h  |   2 +-
>  security/selinux/ss/policydb.c    | 157 ++++++++++++++++++++++++------
>  security/selinux/ss/policydb.h    |  19 +++-
>  security/selinux/ss/services.c    |   2 -
>  6 files changed, 182 insertions(+), 53 deletions(-)
> 
> diff --git a/security/selinux/ss/avtab.c b/security/selinux/ss/avtab.c
> index c2c31521cace..3bd949a200ef 100644
> --- a/security/selinux/ss/avtab.c
> +++ b/security/selinux/ss/avtab.c
> @@ -349,7 +349,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 +361,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;
>  		}

A reminder that magic numbers are a bad thing, if we can't make it clear
what the '5' in the conditional above represents by using a computed
value, let's either use a #define with a helpful name or a comment to
make this a bit more understandable.

> @@ -444,9 +456,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;
>  	}

Let's define a macro in avtab.h with all of the allowed avtab key
values, otherwise I think people are going to forget about this check
when adding a new flag and they are going to get frustrated :)

> @@ -471,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_warn_once_policyload(pol, "SELinux: avtab: unsupported xperm specifier %#x\n",
> +						xperms.specified);
> +		}

Similar to the avtab flags discussion above, can we create a small
inline function in avtab.h that checks to see if an xperm is valid?

  /* feel free to come up with a better name */
  static inline bool avtab_xpermspec_valid(u8 specified)
  {
    if (specified == AVTAB_XPERMS_IOCTLFUNCTION)
      return true;
    elif (...)
      return true;

    return false;
  }

> 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) */

If we're not using this field, let's remove it.  If for some odd reason
we need to keep it here for size reasons, or something similar, let's
turn it into a 'void *unused;' field.

> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index eeca470cc90c..1275fd7d9148 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -634,13 +634,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;
>  }

Hmm, I don't think the code above does the error checking in the same
way, how about something like this:

  int sens_index(...)
  {
    if (isalias)
      return 0;
    if (!level->sens || level->send > levels.nprim)
      return -EINVAL;
    p = ...;
    return 0;
  }

> @@ -653,12 +651,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;
>  }

Similar to the sensitivity level comment above.

> @@ -1136,6 +1133,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;

More magic number problems.

>  	rc = str_read(&key, GFP_KERNEL, fp, len);
>  	if (rc)
> @@ -1170,6 +1170,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;

Magic number.

>  	rc = symtab_init(&comdatum->permissions, nel);
>  	if (rc)
> @@ -1335,6 +1338,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;

Again.

> @@ -1527,7 +1578,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;

Why is this here?  You don't use the @p parameter anywhere in this
patch and it add some code churn in all of the callers.

> @@ -1606,7 +1657,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)
> @@ -1617,13 +1668,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;

Should we have a simple inline function to do the integer boolean check?

Considering all the places we check for 0 and 1, it seems like it might
be a bit cleaner, and would help with self-documenting.

> @@ -2221,7 +2303,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,

This should probably be in patch 10/22, yes?

> @@ -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) */

See my previous comment about unused fields.

>  #endif /* _SS_POLICYDB_H_ */
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index 28c0bdf9fc9d..d5725c768d59 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);

Do we need to convert this to a if-then check that does the proper error
handling, or is it already handled in the other changes in this patch?

--
paul-moore.com

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

[Christian Göttsche]

On Wed, 8 Jan 2025 at 04:00, Paul Moore <paul@paul-moore.com> wrote:
>
> On Dec 16, 2024 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgoettsche@seltendoof.de> wrote:
> >
> > 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>
> > ---
> > v2:
> >   accept unknown xperm specifiers to support backwards compatibility for
> >   future ones, suggested by Thiébaud
> > ---
> >  security/selinux/ss/avtab.c       |  37 +++++--
> >  security/selinux/ss/conditional.c |  18 ++--
> >  security/selinux/ss/constraint.h  |   2 +-
> >  security/selinux/ss/policydb.c    | 157 ++++++++++++++++++++++++------
> >  security/selinux/ss/policydb.h    |  19 +++-
> >  security/selinux/ss/services.c    |   2 -
> >  6 files changed, 182 insertions(+), 53 deletions(-)
> >
> > diff --git a/security/selinux/ss/avtab.c b/security/selinux/ss/avtab.c
> > index c2c31521cace..3bd949a200ef 100644
> > --- a/security/selinux/ss/avtab.c
> > +++ b/security/selinux/ss/avtab.c
> > @@ -349,7 +349,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 +361,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;
> >               }
>
> A reminder that magic numbers are a bad thing, if we can't make it clear
> what the '5' in the conditional above represents by using a computed
> value, let's either use a #define with a helpful name or a comment to
> make this a bit more understandable.
>
> > @@ -444,9 +456,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;
> >       }
>
> Let's define a macro in avtab.h with all of the allowed avtab key
> values, otherwise I think people are going to forget about this check
> when adding a new flag and they are going to get frustrated :)
>
> > @@ -471,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_warn_once_policyload(pol, "SELinux: avtab: unsupported xperm specifier %#x\n",
> > +                                             xperms.specified);
> > +             }
>
> Similar to the avtab flags discussion above, can we create a small
> inline function in avtab.h that checks to see if an xperm is valid?
>
>   /* feel free to come up with a better name */
>   static inline bool avtab_xpermspec_valid(u8 specified)
>   {
>     if (specified == AVTAB_XPERMS_IOCTLFUNCTION)
>       return true;
>     elif (...)
>       return true;
>
>     return false;
>   }
>
> > 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) */
>
> If we're not using this field, let's remove it.  If for some odd reason
> we need to keep it here for size reasons, or something similar, let's
> turn it into a 'void *unused;' field.

This member (and the one later down this patch) is not used
internally, but forwarded to userspace via policydb_write() to
/sys/fs/selinux/policy.

>
> > diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> > index eeca470cc90c..1275fd7d9148 100644
> > --- a/security/selinux/ss/policydb.c
> > +++ b/security/selinux/ss/policydb.c
> > @@ -634,13 +634,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;
> >  }
>
> Hmm, I don't think the code above does the error checking in the same
> way, [...]

That is the point of this change: to also validate the sensitivities aliases.

>
>   int sens_index(...)
>   {
>     if (isalias)
>       return 0;
>     if (!level->sens || level->send > levels.nprim)
>       return -EINVAL;
>     p = ...;
>     return 0;
>   }
>
> > @@ -653,12 +651,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;
> >  }
>
> Similar to the sensitivity level comment above.
>
> > @@ -1136,6 +1133,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;
>
> More magic number problems.
>
> >       rc = str_read(&key, GFP_KERNEL, fp, len);
> >       if (rc)
> > @@ -1170,6 +1170,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;
>
> Magic number.
>
> >       rc = symtab_init(&comdatum->permissions, nel);
> >       if (rc)
> > @@ -1335,6 +1338,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;
>
> Again.
>
> > @@ -1527,7 +1578,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;
>
> Why is this here?  You don't use the @p parameter anywhere in this
> patch and it add some code churn in all of the callers.
>
> > @@ -1606,7 +1657,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)
> > @@ -1617,13 +1668,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;
>
> Should we have a simple inline function to do the integer boolean check?
>
> Considering all the places we check for 0 and 1, it seems like it might
> be a bit cleaner, and would help with self-documenting.
>
> > @@ -2221,7 +2303,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,
>
> This should probably be in patch 10/22, yes?
>
> > @@ -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) */
>
> See my previous comment about unused fields.
>
> >  #endif /* _SS_POLICYDB_H_ */
> > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> > index 28c0bdf9fc9d..d5725c768d59 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);
>
> Do we need to convert this to a if-then check that does the proper error
> handling, or is it already handled in the other changes in this patch?

perm_read() now performs this check at policyload time.

>
> --
> paul-moore.com

[RFC PATCH v2 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 3bd949a200ef..a7bf0ceb45d4 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 35442f4ceedf..de29948efb48 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 1275fd7d9148..4bc1e225f2fe 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -1100,6 +1100,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;
@@ -1174,6 +1177,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;
@@ -1348,6 +1355,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;
@@ -1921,6 +1932,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;
@@ -2689,6 +2704,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;
@@ -2730,6 +2749,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 690dc4a00cf3..828fef98e340 100644
--- a/security/selinux/ss/policydb.h
+++ b/security/selinux/ss/policydb.h
@@ -352,6 +352,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

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

[Paul Moore]

On Dec 16, 2024 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgoettsche@seltendoof.de> wrote:
> 
> 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 3bd949a200ef..a7bf0ceb45d4 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 35442f4ceedf..de29948efb48 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;

Magic number, we should make it obvious why '2' is being used, if we
can't do that we should add a comment.

This comment applies several other places in this patch, I'll refrain
from mentioning all of them.

> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 1275fd7d9148..4bc1e225f2fe 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -1174,6 +1177,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;

Please don't add a comment *inside* code like that, it makes the code
awful to read.

> diff --git a/security/selinux/ss/policydb.h b/security/selinux/ss/policydb.h
> index 690dc4a00cf3..828fef98e340 100644
> --- a/security/selinux/ss/policydb.h
> +++ b/security/selinux/ss/policydb.h
> @@ -352,6 +352,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;
> +}

I'd prefer if we could use a different name than "oom_check()", perhaps
"size_check()"?

--
paul-moore.com

[RFC PATCH v2 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 4bc1e225f2fe..2c2bd0df8645 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -1256,6 +1256,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++) {
@@ -1287,15 +1289,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 d5725c768d59..797b9a0692fd 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 v2 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 de29948efb48..481aa17a6f26 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 2c2bd0df8645..3f85bb63cb5e 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -945,6 +945,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 828fef98e340..615fdd5ef3c3 100644
--- a/security/selinux/ss/policydb.h
+++ b/security/selinux/ss/policydb.h
@@ -323,6 +323,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);
 extern int policydb_read(struct policydb *p, struct policy_file *fp);
 extern int policydb_write(struct policydb *p, struct policy_file *fp);
 
-- 
2.45.2

[RFC PATCH v2 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 43bc19e21960..5d6b5b72b3e5 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 c9569998f287..12bb359e83ff 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

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

[Paul Moore]

On Dec 16, 2024 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgoettsche@seltendoof.de> wrote:
> 
> 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 43bc19e21960..5d6b5b72b3e5 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;
> +}

Squash this patch with 16/22.  We generally don't add code that isn't
used, so squashing this with the caller in 16/22 ensures we have at
least one caller.

--
paul-moore.com

[RFC PATCH v2 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 3f85bb63cb5e..b4381a0b93f6 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -2946,6 +2946,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 v2 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 b4381a0b93f6..348cbe36502e 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -728,7 +728,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,
@@ -2791,6 +2790,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) {
@@ -2802,6 +2805,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)
@@ -2898,10 +2903,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 v2 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 348cbe36502e..a4c9377b8060 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -923,51 +923,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) {
 		/*
@@ -976,24 +984,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 615fdd5ef3c3..aa3b21bd5286 100644
--- a/security/selinux/ss/policydb.h
+++ b/security/selinux/ss/policydb.h
@@ -319,11 +319,11 @@ struct policy_file {
 
 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);
 extern int policydb_read(struct policydb *p, struct policy_file *fp);
 extern int policydb_write(struct policydb *p, struct policy_file *fp);
 
@@ -394,7 +394,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 797b9a0692fd..227fe7832c08 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 v2 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 a4c9377b8060..e9e478650e74 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -673,6 +673,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)
@@ -765,6 +843,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;
@@ -1087,6 +1175,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

Re: [PATCH RFC v2 19/22] selinux: validate symbols

[Paul Moore]

On Dec 16, 2024 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgoettsche@seltendoof.de> wrote:
> 
> 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(+)

Out of curiosity, have you measured the policy load times before and
after this patchset?  I'd like to understand the performance impact of
the additional checks and validations.

--
paul-moore.com

Re: [PATCH RFC v2 19/22] selinux: validate symbols

[Christian Göttsche]

On Wed, 8 Jan 2025 at 04:00, Paul Moore <paul@paul-moore.com> wrote:
>
> On Dec 16, 2024 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgoettsche@seltendoof.de> wrote:
> >
> > 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(+)
>
> Out of curiosity, have you measured the policy load times before and
> after this patchset?  I'd like to understand the performance impact of
> the additional checks and validations.

A trivial benchmark of load_policy(8) inside a virtme-ng environment
showed a slight increase from 82,7ms to 82.9ms.
I'll try some more benchmarks for v3.

> --
> paul-moore.com

[RFC PATCH v2 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 e9e478650e74..57ab2a811a15 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -1020,6 +1020,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)
@@ -1940,6 +1949,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)
 		{
@@ -1977,6 +1992,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)
 		{
@@ -2011,9 +2032,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 aa3b21bd5286..512d2081733b 100644
--- a/security/selinux/ss/policydb.h
+++ b/security/selinux/ss/policydb.h
@@ -323,6 +323,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);
 extern int policydb_read(struct policydb *p, struct policy_file *fp);
 extern int policydb_write(struct policydb *p, struct policy_file *fp);
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 227fe7832c08..6dd281ddef7a 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 v2 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 a7bf0ceb45d4..6b6e1540cf7e 100644
--- a/security/selinux/ss/avtab.c
+++ b/security/selinux/ss/avtab.c
@@ -423,6 +423,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 57ab2a811a15..34db40753654 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -686,7 +686,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;
 	}
 
@@ -1047,6 +1047,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)
@@ -2230,6 +2247,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);
@@ -2352,7 +2371,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 512d2081733b..d979965ef939 100644
--- a/security/selinux/ss/policydb.h
+++ b/security/selinux/ss/policydb.h
@@ -322,6 +322,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 v2 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>
---
v2:
  - add wrappers for str_read() to minimize the usage of magic numbers
  - limit sensitivities to a length of 32, to match categories,
    suggested by Daniel
---
 security/selinux/ss/conditional.c |  2 +-
 security/selinux/ss/policydb.c    | 60 ++++++++++++++++++++-----------
 security/selinux/ss/policydb.h    | 45 ++++++++++++++++++++++-
 3 files changed, 84 insertions(+), 23 deletions(-)

diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
index 481aa17a6f26..7688615a3934 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_bool(&key, GFP_KERNEL, fp, len);
 	if (rc)
 		goto err;
 
diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index 34db40753654..c72182b91f49 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -1226,8 +1226,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;
 
@@ -1237,19 +1238,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)
@@ -1274,7 +1291,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_perm(&key, GFP_KERNEL, fp, len);
 	if (rc)
 		goto bad;
 
@@ -1320,7 +1337,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_class(&key, GFP_KERNEL, fp, len);
 	if (rc)
 		goto bad;
 
@@ -1557,12 +1574,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_class(&key, GFP_KERNEL, fp, len);
 	if (rc)
 		goto bad;
 
 	if (len2) {
-		rc = str_read(&cladatum->comkey, GFP_KERNEL, fp, len2);
+		rc = str_read_class(&cladatum->comkey, GFP_KERNEL, fp, len2);
 		if (rc)
 			goto bad;
 
@@ -1696,7 +1713,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_role(&key, GFP_KERNEL, fp, len);
 	if (rc)
 		goto bad;
 
@@ -1763,7 +1780,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_type(&key, GFP_KERNEL, fp, len);
 	if (rc)
 		goto bad;
 
@@ -1827,7 +1844,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_user(&key, GFP_KERNEL, fp, len);
 	if (rc)
 		goto bad;
 
@@ -1876,7 +1893,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_sens(&key, GFP_KERNEL, fp, len);
 	if (rc)
 		goto bad;
 
@@ -1919,7 +1936,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_cat(&key, GFP_KERNEL, fp, len);
 	if (rc)
 		goto bad;
 
@@ -2225,7 +2242,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;
 
@@ -2324,7 +2341,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;
 
@@ -2478,7 +2495,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;
 
@@ -2517,7 +2534,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;
 
@@ -2620,7 +2637,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;
 
@@ -2688,7 +2705,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;
 
@@ -2754,7 +2771,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;
 
@@ -2822,7 +2839,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 d979965ef939..22e5e5adf73c 100644
--- a/security/selinux/ss/policydb.h
+++ b/security/selinux/ss/policydb.h
@@ -402,7 +402,50 @@ 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);
+
+static inline int str_read_bool(char **strp, gfp_t flags, struct policy_file *fp, u32 len)
+{
+	return str_read(strp, flags, fp, len, STR_IDENTIFIER, 128);
+}
+
+static inline int str_read_cat(char **strp, gfp_t flags, struct policy_file *fp, u32 len)
+{
+	return str_read(strp, flags, fp, len, STR_IDENTIFIER, 32);
+}
+
+static inline int str_read_class(char **strp, gfp_t flags, struct policy_file *fp, u32 len)
+{
+	return str_read(strp, flags, fp, len, STR_IDENTIFIER, 128);
+}
+
+static inline int str_read_perm(char **strp, gfp_t flags, struct policy_file *fp, u32 len)
+{
+	return str_read(strp, flags, fp, len, STR_IDENTIFIER, 128);
+}
+
+static inline int str_read_role(char **strp, gfp_t flags, struct policy_file *fp, u32 len)
+{
+	return str_read(strp, flags, fp, len, STR_IDENTIFIER, 128);
+}
+
+static inline int str_read_sens(char **strp, gfp_t flags, struct policy_file *fp, u32 len)
+{
+	return str_read(strp, flags, fp, len, STR_IDENTIFIER, 32);
+}
+
+static inline int str_read_type(char **strp, gfp_t flags, struct policy_file *fp, u32 len)
+{
+	return str_read(strp, flags, fp, len, STR_IDENTIFIER, 1024);
+}
+
+static inline int str_read_user(char **strp, gfp_t flags, struct policy_file *fp, u32 len)
+{
+	return str_read(strp, flags, fp, len, STR_IDENTIFIER, 128);
+}
 
 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 v2 22/22] selinux: restrict policy strings

[Stephen Smalley]

On Mon, Dec 16, 2024 at 11:42 AM Christian Göttsche
<cgoettsche@seltendoof.de> 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)

One option if we are concerned about breaking backward compatibility
with policies in the wild would be to make these restrictions
conditional on whether the policy is being loaded into a non-init
SELinux namespace, similar to:
https://lore.kernel.org/selinux/20250102164509.25606-38-stephen.smalley.work@gmail.com/T/#u

That said, it seems hard to imagine real-world policies that would
exceed these limits, and likely could make them even smaller.
But as Daniel said, we should make them consistently enforced in both
userspace and kernel, and potentially these should all be #define'd
symbols in a uapi header that can be referenced by both.
Looks like you left the type limit at 1024 despite Daniel's
observation that CIL uses 2048 as the limit, but as you noted, given
the page size limit on the entire context by various kernel
interfaces,
this is likely fine.

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

[Joe Nall]

> On Jan 3, 2025, at 2:12 PM, Stephen Smalley <stephen.smalley.work@gmail.com> wrote:
> 
> On Mon, Dec 16, 2024 at 11:42 AM Christian Göttsche
> <cgoettsche@seltendoof.de> wrote:
>> 
>> From: Christian Göttsche <cgzones@googlemail.com>
>> 
>> Validate the characters and the lengths of strings parsed from binary
>> policies.
Excellent idea.

>>  * Disallow control characters
Fine here.

>>  * Limit characters of identifiers to alphanumeric, underscore, dash,
>>    and dot
Fine again.

>>  * Limit identifiers in length to 128,
Fine again, our longest
 - type is 51 characters
 - attribute is 31
 - boolean is 46
 - role is 12

>> expect types to 1024 and
I don’t understand what this means.

>>    categories to 32, characters (excluding NUL-terminator)
One category or the whole category string? A single category longer than 7 characters seems pretty unlikely and 32 is wildly short for the whole string.

Joe

> One option if we are concerned about breaking backward compatibility
> with policies in the wild would be to make these restrictions
> conditional on whether the policy is being loaded into a non-init
> SELinux namespace, similar to:
> https://lore.kernel.org/selinux/20250102164509.25606-38-stephen.smalley.work@gmail.com/T/#u
> 
> That said, it seems hard to imagine real-world policies that would
> exceed these limits, and likely could make them even smaller.
> But as Daniel said, we should make them consistently enforced in both
> userspace and kernel, and potentially these should all be #define'd
> symbols in a uapi header that can be referenced by both.
> Looks like you left the type limit at 1024 despite Daniel's
> observation that CIL uses 2048 as the limit, but as you noted, given
> the page size limit on the entire context by various kernel
> interfaces,
> this is likely fine.
> 

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

[Christian Göttsche]

On Mon, 6 Jan 2025 at 00:26, Joe Nall <joenall@gmail.com> wrote:
>
>
>
> > On Jan 3, 2025, at 2:12 PM, Stephen Smalley <stephen.smalley.work@gmail.com> wrote:
> >
> > On Mon, Dec 16, 2024 at 11:42 AM Christian Göttsche
> > <cgoettsche@seltendoof.de> wrote:
> >>
> >> From: Christian Göttsche <cgzones@googlemail.com>
> >>
> >> Validate the characters and the lengths of strings parsed from binary
> >> policies.
> Excellent idea.
>
> >>  * Disallow control characters
> Fine here.
>
> >>  * Limit characters of identifiers to alphanumeric, underscore, dash,
> >>    and dot
> Fine again.
>
> >>  * Limit identifiers in length to 128,
> Fine again, our longest
>  - type is 51 characters
>  - attribute is 31
>  - boolean is 46
>  - role is 12
>
> >> expect types to 1024 and
> I don’t understand what this means.

Similar to your list of the length in you policy boolean, role, user,
class, and permission identifiers are limited to 128 charatcers (not
including NUL), types (and attributes, which are just special types)
are limited to 1024 characters, and individual sensitivities and
categories are limited to 32 characters.

>
> >>    categories to 32, characters (excluding NUL-terminator)
> One category or the whole category string? A single category longer than 7 characters seems pretty unlikely and 32 is wildly short for the whole string.

This only affects individual sensitivities and categories, like "s9"
or "c1023", not whole MCS/MLS parts.

> Joe
>
> > One option if we are concerned about breaking backward compatibility
> > with policies in the wild would be to make these restrictions
> > conditional on whether the policy is being loaded into a non-init
> > SELinux namespace, similar to:
> > https://lore.kernel.org/selinux/20250102164509.25606-38-stephen.smalley.work@gmail.com/T/#u
> >
> > That said, it seems hard to imagine real-world policies that would
> > exceed these limits, and likely could make them even smaller.
> > But as Daniel said, we should make them consistently enforced in both
> > userspace and kernel, and potentially these should all be #define'd
> > symbols in a uapi header that can be referenced by both.
> > Looks like you left the type limit at 1024 despite Daniel's
> > observation that CIL uses 2048 as the limit, but as you noted, given
> > the page size limit on the entire context by various kernel
> > interfaces,
> > this is likely fine.

I interpreted Daniels comment more like a assessment what CIL
currently constrains, not as a request for a change, maybe I
misunderstood?

Exporting the limits via a public headers seems reasonable.

Maybe for a smooth transition one could introduce a build time
configuration (CONFIG_SELINUX_STRICT_IDENTIFIERS?).
This configuration can be disabled by default, leading to identifiers
not being rejected only logged.
Than after two releases the default can change to reject instead of log.
And after the next LTS release the configuration can be dropped again.

> >
>

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

[Daniel Burgener]

On 1/7/2025 9:04 AM, Christian Göttsche wrote:
> On Mon, 6 Jan 2025 at 00:26, Joe Nall <joenall@gmail.com> wrote:
>>
>>
>>
>>> On Jan 3, 2025, at 2:12 PM, Stephen Smalley <stephen.smalley.work@gmail.com> wrote:
>>>
>>> On Mon, Dec 16, 2024 at 11:42 AM Christian Göttsche
>>> <cgoettsche@seltendoof.de> wrote:
>>>>
>>>> From: Christian Göttsche <cgzones@googlemail.com>
>>>>
>>>> Validate the characters and the lengths of strings parsed from binary
>>>> policies.
>> Excellent idea.
>>
>>>>   * Disallow control characters
>> Fine here.
>>
>>>>   * Limit characters of identifiers to alphanumeric, underscore, dash,
>>>>     and dot
>> Fine again.
>>
>>>>   * Limit identifiers in length to 128,
>> Fine again, our longest
>>   - type is 51 characters
>>   - attribute is 31
>>   - boolean is 46
>>   - role is 12
>>
>>>> expect types to 1024 and
>> I don’t understand what this means.
> 
> Similar to your list of the length in you policy boolean, role, user,
> class, and permission identifiers are limited to 128 charatcers (not
> including NUL), types (and attributes, which are just special types)
> are limited to 1024 characters, and individual sensitivities and
> categories are limited to 32 characters.
> 
>>
>>>>     categories to 32, characters (excluding NUL-terminator)
>> One category or the whole category string? A single category longer than 7 characters seems pretty unlikely and 32 is wildly short for the whole string.
> 
> This only affects individual sensitivities and categories, like "s9"
> or "c1023", not whole MCS/MLS parts.
> 
>> Joe
>>
>>> One option if we are concerned about breaking backward compatibility
>>> with policies in the wild would be to make these restrictions
>>> conditional on whether the policy is being loaded into a non-init
>>> SELinux namespace, similar to:
>>> https://lore.kernel.org/selinux/20250102164509.25606-38-stephen.smalley.work@gmail.com/T/#u
>>>
>>> That said, it seems hard to imagine real-world policies that would
>>> exceed these limits, and likely could make them even smaller.
>>> But as Daniel said, we should make them consistently enforced in both
>>> userspace and kernel, and potentially these should all be #define'd
>>> symbols in a uapi header that can be referenced by both.
>>> Looks like you left the type limit at 1024 despite Daniel's
>>> observation that CIL uses 2048 as the limit, but as you noted, given
>>> the page size limit on the entire context by various kernel
>>> interfaces,
>>> this is likely fine.
> 
> I interpreted Daniels comment more like a assessment what CIL
> currently constrains, not as a request for a change, maybe I
> misunderstood?

That is what I intended, yes.  My related request was "I would think 
that we'd want to end up in a situation where the kernel is either 
equally restrictive or less restrictive than CIL".  In isolation, my 
opinion is that the 1024 limit is fine, but I've been hoping James would 
chime in about the feasibility of dropping the CIL limit at some point 
to get them in sync.

FWIW we have a few generated type names internally that subjectively 
feel long to humans, but are still under 100 characters.  So 1024 is 
plenty of extra margin in my opinion.

-Daniel

> 
> Exporting the limits via a public headers seems reasonable.
> 
> Maybe for a smooth transition one could introduce a build time
> configuration (CONFIG_SELINUX_STRICT_IDENTIFIERS?).
> This configuration can be disabled by default, leading to identifiers
> not being rejected only logged.
> Than after two releases the default can change to reject instead of log.
> And after the next LTS release the configuration can be dropped again.
> 
>>>
>>

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

[James Carter]

On Tue, Jan 7, 2025 at 11:12 AM Daniel Burgener
<dburgener@linux.microsoft.com> wrote:
>
> On 1/7/2025 9:04 AM, Christian Göttsche wrote:
> > On Mon, 6 Jan 2025 at 00:26, Joe Nall <joenall@gmail.com> wrote:
> >>
> >>
> >>
> >>> On Jan 3, 2025, at 2:12 PM, Stephen Smalley <stephen.smalley.work@gmail.com> wrote:
> >>>
> >>> On Mon, Dec 16, 2024 at 11:42 AM Christian Göttsche
> >>> <cgoettsche@seltendoof.de> wrote:
> >>>>
> >>>> From: Christian Göttsche <cgzones@googlemail.com>
> >>>>
> >>>> Validate the characters and the lengths of strings parsed from binary
> >>>> policies.
> >> Excellent idea.
> >>
> >>>>   * Disallow control characters
> >> Fine here.
> >>
> >>>>   * Limit characters of identifiers to alphanumeric, underscore, dash,
> >>>>     and dot
> >> Fine again.
> >>
> >>>>   * Limit identifiers in length to 128,
> >> Fine again, our longest
> >>   - type is 51 characters
> >>   - attribute is 31
> >>   - boolean is 46
> >>   - role is 12
> >>
> >>>> expect types to 1024 and
> >> I don’t understand what this means.
> >
> > Similar to your list of the length in you policy boolean, role, user,
> > class, and permission identifiers are limited to 128 charatcers (not
> > including NUL), types (and attributes, which are just special types)
> > are limited to 1024 characters, and individual sensitivities and
> > categories are limited to 32 characters.
> >
> >>
> >>>>     categories to 32, characters (excluding NUL-terminator)
> >> One category or the whole category string? A single category longer than 7 characters seems pretty unlikely and 32 is wildly short for the whole string.
> >
> > This only affects individual sensitivities and categories, like "s9"
> > or "c1023", not whole MCS/MLS parts.
> >
> >> Joe
> >>
> >>> One option if we are concerned about breaking backward compatibility
> >>> with policies in the wild would be to make these restrictions
> >>> conditional on whether the policy is being loaded into a non-init
> >>> SELinux namespace, similar to:
> >>> https://lore.kernel.org/selinux/20250102164509.25606-38-stephen.smalley.work@gmail.com/T/#u
> >>>
> >>> That said, it seems hard to imagine real-world policies that would
> >>> exceed these limits, and likely could make them even smaller.
> >>> But as Daniel said, we should make them consistently enforced in both
> >>> userspace and kernel, and potentially these should all be #define'd
> >>> symbols in a uapi header that can be referenced by both.
> >>> Looks like you left the type limit at 1024 despite Daniel's
> >>> observation that CIL uses 2048 as the limit, but as you noted, given
> >>> the page size limit on the entire context by various kernel
> >>> interfaces,
> >>> this is likely fine.
> >
> > I interpreted Daniels comment more like a assessment what CIL
> > currently constrains, not as a request for a change, maybe I
> > misunderstood?
>
> That is what I intended, yes.  My related request was "I would think
> that we'd want to end up in a situation where the kernel is either
> equally restrictive or less restrictive than CIL".  In isolation, my
> opinion is that the 1024 limit is fine, but I've been hoping James would
> chime in about the feasibility of dropping the CIL limit at some point
> to get them in sync.
>

The CIL limit of 2048 is arbitrary and could be changed to 1024
without a problem.
Jim


> FWIW we have a few generated type names internally that subjectively
> feel long to humans, but are still under 100 characters.  So 1024 is
> plenty of extra margin in my opinion.
>
> -Daniel
>
> >
> > Exporting the limits via a public headers seems reasonable.
> >
> > Maybe for a smooth transition one could introduce a build time
> > configuration (CONFIG_SELINUX_STRICT_IDENTIFIERS?).
> > This configuration can be disabled by default, leading to identifiers
> > not being rejected only logged.
> > Than after two releases the default can change to reject instead of log.
> > And after the next LTS release the configuration can be dropped again.
> >
> >>>
> >>
>
>

[RFC PATCH v2 00/22] selinux: harden against malformed policies

[Christian Göttsche]

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

With the SELinux namespace feature on the horizon it becomes important
to identify and reject malformed policies at load time.  Otherwise
memory corruptions can compromise the kernel or NULL-pointer dereferences
and BUG() encounters can bring systems down.  Currently this is not a
security relevant issue since loading a policy requires root privileges
and permission of the current loaded SELinux policy, making it one of the
most privileged operation.

The first 9 patches are cleanup commits with overseeable diffs.

Patch 10 unifies the underlying type used for security class identifiers.

Patch 11 to 21 add various checks at policy load time to reject malformed
policies.

Patch 22 needs some discussion:
It limits the valid set of characters and the length for strings defined
by policies.  Currently there are no restrictions, so control characters
are accepted, e.g. Esc as part of a type name, and their length can be
arbitrary.  Human formatted security contexts however must not be
arbitrarily long, one example is they must fit in a page size for
selinuxfs interaction and network associations.
Thus the patch introduces the following restrictions:
  * 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)
    
p.s.:
On a related note to patch 10, the underlying type for types (and type-
attributes) is also not consistent:
In role, range and filename transitions, and the actual datum u32 is
used, while avtables use u16, practically limiting the number of
available types to 65534 (= U16_MAX - 2 (0 and U16_MAX are invalid)).

v1: https://lore.kernel.org/selinux/20241115133619.114393-23-cgoettsche@seltendoof.de/

v2: 
  - also convert ebitmap_cmp() as suggested by Daniel
  - accept instead of rejecting unknown xperm specifiers to support
    backwards compatibility for future ones, suggested by Thiébaud
  - add wrappers for str_read() to minimize the usage of magic numbers
  - limit sensitivities to a length of 32, to match categories,
    suggested by Daniel
 
Christian Göttsche (22):
  selinux: supply missing field initializers
  selinux: avoid using types indicating user space interaction
  selinux: align and constify functions
  selinux: rework match_ipv6_addrmask()
  selinux: avoid nontransitive comparison
  selinux: rename comparison functions for clarity
  selinux: use known type instead of void pointer
  selinux: avoid unnecessary indirection in struct level_datum
  selinux: make use of str_read()
  selinux: use u16 for security classes
  selinux: more strict policy parsing
  selinux: check length fields in policies
  selinux: validate constraints
  selinux: pre-validate conditional expressions
  selinux: introduce ebitmap_highest_set_bit()
  selinux: check type attr map overflows
  selinux: reorder policydb_index()
  selinux: beef up isvalid checks
  selinux: validate symbols
  selinux: more strict bounds check
  selinux: check for simple types
  selinux: restrict policy strings

 security/selinux/hooks.c               |   2 +-
 security/selinux/include/classmap.h    |   2 +-
 security/selinux/include/conditional.h |   2 +-
 security/selinux/include/security.h    |   4 +-
 security/selinux/selinuxfs.c           |   2 +-
 security/selinux/ss/avtab.c            |  58 +-
 security/selinux/ss/avtab.h            |  11 +-
 security/selinux/ss/conditional.c      | 166 +++---
 security/selinux/ss/conditional.h      |   6 +-
 security/selinux/ss/constraint.h       |   2 +-
 security/selinux/ss/context.c          |   2 +-
 security/selinux/ss/context.h          |  14 +-
 security/selinux/ss/ebitmap.c          |  39 +-
 security/selinux/ss/ebitmap.h          |   8 +-
 security/selinux/ss/hashtab.h          |   4 +-
 security/selinux/ss/mls.c              |  70 ++-
 security/selinux/ss/mls.h              |   6 +-
 security/selinux/ss/mls_types.h        |   2 +-
 security/selinux/ss/policydb.c         | 698 +++++++++++++++++++------
 security/selinux/ss/policydb.h         | 116 +++-
 security/selinux/ss/services.c         |  82 +--
 security/selinux/ss/sidtab.c           |   2 +-
 security/selinux/ss/symtab.c           |   2 +-
 security/selinux/ss/symtab.h           |   2 +-
 24 files changed, 940 insertions(+), 362 deletions(-)

-- 
2.45.2

Re: [PATCH RFC v2 1/22] selinux: supply missing field initializers

[Paul Moore]

On Dec 16, 2024 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgoettsche@seltendoof.de> wrote:
> 
> 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(-)

Merged into selinux/dev, thanks.

In the future, please try to write the commit descriptions such that
they display without line wraps when using 'git log' on an 80-char wide
terminal.

--
paul-moore.com