diff mbox series

[v2,8/9] selinux: policydb: implicit conversions

Message ID 20230728155501.39632-7-cgzones@googlemail.com (mailing list archive)
State Superseded
Delegated to: Paul Moore
Headers show
Series [v2,1/9] selinux: avoid implicit conversions in avtab code | expand

Commit Message

Christian Göttsche July 28, 2023, 3:54 p.m. UTC 
Use the identical type for local variables, e.g. loop counters.

Declare members of struct policydb_compat_info unsigned to consistently
use unsigned iterators.  They hold read-only non-negative numbers in the
global variable policydb_compat.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
v2:
  - avoid declarations in init-clauses of for loops
  - declare members of struct policydb_compat_info unsigned
---
 security/selinux/ss/policydb.c | 93 +++++++++++++++++++++-------------
 1 file changed, 58 insertions(+), 35 deletions(-)

Comments

Paul Moore Aug. 4, 2023, 2:20 a.m. UTC | #1 
On Jul 28, 2023 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgzones@googlemail.com> wrote:
> 
> Use the identical type for local variables, e.g. loop counters.
> 
> Declare members of struct policydb_compat_info unsigned to consistently
> use unsigned iterators.  They hold read-only non-negative numbers in the
> global variable policydb_compat.
> 
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
> v2:
>   - avoid declarations in init-clauses of for loops
>   - declare members of struct policydb_compat_info unsigned
> ---
>  security/selinux/ss/policydb.c | 93 +++++++++++++++++++++-------------
>  1 file changed, 58 insertions(+), 35 deletions(-)
> 
> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index dc66868ff62c..aa2371a422af 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -55,9 +55,9 @@ static const char *const symtab_name[SYM_NUM] = {
>  #endif
>  
>  struct policydb_compat_info {
> -	int version;
> -	int sym_num;
> -	int ocon_num;
> +	unsigned int version;
> +	unsigned int sym_num;
> +	unsigned int ocon_num;
>  };
>  
>  /* These need to be updated if SYM_NUM or OCON_NUM changes */
> @@ -159,9 +159,9 @@ static const struct policydb_compat_info policydb_compat[] = {
>  	},
>  };
>  
> -static const struct policydb_compat_info *policydb_lookup_compat(int version)
> +static const struct policydb_compat_info *policydb_lookup_compat(unsigned int version)
>  {
> -	int i;
> +	u32 i;

Another question of 'why u32'?  I can understand making the iterator
unsigned, but why explicitly make it 32-bits?  Why not just an
unsigned int?

>  	for (i = 0; i < ARRAY_SIZE(policydb_compat); i++) {
>  		if (policydb_compat[i].version == version)
> @@ -359,7 +359,7 @@ static int role_tr_destroy(void *key, void *datum, void *p)
>  	return 0;
>  }
>  
> -static void ocontext_destroy(struct ocontext *c, int i)
> +static void ocontext_destroy(struct ocontext *c, u32 i)

Yes, this should be unsigned, but why not an unsigned it?

>  {
>  	if (!c)
>  		return;
> @@ -781,7 +781,7 @@ void policydb_destroy(struct policydb *p)
>  {
>  	struct ocontext *c, *ctmp;
>  	struct genfs *g, *gtmp;
> -	int i;
> +	u32 i;

Same.

>  	struct role_allow *ra, *lra = NULL;
>  
>  	for (i = 0; i < SYM_NUM; i++) {
> @@ -2237,8 +2240,9 @@ static int genfs_read(struct policydb *p, void *fp)
>  static int ocontext_read(struct policydb *p, const struct policydb_compat_info *info,
>  			 void *fp)
>  {
> -	int i, j, rc;
> -	u32 nel, len;
> +	int rc;
> +	unsigned int i;
> +	u32 j, nel, len, val;
>  	__be64 prefixbuf[1];
>  	__le32 buf[3];
>  	struct ocontext *l, *c;
> @@ -2299,9 +2303,27 @@ static int ocontext_read(struct policydb *p, const struct policydb_compat_info *
>  				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 = -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 > c->u.port.high_port)
> +					goto out;
> +
>  				rc = context_read_and_validate(&c->context[0], p, fp);
>  				if (rc)
>  					goto out;

This entire block of bounds checking for protocols and ports should
be pulled out into its own patch, especially since it isn't mentioned
in the commit description.

--
paul-moore.com
diff mbox series

Patch

diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index dc66868ff62c..aa2371a422af 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -55,9 +55,9 @@  static const char *const symtab_name[SYM_NUM] = {
 #endif
 
 struct policydb_compat_info {
-	int version;
-	int sym_num;
-	int ocon_num;
+	unsigned int version;
+	unsigned int sym_num;
+	unsigned int ocon_num;
 };
 
 /* These need to be updated if SYM_NUM or OCON_NUM changes */
@@ -159,9 +159,9 @@  static const struct policydb_compat_info policydb_compat[] = {
 	},
 };
 
-static const struct policydb_compat_info *policydb_lookup_compat(int version)
+static const struct policydb_compat_info *policydb_lookup_compat(unsigned int version)
 {
-	int i;
+	u32 i;
 
 	for (i = 0; i < ARRAY_SIZE(policydb_compat); i++) {
 		if (policydb_compat[i].version == version)
@@ -359,7 +359,7 @@  static int role_tr_destroy(void *key, void *datum, void *p)
 	return 0;
 }
 
-static void ocontext_destroy(struct ocontext *c, int i)
+static void ocontext_destroy(struct ocontext *c, u32 i)
 {
 	if (!c)
 		return;
@@ -781,7 +781,7 @@  void policydb_destroy(struct policydb *p)
 {
 	struct ocontext *c, *ctmp;
 	struct genfs *g, *gtmp;
-	int i;
+	u32 i;
 	struct role_allow *ra, *lra = NULL;
 
 	for (i = 0; i < SYM_NUM; i++) {
@@ -1154,8 +1154,8 @@  static int common_read(struct policydb *p, struct symtab *s, void *fp)
 	char *key = NULL;
 	struct common_datum *comdatum;
 	__le32 buf[4];
-	u32 len, nel;
-	int i, rc;
+	u32 i, len, nel;
+	int rc;
 
 	comdatum = kzalloc(sizeof(*comdatum), GFP_KERNEL);
 	if (!comdatum)
@@ -1220,13 +1220,13 @@  static int type_set_read(struct type_set *t, void *fp)
 
 static int read_cons_helper(struct policydb *p,
 				struct constraint_node **nodep,
-				int ncons, int allowxtarget, void *fp)
+				u32 ncons, int allowxtarget, void *fp)
 {
 	struct constraint_node *c, *lc;
 	struct constraint_expr *e, *le;
 	__le32 buf[3];
-	u32 nexpr;
-	int rc, i, j, depth;
+	u32 i, j, nexpr;
+	int rc, depth;
 
 	lc = NULL;
 	for (i = 0; i < ncons; i++) {
@@ -1318,8 +1318,8 @@  static int class_read(struct policydb *p, struct symtab *s, void *fp)
 	char *key = NULL;
 	struct class_datum *cladatum;
 	__le32 buf[6];
-	u32 len, len2, ncons, nel;
-	int i, rc;
+	u32 i, len, len2, ncons, nel;
+	int rc;
 
 	cladatum = kzalloc(sizeof(*cladatum), GFP_KERNEL);
 	if (!cladatum)
@@ -1412,7 +1412,8 @@  static int role_read(struct policydb *p, struct symtab *s, void *fp)
 {
 	char *key = NULL;
 	struct role_datum *role;
-	int rc, to_read = 2;
+	int rc;
+	unsigned int to_read = 2;
 	__le32 buf[3];
 	u32 len;
 
@@ -1468,7 +1469,8 @@  static int type_read(struct policydb *p, struct symtab *s, void *fp)
 {
 	char *key = NULL;
 	struct type_datum *typdatum;
-	int rc, to_read = 3;
+	int rc;
+	unsigned int to_read = 3;
 	__le32 buf[4];
 	u32 len;
 
@@ -1542,7 +1544,8 @@  static int user_read(struct policydb *p, struct symtab *s, void *fp)
 {
 	char *key = NULL;
 	struct user_datum *usrdatum;
-	int rc, to_read = 2;
+	int rc;
+	unsigned int to_read = 2;
 	__le32 buf[3];
 	u32 len;
 
@@ -1683,7 +1686,7 @@  static int user_bounds_sanity_check(void *key, void *datum, void *datap)
 	upper = user = datum;
 	while (upper->bounds) {
 		struct ebitmap_node *node;
-		unsigned long bit;
+		u32 bit;
 
 		if (++depth == POLICYDB_BOUNDS_MAXDEPTH) {
 			pr_err("SELinux: user %s: "
@@ -1719,7 +1722,7 @@  static int role_bounds_sanity_check(void *key, void *datum, void *datap)
 	upper = role = datum;
 	while (upper->bounds) {
 		struct ebitmap_node *node;
-		unsigned long bit;
+		u32 bit;
 
 		if (++depth == POLICYDB_BOUNDS_MAXDEPTH) {
 			pr_err("SELinux: role %s: "
@@ -1834,9 +1837,9 @@  static int range_read(struct policydb *p, void *fp)
 {
 	struct range_trans *rt = NULL;
 	struct mls_range *r = NULL;
-	int i, rc;
+	int rc;
 	__le32 buf[2];
-	u32 nel;
+	u32 i, nel;
 
 	if (p->policyvers < POLICYDB_VERSION_MLS)
 		return 0;
@@ -2082,9 +2085,9 @@  static int filename_trans_read_helper(struct policydb *p, void *fp)
 
 static int filename_trans_read(struct policydb *p, void *fp)
 {
-	u32 nel;
+	u32 nel, i;
 	__le32 buf[1];
-	int rc, i;
+	int rc;
 
 	if (p->policyvers < POLICYDB_VERSION_FILENAME_TRANS)
 		return 0;
@@ -2123,8 +2126,8 @@  static int filename_trans_read(struct policydb *p, void *fp)
 
 static int genfs_read(struct policydb *p, void *fp)
 {
-	int i, j, rc;
-	u32 nel, nel2, len, len2;
+	int rc;
+	u32 i, j, nel, nel2, len, len2;
 	__le32 buf[1];
 	struct ocontext *l, *c;
 	struct ocontext *newc = NULL;
@@ -2237,8 +2240,9 @@  static int genfs_read(struct policydb *p, void *fp)
 static int ocontext_read(struct policydb *p, const struct policydb_compat_info *info,
 			 void *fp)
 {
-	int i, j, rc;
-	u32 nel, len;
+	int rc;
+	unsigned int i;
+	u32 j, nel, len, val;
 	__be64 prefixbuf[1];
 	__le32 buf[3];
 	struct ocontext *l, *c;
@@ -2299,9 +2303,27 @@  static int ocontext_read(struct policydb *p, const struct policydb_compat_info *
 				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 = -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 > c->u.port.high_port)
+					goto out;
+
 				rc = context_read_and_validate(&c->context[0], p, fp);
 				if (rc)
 					goto out;
@@ -2429,9 +2451,9 @@  int policydb_read(struct policydb *p, void *fp)
 	struct role_allow *ra, *lra;
 	struct role_trans_key *rtk = NULL;
 	struct role_trans_datum *rtd = NULL;
-	int i, j, rc;
+	int rc;
 	__le32 buf[4];
-	u32 len, nprim, nel, perm;
+	u32 i, j, len, nprim, nel, perm;
 
 	char *policydb_str;
 	const struct policydb_compat_info *info;
@@ -3282,7 +3304,8 @@  static int (*const write_f[SYM_NUM]) (void *key, void *datum, void *datap) = {
 static int ocontext_write(struct policydb *p, const struct policydb_compat_info *info,
 			  void *fp)
 {
-	unsigned int i, j, rc;
+	unsigned int i, j;
+	int rc;
 	size_t nel, len;
 	__be64 prefixbuf[1];
 	__le32 buf[3];
@@ -3631,10 +3654,10 @@  static int filename_trans_write(struct policydb *p, void *fp)
  */
 int policydb_write(struct policydb *p, void *fp)
 {
-	unsigned int i, num_syms;
 	int rc;
+	unsigned int num_syms;
 	__le32 buf[4];
-	u32 config;
+	u32 config, i;
 	size_t len;
 	const struct policydb_compat_info *info;