Linux-Security-Module Archive on lore.kernel.org
 help / color / Atom feed
From: Brendan Higgins <brendanhiggins@google.com>
To: Kees Cook <keescook@chromium.org>
Cc: shuah@kernel.org, john.johansen@canonical.com, jmorris@namei.org,
	serge@hallyn.com, alan.maguire@oracle.com, yzaikin@google.com,
	davidgow@google.com, mcgrof@kernel.org, tytso@mit.edu,
	linux-kernel@vger.kernel.org,
	linux-security-module@vger.kernel.org,
	kunit-dev@googlegroups.com, linux-kselftest@vger.kernel.org,
	Mike Salvatore <mike.salvatore@canonical.com>
Subject: Re: [PATCH linux-kselftest/test v2] apparmor: add AppArmor KUnit tests for policy unpack
Date: Thu, 7 Nov 2019 15:33:37 -0800
Message-ID: <20191107233337.GA191231@google.com> (raw)
In-Reply-To: <201911060916.AC9E14B@keescook>

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.

Cheers!

      reply index

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-06  0:43 Brendan Higgins
2019-11-06  8:05 ` John Johansen
2019-11-06 17:18 ` Kees Cook
2019-11-07 23:33   ` Brendan Higgins [this message]

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191107233337.GA191231@google.com \
    --to=brendanhiggins@google.com \
    --cc=alan.maguire@oracle.com \
    --cc=davidgow@google.com \
    --cc=jmorris@namei.org \
    --cc=john.johansen@canonical.com \
    --cc=keescook@chromium.org \
    --cc=kunit-dev@googlegroups.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=mcgrof@kernel.org \
    --cc=mike.salvatore@canonical.com \
    --cc=serge@hallyn.com \
    --cc=shuah@kernel.org \
    --cc=tytso@mit.edu \
    --cc=yzaikin@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Security-Module Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-security-module/0 linux-security-module/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-security-module linux-security-module/ https://lore.kernel.org/linux-security-module \
		linux-security-module@vger.kernel.org
	public-inbox-index linux-security-module

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-security-module


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git