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@xxxxxxxxxx> 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@xxxxxxxxxxxxx> > > > > > > > > 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@xxxxxxxxxx> > > > > Signed-off-by: Mike Salvatore <mike.salvatore@xxxxxxxxxxxxx> > > > > --- > > > > 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@xxxxxxxxxxxx> -- Kees Cook
