diff mbox series

[05/23] libsepol: avoid implicit conversions

Message ID 20210608155912.32047-6-cgzones@googlemail.com (mailing list archive)
State Accepted
Headers show
Series libsepol: miscellaneous cleanup | expand

Commit Message

Christian Göttsche June 8, 2021, 3:58 p.m. UTC
Avoid implicit conversions from signed to unsigned values, found by
UB sanitizers, by using unsigned values in the first place.

expand.c:1644:18: runtime error: implicit conversion from type 'int' of value -1 (32-bit, signed) to type 'uint32_t' (aka 'unsigned int') changed the value to 4294967295 (32-bit, unsigned)

expand.c:2892:24: runtime error: implicit conversion from type 'int' of value -2 (32-bit, signed) to type 'unsigned int' changed the value to 4294967294 (32-bit, unsigned)

policy_define.c:2344:4: runtime error: implicit conversion from type 'int' of value -1048577 (32-bit, signed) to type 'unsigned int' changed the value to 4293918719 (32-bit, unsigned)

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libsepol/include/sepol/policydb/conditional.h | 2 +-
 libsepol/include/sepol/policydb/policydb.h    | 2 +-
 libsepol/src/expand.c                         | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

Comments

Ondrej Mosnacek June 9, 2021, 1:47 p.m. UTC | #1
On Tue, Jun 8, 2021 at 5:59 PM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> Avoid implicit conversions from signed to unsigned values, found by
> UB sanitizers, by using unsigned values in the first place.
>
> expand.c:1644:18: runtime error: implicit conversion from type 'int' of value -1 (32-bit, signed) to type 'uint32_t' (aka 'unsigned int') changed the value to 4294967295 (32-bit, unsigned)
>
> expand.c:2892:24: runtime error: implicit conversion from type 'int' of value -2 (32-bit, signed) to type 'unsigned int' changed the value to 4294967294 (32-bit, unsigned)
>
> policy_define.c:2344:4: runtime error: implicit conversion from type 'int' of value -1048577 (32-bit, signed) to type 'unsigned int' changed the value to 4293918719 (32-bit, unsigned)
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
>  libsepol/include/sepol/policydb/conditional.h | 2 +-
>  libsepol/include/sepol/policydb/policydb.h    | 2 +-
>  libsepol/src/expand.c                         | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/libsepol/include/sepol/policydb/conditional.h b/libsepol/include/sepol/policydb/conditional.h
> index 9c3df3ef..db3ef98d 100644
> --- a/libsepol/include/sepol/policydb/conditional.h
> +++ b/libsepol/include/sepol/policydb/conditional.h
> @@ -90,7 +90,7 @@ typedef struct cond_node {
>         uint32_t expr_pre_comp;
>         struct cond_node *next;
>         /* a tunable conditional, calculated and used at expansion */
> -#define        COND_NODE_FLAGS_TUNABLE 0x01
> +#define        COND_NODE_FLAGS_TUNABLE 0x01U

UINT32_C(0x01) would be better here.

>         uint32_t flags;
>  } cond_node_t;
>
> diff --git a/libsepol/include/sepol/policydb/policydb.h b/libsepol/include/sepol/policydb/policydb.h
> index 9ef43abc..c29339dc 100644
> --- a/libsepol/include/sepol/policydb/policydb.h
> +++ b/libsepol/include/sepol/policydb/policydb.h
> @@ -253,7 +253,7 @@ typedef struct class_perm_node {
>
>  #define xperm_test(x, p) (1 & (p[x >> 5] >> (x & 0x1f)))
>  #define xperm_set(x, p) (p[x >> 5] |= (1 << (x & 0x1f)))
> -#define xperm_clear(x, p) (p[x >> 5] &= ~(1 << (x & 0x1f)))
> +#define xperm_clear(x, p) (p[x >> 5] &= ~(1U << (x & 0x1f)))

Ditto here (used on av_extended_perms_t::perms, which is uint32_t).
And xperm_test() and xperm_set() should be also updated in the same
way.

>  #define EXTENDED_PERMS_LEN 8
>
>  typedef struct av_extended_perms {
> diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c
> index 84bfcfa3..35e45780 100644
> --- a/libsepol/src/expand.c
> +++ b/libsepol/src/expand.c
> @@ -1641,7 +1641,7 @@ static avtab_ptr_t find_avtab_node(sepol_handle_t * handle,
>                  * AUDITDENY, aka DONTAUDIT, are &= assigned, versus |= for
>                  * others. Initialize the data accordingly.
>                  */
> -               avdatum.data = key->specified == AVTAB_AUDITDENY ? ~0 : 0;
> +               avdatum.data = key->specified == AVTAB_AUDITDENY ? ~0U : 0U;

Also UINT32_C().

>                 /* this is used to get the node - insertion is actually unique */
>                 node = avtab_insert_nonunique(avtab, key, &avdatum);
>                 if (!node) {
> --
> 2.32.0
>

--
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.
diff mbox series

Patch

diff --git a/libsepol/include/sepol/policydb/conditional.h b/libsepol/include/sepol/policydb/conditional.h
index 9c3df3ef..db3ef98d 100644
--- a/libsepol/include/sepol/policydb/conditional.h
+++ b/libsepol/include/sepol/policydb/conditional.h
@@ -90,7 +90,7 @@  typedef struct cond_node {
 	uint32_t expr_pre_comp;
 	struct cond_node *next;
 	/* a tunable conditional, calculated and used at expansion */
-#define	COND_NODE_FLAGS_TUNABLE	0x01
+#define	COND_NODE_FLAGS_TUNABLE	0x01U
 	uint32_t flags;
 } cond_node_t;
 
diff --git a/libsepol/include/sepol/policydb/policydb.h b/libsepol/include/sepol/policydb/policydb.h
index 9ef43abc..c29339dc 100644
--- a/libsepol/include/sepol/policydb/policydb.h
+++ b/libsepol/include/sepol/policydb/policydb.h
@@ -253,7 +253,7 @@  typedef struct class_perm_node {
 
 #define xperm_test(x, p) (1 & (p[x >> 5] >> (x & 0x1f)))
 #define xperm_set(x, p) (p[x >> 5] |= (1 << (x & 0x1f)))
-#define xperm_clear(x, p) (p[x >> 5] &= ~(1 << (x & 0x1f)))
+#define xperm_clear(x, p) (p[x >> 5] &= ~(1U << (x & 0x1f)))
 #define EXTENDED_PERMS_LEN 8
 
 typedef struct av_extended_perms {
diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c
index 84bfcfa3..35e45780 100644
--- a/libsepol/src/expand.c
+++ b/libsepol/src/expand.c
@@ -1641,7 +1641,7 @@  static avtab_ptr_t find_avtab_node(sepol_handle_t * handle,
 		 * AUDITDENY, aka DONTAUDIT, are &= assigned, versus |= for
 		 * others. Initialize the data accordingly.
 		 */
-		avdatum.data = key->specified == AVTAB_AUDITDENY ? ~0 : 0;
+		avdatum.data = key->specified == AVTAB_AUDITDENY ? ~0U : 0U;
 		/* this is used to get the node - insertion is actually unique */
 		node = avtab_insert_nonunique(avtab, key, &avdatum);
 		if (!node) {