diff mbox series

selinux: use unsigned char for boolean values

Message ID 20220502135907.31035-1-cgzones@googlemail.com (mailing list archive)
State New, archived
Delegated to: Paul Moore
Headers show
Series selinux: use unsigned char for boolean values | expand

Commit Message

Christian Göttsche May 2, 2022, 1:59 p.m. UTC 
Reported by sparse:

    security/selinux/selinuxfs.c:1483:30: warning: incorrect type in assignment (different signedness)
    security/selinux/selinuxfs.c:1483:30:    expected unsigned int *
    security/selinux/selinuxfs.c:1483:30:    got int *[addressable] values
    security/selinux/selinuxfs.c:1400:48: warning: incorrect type in argument 3 (different signedness)
    security/selinux/selinuxfs.c:1400:48:    expected int *values
    security/selinux/selinuxfs.c:1400:48:    got unsigned int *bool_pending_values

Also mark the read-only boolean array parameter of security_set_bools()
const.

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

Comments

Paul Moore May 3, 2022, 8:18 p.m. UTC | #1 
On Mon, May 2, 2022 at 9:59 AM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> Reported by sparse:
>
>     security/selinux/selinuxfs.c:1483:30: warning: incorrect type in assignment (different signedness)
>     security/selinux/selinuxfs.c:1483:30:    expected unsigned int *
>     security/selinux/selinuxfs.c:1483:30:    got int *[addressable] values
>     security/selinux/selinuxfs.c:1400:48: warning: incorrect type in argument 3 (different signedness)
>     security/selinux/selinuxfs.c:1400:48:    expected int *values
>     security/selinux/selinuxfs.c:1400:48:    got unsigned int *bool_pending_values
>
> Also mark the read-only boolean array parameter of security_set_bools()
> const.
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
>  security/selinux/include/conditional.h |  4 ++--
>  security/selinux/selinuxfs.c           | 12 ++++++------
>  security/selinux/ss/policydb.h         |  2 +-
>  security/selinux/ss/services.c         | 13 +++++++------
>  4 files changed, 16 insertions(+), 15 deletions(-)

I could understand fixing the signed/unsigned type mismatch, but I
don't quite understand the move from an int type to a char; is it
simply to save space, i.e. 32-bits vs 8-bits?  I think I would prefer
either simply fixing the signed/unsigned mismatch and leaving the
booleans as ints, or moving completely to a bool type, although that
is likely to be much more involved.
diff mbox series

Patch

diff --git a/security/selinux/include/conditional.h b/security/selinux/include/conditional.h
index b09343346e3f..f1b52115e0a3 100644
--- a/security/selinux/include/conditional.h
+++ b/security/selinux/include/conditional.h
@@ -14,9 +14,9 @@ 
 #include "security.h"
 
 int security_get_bools(struct selinux_policy *policy,
-		       u32 *len, char ***names, int **values);
+		       u32 *len, char ***names, unsigned char **values);
 
-int security_set_bools(struct selinux_state *state, u32 len, int *values);
+int security_set_bools(struct selinux_state *state, u32 len, const unsigned char *values);
 
 int security_get_bool_value(struct selinux_state *state, u32 index);
 
diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index 8fcdd494af27..404b4561f8b0 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -71,7 +71,7 @@  struct selinux_fs_info {
 	struct dentry *bool_dir;
 	unsigned int bool_num;
 	char **bool_pending_names;
-	unsigned int *bool_pending_values;
+	unsigned char *bool_pending_values;
 	struct dentry *class_dir;
 	unsigned long last_class_ino;
 	bool policy_opened;
@@ -356,7 +356,7 @@  static const struct file_operations sel_policyvers_ops = {
 /* declaration for sel_write_load */
 static int sel_make_bools(struct selinux_policy *newpolicy, struct dentry *bool_dir,
 			  unsigned int *bool_num, char ***bool_pending_names,
-			  unsigned int **bool_pending_values);
+			  unsigned char **bool_pending_values);
 static int sel_make_classes(struct selinux_policy *newpolicy,
 			    struct dentry *class_dir,
 			    unsigned long *last_class_ino);
@@ -527,7 +527,7 @@  static const struct file_operations sel_policy_ops = {
 };
 
 static void sel_remove_old_bool_data(unsigned int bool_num, char **bool_names,
-				unsigned int *bool_values)
+				unsigned char *bool_values)
 {
 	u32 i;
 
@@ -545,7 +545,7 @@  static int sel_make_policy_nodes(struct selinux_fs_info *fsi,
 	struct dentry *tmp_parent, *tmp_bool_dir, *tmp_class_dir, *old_dentry;
 	unsigned int tmp_bool_num, old_bool_num;
 	char **tmp_bool_names, **old_bool_names;
-	unsigned int *tmp_bool_values, *old_bool_values;
+	unsigned char *tmp_bool_values, *old_bool_values;
 	unsigned long tmp_ino = fsi->last_ino; /* Don't increment last_ino in this function */
 
 	tmp_parent = sel_make_disconnected_dir(fsi->sb, &tmp_ino);
@@ -1423,7 +1423,7 @@  static void sel_remove_entries(struct dentry *de)
 
 static int sel_make_bools(struct selinux_policy *newpolicy, struct dentry *bool_dir,
 			  unsigned int *bool_num, char ***bool_pending_names,
-			  unsigned int **bool_pending_values)
+			  unsigned char **bool_pending_values)
 {
 	int ret;
 	ssize_t len;
@@ -1432,7 +1432,7 @@  static int sel_make_bools(struct selinux_policy *newpolicy, struct dentry *bool_
 	struct inode_security_struct *isec;
 	char **names = NULL, *page;
 	u32 i, num;
-	int *values = NULL;
+	unsigned char *values = NULL;
 	u32 sid;
 
 	ret = -ENOMEM;
diff --git a/security/selinux/ss/policydb.h b/security/selinux/ss/policydb.h
index c24d4e1063ea..f85e875a7799 100644
--- a/security/selinux/ss/policydb.h
+++ b/security/selinux/ss/policydb.h
@@ -148,7 +148,7 @@  struct range_trans {
 /* Boolean data type */
 struct cond_bool_datum {
 	__u32 value;		/* internal type value */
-	int state;
+	unsigned char state;
 };
 
 struct cond_node;
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 802a80648c6c..7349ed4a4d0d 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -3022,7 +3022,7 @@  int security_fs_use(struct selinux_state *state, struct super_block *sb)
 }
 
 int security_get_bools(struct selinux_policy *policy,
-		       u32 *len, char ***names, int **values)
+		       u32 *len, char ***names, unsigned char **values)
 {
 	struct policydb *policydb;
 	u32 i;
@@ -3044,7 +3044,7 @@  int security_get_bools(struct selinux_policy *policy,
 		goto err;
 
 	rc = -ENOMEM;
-	*values = kcalloc(*len, sizeof(int), GFP_ATOMIC);
+	*values = kcalloc(*len, sizeof(unsigned char), GFP_ATOMIC);
 	if (!*values)
 		goto err;
 
@@ -3074,7 +3074,7 @@  int security_get_bools(struct selinux_policy *policy,
 }
 
 
-int security_set_bools(struct selinux_state *state, u32 len, int *values)
+int security_set_bools(struct selinux_state *state, u32 len, const unsigned char *values)
 {
 	struct selinux_policy *newpolicy, *oldpolicy;
 	int rc;
@@ -3106,8 +3106,8 @@  int security_set_bools(struct selinux_state *state, u32 len, int *values)
 
 	/* Update the boolean states in the copy */
 	for (i = 0; i < len; i++) {
-		int new_state = !!values[i];
-		int old_state = newpolicy->policydb.bool_val_to_struct[i]->state;
+		unsigned char new_state = !!values[i];
+		unsigned char old_state = newpolicy->policydb.bool_val_to_struct[i]->state;
 
 		if (new_state != old_state) {
 			audit_log(audit_context(), GFP_ATOMIC,
@@ -3174,7 +3174,8 @@  int security_get_bool_value(struct selinux_state *state,
 static int security_preserve_bools(struct selinux_policy *oldpolicy,
 				struct selinux_policy *newpolicy)
 {
-	int rc, *bvalues = NULL;
+	int rc;
+	unsigned char *bvalues = NULL;
 	char **bnames = NULL;
 	struct cond_bool_datum *booldatum;
 	u32 i, nbools = 0;