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

On Thu, Nov 7, 2019 at 3:33 PM Brendan Higgins brendanhiggins@google.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@canonical.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@google.com Signed-off-by: Mike Salvatore mike.salvatore@canonical.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?

Thanks!