linux-kernel - Re: [PATCH linux-kselftest/test v1] apparmor: add AppArmor KUnit tests for policy unpack

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 5 Nov 2019 16:35:22 -0800
From:   Brendan Higgins <brendanhiggins@...gle.com>
To:     Kees Cook <keescook@...omium.org>
Cc:     Iurii Zaikin <yzaikin@...gle.com>, shuah <shuah@...nel.org>,
        John Johansen <john.johansen@...onical.com>, jmorris@...ei.org,
        serge@...lyn.com, Alan Maguire <alan.maguire@...cle.com>,
        David Gow <davidgow@...gle.com>,
        Luis Chamberlain <mcgrof@...nel.org>,
        "Theodore Ts'o" <tytso@....edu>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-security-module@...r.kernel.org,
        KUnit Development <kunit-dev@...glegroups.com>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@...r.kernel.org>,
        Mike Salvatore <mike.salvatore@...onical.com>
Subject: Re: [PATCH linux-kselftest/test v1] apparmor: add AppArmor KUnit
 tests for policy unpack

On Wed, Oct 30, 2019 at 11:59 AM Kees Cook <keescook@...omium.org> wrote:
>
> On Thu, Oct 17, 2019 at 05:33:56PM -0700, Iurii Zaikin wrote:
> > On Thu, Oct 17, 2019 at 5:19 PM Brendan Higgins
> > <brendanhiggins@...gle.com> wrote:
> >
> > > +config SECURITY_APPARMOR_TEST
> > > +       bool "Build KUnit tests for policy_unpack.c"
> > > +       default n
>
> New options already already default n, this can be left off.
>
> > > +       depends on KUNIT && SECURITY_APPARMOR
> > > +       help
> > >
> > select SECURITY_APPARMOR ?
>
> "select" doesn't enforce dependencies, so just a "depends ..." is
> correct.
>
> > > +       KUNIT_EXPECT_EQ(test, size, TEST_BLOB_DATA_SIZE);
> > > +       KUNIT_EXPECT_TRUE(test,
> > > +               memcmp(blob, TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE) == 0);
> > I think this must be  KUNIT_ASSERT_EQ(test, size, TEST_BLOB_DATA_SIZE);,
> > otherwise there could be a buffer overflow in memcmp. All tests that
> > follow such pattern
>
> Agreed.
>
> > are suspect. Also, not sure about your stylistic preference for
> > KUNIT_EXPECT_TRUE(test,
> >                memcmp(blob, TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE) == 0);
> > vs
> > KUNIT_EXPECT_EQ(test,
> >                0,
> >                memcmp(blob, TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE));
>
> I like == 0.

Oh, I almost missed this. I think the *_EQ(...) is better than the
*_TRUE(...) because the EQ is able to provide more debug information
if the test fails (otherwise there would really be no point in
providing all these variants).

Any objections?

Thanks for the catch Iurii!