selinux: use correct type for context length

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 <[email protected]>

---
 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(-)

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 <[email protected]>

---
A more invasive change would be to change all boolean arrays to bool*.

Quoted 13 lines · 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

Quoted 4 lines

Signed-off-by Christian Göttsche <[email protected]>

---
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.

security/selinux/ss/services.c int security_get_bool_value(struct selinux_state *state, · line 3179

Quoted 11 lines

security/selinux/ss/services.c +2 -1

index 6901dc07680d..7865926962ab 100644
@@ -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

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 <[email protected]>

---
 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(-)

scripts/selinux/genheaders/genheaders.c +2 -0

index f355b3e0e968..5f7c0b7d9260 100644
@@ -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"

security/selinux/include/classmap.h +7 -1

index 35aac62a662e..07ade4af85ff 100644
@@ -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",

security/selinux/include/initial_sid_to_string.h +1 -1

index 5d332aeb8b6c..915283cd89bd 100644
@@ -1,5 +1,12 @@
 /* SPDX-License-Identifier: GPL-2.0 */
-static const char *initial_sid_to_string[] =
+

Quoted 9 lines · Christian Göttsche

security/selinux/include/initial_sid_to_string.h +1 -1

index 5d332aeb8b6c..915283cd89bd 100644
@@ -1,5 +1,12 @@
 /* SPDX-License-Identifier: GPL-2.0 */
-static const char *initial_sid_to_string[] =
+

security/selinux/include/initial_sid_to_string.h line 12

Quoted 10 lines

security/selinux/include/initial_sid_to_string.h +7 -0

+#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?

Quoted 22 lines · Paul Moore

Quoted 9 lines · Christian Göttsche

security/selinux/include/initial_sid_to_string.h +1 -1

index 5d332aeb8b6c..915283cd89bd 100644
@@ -1,5 +1,12 @@
 /* SPDX-License-Identifier: GPL-2.0 */
-static const char *initial_sid_to_string[] =
+

security/selinux/include/initial_sid_to_string.h line 12

Quoted 10 lines

security/selinux/include/initial_sid_to_string.h +7 -0

+#ifdef __SELINUX_GENHEADERS__
+# define const_qual
+#else
+# define const_qual const
+#endif
+
+static const char *const_qual initial_sid_to_string[] =
 {
        NULL,
        "kernel",

Quoted 7 lines

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.

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.

Do not override the classmap and the initial sid list in the build time script genheaders, by using a static buffer in the conversion function stoupperx(). In cases we need to compare or print more than one identifier allocate a temporary copy.

Signed-off-by Christian Göttsche <[email protected]>

---
v2:
Drop const exemption for genheaders script by rewriting stoupperx().

---
 scripts/selinux/genheaders/genheaders.c       | 76 ++++++++++---------
 scripts/selinux/mdp/mdp.c                     |  4 +-
 security/selinux/avc.c                        |  2 +-
 security/selinux/include/avc_ss.h             |  2 +-
 security/selinux/include/classmap.h           |  2 +-
 .../selinux/include/initial_sid_to_string.h   |  3 +-
 security/selinux/include/policycap.h          |  2 +-
 security/selinux/include/policycap_names.h    |  2 +-
 security/selinux/ss/services.c                |  4 +-
 9 files changed, 51 insertions(+), 46 deletions(-)

scripts/selinux/genheaders/genheaders.c +9 -5

index f355b3e0e968..a2caff3c997f 100644
@@ -26,19 +26,23 @@ static void usage(void)
        exit(1);
 }
 
-static char *stoupperx(const char *s)
+static const char *stoupperx(const char *s)
 {
-       char *s2 = strdup(s);
-       char *p;
+       static char buffer[256];
+       unsigned int i;
+       char *p = buffer;
 
-       if (!s2) {
-               fprintf(stderr, "%s:  out of memory\n", progname);
+       for (i = 0; i < (sizeof(buffer) - 1) && *s; i++)
+               *p++ = toupper(*s++);
+
+       if (*s) {
+               fprintf(stderr, "%s:  buffer too small\n", progname);

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.

Do not override the classmap and the initial sid list in the build time script genheaders.

Check flose(3) is successful in genheaders.c, otherwise the written data might be corrupted or incomplete.

Signed-off-by Christian Göttsche <[email protected]>

---
v2:
Drop const exemption for genheaders script by rewriting stoupperx().
v3:
- Declare some additional data array const
- Do not use static buffer in genheaders.c::stoupperx()
- Check fclose(3) in genheaders.c

---
 scripts/selinux/genheaders/genheaders.c       | 75 +++++++++++--------
 scripts/selinux/mdp/mdp.c                     |  4 +-
 security/selinux/avc.c                        |  2 +-
 security/selinux/include/avc_ss.h             |  2 +-
 security/selinux/include/classmap.h           |  2 +-
 .../selinux/include/initial_sid_to_string.h   |  4 +-
 security/selinux/include/policycap.h          |  2 +-
 security/selinux/include/policycap_names.h    |  2 +-
 security/selinux/ss/avtab.c                   |  2 +-
 security/selinux/ss/policydb.c                | 36 ++++-----
 security/selinux/ss/services.c                |  4 +-
 11 files changed, 72 insertions(+), 63 deletions(-)

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 <[email protected]>

---
 security/selinux/hooks.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

security/selinux/hooks.c +1 -2

index 1e69f88eb326..ac802b99d36c 100644
@@ -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

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 <[email protected]>

---
 security/selinux/hooks.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

security/selinux/hooks.c +0 -0

index 1e69f88eb326..ac802b99d36c 100644

security/selinux/hooks.c +1 -2

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

2.35.1

Quoted 10 lines · 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:

Quoted 7 lines

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?

Quoted 12 lines

Signed-off-by Christian Göttsche <[email protected]>

---
 security/selinux/hooks.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

security/selinux/hooks.c +0 -0

index 1e69f88eb326..ac802b99d36c 100644

security/selinux/hooks.c static int show_sid(struct seq_file *m, u32 sid) · line 1022

Quoted 3 lines

security/selinux/hooks.c +0 -0

@@ -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.

Quoted 9 lines

security/selinux/hooks.c +1 -2

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

2.35.1

--
Thanks,
~Nick Desaulniers

Quoted 21 lines · Nick Desaulniers

Quoted 10 lines · 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:

Quoted 7 lines

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);
^~~~~~~~~~~

Quoted 1 line

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

Yes, complete output appended at the end.

Quoted 20 lines

Quoted 12 lines

Signed-off-by Christian Göttsche <[email protected]>

---
 security/selinux/hooks.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

security/selinux/hooks.c +0 -0

index 1e69f88eb326..ac802b99d36c 100644

security/selinux/hooks.c static int show_sid(struct seq_file *m, u32 sid) · line 1022

Quoted 3 lines

security/selinux/hooks.c +0 -0

@@ -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) {

Quoted 2 lines

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).

Quoted 11 lines

Quoted 9 lines

security/selinux/hooks.c +1 -2

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

2.35.1

Quoted 3 lines

--
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);
                               ^      ~~~

Quoted 47 lines · Christian Göttsche

Quoted 21 lines · Nick Desaulniers

Quoted 10 lines · 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:

Quoted 7 lines

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);
^~~~~~~~~~~

Quoted 1 line

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

Yes, complete output appended at the end.

Quoted 20 lines

Quoted 12 lines

Signed-off-by Christian Göttsche <[email protected]>

---
 security/selinux/hooks.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

security/selinux/hooks.c +0 -0

index 1e69f88eb326..ac802b99d36c 100644

security/selinux/hooks.c static int show_sid(struct seq_file *m, u32 sid) · line 1022

Quoted 3 lines

security/selinux/hooks.c +0 -0

@@ -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) {

Quoted 8 lines

Quoted 2 lines

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?

Quoted 76 lines

Quoted 12 lines

Quoted 10 lines

security/selinux/hooks.c +1 -2

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

2.35.1

Quoted 3 lines

--
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);

Quoted 8 lines

                          ^
././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);
                               ^      ~~~

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 <[email protected]>

---
 security/selinux/hooks.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 18 lines · 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);
^~~~~~~~~~~

Quoted 4 lines

Signed-off-by Christian Göttsche <[email protected]>

---
 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

Quoted 26 lines · Paul Moore

Quoted 22 lines · 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 <[email protected]>

---
 security/selinux/hooks.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 3 lines

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. ;)

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 <[email protected]>

---
 security/selinux/xfrm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)