[PATCH 2/9] selinux: declare path parameters of _genfs_sid const

[PATCH 2/9] selinux: declare path parameters of _genfs_sid const

[Christian Göttsche]

The path parameter is only read from in security_genfs_sid(),
selinux_policy_genfs_sid() and __security_genfs_sid(). Since a string
literal is passed as argument, declare the parameter const.
Also align the parameter names in the declaration and definition.

Reported by clang [-Wwrite-strings]:

    security/selinux/hooks.c:553:60: error: passing 'const char [2]' to parameter of type 'char *' discards qualifiers [-Wincompatible-pointer-types-discards-qualifiers]
            rc = security_genfs_sid(&selinux_state, sb->s_type->name, /,
                                                                      ^~~
    ./security/selinux/include/security.h:389:36: note: passing argument to parameter 'name' here
                           const char *fstype, char *name, u16 sclass,
                                                     ^

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 security/selinux/include/security.h | 4 ++--
 security/selinux/ss/services.c      | 6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
index ac0ece01305a..6482e0efb368 100644
--- a/security/selinux/include/security.h
+++ b/security/selinux/include/security.h
@@ -386,11 +386,11 @@ int security_get_allow_unknown(struct selinux_state *state);
 int security_fs_use(struct selinux_state *state, struct super_block *sb);
 
 int security_genfs_sid(struct selinux_state *state,
-		       const char *fstype, char *name, u16 sclass,
+		       const char *fstype, const char *path, u16 sclass,
 		       u32 *sid);
 
 int selinux_policy_genfs_sid(struct selinux_policy *policy,
-		       const char *fstype, char *name, u16 sclass,
+		       const char *fstype, const char *path, u16 sclass,
 		       u32 *sid);
 
 #ifdef CONFIG_NETLABEL
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 8e92af7dd284..5a7df45bdab1 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -2875,7 +2875,7 @@ int security_get_user_sids(struct selinux_state *state,
  */
 static inline int __security_genfs_sid(struct selinux_policy *policy,
 				       const char *fstype,
-				       char *path,
+				       const char *path,
 				       u16 orig_sclass,
 				       u32 *sid)
 {
@@ -2928,7 +2928,7 @@ static inline int __security_genfs_sid(struct selinux_policy *policy,
  */
 int security_genfs_sid(struct selinux_state *state,
 		       const char *fstype,
-		       char *path,
+		       const char *path,
 		       u16 orig_sclass,
 		       u32 *sid)
 {
@@ -2952,7 +2952,7 @@ int security_genfs_sid(struct selinux_state *state,
 
 int selinux_policy_genfs_sid(struct selinux_policy *policy,
 			const char *fstype,
-			char *path,
+			const char *path,
 			u16 orig_sclass,
 			u32 *sid)
 {
-- 
2.34.1

[PATCH 3/9] selinux: declare name parameter of hash_eval const

[Christian Göttsche]

String literals are passed as second argument to hash_eval(). Also the
parameter is already declared const in the DEBUG_HASHES configuration.

Reported by clang [-Wwrite-strings]:

    security/selinux/ss/policydb.c:1881:26: error: passing 'const char [8]' to parameter of type 'char *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
            hash_eval(&p->range_tr, rangetr);
                                    ^~~~~~~~~
    security/selinux/ss/policydb.c:707:55: note: passing argument to parameter 'hash_name' here
    static inline void hash_eval(struct hashtab *h, char *hash_name)
                                                          ^
    security/selinux/ss/policydb.c:2099:32: error: passing 'const char [11]' to parameter of type 'char *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
            hash_eval(&p->filename_trans, filenametr);
                                          ^~~~~~~~~~~~
    security/selinux/ss/policydb.c:707:55: note: passing argument to parameter 'hash_name' here
    static inline void hash_eval(struct hashtab *h, char *hash_name)
                                                          ^

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 security/selinux/ss/policydb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index 0ae1b718194a..67e03f6e8966 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -704,7 +704,7 @@ static void symtab_hash_eval(struct symtab *s)
 }
 
 #else
-static inline void hash_eval(struct hashtab *h, char *hash_name)
+static inline void hash_eval(struct hashtab *h, const char *hash_name)
 {
 }
 #endif
-- 
2.34.1

Re: [PATCH 3/9] selinux: declare name parameter of hash_eval const

[Paul Moore]

On Tue, Jan 25, 2022 at 9:14 AM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> String literals are passed as second argument to hash_eval(). Also the
> parameter is already declared const in the DEBUG_HASHES configuration.
>
> Reported by clang [-Wwrite-strings]:
>
>     security/selinux/ss/policydb.c:1881:26: error: passing 'const char [8]' to parameter of type 'char *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
>             hash_eval(&p->range_tr, rangetr);
>                                     ^~~~~~~~~
>     security/selinux/ss/policydb.c:707:55: note: passing argument to parameter 'hash_name' here
>     static inline void hash_eval(struct hashtab *h, char *hash_name)
>                                                           ^
>     security/selinux/ss/policydb.c:2099:32: error: passing 'const char [11]' to parameter of type 'char *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
>             hash_eval(&p->filename_trans, filenametr);
>                                           ^~~~~~~~~~~~
>     security/selinux/ss/policydb.c:707:55: note: passing argument to parameter 'hash_name' here
>     static inline void hash_eval(struct hashtab *h, char *hash_name)
>                                                           ^
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
>  security/selinux/ss/policydb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Merged into selinux/next, thanks!

-- 
paul-moore.com

[PATCH 4/9] selinux: enclose macro arguments in parenthesis

[Christian Göttsche]

Enclose the macro arguments in parenthesis to avoid potential evaluation
order issues.

Note the xperm and ebitmap macros are still not side-effect safe due to
double evaluation.

Reported by clang-tidy [bugprone-macro-parentheses]

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 security/selinux/include/security.h | 4 ++--
 security/selinux/ss/ebitmap.h       | 6 +++---
 security/selinux/ss/sidtab.c        | 4 ++--
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
index 6482e0efb368..d91a5672de99 100644
--- a/security/selinux/include/security.h
+++ b/security/selinux/include/security.h
@@ -254,8 +254,8 @@ struct av_decision {
 #define XPERMS_AUDITALLOW 2
 #define XPERMS_DONTAUDIT 4
 
-#define security_xperm_set(perms, x) (perms[x >> 5] |= 1 << (x & 0x1f))
-#define security_xperm_test(perms, x) (1 & (perms[x >> 5] >> (x & 0x1f)))
+#define security_xperm_set(perms, x) ((perms)[(x) >> 5] |= 1 << ((x) & 0x1f))
+#define security_xperm_test(perms, x) (1 & ((perms)[(x) >> 5] >> ((x) & 0x1f)))
 struct extended_perms_data {
 	u32 p[8];
 };
diff --git a/security/selinux/ss/ebitmap.h b/security/selinux/ss/ebitmap.h
index 9eb2d0af2805..58eb822f11ee 100644
--- a/security/selinux/ss/ebitmap.h
+++ b/security/selinux/ss/ebitmap.h
@@ -118,9 +118,9 @@ static inline void ebitmap_node_clr_bit(struct ebitmap_node *n,
 }
 
 #define ebitmap_for_each_positive_bit(e, n, bit)	\
-	for (bit = ebitmap_start_positive(e, &n);	\
-	     bit < ebitmap_length(e);			\
-	     bit = ebitmap_next_positive(e, &n, bit))	\
+	for ((bit) = ebitmap_start_positive(e, &(n));	\
+	     (bit) < ebitmap_length(e);			\
+	     (bit) = ebitmap_next_positive(e, &(n), bit))	\
 
 int ebitmap_cmp(struct ebitmap *e1, struct ebitmap *e2);
 int ebitmap_cpy(struct ebitmap *dst, struct ebitmap *src);
diff --git a/security/selinux/ss/sidtab.c b/security/selinux/ss/sidtab.c
index 293ec048af08..a54b8652bfb5 100644
--- a/security/selinux/ss/sidtab.c
+++ b/security/selinux/ss/sidtab.c
@@ -27,8 +27,8 @@ struct sidtab_str_cache {
 	char str[];
 };
 
-#define index_to_sid(index) (index + SECINITSID_NUM + 1)
-#define sid_to_index(sid) (sid - (SECINITSID_NUM + 1))
+#define index_to_sid(index) ((index) + SECINITSID_NUM + 1)
+#define sid_to_index(sid) ((sid) - (SECINITSID_NUM + 1))
 
 int sidtab_init(struct sidtab *s)
 {
-- 
2.34.1

Re: [PATCH 4/9] selinux: enclose macro arguments in parenthesis

[Nick Desaulniers]

On Tue, Jan 25, 2022 at 6:14 AM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> Enclose the macro arguments in parenthesis to avoid potential evaluation
> order issues.
>
> Note the xperm and ebitmap macros are still not side-effect safe due to
> double evaluation.
>
> Reported by clang-tidy [bugprone-macro-parentheses]

Good idea to run clang-tidy :)
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
>  security/selinux/include/security.h | 4 ++--
>  security/selinux/ss/ebitmap.h       | 6 +++---
>  security/selinux/ss/sidtab.c        | 4 ++--
>  3 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
> index 6482e0efb368..d91a5672de99 100644
> --- a/security/selinux/include/security.h
> +++ b/security/selinux/include/security.h
> @@ -254,8 +254,8 @@ struct av_decision {
>  #define XPERMS_AUDITALLOW 2
>  #define XPERMS_DONTAUDIT 4
>
> -#define security_xperm_set(perms, x) (perms[x >> 5] |= 1 << (x & 0x1f))
> -#define security_xperm_test(perms, x) (1 & (perms[x >> 5] >> (x & 0x1f)))
> +#define security_xperm_set(perms, x) ((perms)[(x) >> 5] |= 1 << ((x) & 0x1f))
> +#define security_xperm_test(perms, x) (1 & ((perms)[(x) >> 5] >> ((x) & 0x1f)))
>  struct extended_perms_data {
>         u32 p[8];
>  };
> diff --git a/security/selinux/ss/ebitmap.h b/security/selinux/ss/ebitmap.h
> index 9eb2d0af2805..58eb822f11ee 100644
> --- a/security/selinux/ss/ebitmap.h
> +++ b/security/selinux/ss/ebitmap.h
> @@ -118,9 +118,9 @@ static inline void ebitmap_node_clr_bit(struct ebitmap_node *n,
>  }
>
>  #define ebitmap_for_each_positive_bit(e, n, bit)       \
> -       for (bit = ebitmap_start_positive(e, &n);       \
> -            bit < ebitmap_length(e);                   \
> -            bit = ebitmap_next_positive(e, &n, bit))   \
> +       for ((bit) = ebitmap_start_positive(e, &(n));   \
> +            (bit) < ebitmap_length(e);                 \
> +            (bit) = ebitmap_next_positive(e, &(n), bit))       \
>
>  int ebitmap_cmp(struct ebitmap *e1, struct ebitmap *e2);
>  int ebitmap_cpy(struct ebitmap *dst, struct ebitmap *src);
> diff --git a/security/selinux/ss/sidtab.c b/security/selinux/ss/sidtab.c
> index 293ec048af08..a54b8652bfb5 100644
> --- a/security/selinux/ss/sidtab.c
> +++ b/security/selinux/ss/sidtab.c
> @@ -27,8 +27,8 @@ struct sidtab_str_cache {
>         char str[];
>  };
>
> -#define index_to_sid(index) (index + SECINITSID_NUM + 1)
> -#define sid_to_index(sid) (sid - (SECINITSID_NUM + 1))
> +#define index_to_sid(index) ((index) + SECINITSID_NUM + 1)
> +#define sid_to_index(sid) ((sid) - (SECINITSID_NUM + 1))
>
>  int sidtab_init(struct sidtab *s)
>  {
> --
> 2.34.1
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH 4/9] selinux: enclose macro arguments in parenthesis

[Paul Moore]

On Tue, Jan 25, 2022 at 9:14 AM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> Enclose the macro arguments in parenthesis to avoid potential evaluation
> order issues.
>
> Note the xperm and ebitmap macros are still not side-effect safe due to
> double evaluation.
>
> Reported by clang-tidy [bugprone-macro-parentheses]
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
>  security/selinux/include/security.h | 4 ++--
>  security/selinux/ss/ebitmap.h       | 6 +++---
>  security/selinux/ss/sidtab.c        | 4 ++--
>  3 files changed, 7 insertions(+), 7 deletions(-)

Merged, thanks.

-- 
paul-moore.com

[PATCH 5/9] selinux: drop cast to same type

[Christian Göttsche]

Both the lvalue scontextp and rvalue scontext are of the type char*.
Drop the redundant explicit cast not needed since commit 9a59daa03df7
("SELinux: fix sleeping allocation in security_context_to_sid"), where
the type of scontext changed from const char* to char*.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 security/selinux/ss/services.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 5a7df45bdab1..2f8db93e53b2 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -1452,7 +1452,7 @@ static int string_to_context_struct(struct policydb *pol,
 	/* Parse the security context. */
 
 	rc = -EINVAL;
-	scontextp = (char *) scontext;
+	scontextp = scontext;
 
 	/* Extract the user. */
 	p = scontextp;
-- 
2.34.1

Re: [PATCH 5/9] selinux: drop cast to same type

[Paul Moore]

On Tue, Jan 25, 2022 at 9:14 AM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> Both the lvalue scontextp and rvalue scontext are of the type char*.
> Drop the redundant explicit cast not needed since commit 9a59daa03df7
> ("SELinux: fix sleeping allocation in security_context_to_sid"), where
> the type of scontext changed from const char* to char*.
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
>  security/selinux/ss/services.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Merged, thanks.

-- 
paul-moore.com

[PATCH 6/9] selinux: drop unused parameter of avtab_insert_node

[Christian Göttsche]

The parameter cur is not used in avtab_insert_node().

Reported by clang [-Wunused-parameter]

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 security/selinux/ss/avtab.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/security/selinux/ss/avtab.c b/security/selinux/ss/avtab.c
index c97695ae508f..cfdae20792e1 100644
--- a/security/selinux/ss/avtab.c
+++ b/security/selinux/ss/avtab.c
@@ -67,7 +67,7 @@ static inline int avtab_hash(const struct avtab_key *keyp, u32 mask)
 
 static struct avtab_node*
 avtab_insert_node(struct avtab *h, int hvalue,
-		  struct avtab_node *prev, struct avtab_node *cur,
+		  struct avtab_node *prev,
 		  const struct avtab_key *key, const struct avtab_datum *datum)
 {
 	struct avtab_node *newnode;
@@ -137,7 +137,7 @@ static int avtab_insert(struct avtab *h, const struct avtab_key *key,
 			break;
 	}
 
-	newnode = avtab_insert_node(h, hvalue, prev, cur, key, datum);
+	newnode = avtab_insert_node(h, hvalue, prev, key, datum);
 	if (!newnode)
 		return -ENOMEM;
 
@@ -177,7 +177,7 @@ struct avtab_node *avtab_insert_nonunique(struct avtab *h,
 		    key->target_class < cur->key.target_class)
 			break;
 	}
-	return avtab_insert_node(h, hvalue, prev, cur, key, datum);
+	return avtab_insert_node(h, hvalue, prev, key, datum);
 }
 
 struct avtab_datum *avtab_search(struct avtab *h, const struct avtab_key *key)
-- 
2.34.1

Re: [PATCH 6/9] selinux: drop unused parameter of avtab_insert_node

[Paul Moore]

On Tue, Jan 25, 2022 at 9:14 AM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> The parameter cur is not used in avtab_insert_node().
>
> Reported by clang [-Wunused-parameter]
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
>  security/selinux/ss/avtab.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Merged, thanks.

-- 
paul-moore.com

[PATCH 7/9] selinux: do not discard const qualifier in cast

[Christian Göttsche]

Do not discard the const qualifier on the cast from const void* to
__be32*; the addressed value is not modified.

Reported by clang [-Wcast-qual]

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 security/selinux/netnode.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/security/selinux/netnode.c b/security/selinux/netnode.c
index 4a7d2ab5b960..889552db0d31 100644
--- a/security/selinux/netnode.c
+++ b/security/selinux/netnode.c
@@ -107,7 +107,7 @@ static struct sel_netnode *sel_netnode_find(const void *addr, u16 family)
 
 	switch (family) {
 	case PF_INET:
-		idx = sel_netnode_hashfn_ipv4(*(__be32 *)addr);
+		idx = sel_netnode_hashfn_ipv4(*(const __be32 *)addr);
 		break;
 	case PF_INET6:
 		idx = sel_netnode_hashfn_ipv6(addr);
@@ -121,7 +121,7 @@ static struct sel_netnode *sel_netnode_find(const void *addr, u16 family)
 		if (node->nsec.family == family)
 			switch (family) {
 			case PF_INET:
-				if (node->nsec.addr.ipv4 == *(__be32 *)addr)
+				if (node->nsec.addr.ipv4 == *(const __be32 *)addr)
 					return node;
 				break;
 			case PF_INET6:
-- 
2.34.1

Re: [PATCH 7/9] selinux: do not discard const qualifier in cast

[Paul Moore]

On Tue, Jan 25, 2022 at 9:14 AM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> Do not discard the const qualifier on the cast from const void* to
> __be32*; the addressed value is not modified.
>
> Reported by clang [-Wcast-qual]
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
>  security/selinux/netnode.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Merged, thanks.

-- 
paul-moore.com

[PATCH 8/9] selinux: simplify cred_init_security

[Christian Göttsche]

The parameter of selinux_cred() is declared const, so an explicit cast
dropping the const qualifier is not necessary. Without the cast the
local variable cred serves no purpose.

Reported by clang [-Wcast-qual]

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 security/selinux/hooks.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 5b6895e4fc29..a840c8c1ec35 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -211,10 +211,9 @@ static int selinux_lsm_notifier_avc_callback(u32 event)
  */
 static void cred_init_security(void)
 {
-	struct cred *cred = (struct cred *) current->real_cred;
 	struct task_security_struct *tsec;
 
-	tsec = selinux_cred(cred);
+	tsec = selinux_cred(current->real_cred);
 	tsec->osid = tsec->sid = SECINITSID_KERNEL;
 }
 
-- 
2.34.1

Re: [PATCH 8/9] selinux: simplify cred_init_security

[Paul Moore]

On Tue, Jan 25, 2022 at 9:15 AM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> The parameter of selinux_cred() is declared const, so an explicit cast
> dropping the const qualifier is not necessary. Without the cast the
> local variable cred serves no purpose.
>
> Reported by clang [-Wcast-qual]
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
>  security/selinux/hooks.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Merged, thanks.

-- 
paul-moore.com

[PATCH 9/9] selinux: drop unused macro

[Christian Göttsche]

The macro _DEBUG_HASHES is nowhere used. The configuration DEBUG_HASHES
enables debugging of the SELinux hash tables, but the with an underscore
prefixed macro definition has no direct impact or any documentation.

Reported by clang [-Wunused-macros]

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 security/selinux/ss/policydb.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index 67e03f6e8966..d036e1238e77 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -41,8 +41,6 @@
 #include "mls.h"
 #include "services.h"
 
-#define _DEBUG_HASHES
-
 #ifdef DEBUG_HASHES
 static const char *symtab_name[SYM_NUM] = {
 	"common prefixes",
-- 
2.34.1

Re: [PATCH 9/9] selinux: drop unused macro

[Nick Desaulniers]

On Tue, Jan 25, 2022 at 6:15 AM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> The macro _DEBUG_HASHES is nowhere used. The configuration DEBUG_HASHES
> enables debugging of the SELinux hash tables, but the with an underscore
> prefixed macro definition has no direct impact or any documentation.
>
> Reported by clang [-Wunused-macros]
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>

Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

> ---
>  security/selinux/ss/policydb.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 67e03f6e8966..d036e1238e77 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -41,8 +41,6 @@
>  #include "mls.h"
>  #include "services.h"
>
> -#define _DEBUG_HASHES
> -
>  #ifdef DEBUG_HASHES
>  static const char *symtab_name[SYM_NUM] = {
>         "common prefixes",
> --
> 2.34.1
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH 9/9] selinux: drop unused macro

[Paul Moore]

On Tue, Jan 25, 2022 at 9:15 AM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> The macro _DEBUG_HASHES is nowhere used. The configuration DEBUG_HASHES
> enables debugging of the SELinux hash tables, but the with an underscore
> prefixed macro definition has no direct impact or any documentation.
>
> Reported by clang [-Wunused-macros]
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
>  security/selinux/ss/policydb.c | 2 --
>  1 file changed, 2 deletions(-)

Merged into selinux/next, thanks Christian.  This macro definition
predates the move to git so there is no quick answer to "why is this
here?", but my best guess is that it is an artifact of a developer
"disabling" the DEBUG_HASHES macro by adding an underscore to the
front.  Regardless of the reason behind it, I agree it should be
removed.

-- 
paul-moore.com

[PATCH 1/9] selinux: check return value of sel_make_avc_files

[Christian Göttsche]

sel_make_avc_files() might fail and return a negative errno value on
memory allocation failures. Re-add the check of the return value,
dropped in 66f8e2f03c02.

Reported by clang-analyzer:

    security/selinux/selinuxfs.c:2129:2: warning: Value stored to 'ret' is never read [deadcode.DeadStores]
            ret = sel_make_avc_files(dentry);
            ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~

Fixes: 66f8e2f03c02 ("selinux: sidtab reverse lookup hash table")
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 security/selinux/selinuxfs.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index e4cd7cb856f3..f2f6203e0fff 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -2127,6 +2127,8 @@ static int sel_fill_super(struct super_block *sb, struct fs_context *fc)
 	}
 
 	ret = sel_make_avc_files(dentry);
+	if (ret)
+		goto err;
 
 	dentry = sel_make_dir(sb->s_root, "ss", &fsi->last_ino);
 	if (IS_ERR(dentry)) {
-- 
2.34.1

Re: [PATCH 1/9] selinux: check return value of sel_make_avc_files

[Nick Desaulniers]

On Tue, Jan 25, 2022 at 6:15 AM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> sel_make_avc_files() might fail and return a negative errno value on
> memory allocation failures. Re-add the check of the return value,
> dropped in 66f8e2f03c02.
>
> Reported by clang-analyzer:
>
>     security/selinux/selinuxfs.c:2129:2: warning: Value stored to 'ret' is never read [deadcode.DeadStores]
>             ret = sel_make_avc_files(dentry);
>             ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Fixes: 66f8e2f03c02 ("selinux: sidtab reverse lookup hash table")
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>

Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

> ---
>  security/selinux/selinuxfs.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> index e4cd7cb856f3..f2f6203e0fff 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -2127,6 +2127,8 @@ static int sel_fill_super(struct super_block *sb, struct fs_context *fc)
>         }
>
>         ret = sel_make_avc_files(dentry);
> +       if (ret)
> +               goto err;
>
>         dentry = sel_make_dir(sb->s_root, "ss", &fsi->last_ino);
>         if (IS_ERR(dentry)) {
> --
> 2.34.1
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH 1/9] selinux: check return value of sel_make_avc_files

[Paul Moore]

On Tue, Jan 25, 2022 at 9:15 AM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> sel_make_avc_files() might fail and return a negative errno value on
> memory allocation failures. Re-add the check of the return value,
> dropped in 66f8e2f03c02.
>
> Reported by clang-analyzer:
>
>     security/selinux/selinuxfs.c:2129:2: warning: Value stored to 'ret' is never read [deadcode.DeadStores]
>             ret = sel_make_avc_files(dentry);
>             ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Fixes: 66f8e2f03c02 ("selinux: sidtab reverse lookup hash table")
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
>  security/selinux/selinuxfs.c | 2 ++
>  1 file changed, 2 insertions(+)

Merged into selinux/next, thanks Christian.

-- 
paul-moore.com

Re: [PATCH 2/9] selinux: declare path parameters of _genfs_sid const

[Paul Moore]

On Tue, Jan 25, 2022 at 9:14 AM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> The path parameter is only read from in security_genfs_sid(),
> selinux_policy_genfs_sid() and __security_genfs_sid(). Since a string
> literal is passed as argument, declare the parameter const.
> Also align the parameter names in the declaration and definition.
>
> Reported by clang [-Wwrite-strings]:
>
>     security/selinux/hooks.c:553:60: error: passing 'const char [2]' to parameter of type 'char *' discards qualifiers [-Wincompatible-pointer-types-discards-qualifiers]
>             rc = security_genfs_sid(&selinux_state, sb->s_type->name, /,
>                                                                       ^~~
>     ./security/selinux/include/security.h:389:36: note: passing argument to parameter 'name' here
>                            const char *fstype, char *name, u16 sclass,
>                                                      ^
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
>  security/selinux/include/security.h | 4 ++--
>  security/selinux/ss/services.c      | 6 +++---
>  2 files changed, 5 insertions(+), 5 deletions(-)

Merged, thanks!

-- 
paul-moore.com