| Message ID | 20220217142133.72205-4-cgzones@googlemail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [1/5] selinux: drop return statement at end of void functions | expand |
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
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 >
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); ^ ~~~
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); > ^ ~~~
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.
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.
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
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 :)
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)
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(-)