[PATCH 1/4] libsepol: Fix potential undefined shifts

[PATCH 1/4] libsepol: Fix potential undefined shifts

[James Carter]

An expression of the form "1 << x" is undefined if x == 31 because
the "1" is an int and cannot be left shifted by 31.

Instead, use "UINT32_C(1) << x" which will be an unsigned int of
at least 32 bits.

Signed-off-by: James Carter <jwcart2@gmail.com>
---
 libsepol/src/avtab.c       | 2 +-
 libsepol/src/conditional.c | 6 +++---
 libsepol/src/link.c        | 4 ++--
 libsepol/src/policydb.c    | 4 ++--
 libsepol/src/services.c    | 4 ++--
 5 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/libsepol/src/avtab.c b/libsepol/src/avtab.c
index 93505b20..46e1e75d 100644
--- a/libsepol/src/avtab.c
+++ b/libsepol/src/avtab.c
@@ -376,7 +376,7 @@ int avtab_alloc(avtab_t *h, uint32_t nrules)
 	}
 	if (shift > 2)
 		shift = shift - 2;
-	nslot = 1 << shift;
+	nslot = UINT32_C(1) << shift;
 	if (nslot > MAX_AVTAB_HASH_BUCKETS)
 		nslot = MAX_AVTAB_HASH_BUCKETS;
 	mask = nslot - 1;
diff --git a/libsepol/src/conditional.c b/libsepol/src/conditional.c
index e3ede694..037dc7e2 100644
--- a/libsepol/src/conditional.c
+++ b/libsepol/src/conditional.c
@@ -411,13 +411,13 @@ int cond_normalize_expr(policydb_t * p, cond_node_t * cn)
 		}
 
 		/* loop through all possible combinations of values for bools in expression */
-		for (test = 0x0; test < (0x1U << cn->nbools); test++) {
+		for (test = 0x0; test < (UINT32_C(1) << cn->nbools); test++) {
 			/* temporarily set the value for all the bools in the
 			 * expression using the corr.  bit in test */
 			for (j = 0; j < cn->nbools; j++) {
 				p->bool_val_to_struct[cn->bool_ids[j] -
 						      1]->state =
-				    (test & (0x1 << j)) ? 1 : 0;
+				    (test & (UINT32_C(1) << j)) ? 1 : 0;
 			}
 			k = cond_evaluate_expr(p, cn->expr);
 			if (k == -1) {
@@ -428,7 +428,7 @@ int cond_normalize_expr(policydb_t * p, cond_node_t * cn)
 			}
 			/* set the bit if expression evaluates true */
 			if (k)
-				cn->expr_pre_comp |= 0x1 << test;
+				cn->expr_pre_comp |= UINT32_C(1) << test;
 		}
 
 		/* restore bool default values */
diff --git a/libsepol/src/link.c b/libsepol/src/link.c
index 461d2feb..7512a4d9 100644
--- a/libsepol/src/link.c
+++ b/libsepol/src/link.c
@@ -1291,10 +1291,10 @@ static int copy_avrule_list(avrule_t * list, avrule_t ** dst,
 				     i <
 				     module->perm_map_len[cur_perm->tclass - 1];
 				     i++) {
-					if (!(cur_perm->data & (1U << i)))
+					if (!(cur_perm->data & (UINT32_C(1) << i)))
 						continue;
 					new_perm->data |=
-					    (1U <<
+					    (UINT32_C(1) <<
 					     (module->
 					      perm_map[cur_perm->tclass - 1][i] -
 					      1));
diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c
index 7093d9b7..587ba64a 100644
--- a/libsepol/src/policydb.c
+++ b/libsepol/src/policydb.c
@@ -4166,7 +4166,7 @@ static sepol_access_vector_t policydb_string_to_av_perm(
 			hashtab_search(tclass_datum->permissions.table,
 			(hashtab_key_t)perm_name);
 	if (perm_datum != NULL)
-		return 0x1U << (perm_datum->s.value - 1);
+		return UINT32_C(1) << (perm_datum->s.value - 1);
 
 	if (tclass_datum->comdatum == NULL)
 		return 0;
@@ -4176,7 +4176,7 @@ static sepol_access_vector_t policydb_string_to_av_perm(
 			(hashtab_key_t)perm_name);
 
 	if (perm_datum != NULL)
-		return 0x1U << (perm_datum->s.value - 1);
+		return UINT32_C(1) << (perm_datum->s.value - 1);
 
 	return 0;
 }
diff --git a/libsepol/src/services.c b/libsepol/src/services.c
index 673b3971..3407058f 100644
--- a/libsepol/src/services.c
+++ b/libsepol/src/services.c
@@ -1213,7 +1213,7 @@ int sepol_string_to_av_perm(sepol_security_class_t tclass,
 			hashtab_search(tclass_datum->permissions.table,
 			perm_name);
 	if (perm_datum != NULL) {
-		*av = 0x1 << (perm_datum->s.value - 1);
+		*av = UINT32_C(1) << (perm_datum->s.value - 1);
 		return STATUS_SUCCESS;
 	}
 
@@ -1225,7 +1225,7 @@ int sepol_string_to_av_perm(sepol_security_class_t tclass,
 			perm_name);
 
 	if (perm_datum != NULL) {
-		*av = 0x1 << (perm_datum->s.value - 1);
+		*av = UINT32_C(1) << (perm_datum->s.value - 1);
 		return STATUS_SUCCESS;
 	}
 out:
-- 
2.31.1

[PATCH 2/4] libsepol/cil: Fix potential undefined shifts

[James Carter]

An expression of the form "1 << x" is undefined if x == 31 because
the "1" is an int and cannot be left shifted by 31.

Instead, use "UINT32_C(1) << x" which will be an unsigned int of
at least 32 bits.

This bug was found by the secilc-fuzzer.

Signed-off-by: James Carter <jwcart2@gmail.com>
---
 libsepol/cil/src/cil_binary.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/libsepol/cil/src/cil_binary.c b/libsepol/cil/src/cil_binary.c
index ec5f01e5..34dc63c7 100644
--- a/libsepol/cil/src/cil_binary.c
+++ b/libsepol/cil/src/cil_binary.c
@@ -1225,7 +1225,7 @@ int __perm_str_to_datum(char *perm_str, class_datum_t *sepol_class, uint32_t *da
 			goto exit;
 		}
 	}
-	*datum |= 1 << (sepol_perm->s.value - 1);
+	*datum |= UINT32_C(1) << (sepol_perm->s.value - 1);
 
 	return SEPOL_OK;
 
@@ -1523,7 +1523,7 @@ int cil_avrule_to_policydb(policydb_t *pdb, const struct cil_db *db, struct cil_
 /* index of the u32 containing the permission */
 #define XPERM_IDX(x) (x >> 5)
 /* set bits 0 through x-1 within the u32 */
-#define XPERM_SETBITS(x) ((1U << (x & 0x1f)) - 1)
+#define XPERM_SETBITS(x) (UINT32_C(1) << (x & 0x1f)) - 1)
 /* low value for this u32 */
 #define XPERM_LOW(x) (x << 5)
 /* high value for this u32 */
@@ -4760,7 +4760,7 @@ static struct cil_list *cil_classperms_from_sepol(policydb_t *pdb, uint16_t clas
 	cil_list_init(&cp->perms, CIL_PERM);
 	for (i = 0; i < sepol_class->permissions.nprim; i++) {
 		struct cil_perm *perm;
-		if ((data & (1 << i)) == 0) continue;
+		if ((data & (UINT32_C(1) << i)) == 0) continue;
 		perm = perm_value_to_cil[class][i+1];
 		if (!perm) goto exit;
 		cil_list_append(cp->perms, CIL_PERM, perm);
-- 
2.31.1

[PATCH 3/4] checkpolicy: Fix potential undefined shifts

[James Carter]

An expression of the form "1 << x" is undefined if x == 31 because
the "1" is an int and cannot be left shifted by 31.

Instead, use "UINT32_C(1) << x" which will be an unsigned int of
at least 32 bits.

Signed-off-by: James Carter <jwcart2@gmail.com>
---
 checkpolicy/checkpolicy.c   | 2 +-
 checkpolicy/policy_define.c | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/checkpolicy/checkpolicy.c b/checkpolicy/checkpolicy.c
index 6740c6d4..926ce72c 100644
--- a/checkpolicy/checkpolicy.c
+++ b/checkpolicy/checkpolicy.c
@@ -794,7 +794,7 @@ int main(int argc, char **argv)
 			case 0:
 				printf("\nallowed {");
 				for (i = 1; i <= sizeof(avd.allowed) * 8; i++) {
-					if (avd.allowed & (1 << (i - 1))) {
+					if (avd.allowed & (UINT32_C(1) << (i - 1))) {
 						v.val = i;
 						ret =
 						    hashtab_map(cladatum->
diff --git a/checkpolicy/policy_define.c b/checkpolicy/policy_define.c
index cda3337b..d3eb6111 100644
--- a/checkpolicy/policy_define.c
+++ b/checkpolicy/policy_define.c
@@ -2126,7 +2126,7 @@ static int define_te_avtab_xperms_helper(int which, avrule_t ** rule)
 			     policydbp->p_class_val_to_name[i]);
 			continue;
 		} else {
-			cur_perms->data |= 1U << (perdatum->s.value - 1);
+			cur_perms->data |= UINT32_C(1) << (perdatum->s.value - 1);
 		}
 	}
 
@@ -2142,7 +2142,7 @@ out:
 /* index of the u32 containing the permission */
 #define XPERM_IDX(x) ((x) >> 5)
 /* set bits 0 through x-1 within the u32 */
-#define XPERM_SETBITS(x) ((1U << ((x) & 0x1f)) - 1)
+#define XPERM_SETBITS(x) ((UINT32_C(1) << ((x) & 0x1f)) - 1)
 /* low value for this u32 */
 #define XPERM_LOW(x) ((x) << 5)
 /* high value for this u32 */
@@ -2612,7 +2612,7 @@ static int define_te_avtab_helper(int which, avrule_t ** rule)
 				}
 				continue;
 			} else {
-				cur_perms->data |= 1U << (perdatum->s.value - 1);
+				cur_perms->data |= UINT32_C(1) << (perdatum->s.value - 1);
 			}
 		      next:
 			cur_perms = cur_perms->next;
@@ -3615,7 +3615,7 @@ int define_constraint(constraint_expr_t * expr)
 					return -1;
 				}
 			}
-			node->permissions |= (1 << (perdatum->s.value - 1));
+			node->permissions |= (UINT32_C(1) << (perdatum->s.value - 1));
 		}
 		free(id);
 	}
-- 
2.31.1

[PATCH 4/4] libselinux: Fix potential undefined shifts

[James Carter]

An expression of the form "1 << x" is undefined if x == 31 because
the "1" is an int and cannot be left shifted by 31.

Instead, use "UINT32_C(1) << x" which will be an unsigned int of
at least 32 bits.

Signed-off-by: James Carter <jwcart2@gmail.com>
---
 libselinux/src/mapping.c   | 22 +++++++++++-----------
 libselinux/src/stringrep.c |  8 ++++----
 2 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/libselinux/src/mapping.c b/libselinux/src/mapping.c
index 96395fd4..dd2f1039 100644
--- a/libselinux/src/mapping.c
+++ b/libselinux/src/mapping.c
@@ -144,9 +144,9 @@ unmap_perm(security_class_t tclass, access_vector_t tperm)
 		access_vector_t kperm = 0;
 
 		for (i = 0; i < current_mapping[tclass].num_perms; i++)
-			if (tperm & (1<<i)) {
+			if (tperm & (UINT32_C(1)<<i)) {
 				kperm |= current_mapping[tclass].perms[i];
-				tperm &= ~(1<<i);
+				tperm &= ~(UINT32_C(1)<<i);
 			}
 		return kperm;
 	}
@@ -191,7 +191,7 @@ map_perm(security_class_t tclass, access_vector_t kperm)
 
 		for (i = 0; i < current_mapping[tclass].num_perms; i++)
 			if (kperm & current_mapping[tclass].perms[i]) {
-				tperm |= 1<<i;
+				tperm |= UINT32_C(1)<<i;
 				kperm &= ~current_mapping[tclass].perms[i];
 			}
 
@@ -216,30 +216,30 @@ map_decision(security_class_t tclass, struct av_decision *avd)
 
 		for (i = 0, result = 0; i < n; i++) {
 			if (avd->allowed & mapping->perms[i])
-				result |= 1<<i;
+				result |= UINT32_C(1)<<i;
 			else if (allow_unknown && !mapping->perms[i])
-				result |= 1<<i;
+				result |= UINT32_C(1)<<i;
 		}
 		avd->allowed = result;
 
 		for (i = 0, result = 0; i < n; i++) {
 			if (avd->decided & mapping->perms[i])
-				result |= 1<<i;
+				result |= UINT32_C(1)<<i;
 			else if (allow_unknown && !mapping->perms[i])
-				result |= 1<<i;
+				result |= UINT32_C(1)<<i;
 		}
 		avd->decided = result;
 
 		for (i = 0, result = 0; i < n; i++)
 			if (avd->auditallow & mapping->perms[i])
-				result |= 1<<i;
+				result |= UINT32_C(1)<<i;
 		avd->auditallow = result;
 
 		for (i = 0, result = 0; i < n; i++) {
 			if (avd->auditdeny & mapping->perms[i])
-				result |= 1<<i;
+				result |= UINT32_C(1)<<i;
 			else if (!allow_unknown && !mapping->perms[i])
-				result |= 1<<i;
+				result |= UINT32_C(1)<<i;
 		}
 
 		/*
@@ -248,7 +248,7 @@ map_decision(security_class_t tclass, struct av_decision *avd)
 		 * a bug in the object manager.
 		 */
 		for (; i < (sizeof(result)*8); i++)
-			result |= 1<<i;
+			result |= UINT32_C(1)<<i;
 		avd->auditdeny = result;
 	}
 }
diff --git a/libselinux/src/stringrep.c b/libselinux/src/stringrep.c
index 012a740a..2fe69f43 100644
--- a/libselinux/src/stringrep.c
+++ b/libselinux/src/stringrep.c
@@ -229,7 +229,7 @@ access_vector_t string_to_av_perm(security_class_t tclass, const char *s)
 		size_t i;
 		for (i = 0; i < MAXVECTORS && node->perms[i] != NULL; i++)
 			if (strcmp(node->perms[i],s) == 0)
-				return map_perm(tclass, 1<<i);
+				return map_perm(tclass, UINT32_C(1)<<i);
 	}
 
 	errno = EINVAL;
@@ -261,7 +261,7 @@ const char *security_av_perm_to_string(security_class_t tclass,
 	node = get_class_cache_entry_value(tclass);
 	if (av && node)
 		for (i = 0; i<MAXVECTORS; i++)
-			if ((1<<i) & av)
+			if ((UINT32_C(1)<<i) & av)
 				return node->perms[i];
 
 	return NULL;
@@ -279,7 +279,7 @@ int security_av_string(security_class_t tclass, access_vector_t av, char **res)
 	/* first pass computes the required length */
 	for (i = 0; tmp; tmp >>= 1, i++) {
 		if (tmp & 1) {
-			str = security_av_perm_to_string(tclass, av & (1<<i));
+			str = security_av_perm_to_string(tclass, av & (UINT32_C(1)<<i));
 			if (str)
 				len += strlen(str) + 1;
 		}
@@ -303,7 +303,7 @@ int security_av_string(security_class_t tclass, access_vector_t av, char **res)
 	ptr += sprintf(ptr, "{ ");
 	for (i = 0; tmp; tmp >>= 1, i++) {
 		if (tmp & 1) {
-			str = security_av_perm_to_string(tclass, av & (1<<i));
+			str = security_av_perm_to_string(tclass, av & (UINT32_C(1)<<i));
 			if (str)
 				ptr += sprintf(ptr, "%s ", str);
 		}
-- 
2.31.1

Re: [PATCH 2/4] libsepol/cil: Fix potential undefined shifts

[Nicolas Iooss]

On Fri, Oct 8, 2021 at 11:10 PM James Carter <jwcart2@gmail.com> wrote:
>
> An expression of the form "1 << x" is undefined if x == 31 because
> the "1" is an int and cannot be left shifted by 31.
>
> Instead, use "UINT32_C(1) << x" which will be an unsigned int of
> at least 32 bits.
>
> This bug was found by the secilc-fuzzer.
>
> Signed-off-by: James Carter <jwcart2@gmail.com>
> ---
>  libsepol/cil/src/cil_binary.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/libsepol/cil/src/cil_binary.c b/libsepol/cil/src/cil_binary.c
> index ec5f01e5..34dc63c7 100644
> --- a/libsepol/cil/src/cil_binary.c
> +++ b/libsepol/cil/src/cil_binary.c
> @@ -1225,7 +1225,7 @@ int __perm_str_to_datum(char *perm_str, class_datum_t *sepol_class, uint32_t *da
>                         goto exit;
>                 }
>         }
> -       *datum |= 1 << (sepol_perm->s.value - 1);
> +       *datum |= UINT32_C(1) << (sepol_perm->s.value - 1);
>
>         return SEPOL_OK;
>
> @@ -1523,7 +1523,7 @@ int cil_avrule_to_policydb(policydb_t *pdb, const struct cil_db *db, struct cil_
>  /* index of the u32 containing the permission */
>  #define XPERM_IDX(x) (x >> 5)
>  /* set bits 0 through x-1 within the u32 */
> -#define XPERM_SETBITS(x) ((1U << (x & 0x1f)) - 1)
> +#define XPERM_SETBITS(x) (UINT32_C(1) << (x & 0x1f)) - 1)

There is a missing parenthesis here: "UINT32_C(1)" -> "(UINT32_C(1)"

Other than this, the 4 patches look good to me:

Acked-by: Nicolas Iooss <nicolas.iooss@m4x.org>

Thanks!
Nicolas

>  /* low value for this u32 */
>  #define XPERM_LOW(x) (x << 5)
>  /* high value for this u32 */
> @@ -4760,7 +4760,7 @@ static struct cil_list *cil_classperms_from_sepol(policydb_t *pdb, uint16_t clas
>         cil_list_init(&cp->perms, CIL_PERM);
>         for (i = 0; i < sepol_class->permissions.nprim; i++) {
>                 struct cil_perm *perm;
> -               if ((data & (1 << i)) == 0) continue;
> +               if ((data & (UINT32_C(1) << i)) == 0) continue;
>                 perm = perm_value_to_cil[class][i+1];
>                 if (!perm) goto exit;
>                 cil_list_append(cp->perms, CIL_PERM, perm);
> --
> 2.31.1
>

Re: [PATCH 2/4] libsepol/cil: Fix potential undefined shifts

[James Carter]

On Sun, Oct 10, 2021 at 4:40 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
> On Fri, Oct 8, 2021 at 11:10 PM James Carter <jwcart2@gmail.com> wrote:
> >
> > An expression of the form "1 << x" is undefined if x == 31 because
> > the "1" is an int and cannot be left shifted by 31.
> >
> > Instead, use "UINT32_C(1) << x" which will be an unsigned int of
> > at least 32 bits.
> >
> > This bug was found by the secilc-fuzzer.
> >
> > Signed-off-by: James Carter <jwcart2@gmail.com>
> > ---
> >  libsepol/cil/src/cil_binary.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/libsepol/cil/src/cil_binary.c b/libsepol/cil/src/cil_binary.c
> > index ec5f01e5..34dc63c7 100644
> > --- a/libsepol/cil/src/cil_binary.c
> > +++ b/libsepol/cil/src/cil_binary.c
> > @@ -1225,7 +1225,7 @@ int __perm_str_to_datum(char *perm_str, class_datum_t *sepol_class, uint32_t *da
> >                         goto exit;
> >                 }
> >         }
> > -       *datum |= 1 << (sepol_perm->s.value - 1);
> > +       *datum |= UINT32_C(1) << (sepol_perm->s.value - 1);
> >
> >         return SEPOL_OK;
> >
> > @@ -1523,7 +1523,7 @@ int cil_avrule_to_policydb(policydb_t *pdb, const struct cil_db *db, struct cil_
> >  /* index of the u32 containing the permission */
> >  #define XPERM_IDX(x) (x >> 5)
> >  /* set bits 0 through x-1 within the u32 */
> > -#define XPERM_SETBITS(x) ((1U << (x & 0x1f)) - 1)
> > +#define XPERM_SETBITS(x) (UINT32_C(1) << (x & 0x1f)) - 1)
>
> There is a missing parenthesis here: "UINT32_C(1)" -> "(UINT32_C(1)"
>
> Other than this, the 4 patches look good to me:
>
> Acked-by: Nicolas Iooss <nicolas.iooss@m4x.org>
>

I fixed the missing parenthesis and merged these patches.
Thanks,
Jim

> Thanks!
> Nicolas
>
> >  /* low value for this u32 */
> >  #define XPERM_LOW(x) (x << 5)
> >  /* high value for this u32 */
> > @@ -4760,7 +4760,7 @@ static struct cil_list *cil_classperms_from_sepol(policydb_t *pdb, uint16_t clas
> >         cil_list_init(&cp->perms, CIL_PERM);
> >         for (i = 0; i < sepol_class->permissions.nprim; i++) {
> >                 struct cil_perm *perm;
> > -               if ((data & (1 << i)) == 0) continue;
> > +               if ((data & (UINT32_C(1) << i)) == 0) continue;
> >                 perm = perm_value_to_cil[class][i+1];
> >                 if (!perm) goto exit;
> >                 cil_list_append(cp->perms, CIL_PERM, perm);
> > --
> > 2.31.1
> >
>