On Mon, Nov 18, 2019 at 04:34:53PM -0800, Brendan Higgins wrote:

On Thu, Nov 7, 2019 at 3:33 PM Brendan Higgins
[off-list ref] wrote:

quoted

On Wed, Nov 06, 2019 at 09:18:27AM -0800, Kees Cook wrote:

quoted

On Tue, Nov 05, 2019 at 04:43:29PM -0800, Brendan Higgins wrote:

quoted

From: Mike Salvatore <redacted>

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 <redacted>
Signed-off-by: Mike Salvatore <redacted>
---
 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.

quoted

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 <redacted>


-- 
Kees Cook