From: Ondrej Mosnacek <omosnace@redhat.com>
To: "Christian Göttsche" <cgzones@googlemail.com>
Cc: Nick Desaulniers <ndesaulniers@google.com>,
SElinux list <selinux@vger.kernel.org>,
Paul Moore <paul@paul-moore.com>,
Stephen Smalley <stephen.smalley.work@gmail.com>,
Eric Paris <eparis@parisplace.org>,
Nathan Chancellor <nathan@kernel.org>,
Serge Hallyn <serge@hallyn.com>, Austin Kim <austin.kim@lge.com>,
Jiapeng Chong <jiapeng.chong@linux.alibaba.com>,
Casey Schaufler <casey@schaufler-ca.com>,
Yang Li <yang.lee@linux.alibaba.com>,
Linux kernel mailing list <linux-kernel@vger.kernel.org>,
llvm@lists.linux.dev
Subject: Re: [PATCH 5/5] selinux: drop unnecessary NULL check
Date: Wed, 4 May 2022 13:15:12 +0200 [thread overview]
Message-ID: <CAFqZXNvgByrTLbGMRz20Kcu1473YMLmxGeQev53qM7jNSSYFbQ@mail.gmail.com> (raw)
In-Reply-To: <CAJ2a_De2o7+v2v4QDnXNB=q9N6J84iMz8QMe6vK7ojvxVhfqqQ@mail.gmail.com>
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.
next prev parent reply other threads:[~2022-05-04 11:15 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-02-17 14:21 [PATCH 2/5] selinux: use correct type for context length Christian Göttsche
2022-02-17 14:21 ` [PATCH 3/5] selinux: use consistent pointer types for boolean arrays Christian Göttsche
2022-02-18 16:01 ` Paul Moore
2022-03-08 15:57 ` Christian Göttsche
2022-02-17 14:21 ` [PATCH 4/5] selinux: declare data arrays const Christian Göttsche
2022-02-18 16:13 ` Paul Moore
2022-02-18 17:24 ` Nick Desaulniers
2022-02-22 23:16 ` Paul Moore
2022-03-08 16:55 ` [PATCH v2 " Christian Göttsche
2022-04-04 20:03 ` Paul Moore
2022-05-02 14:43 ` [PATCH v3] " Christian Göttsche
2022-05-03 19:59 ` Paul Moore
2022-02-17 14:21 ` [PATCH 5/5] selinux: drop unnecessary NULL check Christian Göttsche
2022-02-18 16:22 ` Paul Moore
2022-02-18 17:31 ` Nick Desaulniers
2022-03-08 16:09 ` Christian Göttsche
2022-05-02 13:43 ` Christian Göttsche
2022-05-04 11:15 ` Ondrej Mosnacek [this message]
2022-06-07 21:22 ` Paul Moore
2022-06-07 21:26 ` Nick Desaulniers
2022-06-07 21:35 ` Paul Moore
2022-02-17 14:21 ` [PATCH 1/5] selinux: drop return statement at end of void functions Christian Göttsche
2022-02-18 15:44 ` Paul Moore
2022-02-18 15:47 ` [PATCH 2/5] selinux: use correct type for context length Paul Moore
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAFqZXNvgByrTLbGMRz20Kcu1473YMLmxGeQev53qM7jNSSYFbQ@mail.gmail.com \
--to=omosnace@redhat.com \
--cc=austin.kim@lge.com \
--cc=casey@schaufler-ca.com \
--cc=cgzones@googlemail.com \
--cc=eparis@parisplace.org \
--cc=jiapeng.chong@linux.alibaba.com \
--cc=linux-kernel@vger.kernel.org \
--cc=llvm@lists.linux.dev \
--cc=nathan@kernel.org \
--cc=ndesaulniers@google.com \
--cc=paul@paul-moore.com \
--cc=selinux@vger.kernel.org \
--cc=serge@hallyn.com \
--cc=stephen.smalley.work@gmail.com \
--cc=yang.lee@linux.alibaba.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).