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

From: "Christian Göttsche" <cgzones@googlemail.com>
To: Nick Desaulniers <ndesaulniers@google.com>
Cc: 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>,
	Ondrej Mosnacek <omosnace@redhat.com>,
	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: Tue, 8 Mar 2022 17:09:58 +0100	[thread overview]
Message-ID: <CAJ2a_DcY3=jz_zBQ7QZ_gycsvL1mn=TxKaWqWr3gGLhEAXTcQA@mail.gmail.com> (raw)
In-Reply-To: <CAKwvOdkNZ3W9amcQKHOEfQLbdWC=4VDtrBdbOVHtWruipQnyKg@mail.gmail.com>

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