[PATCH 2/5] selinux: use correct type for context length

[PATCH 2/5] selinux: use correct type for context length

[Christian Göttsche]

security_sid_to_context() expects a pointer to an u32 as the address
where to store the length of the computed context.

Reported by sparse:

    security/selinux/xfrm.c:359:39: warning: incorrect type in argument 4 (different signedness)
    security/selinux/xfrm.c:359:39:    expected unsigned int [usertype] *scontext_len
    security/selinux/xfrm.c:359:39:    got int *

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

diff --git a/security/selinux/xfrm.c b/security/selinux/xfrm.c
index 90697317895f..c576832febc6 100644
--- a/security/selinux/xfrm.c
+++ b/security/selinux/xfrm.c
@@ -347,7 +347,7 @@ int selinux_xfrm_state_alloc_acquire(struct xfrm_state *x,
 	int rc;
 	struct xfrm_sec_ctx *ctx;
 	char *ctx_str = NULL;
-	int str_len;
+	u32 str_len;
 
 	if (!polsec)
 		return 0;
-- 
2.35.1

[PATCH 3/5] selinux: use consistent pointer types for boolean arrays

[Christian Göttsche]

Use a consistent type of unsigned int* for boolean arrays, instead of
using implicit casts to and from int*.

Reported by sparse:

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

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>

---
A more invasive change would be to change all boolean arrays to bool*.
---
 security/selinux/include/conditional.h | 4 ++--
 security/selinux/selinuxfs.c           | 2 +-
 security/selinux/ss/services.c         | 9 +++++----
 3 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/security/selinux/include/conditional.h b/security/selinux/include/conditional.h
index b09343346e3f..9e65aa409318 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 int **values);
 
-int security_set_bools(struct selinux_state *state, u32 len, int *values);
+int security_set_bools(struct selinux_state *state, u32 len, unsigned int *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 f2f6203e0fff..5216a321bbb0 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -1428,7 +1428,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 int *values = NULL;
 	u32 sid;
 
 	ret = -ENOMEM;
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 6901dc07680d..7865926962ab 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -3023,7 +3023,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 int **values)
 {
 	struct policydb *policydb;
 	u32 i;
@@ -3045,7 +3045,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 int), GFP_ATOMIC);
 	if (!*values)
 		goto err;
 
@@ -3075,7 +3075,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, unsigned int *values)
 {
 	struct selinux_policy *newpolicy, *oldpolicy;
 	int rc;
@@ -3175,7 +3175,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 int *bvalues = NULL;
 	char **bnames = NULL;
 	struct cond_bool_datum *booldatum;
 	u32 i, nbools = 0;
-- 
2.35.1

Re: [PATCH 3/5] selinux: use consistent pointer types for boolean arrays

[Paul Moore]

On Thu, Feb 17, 2022 at 9:21 AM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> Use a consistent type of unsigned int* for boolean arrays, instead of
> using implicit casts to and from int*.
>
> Reported by sparse:
>
>     security/selinux/selinuxfs.c:1481:30: warning: incorrect type in assignment (different signedness)
>     security/selinux/selinuxfs.c:1481:30:    expected unsigned int *
>     security/selinux/selinuxfs.c:1481:30:    got int *[addressable] values
>     security/selinux/selinuxfs.c:1398:48: warning: incorrect type in argument 3 (different signedness)
>     security/selinux/selinuxfs.c:1398:48:    expected int *values
>     security/selinux/selinuxfs.c:1398:48:    got unsigned int *bool_pending_values
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
>
> ---
> A more invasive change would be to change all boolean arrays to bool*.

I think that might be a worthwhile change, although that can happen at
a later date.

A quick general comment: please try to stick to 80-char long lines.  I
realize Linus/checkpatch.pl has started to allow longer lines but I
would still like SELinux to try and keep to 80-chars or under.

> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index 6901dc07680d..7865926962ab 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -3175,7 +3175,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 int *bvalues = NULL;

Doesn't this cause a type mismatch (unsigned int vs int) when an entry
from bvalues[] is assigned to cond_bool_datum::state later in the
security_preserve_bools() function?

-- 
paul-moore.com

Re: [PATCH 3/5] selinux: use consistent pointer types for boolean arrays

[Christian Göttsche]

On Fri, 18 Feb 2022 at 17:01, Paul Moore <paul@paul-moore.com> wrote:
>
> On Thu, Feb 17, 2022 at 9:21 AM Christian Göttsche
> <cgzones@googlemail.com> wrote:
> >
> > Use a consistent type of unsigned int* for boolean arrays, instead of
> > using implicit casts to and from int*.
> >
> > Reported by sparse:
> >
> >     security/selinux/selinuxfs.c:1481:30: warning: incorrect type in assignment (different signedness)
> >     security/selinux/selinuxfs.c:1481:30:    expected unsigned int *
> >     security/selinux/selinuxfs.c:1481:30:    got int *[addressable] values
> >     security/selinux/selinuxfs.c:1398:48: warning: incorrect type in argument 3 (different signedness)
> >     security/selinux/selinuxfs.c:1398:48:    expected int *values
> >     security/selinux/selinuxfs.c:1398:48:    got unsigned int *bool_pending_values
> >
> > Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> >
> > ---
> > A more invasive change would be to change all boolean arrays to bool*.
>
> I think that might be a worthwhile change, although that can happen at
> a later date.
>
> A quick general comment: please try to stick to 80-char long lines.  I
> realize Linus/checkpatch.pl has started to allow longer lines but I
> would still like SELinux to try and keep to 80-chars or under.
>
> > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> > index 6901dc07680d..7865926962ab 100644
> > --- a/security/selinux/ss/services.c
> > +++ b/security/selinux/ss/services.c
> > @@ -3175,7 +3175,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 int *bvalues = NULL;
>
> Doesn't this cause a type mismatch (unsigned int vs int) when an entry
> from bvalues[] is assigned to cond_bool_datum::state later in the
> security_preserve_bools() function?

Yes, but those variables *should* only hold the values 0 or 1.
But probably it's better to re-spin for 5.19 with all arrays and
cond_bool_datum::state converted to literal bool type.

>
> --
> paul-moore.com

[PATCH 4/5] selinux: declare data arrays const

[Christian Göttsche]

The arrays for the policy capability names, the initial sid identifiers
and the class and permission names are not changed at runtime.  Declare
them const to avoid accidental modification.

The build time script genheaders needs to be exempted, since it converts
the entries to uppercase.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 scripts/selinux/genheaders/genheaders.c          | 2 ++
 scripts/selinux/mdp/mdp.c                        | 4 ++--
 security/selinux/avc.c                           | 2 +-
 security/selinux/include/avc_ss.h                | 2 +-
 security/selinux/include/classmap.h              | 8 +++++++-
 security/selinux/include/initial_sid_to_string.h | 9 ++++++++-
 security/selinux/include/policycap.h             | 2 +-
 security/selinux/include/policycap_names.h       | 2 +-
 security/selinux/ss/services.c                   | 4 ++--
 9 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/scripts/selinux/genheaders/genheaders.c b/scripts/selinux/genheaders/genheaders.c
index f355b3e0e968..5f7c0b7d9260 100644
--- a/scripts/selinux/genheaders/genheaders.c
+++ b/scripts/selinux/genheaders/genheaders.c
@@ -15,6 +15,8 @@ struct security_class_mapping {
 	const char *perms[sizeof(unsigned) * 8 + 1];
 };
 
+/* Allow to convert entries in mappings to uppercase */
+#define __SELINUX_GENHEADERS__
 #include "classmap.h"
 #include "initial_sid_to_string.h"
 
diff --git a/scripts/selinux/mdp/mdp.c b/scripts/selinux/mdp/mdp.c
index 105c1c31a316..1415604c3d24 100644
--- a/scripts/selinux/mdp/mdp.c
+++ b/scripts/selinux/mdp/mdp.c
@@ -82,7 +82,7 @@ int main(int argc, char *argv[])
 
 	/* print out the class permissions */
 	for (i = 0; secclass_map[i].name; i++) {
-		struct security_class_mapping *map = &secclass_map[i];
+		const struct security_class_mapping *map = &secclass_map[i];
 		fprintf(fout, "class %s\n", map->name);
 		fprintf(fout, "{\n");
 		for (j = 0; map->perms[j]; j++)
@@ -103,7 +103,7 @@ int main(int argc, char *argv[])
 #define SYSTEMLOW "s0"
 #define SYSTEMHIGH "s1:c0.c1"
 		for (i = 0; secclass_map[i].name; i++) {
-			struct security_class_mapping *map = &secclass_map[i];
+			const struct security_class_mapping *map = &secclass_map[i];
 
 			fprintf(fout, "mlsconstrain %s {\n", map->name);
 			for (j = 0; map->perms[j]; j++)
diff --git a/security/selinux/avc.c b/security/selinux/avc.c
index abcd9740d10f..020985a53d8f 100644
--- a/security/selinux/avc.c
+++ b/security/selinux/avc.c
@@ -668,7 +668,7 @@ static void avc_audit_pre_callback(struct audit_buffer *ab, void *a)
 	struct common_audit_data *ad = a;
 	struct selinux_audit_data *sad = ad->selinux_audit_data;
 	u32 av = sad->audited;
-	const char **perms;
+	const char *const *perms;
 	int i, perm;
 
 	audit_log_format(ab, "avc:  %s ", sad->denied ? "denied" : "granted");
diff --git a/security/selinux/include/avc_ss.h b/security/selinux/include/avc_ss.h
index 88c384c5c09e..b38974e22d81 100644
--- a/security/selinux/include/avc_ss.h
+++ b/security/selinux/include/avc_ss.h
@@ -18,7 +18,7 @@ struct security_class_mapping {
 	const char *perms[sizeof(u32) * 8 + 1];
 };
 
-extern struct security_class_mapping secclass_map[];
+extern const struct security_class_mapping secclass_map[];
 
 #endif /* _SELINUX_AVC_SS_H_ */
 
diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
index 35aac62a662e..07ade4af85ff 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -2,6 +2,12 @@
 #include <linux/capability.h>
 #include <linux/socket.h>
 
+#ifdef __SELINUX_GENHEADERS__
+# define const_qual
+#else
+# define const_qual const
+#endif
+
 #define COMMON_FILE_SOCK_PERMS "ioctl", "read", "write", "create", \
     "getattr", "setattr", "lock", "relabelfrom", "relabelto", "append", "map"
 
@@ -38,7 +44,7 @@
  * Note: The name for any socket class should be suffixed by "socket",
  *	 and doesn't contain more than one substr of "socket".
  */
-struct security_class_mapping secclass_map[] = {
+const_qual struct security_class_mapping secclass_map[] = {
 	{ "security",
 	  { "compute_av", "compute_create", "compute_member",
 	    "check_context", "load_policy", "compute_relabel",
diff --git a/security/selinux/include/initial_sid_to_string.h b/security/selinux/include/initial_sid_to_string.h
index 5d332aeb8b6c..915283cd89bd 100644
--- a/security/selinux/include/initial_sid_to_string.h
+++ b/security/selinux/include/initial_sid_to_string.h
@@ -1,5 +1,12 @@
 /* SPDX-License-Identifier: GPL-2.0 */
-static const char *initial_sid_to_string[] =
+
+#ifdef __SELINUX_GENHEADERS__
+# define const_qual
+#else
+# define const_qual const
+#endif
+
+static const char *const_qual initial_sid_to_string[] =
 {
 	NULL,
 	"kernel",
diff --git a/security/selinux/include/policycap.h b/security/selinux/include/policycap.h
index 2ec038efbb03..3207a4e8c899 100644
--- a/security/selinux/include/policycap.h
+++ b/security/selinux/include/policycap.h
@@ -15,6 +15,6 @@ enum {
 };
 #define POLICYDB_CAPABILITY_MAX (__POLICYDB_CAPABILITY_MAX - 1)
 
-extern const char *selinux_policycap_names[__POLICYDB_CAPABILITY_MAX];
+extern const char *const selinux_policycap_names[__POLICYDB_CAPABILITY_MAX];
 
 #endif /* _SELINUX_POLICYCAP_H_ */
diff --git a/security/selinux/include/policycap_names.h b/security/selinux/include/policycap_names.h
index b89289f092c9..51da36e37d21 100644
--- a/security/selinux/include/policycap_names.h
+++ b/security/selinux/include/policycap_names.h
@@ -5,7 +5,7 @@
 #include "policycap.h"
 
 /* Policy capability names */
-const char *selinux_policycap_names[__POLICYDB_CAPABILITY_MAX] = {
+const char *const selinux_policycap_names[__POLICYDB_CAPABILITY_MAX] = {
 	"network_peer_controls",
 	"open_perms",
 	"extended_socket_class",
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 7865926962ab..25c287324059 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -99,7 +99,7 @@ static void context_struct_compute_av(struct policydb *policydb,
 				      struct extended_perms *xperms);
 
 static int selinux_set_mapping(struct policydb *pol,
-			       struct security_class_mapping *map,
+			       const struct security_class_mapping *map,
 			       struct selinux_map *out_map)
 {
 	u16 i, j;
@@ -121,7 +121,7 @@ static int selinux_set_mapping(struct policydb *pol,
 	/* Store the raw class and permission values */
 	j = 0;
 	while (map[j].name) {
-		struct security_class_mapping *p_in = map + (j++);
+		const struct security_class_mapping *p_in = map + (j++);
 		struct selinux_mapping *p_out = out_map->mapping + j;
 
 		/* An empty class string skips ahead */
-- 
2.35.1

Re: [PATCH 4/5] selinux: declare data arrays const

[Paul Moore]

On Thu, Feb 17, 2022 at 9:21 AM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> The arrays for the policy capability names, the initial sid identifiers
> and the class and permission names are not changed at runtime.  Declare
> them const to avoid accidental modification.
>
> The build time script genheaders needs to be exempted, since it converts
> the entries to uppercase.
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
>  scripts/selinux/genheaders/genheaders.c          | 2 ++
>  scripts/selinux/mdp/mdp.c                        | 4 ++--
>  security/selinux/avc.c                           | 2 +-
>  security/selinux/include/avc_ss.h                | 2 +-
>  security/selinux/include/classmap.h              | 8 +++++++-
>  security/selinux/include/initial_sid_to_string.h | 9 ++++++++-
>  security/selinux/include/policycap.h             | 2 +-
>  security/selinux/include/policycap_names.h       | 2 +-
>  security/selinux/ss/services.c                   | 4 ++--
>  9 files changed, 25 insertions(+), 10 deletions(-)

...

> diff --git a/scripts/selinux/genheaders/genheaders.c b/scripts/selinux/genheaders/genheaders.c
> index f355b3e0e968..5f7c0b7d9260 100644
> --- a/scripts/selinux/genheaders/genheaders.c
> +++ b/scripts/selinux/genheaders/genheaders.c
> @@ -15,6 +15,8 @@ struct security_class_mapping {
>         const char *perms[sizeof(unsigned) * 8 + 1];
>  };
>
> +/* Allow to convert entries in mappings to uppercase */
> +#define __SELINUX_GENHEADERS__
>  #include "classmap.h"
>  #include "initial_sid_to_string.h"

...

> diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
> index 35aac62a662e..07ade4af85ff 100644
> --- a/security/selinux/include/classmap.h
> +++ b/security/selinux/include/classmap.h
> @@ -2,6 +2,12 @@
>  #include <linux/capability.h>
>  #include <linux/socket.h>
>
> +#ifdef __SELINUX_GENHEADERS__
> +# define const_qual
> +#else
> +# define const_qual const
> +#endif
> +
>  #define COMMON_FILE_SOCK_PERMS "ioctl", "read", "write", "create", \
>      "getattr", "setattr", "lock", "relabelfrom", "relabelto", "append", "map"
>
> @@ -38,7 +44,7 @@
>   * Note: The name for any socket class should be suffixed by "socket",
>   *      and doesn't contain more than one substr of "socket".
>   */
> -struct security_class_mapping secclass_map[] = {
> +const_qual struct security_class_mapping secclass_map[] = {
>         { "security",
>           { "compute_av", "compute_create", "compute_member",
>             "check_context", "load_policy", "compute_relabel",

...

> diff --git a/security/selinux/include/initial_sid_to_string.h b/security/selinux/include/initial_sid_to_string.h
> index 5d332aeb8b6c..915283cd89bd 100644
> --- a/security/selinux/include/initial_sid_to_string.h
> +++ b/security/selinux/include/initial_sid_to_string.h
> @@ -1,5 +1,12 @@
>  /* SPDX-License-Identifier: GPL-2.0 */
> -static const char *initial_sid_to_string[] =
> +
> +#ifdef __SELINUX_GENHEADERS__
> +# define const_qual
> +#else
> +# define const_qual const
> +#endif
> +
> +static const char *const_qual initial_sid_to_string[] =
>  {
>         NULL,
>         "kernel",

Thanks for this Christian.  I generally like when we can const'ify
things like this, but I'm not excited about the const_qual hack on
core SELinux kernel code to satisfy genheaders.c.  I understand why it
is needed, but I would rather clutter the genheaders.c code than the
core SELinux kernel code.  If we can't cast away the const'ification
in genheaders.c could we simply allocate duplicate arrays in
genheaders.c and store the transformed strings into the new arrays?

-- 
paul-moore.com

Re: [PATCH 4/5] selinux: declare data arrays const

[Nick Desaulniers]

On Fri, Feb 18, 2022 at 8:13 AM Paul Moore <paul@paul-moore.com> wrote:
>
> On Thu, Feb 17, 2022 at 9:21 AM Christian Göttsche
> <cgzones@googlemail.com> wrote:
> >
> > diff --git a/security/selinux/include/initial_sid_to_string.h b/security/selinux/include/initial_sid_to_string.h
> > index 5d332aeb8b6c..915283cd89bd 100644
> > --- a/security/selinux/include/initial_sid_to_string.h
> > +++ b/security/selinux/include/initial_sid_to_string.h
> > @@ -1,5 +1,12 @@
> >  /* SPDX-License-Identifier: GPL-2.0 */
> > -static const char *initial_sid_to_string[] =
> > +
> > +#ifdef __SELINUX_GENHEADERS__
> > +# define const_qual
> > +#else
> > +# define const_qual const
> > +#endif
> > +
> > +static const char *const_qual initial_sid_to_string[] =
> >  {
> >         NULL,
> >         "kernel",
>
> Thanks for this Christian.  I generally like when we can const'ify
> things like this, but I'm not excited about the const_qual hack on
> core SELinux kernel code to satisfy genheaders.c.  I understand why it
> is needed, but I would rather clutter the genheaders.c code than the
> core SELinux kernel code.  If we can't cast away the const'ification
> in genheaders.c could we simply allocate duplicate arrays in
> genheaders.c and store the transformed strings into the new arrays?

Note: casting off const is UB. I've had to fix multiple bugs where
clang will drop writes to variables declared const but had const'ness
casted away.
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH 4/5] selinux: declare data arrays const

[Paul Moore]

On Fri, Feb 18, 2022 at 12:24 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
> On Fri, Feb 18, 2022 at 8:13 AM Paul Moore <paul@paul-moore.com> wrote:
> > On Thu, Feb 17, 2022 at 9:21 AM Christian Göttsche
> > <cgzones@googlemail.com> wrote:
> > >
> > > diff --git a/security/selinux/include/initial_sid_to_string.h b/security/selinux/include/initial_sid_to_string.h
> > > index 5d332aeb8b6c..915283cd89bd 100644
> > > --- a/security/selinux/include/initial_sid_to_string.h
> > > +++ b/security/selinux/include/initial_sid_to_string.h
> > > @@ -1,5 +1,12 @@
> > >  /* SPDX-License-Identifier: GPL-2.0 */
> > > -static const char *initial_sid_to_string[] =
> > > +
> > > +#ifdef __SELINUX_GENHEADERS__
> > > +# define const_qual
> > > +#else
> > > +# define const_qual const
> > > +#endif
> > > +
> > > +static const char *const_qual initial_sid_to_string[] =
> > >  {
> > >         NULL,
> > >         "kernel",
> >
> > Thanks for this Christian.  I generally like when we can const'ify
> > things like this, but I'm not excited about the const_qual hack on
> > core SELinux kernel code to satisfy genheaders.c.  I understand why it
> > is needed, but I would rather clutter the genheaders.c code than the
> > core SELinux kernel code.  If we can't cast away the const'ification
> > in genheaders.c could we simply allocate duplicate arrays in
> > genheaders.c and store the transformed strings into the new arrays?
>
> Note: casting off const is UB. I've had to fix multiple bugs where
> clang will drop writes to variables declared const but had const'ness
> casted away.

Then let's just memcpy the array in genheaders.c.  I'm okay with
genheaders being a little ugly if it helps keep the core code cleaner.

-- 
paul-moore.com

[PATCH 5/5] selinux: drop unnecessary NULL check

[Christian Göttsche]

Commit e3489f8974e1 ("selinux: kill selinux_sb_get_mnt_opts()")
introduced a NULL check on the context after a successful call to
security_sid_to_context().  This is on the one hand redundant after
checking for success and on the other hand insufficient on an actual
NULL pointer, since the context is passed to seq_escape() leading to a
call of strlen() on it.

Reported by Clang analyzer:

    In file included from security/selinux/hooks.c:28:
    In file included from ./include/linux/tracehook.h:50:
    In file included from ./include/linux/memcontrol.h:13:
    In file included from ./include/linux/cgroup.h:18:
    ./include/linux/seq_file.h:136:25: warning: Null pointer passed as 1st argument to string length function [unix.cstring.NullArg]
            seq_escape_mem(m, src, strlen(src), flags, esc);
                                   ^~~~~~~~~~~

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

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 1e69f88eb326..ac802b99d36c 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1020,7 +1020,7 @@ static int show_sid(struct seq_file *m, u32 sid)
 	rc = security_sid_to_context(&selinux_state, sid,
 					     &context, &len);
 	if (!rc) {
-		bool has_comma = context && strchr(context, ',');
+		bool has_comma = strchr(context, ',');
 
 		seq_putc(m, '=');
 		if (has_comma)
-- 
2.35.1

Re: [PATCH 5/5] selinux: drop unnecessary NULL check

[Paul Moore]

On Thu, Feb 17, 2022 at 9:22 AM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> Commit e3489f8974e1 ("selinux: kill selinux_sb_get_mnt_opts()")
> introduced a NULL check on the context after a successful call to
> security_sid_to_context().  This is on the one hand redundant after
> checking for success and on the other hand insufficient on an actual
> NULL pointer, since the context is passed to seq_escape() leading to a
> call of strlen() on it.
>
> Reported by Clang analyzer:
>
>     In file included from security/selinux/hooks.c:28:
>     In file included from ./include/linux/tracehook.h:50:
>     In file included from ./include/linux/memcontrol.h:13:
>     In file included from ./include/linux/cgroup.h:18:
>     ./include/linux/seq_file.h:136:25: warning: Null pointer passed as 1st argument to string length function [unix.cstring.NullArg]
>             seq_escape_mem(m, src, strlen(src), flags, esc);
>                                    ^~~~~~~~~~~

Interesting.  If I'm understanding this correctly, Clang is reporting
on a potential NULL pointer simply because we are checking for a NULL
pointer a few lines earlier, even though @context should not be NULL
if (rc != 0)?

> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
>  security/selinux/hooks.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 1e69f88eb326..ac802b99d36c 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -1020,7 +1020,7 @@ static int show_sid(struct seq_file *m, u32 sid)
>         rc = security_sid_to_context(&selinux_state, sid,
>                                              &context, &len);
>         if (!rc) {
> -               bool has_comma = context && strchr(context, ',');
> +               bool has_comma = strchr(context, ',');
>
>                 seq_putc(m, '=');
>                 if (has_comma)
> --
> 2.35.1

-- 
paul-moore.com

Re: [PATCH 5/5] selinux: drop unnecessary NULL check

[Nick Desaulniers]

On Thu, Feb 17, 2022 at 6:22 AM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> Commit e3489f8974e1 ("selinux: kill selinux_sb_get_mnt_opts()")
> introduced a NULL check on the context after a successful call to
> security_sid_to_context().  This is on the one hand redundant after
> checking for success and on the other hand insufficient on an actual
> NULL pointer, since the context is passed to seq_escape() leading to a
> call of strlen() on it.
>
> Reported by Clang analyzer:
>
>     In file included from security/selinux/hooks.c:28:
>     In file included from ./include/linux/tracehook.h:50:
>     In file included from ./include/linux/memcontrol.h:13:
>     In file included from ./include/linux/cgroup.h:18:
>     ./include/linux/seq_file.h:136:25: warning: Null pointer passed as 1st argument to string length function [unix.cstring.NullArg]
>             seq_escape_mem(m, src, strlen(src), flags, esc);
>                                    ^~~~~~~~~~~

I'm guessing there was more to this trace for this instance of this warning?

>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
>  security/selinux/hooks.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 1e69f88eb326..ac802b99d36c 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -1020,7 +1020,7 @@ static int show_sid(struct seq_file *m, u32 sid)
>         rc = security_sid_to_context(&selinux_state, sid,
>                                              &context, &len);
>         if (!rc) {

^ perhaps changing this condition to:

if (!rc && context) {

It might be nice to retain the null ptr check should the semantics of
security_sid_to_context ever change.

> -               bool has_comma = context && strchr(context, ',');
> +               bool has_comma = strchr(context, ',');
>
>                 seq_putc(m, '=');
>                 if (has_comma)
> --
> 2.35.1
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH 5/5] selinux: drop unnecessary NULL check

[Christian Göttsche]

On Fri, 18 Feb 2022 at 18:31, Nick Desaulniers <ndesaulniers@google.com> wrote:
>
> On Thu, Feb 17, 2022 at 6:22 AM Christian Göttsche
> <cgzones@googlemail.com> wrote:
> >
> > Commit e3489f8974e1 ("selinux: kill selinux_sb_get_mnt_opts()")
> > introduced a NULL check on the context after a successful call to
> > security_sid_to_context().  This is on the one hand redundant after
> > checking for success and on the other hand insufficient on an actual
> > NULL pointer, since the context is passed to seq_escape() leading to a
> > call of strlen() on it.
> >
> > Reported by Clang analyzer:
> >
> >     In file included from security/selinux/hooks.c:28:
> >     In file included from ./include/linux/tracehook.h:50:
> >     In file included from ./include/linux/memcontrol.h:13:
> >     In file included from ./include/linux/cgroup.h:18:
> >     ./include/linux/seq_file.h:136:25: warning: Null pointer passed as 1st argument to string length function [unix.cstring.NullArg]
> >             seq_escape_mem(m, src, strlen(src), flags, esc);
> >                                    ^~~~~~~~~~~
>
> I'm guessing there was more to this trace for this instance of this warning?

Yes, complete output appended at the end.

>
> >
> > Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> > ---
> >  security/selinux/hooks.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 1e69f88eb326..ac802b99d36c 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -1020,7 +1020,7 @@ static int show_sid(struct seq_file *m, u32 sid)
> >         rc = security_sid_to_context(&selinux_state, sid,
> >                                              &context, &len);
> >         if (!rc) {
>
> ^ perhaps changing this condition to:
>
> if (!rc && context) {
>
> It might be nice to retain the null ptr check should the semantics of
> security_sid_to_context ever change.

If I read the implementation of security_sid_to_context() and its callees
correctly it should never return 0 (success) and not have populated its 3
argument, unless the passed pointer was zero, which by passing the address
of a stack variable - &context - is not the case).

>
> > -               bool has_comma = context && strchr(context, ',');
> > +               bool has_comma = strchr(context, ',');
> >
> >                 seq_putc(m, '=');
> >                 if (has_comma)
> > --
> > 2.35.1
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers


clang-tidy report:

./include/linux/seq_file.h:136:25: warning: Null pointer passed as 1st
argument to string length function
[clang-analyzer-unix.cstring.NullArg]
        seq_escape_mem(m, src, strlen(src), flags, esc);
                               ^
./security/selinux/hooks.c:1041:6: note: Assuming the condition is false
        if (!(sbsec->flags & SE_SBINITIALIZED))
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./security/selinux/hooks.c:1041:2: note: Taking false branch
        if (!(sbsec->flags & SE_SBINITIALIZED))
        ^
./security/selinux/hooks.c:1044:6: note: Assuming the condition is false
        if (!selinux_initialized(&selinux_state))
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./security/selinux/hooks.c:1044:2: note: Taking false branch
        if (!selinux_initialized(&selinux_state))
        ^
./security/selinux/hooks.c:1047:6: note: Assuming the condition is true
        if (sbsec->flags & FSCONTEXT_MNT) {
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
./security/selinux/hooks.c:1047:2: note: Taking true branch
        if (sbsec->flags & FSCONTEXT_MNT) {
        ^
./security/selinux/hooks.c:1050:8: note: Calling 'show_sid'
                rc = show_sid(m, sbsec->sid);
                     ^~~~~~~~~~~~~~~~~~~~~~~
./security/selinux/hooks.c:1020:7: note: Value assigned to 'context'
        rc = security_sid_to_context(&selinux_state, sid,
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./security/selinux/hooks.c:1022:6: note: Assuming 'rc' is 0
        if (!rc) {
            ^~~
./security/selinux/hooks.c:1022:2: note: Taking true branch
        if (!rc) {
        ^
./security/selinux/hooks.c:1023:20: note: Assuming 'context' is null
                bool has_comma = context && strchr(context, ',');
                                 ^~~~~~~
./security/selinux/hooks.c:1023:28: note: Left side of '&&' is false
                bool has_comma = context && strchr(context, ',');
                                         ^
./security/selinux/hooks.c:1026:7: note: 'has_comma' is false
                if (has_comma)
                    ^~~~~~~~~
./security/selinux/hooks.c:1026:3: note: Taking false branch
                if (has_comma)
                ^
./security/selinux/hooks.c:1028:17: note: Passing null pointer value
via 2nd parameter 's'
                seq_escape(m, context, "\"\n\\");
                              ^~~~~~~
./security/selinux/hooks.c:1028:3: note: Calling 'seq_escape'
                seq_escape(m, context, "\"\n\\");
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
././include/linux/seq_file.h:152:20: note: Passing null pointer value
via 2nd parameter 'src'
        seq_escape_str(m, s, ESCAPE_OCTAL, esc);
                          ^
././include/linux/seq_file.h:152:2: note: Calling 'seq_escape_str'
        seq_escape_str(m, s, ESCAPE_OCTAL, esc);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
././include/linux/seq_file.h:136:25: note: Null pointer passed as 1st
argument to string length function
        seq_escape_mem(m, src, strlen(src), flags, esc);
                               ^      ~~~

Re: [PATCH 5/5] selinux: drop unnecessary NULL check

[Christian Göttsche]

On Tue, 8 Mar 2022 at 17:09, Christian Göttsche <cgzones@googlemail.com> wrote:
>
> On Fri, 18 Feb 2022 at 18:31, Nick Desaulniers <ndesaulniers@google.com> wrote:
> >
> > On Thu, Feb 17, 2022 at 6:22 AM Christian Göttsche
> > <cgzones@googlemail.com> wrote:
> > >
> > > Commit e3489f8974e1 ("selinux: kill selinux_sb_get_mnt_opts()")
> > > introduced a NULL check on the context after a successful call to
> > > security_sid_to_context().  This is on the one hand redundant after
> > > checking for success and on the other hand insufficient on an actual
> > > NULL pointer, since the context is passed to seq_escape() leading to a
> > > call of strlen() on it.
> > >
> > > Reported by Clang analyzer:
> > >
> > >     In file included from security/selinux/hooks.c:28:
> > >     In file included from ./include/linux/tracehook.h:50:
> > >     In file included from ./include/linux/memcontrol.h:13:
> > >     In file included from ./include/linux/cgroup.h:18:
> > >     ./include/linux/seq_file.h:136:25: warning: Null pointer passed as 1st argument to string length function [unix.cstring.NullArg]
> > >             seq_escape_mem(m, src, strlen(src), flags, esc);
> > >                                    ^~~~~~~~~~~
> >
> > I'm guessing there was more to this trace for this instance of this warning?
>
> Yes, complete output appended at the end.
>
> >
> > >
> > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> > > ---
> > >  security/selinux/hooks.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > > index 1e69f88eb326..ac802b99d36c 100644
> > > --- a/security/selinux/hooks.c
> > > +++ b/security/selinux/hooks.c
> > > @@ -1020,7 +1020,7 @@ static int show_sid(struct seq_file *m, u32 sid)
> > >         rc = security_sid_to_context(&selinux_state, sid,
> > >                                              &context, &len);
> > >         if (!rc) {
> >
> > ^ perhaps changing this condition to:
> >
> > if (!rc && context) {
> >
> > It might be nice to retain the null ptr check should the semantics of
> > security_sid_to_context ever change.
>
> If I read the implementation of security_sid_to_context() and its callees
> correctly it should never return 0 (success) and not have populated its 3
> argument, unless the passed pointer was zero, which by passing the address
> of a stack variable - &context - is not the case).
>

Kindly ping;
is my analysis correct that after

    rc = security_sid_to_context(&selinux_state, sid,
                                                  &context, &len);

there is no possibility that rc is 0 AND context is NULL?

> >
> > > -               bool has_comma = context && strchr(context, ',');
> > > +               bool has_comma = strchr(context, ',');
> > >
> > >                 seq_putc(m, '=');
> > >                 if (has_comma)
> > > --
> > > 2.35.1
> > >
> >
> >
> > --
> > Thanks,
> > ~Nick Desaulniers
>
>
> clang-tidy report:
>
> ./include/linux/seq_file.h:136:25: warning: Null pointer passed as 1st
> argument to string length function
> [clang-analyzer-unix.cstring.NullArg]
>         seq_escape_mem(m, src, strlen(src), flags, esc);
>                                ^
> ./security/selinux/hooks.c:1041:6: note: Assuming the condition is false
>         if (!(sbsec->flags & SE_SBINITIALIZED))
>             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ./security/selinux/hooks.c:1041:2: note: Taking false branch
>         if (!(sbsec->flags & SE_SBINITIALIZED))
>         ^
> ./security/selinux/hooks.c:1044:6: note: Assuming the condition is false
>         if (!selinux_initialized(&selinux_state))
>             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ./security/selinux/hooks.c:1044:2: note: Taking false branch
>         if (!selinux_initialized(&selinux_state))
>         ^
> ./security/selinux/hooks.c:1047:6: note: Assuming the condition is true
>         if (sbsec->flags & FSCONTEXT_MNT) {
>             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ./security/selinux/hooks.c:1047:2: note: Taking true branch
>         if (sbsec->flags & FSCONTEXT_MNT) {
>         ^
> ./security/selinux/hooks.c:1050:8: note: Calling 'show_sid'
>                 rc = show_sid(m, sbsec->sid);
>                      ^~~~~~~~~~~~~~~~~~~~~~~
> ./security/selinux/hooks.c:1020:7: note: Value assigned to 'context'
>         rc = security_sid_to_context(&selinux_state, sid,
>              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ./security/selinux/hooks.c:1022:6: note: Assuming 'rc' is 0
>         if (!rc) {
>             ^~~
> ./security/selinux/hooks.c:1022:2: note: Taking true branch
>         if (!rc) {
>         ^
> ./security/selinux/hooks.c:1023:20: note: Assuming 'context' is null
>                 bool has_comma = context && strchr(context, ',');
>                                  ^~~~~~~
> ./security/selinux/hooks.c:1023:28: note: Left side of '&&' is false
>                 bool has_comma = context && strchr(context, ',');
>                                          ^
> ./security/selinux/hooks.c:1026:7: note: 'has_comma' is false
>                 if (has_comma)
>                     ^~~~~~~~~
> ./security/selinux/hooks.c:1026:3: note: Taking false branch
>                 if (has_comma)
>                 ^
> ./security/selinux/hooks.c:1028:17: note: Passing null pointer value
> via 2nd parameter 's'
>                 seq_escape(m, context, "\"\n\\");
>                               ^~~~~~~
> ./security/selinux/hooks.c:1028:3: note: Calling 'seq_escape'
>                 seq_escape(m, context, "\"\n\\");
>                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ././include/linux/seq_file.h:152:20: note: Passing null pointer value
> via 2nd parameter 'src'
>         seq_escape_str(m, s, ESCAPE_OCTAL, esc);
>                           ^
> ././include/linux/seq_file.h:152:2: note: Calling 'seq_escape_str'
>         seq_escape_str(m, s, ESCAPE_OCTAL, esc);
>         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ././include/linux/seq_file.h:136:25: note: Null pointer passed as 1st
> argument to string length function
>         seq_escape_mem(m, src, strlen(src), flags, esc);
>                                ^      ~~~

Re: [PATCH 5/5] selinux: drop unnecessary NULL check

[Ondrej Mosnacek]

On Mon, May 2, 2022 at 3:43 PM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> On Tue, 8 Mar 2022 at 17:09, Christian Göttsche <cgzones@googlemail.com> wrote:
> >
> > On Fri, 18 Feb 2022 at 18:31, Nick Desaulniers <ndesaulniers@google.com> wrote:
> > >
> > > On Thu, Feb 17, 2022 at 6:22 AM Christian Göttsche
> > > <cgzones@googlemail.com> wrote:
> > > >
> > > > Commit e3489f8974e1 ("selinux: kill selinux_sb_get_mnt_opts()")
> > > > introduced a NULL check on the context after a successful call to
> > > > security_sid_to_context().  This is on the one hand redundant after
> > > > checking for success and on the other hand insufficient on an actual
> > > > NULL pointer, since the context is passed to seq_escape() leading to a
> > > > call of strlen() on it.
> > > >
> > > > Reported by Clang analyzer:
> > > >
> > > >     In file included from security/selinux/hooks.c:28:
> > > >     In file included from ./include/linux/tracehook.h:50:
> > > >     In file included from ./include/linux/memcontrol.h:13:
> > > >     In file included from ./include/linux/cgroup.h:18:
> > > >     ./include/linux/seq_file.h:136:25: warning: Null pointer passed as 1st argument to string length function [unix.cstring.NullArg]
> > > >             seq_escape_mem(m, src, strlen(src), flags, esc);
> > > >                                    ^~~~~~~~~~~
> > >
> > > I'm guessing there was more to this trace for this instance of this warning?
> >
> > Yes, complete output appended at the end.
> >
> > >
> > > >
> > > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> > > > ---
> > > >  security/selinux/hooks.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > > > index 1e69f88eb326..ac802b99d36c 100644
> > > > --- a/security/selinux/hooks.c
> > > > +++ b/security/selinux/hooks.c
> > > > @@ -1020,7 +1020,7 @@ static int show_sid(struct seq_file *m, u32 sid)
> > > >         rc = security_sid_to_context(&selinux_state, sid,
> > > >                                              &context, &len);
> > > >         if (!rc) {
> > >
> > > ^ perhaps changing this condition to:
> > >
> > > if (!rc && context) {
> > >
> > > It might be nice to retain the null ptr check should the semantics of
> > > security_sid_to_context ever change.
> >
> > If I read the implementation of security_sid_to_context() and its callees
> > correctly it should never return 0 (success) and not have populated its 3
> > argument, unless the passed pointer was zero, which by passing the address
> > of a stack variable - &context - is not the case).
> >
>
> Kindly ping;
> is my analysis correct that after
>
>     rc = security_sid_to_context(&selinux_state, sid,
>                                                   &context, &len);
>
> there is no possibility that rc is 0 AND context is NULL?

Yes, I think this patch is good. rc == 0 means success, which in this
case means that a valid context string has been returned. Thus, there
is no point in checking for NULL, other than being super-defensive
about future changes to security_sid_to_context() messing something up
(if we did this everywhere, then there would be NULL checks all over
the place...).

>
> > >
> > > > -               bool has_comma = context && strchr(context, ',');
> > > > +               bool has_comma = strchr(context, ',');
> > > >
> > > >                 seq_putc(m, '=');
> > > >                 if (has_comma)
> > > > --
> > > > 2.35.1
> > > >
> > >
> > >
> > > --
> > > Thanks,
> > > ~Nick Desaulniers
> >
> >
> > clang-tidy report:
> >
> > ./include/linux/seq_file.h:136:25: warning: Null pointer passed as 1st
> > argument to string length function
> > [clang-analyzer-unix.cstring.NullArg]
> >         seq_escape_mem(m, src, strlen(src), flags, esc);
> >                                ^
> > ./security/selinux/hooks.c:1041:6: note: Assuming the condition is false
> >         if (!(sbsec->flags & SE_SBINITIALIZED))
> >             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > ./security/selinux/hooks.c:1041:2: note: Taking false branch
> >         if (!(sbsec->flags & SE_SBINITIALIZED))
> >         ^
> > ./security/selinux/hooks.c:1044:6: note: Assuming the condition is false
> >         if (!selinux_initialized(&selinux_state))
> >             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > ./security/selinux/hooks.c:1044:2: note: Taking false branch
> >         if (!selinux_initialized(&selinux_state))
> >         ^
> > ./security/selinux/hooks.c:1047:6: note: Assuming the condition is true
> >         if (sbsec->flags & FSCONTEXT_MNT) {
> >             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > ./security/selinux/hooks.c:1047:2: note: Taking true branch
> >         if (sbsec->flags & FSCONTEXT_MNT) {
> >         ^
> > ./security/selinux/hooks.c:1050:8: note: Calling 'show_sid'
> >                 rc = show_sid(m, sbsec->sid);
> >                      ^~~~~~~~~~~~~~~~~~~~~~~
> > ./security/selinux/hooks.c:1020:7: note: Value assigned to 'context'
> >         rc = security_sid_to_context(&selinux_state, sid,
> >              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > ./security/selinux/hooks.c:1022:6: note: Assuming 'rc' is 0
> >         if (!rc) {
> >             ^~~
> > ./security/selinux/hooks.c:1022:2: note: Taking true branch
> >         if (!rc) {
> >         ^
> > ./security/selinux/hooks.c:1023:20: note: Assuming 'context' is null
> >                 bool has_comma = context && strchr(context, ',');
> >                                  ^~~~~~~
> > ./security/selinux/hooks.c:1023:28: note: Left side of '&&' is false
> >                 bool has_comma = context && strchr(context, ',');
> >                                          ^
> > ./security/selinux/hooks.c:1026:7: note: 'has_comma' is false
> >                 if (has_comma)
> >                     ^~~~~~~~~
> > ./security/selinux/hooks.c:1026:3: note: Taking false branch
> >                 if (has_comma)
> >                 ^
> > ./security/selinux/hooks.c:1028:17: note: Passing null pointer value
> > via 2nd parameter 's'
> >                 seq_escape(m, context, "\"\n\\");
> >                               ^~~~~~~
> > ./security/selinux/hooks.c:1028:3: note: Calling 'seq_escape'
> >                 seq_escape(m, context, "\"\n\\");
> >                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > ././include/linux/seq_file.h:152:20: note: Passing null pointer value
> > via 2nd parameter 'src'
> >         seq_escape_str(m, s, ESCAPE_OCTAL, esc);
> >                           ^
> > ././include/linux/seq_file.h:152:2: note: Calling 'seq_escape_str'
> >         seq_escape_str(m, s, ESCAPE_OCTAL, esc);
> >         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > ././include/linux/seq_file.h:136:25: note: Null pointer passed as 1st
> > argument to string length function
> >         seq_escape_mem(m, src, strlen(src), flags, esc);
> >                                ^      ~~~
>

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

Re: [PATCH 5/5] selinux: drop unnecessary NULL check

[Paul Moore]

On Thu, Feb 17, 2022 at 9:22 AM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> Commit e3489f8974e1 ("selinux: kill selinux_sb_get_mnt_opts()")
> introduced a NULL check on the context after a successful call to
> security_sid_to_context().  This is on the one hand redundant after
> checking for success and on the other hand insufficient on an actual
> NULL pointer, since the context is passed to seq_escape() leading to a
> call of strlen() on it.
>
> Reported by Clang analyzer:
>
>     In file included from security/selinux/hooks.c:28:
>     In file included from ./include/linux/tracehook.h:50:
>     In file included from ./include/linux/memcontrol.h:13:
>     In file included from ./include/linux/cgroup.h:18:
>     ./include/linux/seq_file.h:136:25: warning: Null pointer passed as 1st argument to string length function [unix.cstring.NullArg]
>             seq_escape_mem(m, src, strlen(src), flags, esc);
>                                    ^~~~~~~~~~~
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
>  security/selinux/hooks.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

I was waiting for Nick to reply, but he never did, and this looks good
to me so I just merged it into selinux/next.  Thanks for your patience
Christian.

-- 
paul-moore.com

Re: [PATCH 5/5] selinux: drop unnecessary NULL check

[Nick Desaulniers]

On Tue, Jun 7, 2022 at 2:22 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Thu, Feb 17, 2022 at 9:22 AM Christian Göttsche
> <cgzones@googlemail.com> wrote:
> >
> > Commit e3489f8974e1 ("selinux: kill selinux_sb_get_mnt_opts()")
> > introduced a NULL check on the context after a successful call to
> > security_sid_to_context().  This is on the one hand redundant after
> > checking for success and on the other hand insufficient on an actual
> > NULL pointer, since the context is passed to seq_escape() leading to a
> > call of strlen() on it.
> >
> > Reported by Clang analyzer:
> >
> >     In file included from security/selinux/hooks.c:28:
> >     In file included from ./include/linux/tracehook.h:50:
> >     In file included from ./include/linux/memcontrol.h:13:
> >     In file included from ./include/linux/cgroup.h:18:
> >     ./include/linux/seq_file.h:136:25: warning: Null pointer passed as 1st argument to string length function [unix.cstring.NullArg]
> >             seq_escape_mem(m, src, strlen(src), flags, esc);
> >                                    ^~~~~~~~~~~
> >
> > Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> > ---
> >  security/selinux/hooks.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
>
> I was waiting for Nick to reply, but he never did, and this looks good
> to me so I just merged it into selinux/next.  Thanks for your patience
> Christian.

LGTM; you can ping me on irc #ndesaulniers on most kernel channels if
you're waiting on me. ;)

>
> --
> paul-moore.com



-- 
Thanks,
~Nick Desaulniers

Re: [PATCH 5/5] selinux: drop unnecessary NULL check

[Paul Moore]

On Tue, Jun 7, 2022 at 5:26 PM Nick Desaulniers <ndesaulniers@google.com> wrote:
>
> On Tue, Jun 7, 2022 at 2:22 PM Paul Moore <paul@paul-moore.com> wrote:
> >
> > On Thu, Feb 17, 2022 at 9:22 AM Christian Göttsche
> > <cgzones@googlemail.com> wrote:
> > >
> > > Commit e3489f8974e1 ("selinux: kill selinux_sb_get_mnt_opts()")
> > > introduced a NULL check on the context after a successful call to
> > > security_sid_to_context().  This is on the one hand redundant after
> > > checking for success and on the other hand insufficient on an actual
> > > NULL pointer, since the context is passed to seq_escape() leading to a
> > > call of strlen() on it.
> > >
> > > Reported by Clang analyzer:
> > >
> > >     In file included from security/selinux/hooks.c:28:
> > >     In file included from ./include/linux/tracehook.h:50:
> > >     In file included from ./include/linux/memcontrol.h:13:
> > >     In file included from ./include/linux/cgroup.h:18:
> > >     ./include/linux/seq_file.h:136:25: warning: Null pointer passed as 1st argument to string length function [unix.cstring.NullArg]
> > >             seq_escape_mem(m, src, strlen(src), flags, esc);
> > >                                    ^~~~~~~~~~~
> > >
> > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> > > ---
> > >  security/selinux/hooks.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > I was waiting for Nick to reply, but he never did, and this looks good
> > to me so I just merged it into selinux/next.  Thanks for your patience
> > Christian.
>
> LGTM; you can ping me on irc #ndesaulniers on most kernel channels if
> you're waiting on me. ;)

Thanks, but I generally don't have the spare cycles to keep track of
everyone's prefered method of interaction, that's why we've got the
mailing list (warts and all) :)

For what it's worth, I was waiting on you because you asked about the
additional trace info and without any context I thought you might be
looking for something else (?).  In the end, I think everyone agreed
that the patch was good so I merged it.  I think as a general rule
it's a good practice to follow-up with a reply when people provide
additional information that you've requested.  Not only is it the
polite thing to do, it helps clarify things with everyone else that
there is no hidden "gotcha!" in the patch.

Regardless, thanks for checking back on this :)

-- 
paul-moore.com

[PATCH 1/5] selinux: drop return statement at end of void functions

[Christian Göttsche]

Those return statements at the end of a void function are redundant.

Reported by clang-tidy [readability-redundant-control-flow]

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 security/selinux/hooks.c          | 2 --
 security/selinux/ss/conditional.c | 2 --
 security/selinux/ss/ebitmap.c     | 1 -
 security/selinux/ss/mls.c         | 1 -
 security/selinux/ss/services.c    | 2 --
 5 files changed, 8 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index dafabb4dcc64..1e69f88eb326 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3284,8 +3284,6 @@ static void selinux_inode_post_setxattr(struct dentry *dentry, const char *name,
 	isec->sid = newsid;
 	isec->initialized = LABEL_INITIALIZED;
 	spin_unlock(&isec->lock);
-
-	return;
 }
 
 static int selinux_inode_getxattr(struct dentry *dentry, const char *name)
diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
index 2ec6e5cd25d9..c46c419af512 100644
--- a/security/selinux/ss/conditional.c
+++ b/security/selinux/ss/conditional.c
@@ -566,8 +566,6 @@ void cond_compute_xperms(struct avtab *ctab, struct avtab_key *key,
 		if (node->key.specified & AVTAB_ENABLED)
 			services_compute_xperms_decision(xpermd, node);
 	}
-	return;
-
 }
 /* Determine whether additional permissions are granted by the conditional
  * av table, and if so, add them to the result
diff --git a/security/selinux/ss/ebitmap.c b/security/selinux/ss/ebitmap.c
index 61fcbb8d0f88..abde349c8321 100644
--- a/security/selinux/ss/ebitmap.c
+++ b/security/selinux/ss/ebitmap.c
@@ -359,7 +359,6 @@ void ebitmap_destroy(struct ebitmap *e)
 
 	e->highbit = 0;
 	e->node = NULL;
-	return;
 }
 
 int ebitmap_read(struct ebitmap *e, void *fp)
diff --git a/security/selinux/ss/mls.c b/security/selinux/ss/mls.c
index 3f5fd124342c..99571b19d4a9 100644
--- a/security/selinux/ss/mls.c
+++ b/security/selinux/ss/mls.c
@@ -156,7 +156,6 @@ void mls_sid_to_context(struct policydb *p,
 	}
 
 	*scontext = scontextp;
-	return;
 }
 
 int mls_level_isvalid(struct policydb *p, struct mls_level *l)
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 2f8db93e53b2..6901dc07680d 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -529,8 +529,6 @@ static void security_dump_masked_av(struct policydb *policydb,
 	/* release scontext/tcontext */
 	kfree(tcontext_name);
 	kfree(scontext_name);
-
-	return;
 }
 
 /*
-- 
2.35.1

Re: [PATCH 1/5] selinux: drop return statement at end of void functions

[Paul Moore]

On Thu, Feb 17, 2022 at 9:22 AM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> Those return statements at the end of a void function are redundant.
>
> Reported by clang-tidy [readability-redundant-control-flow]
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
>  security/selinux/hooks.c          | 2 --
>  security/selinux/ss/conditional.c | 2 --
>  security/selinux/ss/ebitmap.c     | 1 -
>  security/selinux/ss/mls.c         | 1 -
>  security/selinux/ss/services.c    | 2 --
>  5 files changed, 8 deletions(-)

Merged into selinux/next, thanks.

-- 
paul-moore.com

Re: [PATCH 2/5] selinux: use correct type for context length

[Paul Moore]

On Thu, Feb 17, 2022 at 9:21 AM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> security_sid_to_context() expects a pointer to an u32 as the address
> where to store the length of the computed context.
>
> Reported by sparse:
>
>     security/selinux/xfrm.c:359:39: warning: incorrect type in argument 4 (different signedness)
>     security/selinux/xfrm.c:359:39:    expected unsigned int [usertype] *scontext_len
>     security/selinux/xfrm.c:359:39:    got int *
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
>  security/selinux/xfrm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Merged into selinux/next, thanks.

-- 
paul-moore.com