Message-ID: <201912101147.7BCF728@keescook>
Date:   Tue, 10 Dec 2019 11:48:04 -0800
From:   Kees Cook <keescook@...omium.org>
To:     Brendan Higgins <brendanhiggins@...gle.com>
Cc:     shuah <shuah@...nel.org>,
        John Johansen <john.johansen@...onical.com>, jmorris@...ei.org,
        serge@...lyn.com, Alan Maguire <alan.maguire@...cle.com>,
        Iurii Zaikin <yzaikin@...gle.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 v2] apparmor: add AppArmor KUnit
 tests for policy unpack

On Mon, Nov 18, 2019 at 04:34:53PM -0800, Brendan Higgins wrote:
> On Thu, Nov 7, 2019 at 3:33 PM Brendan Higgins
> <brendanhiggins@...gle.com> wrote:
> >
> > On Wed, Nov 06, 2019 at 09:18:27AM -0800, Kees Cook wrote:
> > > On Tue, Nov 05, 2019 at 04:43:29PM -0800, Brendan Higgins wrote:
> > > > From: Mike Salvatore <mike.salvatore@...onical.com>
> > > >
> > > > Add KUnit tests to test AppArmor unpacking of userspace policies.
> > > > AppArmor uses a serialized binary format for loading policies. To find
> > > > policy format documentation see
> > > > Documentation/admin-guide/LSM/apparmor.rst.
> > > >
> > > > In order to write the tests against the policy unpacking code, some
> > > > static functions needed to be exposed for testing purposes. One of the
> > > > goals of this patch is to establish a pattern for which testing these
> > > > kinds of functions should be done in the future.
> > > >
> > > > Signed-off-by: Brendan Higgins <brendanhiggins@...gle.com>
> > > > Signed-off-by: Mike Salvatore <mike.salvatore@...onical.com>
> > > > ---
> > > >  security/apparmor/Kconfig              |  16 +
> > > >  security/apparmor/policy_unpack.c      |   4 +
> > > >  security/apparmor/policy_unpack_test.c | 607 +++++++++++++++++++++++++
> > > >  3 files changed, 627 insertions(+)
> > > >  create mode 100644 security/apparmor/policy_unpack_test.c
> > > >
> > > > diff --git a/security/apparmor/Kconfig b/security/apparmor/Kconfig
> > > > index d8b1a360a6368..78a33ccac2574 100644
> > > > --- a/security/apparmor/Kconfig
> > > > +++ b/security/apparmor/Kconfig
> > > > @@ -66,3 +66,19 @@ config SECURITY_APPARMOR_DEBUG_MESSAGES
> > > >       Set the default value of the apparmor.debug kernel parameter.
> > > >       When enabled, various debug messages will be logged to
> > > >       the kernel message buffer.
> > > > +
> > > > +config SECURITY_APPARMOR_KUNIT_TEST
> > > > +   bool "Build KUnit tests for policy_unpack.c"
> > > > +   depends on KUNIT && SECURITY_APPARMOR
> > > > +   help
> > > > +     This builds the AppArmor KUnit tests.
> > > > +
> > > > +     KUnit tests run during boot and output the results to the debug log
> > > > +     in TAP format (http://testanything.org/). Only useful for kernel devs
> > > > +     running KUnit test harness and are not for inclusion into a
> > > > +     production build.
> > > > +
> > > > +     For more information on KUnit and unit tests in general please refer
> > > > +     to the KUnit documentation in Documentation/dev-tools/kunit/.
> > > > +
> > > > +     If unsure, say N.
> > > > diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c
> > > > index 8cfc9493eefc7..37c1dd3178fc0 100644
> > > > --- a/security/apparmor/policy_unpack.c
> > > > +++ b/security/apparmor/policy_unpack.c
> > > > @@ -1120,3 +1120,7 @@ int aa_unpack(struct aa_loaddata *udata, struct list_head *lh,
> > > >
> > > >     return error;
> > > >  }
> > > > +
> > > > +#ifdef CONFIG_SECURITY_APPARMOR_KUNIT_TEST
> > > > +#include "policy_unpack_test.c"
> > > > +#endif /* CONFIG_SECURITY_APPARMOR_KUNIT_TEST */
> > >
> > > To make this even LESS intrusive, the ifdefs could live in ..._test.c.
> >
> > Less intrusive, yes, but I think I actually like the ifdef here; it
> > makes it clear from the source that the test is only a part of the build
> > when configured to do so. Nevertheless, I will change it if anyone feels
> > strongly about it.
> >
> > > Also, while I *think* the kernel build system will correctly track this
> > > dependency, can you double-check that changes to ..._test.c correctly
> > > trigger a recompile of policy_unpack.c?
> >
> > Yep, just verified, first I ran the tests and everything passed. Then I
> > applied the following diff:
> >
> > diff --git a/security/apparmor/policy_unpack_test.c b/security/apparmor/policy_unpack_test.c
> > index 533137f45361c..e1b0670dbdc27 100644
> > --- a/security/apparmor/policy_unpack_test.c
> > +++ b/security/apparmor/policy_unpack_test.c
> > @@ -161,7 +161,7 @@ static void policy_unpack_test_unpack_array_with_name(struct kunit *test)
> >
> >         array_size = unpack_array(puf->e, name);
> >
> > -       KUNIT_EXPECT_EQ(test, array_size, (u16)TEST_ARRAY_SIZE);
> > +       KUNIT_EXPECT_EQ(test, array_size + 1, (u16)TEST_ARRAY_SIZE);
> >         KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
> >                 puf->e->start + TEST_ARRAY_BUF_OFFSET + sizeof(u16) + 1);
> >  }
> >
> > and reran the tests (to trigger an incremental build) and the test
> > failed as expected indicating that the dependency is properly tracked.
> 
> Hey Kees,
> 
> Since it looks like you already took a pretty close look at this,
> would you mind giving me a review?

Yes! Thanks for checking on those items. :)

Reviewed-by: Kees Cook <keescook@...omium.org>


-- 
Kees Cook

Powered by blists - more mailing lists