Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <20220217142133.72205-1-cgzones@googlemail.com>
 <20220217142133.72205-4-cgzones@googlemail.com> <CAKwvOdkNZ3W9amcQKHOEfQLbdWC=4VDtrBdbOVHtWruipQnyKg@mail.gmail.com>
 <CAJ2a_DcY3=jz_zBQ7QZ_gycsvL1mn=TxKaWqWr3gGLhEAXTcQA@mail.gmail.com>
In-Reply-To: <CAJ2a_DcY3=jz_zBQ7QZ_gycsvL1mn=TxKaWqWr3gGLhEAXTcQA@mail.gmail.com>
From:   =?UTF-8?Q?Christian_G=C3=B6ttsche?= <cgzones@googlemail.com>
Date:   Mon, 2 May 2022 15:43:35 +0200
Message-ID: <CAJ2a_De2o7+v2v4QDnXNB=q9N6J84iMz8QMe6vK7ojvxVhfqqQ@mail.gmail.com>
Subject: Re: [PATCH 5/5] selinux: drop unnecessary NULL check
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
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, 8 Mar 2022 at 17:09, Christian G=C3=B6ttsche <cgzones@googlemail.co=
m> 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=C3=B6ttsche
> > <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 a=
s 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 war=
ning?
>
> Yes, complete output appended at the end.
>
> >
> > >
> > > Signed-off-by: Christian G=C3=B6ttsche <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 =3D 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 addres=
s
> of a stack variable - &context - is not the case).
>

Kindly ping;
is my analysis correct that after

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

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

> >
> > > -               bool has_comma =3D context && strchr(context, ',');
> > > +               bool has_comma =3D strchr(context, ',');
> > >
> > >                 seq_putc(m, '=3D');
> > >                 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 =3D show_sid(m, sbsec->sid);
>                      ^~~~~~~~~~~~~~~~~~~~~~~
> ./security/selinux/hooks.c:1020:7: note: Value assigned to 'context'
>         rc =3D 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 =3D context && strchr(context, ',');
>                                  ^~~~~~~
> ./security/selinux/hooks.c:1023:28: note: Left side of '&&' is false
>                 bool has_comma =3D 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);
>                                ^      ~~~