[PATCH v1] landlock: Add support for KUnit tests

Tue Jan 23 11:05:16 UTC 2024

On Fri, Jan 19, 2024 at 01:25:06PM +0100, Günther Noack wrote:
> Thank you, this is really nice!
> 
> Only tiny style nitpicks here.
> 
> Reviewed-by: Gï¿½nther Noack <gnoack at google.com>
> 
> On Thu, Jan 18, 2024 at 12:36:32PM +0100, Mickaï¿½l Salaï¿½n wrote:
> > Add the SECURITY_LANDLOCK_KUNIT_TEST option to enable KUnit tests for
> > Landlock.  The minimal required configuration is listed in the
> > security/landlock/.kunitconfig file.
> > 
> > Add an initial landlock_fs KUnit test suite with 7 test cases for
> > filesystem helpers.  These are related to the LANDLOCK_ACCESS_FS_REFER
> > right.
> > 
> > There is one KUnit test case per:
> > * mutated state (e.g. test_scope_to_request_*) or,
> > * shared state between tests (e.g. test_is_eaccess_*).
> > 
> > Add macros to improve readability of tests (i.e. one per line).  Test
> > cases are collocated with the tested functions to help maintenance and
> > improve documentation.  This is why SECURITY_LANDLOCK_KUNIT_TEST cannot
> > be set as module.
> > 
> > This is a nice complement to Landlock's user space kselftests.  We
> > expect new Landlock features to come with KUnit tests as well.
> > 
> > Thanks to UML support, we can run all KUnit tests for Landlock with:
> > ./tools/testing/kunit/kunit.py run --kunitconfig security/landlock
> > 
> > [00:00:00] ======================= landlock_fs  =======================
> > [00:00:00] [PASSED] test_no_more_access
> > [00:00:00] [PASSED] test_scope_to_request_with_exec_none
> > [00:00:00] [PASSED] test_scope_to_request_with_exec_some
> > [00:00:00] [PASSED] test_scope_to_request_without_access
> > [00:00:00] [PASSED] test_is_eacces_with_none
> > [00:00:00] [PASSED] test_is_eacces_with_refer
> > [00:00:00] [PASSED] test_is_eacces_with_write
> > [00:00:00] =================== [PASSED] landlock_fs ===================
> > [00:00:00] ============================================================
> > [00:00:00] Testing complete. Ran 7 tests: passed: 7
> > 
> > Cc: Gï¿½nther Noack <gnoack at google.com>
> > Cc: Konstantin Meskhidze <konstantin.meskhidze at huawei.com>
> > Signed-off-by: Mickaï¿½l Salaï¿½n <mic at digikod.net>
> > ---
> >  security/landlock/.kunitconfig               |   4 +
> >  security/landlock/Kconfig                    |  15 ++
> >  security/landlock/common.h                   |   2 +
> >  security/landlock/fs.c                       | 234 +++++++++++++++++++
> >  tools/testing/kunit/configs/all_tests.config |   1 +
> >  5 files changed, 256 insertions(+)
> >  create mode 100644 security/landlock/.kunitconfig
> > 
> > diff --git a/security/landlock/.kunitconfig b/security/landlock/.kunitconfig
> > new file mode 100644
> > index 000000000000..03e119466604
> > --- /dev/null
> > +++ b/security/landlock/.kunitconfig
> > @@ -0,0 +1,4 @@
> > +CONFIG_KUNIT=y
> > +CONFIG_SECURITY=y
> > +CONFIG_SECURITY_LANDLOCK=y
> > +CONFIG_SECURITY_LANDLOCK_KUNIT_TEST=y
> > diff --git a/security/landlock/Kconfig b/security/landlock/Kconfig
> > index c4bf0d5eff39..3f1493402052 100644
> > --- a/security/landlock/Kconfig
> > +++ b/security/landlock/Kconfig
> > @@ -20,3 +20,18 @@ config SECURITY_LANDLOCK
> >  	  If you are unsure how to answer this question, answer N.  Otherwise,
> >  	  you should also prepend "landlock," to the content of CONFIG_LSM to
> >  	  enable Landlock at boot time.
> > +
> > +config SECURITY_LANDLOCK_KUNIT_TEST
> > +	bool "KUnit tests for Landlock" if !KUNIT_ALL_TESTS
> > +	depends on KUNIT=y
> > +	depends on SECURITY_LANDLOCK
> > +	default KUNIT_ALL_TESTS
> > +	help
> > +	  Build KUnit tests for Landlock.
> > +
> > +	  See the KUnit documentation in Documentation/dev-tools/kunit
> > +
> > +	  Run all KUnit tests for Landlock with:
> > +	  ./tools/testing/kunit/kunit.py run --kunitconfig security/landlock
> > +
> > +	  If you are unsure how to answer this question, answer N.
> > diff --git a/security/landlock/common.h b/security/landlock/common.h
> > index 5dc0fe15707d..0eb1d34c2eae 100644
> > --- a/security/landlock/common.h
> > +++ b/security/landlock/common.h
> > @@ -17,4 +17,6 @@
> >  
> >  #define pr_fmt(fmt) LANDLOCK_NAME ": " fmt
> >  
> > +#define BIT_INDEX(bit) HWEIGHT(bit - 1)
> > +
> >  #endif /* _SECURITY_LANDLOCK_COMMON_H */
> > diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> > index 9ba989ef46a5..a2fdbd560105 100644
> > --- a/security/landlock/fs.c
> > +++ b/security/landlock/fs.c
> > @@ -7,6 +7,7 @@
> >   * Copyright ï¿½ 2021-2022 Microsoft Corporation
> >   */
> >  
> > +#include <kunit/test.h>
> >  #include <linux/atomic.h>
> >  #include <linux/bitops.h>
> >  #include <linux/bits.h>
> > @@ -311,6 +312,119 @@ static bool no_more_access(
> >  	return true;
> >  }
> >  
> > +#define NMA_TRUE(...) KUNIT_EXPECT_TRUE(test, no_more_access(__VA_ARGS__))
> > +#define NMA_FALSE(...) KUNIT_EXPECT_FALSE(test, no_more_access(__VA_ARGS__))
> > +
> > +#ifdef CONFIG_SECURITY_LANDLOCK_KUNIT_TEST
> > +
> > +static void test_no_more_access(struct kunit *const test)
> > +{
> > +	const layer_mask_t rx0[LANDLOCK_NUM_ACCESS_FS] = {
> > +		[BIT_INDEX(LANDLOCK_ACCESS_FS_EXECUTE)] = BIT_ULL(0),
> > +		[BIT_INDEX(LANDLOCK_ACCESS_FS_READ_FILE)] = BIT_ULL(0),
> > +	};
> > +	const layer_mask_t mx0[LANDLOCK_NUM_ACCESS_FS] = {
> > +		[BIT_INDEX(LANDLOCK_ACCESS_FS_EXECUTE)] = BIT_ULL(0),
> > +		[BIT_INDEX(LANDLOCK_ACCESS_FS_MAKE_REG)] = BIT_ULL(0),
> > +	};
> > +	const layer_mask_t x0[LANDLOCK_NUM_ACCESS_FS] = {
> > +		[BIT_INDEX(LANDLOCK_ACCESS_FS_EXECUTE)] = BIT_ULL(0),
> > +	};
> > +	const layer_mask_t x1[LANDLOCK_NUM_ACCESS_FS] = {
> > +		[BIT_INDEX(LANDLOCK_ACCESS_FS_EXECUTE)] = BIT_ULL(1),
> > +	};
> > +	const layer_mask_t x01[LANDLOCK_NUM_ACCESS_FS] = {
> > +		[BIT_INDEX(LANDLOCK_ACCESS_FS_EXECUTE)] = BIT_ULL(0) |
> > +							  BIT_ULL(1),
> > +	};
> > +	const layer_mask_t allows_all[LANDLOCK_NUM_ACCESS_FS] = {};
> > +
> > +	/* Checks without restriction. */
> > +	NMA_TRUE(&x0, &allows_all, false, &allows_all, NULL, false);
> > +	NMA_TRUE(&allows_all, &x0, false, &allows_all, NULL, false);
> > +	NMA_FALSE(&x0, &x0, false, &allows_all, NULL, false);
> > +
> > +	/*
> > +	 * Checks that we can only refer a file if no more access could be
> > +	 * inherited.
> > +	 */
> > +	NMA_TRUE(&x0, &x0, false, &rx0, NULL, false);
> > +	NMA_TRUE(&rx0, &rx0, false, &rx0, NULL, false);
> > +	NMA_FALSE(&rx0, &rx0, false, &x0, NULL, false);
> > +	NMA_FALSE(&rx0, &rx0, false, &x1, NULL, false);
> > +
> > +	/* Checks allowed referring with different nested domains. */
> > +	NMA_TRUE(&x0, &x1, false, &x0, NULL, false);
> > +	NMA_TRUE(&x1, &x0, false, &x0, NULL, false);
> > +	NMA_TRUE(&x0, &x01, false, &x0, NULL, false);
> > +	NMA_TRUE(&x0, &x01, false, &rx0, NULL, false);
> > +	NMA_TRUE(&x01, &x0, false, &x0, NULL, false);
> > +	NMA_TRUE(&x01, &x0, false, &rx0, NULL, false);
> > +	NMA_FALSE(&x01, &x01, false, &x0, NULL, false);
> > +
> > +	/* Checks that file access rights are also enforced for a directory. */
> > +	NMA_FALSE(&rx0, &rx0, true, &x0, NULL, false);
> > +
> > +	/* Checks that directory access rights don't impact file referring... */
> > +	NMA_TRUE(&mx0, &mx0, false, &x0, NULL, false);
> > +	/* ...but only directory referring. */
> > +	NMA_FALSE(&mx0, &mx0, true, &x0, NULL, false);
> > +
> > +	/* Checks directory exchange. */
> > +	NMA_TRUE(&mx0, &mx0, true, &mx0, &mx0, true);
> > +	NMA_TRUE(&mx0, &mx0, true, &mx0, &x0, true);
> > +	NMA_FALSE(&mx0, &mx0, true, &x0, &mx0, true);
> > +	NMA_FALSE(&mx0, &mx0, true, &x0, &x0, true);
> > +	NMA_FALSE(&mx0, &mx0, true, &x1, &x1, true);
> > +
> > +	/* Checks file exchange with directory access rights... */
> > +	NMA_TRUE(&mx0, &mx0, false, &mx0, &mx0, false);
> > +	NMA_TRUE(&mx0, &mx0, false, &mx0, &x0, false);
> > +	NMA_TRUE(&mx0, &mx0, false, &x0, &mx0, false);
> > +	NMA_TRUE(&mx0, &mx0, false, &x0, &x0, false);
> > +	/* ...and with file access rights. */
> > +	NMA_TRUE(&rx0, &rx0, false, &rx0, &rx0, false);
> > +	NMA_TRUE(&rx0, &rx0, false, &rx0, &x0, false);
> > +	NMA_FALSE(&rx0, &rx0, false, &x0, &rx0, false);
> > +	NMA_FALSE(&rx0, &rx0, false, &x0, &x0, false);
> > +	NMA_FALSE(&rx0, &rx0, false, &x1, &x1, false);
> > +
> > +	/*
> > +	 * Allowing the following requests should not be a security risk
> > +	 * because domain 0 denies execute access, and domain 1 is always
> > +	 * nested with domain 0.  However, adding an exception for this case
> > +	 * would mean to check all nested domains to make sure none can get
> > +	 * more privileges (e.g. processes only sandboxed by domain 0).
> > +	 * Moreover, this behavior (i.e. composition of N domains) could then
> > +	 * be inconsistent compared to domain 1's ruleset alone (e.g. it might
> > +	 * be denied to link/rename with domain 1's ruleset, whereas it would
> > +	 * be allowed if nested on top of domain 0).  Another drawback would be
> > +	 * to create a cover channel that could enable sandboxed processes to
> > +	 * infer most of the filesystem restrictions from their domain.  To
> > +	 * make it simple, efficient, safe, and more consistent, this case is
> > +	 * always denied.
> > +	 */
> > +	NMA_FALSE(&x1, &x1, false, &x0, NULL, false);
> > +	NMA_FALSE(&x1, &x1, false, &rx0, NULL, false);
> > +	NMA_FALSE(&x1, &x1, true, &x0, NULL, false);
> > +	NMA_FALSE(&x1, &x1, true, &rx0, NULL, false);
> > +
> > +	/* Checks the same case of exclusive domains with a file... */
> > +	NMA_TRUE(&x1, &x1, false, &x01, NULL, false);
> > +	NMA_FALSE(&x1, &x1, false, &x01, &x0, false);
> > +	NMA_FALSE(&x1, &x1, false, &x01, &x01, false);
> > +	NMA_FALSE(&x1, &x1, false, &x0, &x0, false);
> > +	/* ...and with a directory. */
> > +	NMA_FALSE(&x1, &x1, false, &x0, &x0, true);
> > +	NMA_FALSE(&x1, &x1, true, &x0, &x0, false);
> > +	NMA_FALSE(&x1, &x1, true, &x0, &x0, true);
> > +}
> > +
> > +#endif /* CONFIG_SECURITY_LANDLOCK_KUNIT_TEST */
> > +
> > +#undef NMA_TRUE
> > +#undef NMA_FALSE
> > +
> >  /*
> >   * Removes @layer_masks accesses that are not requested.
> >   *
> > @@ -331,6 +445,57 @@ scope_to_request(const access_mask_t access_request,
> >  	return !memchr_inv(layer_masks, 0, sizeof(*layer_masks));
> >  }
> >  
> > +#ifdef CONFIG_SECURITY_LANDLOCK_KUNIT_TEST
> > +
> > +static void test_scope_to_request_with_exec_none(struct kunit *const test)
> > +{
> > +	/* Allows everything. */
> > +	layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {};
> > +
> > +	/* Checks and scopes with execute. */
> > +	KUNIT_EXPECT_TRUE(test, scope_to_request(LANDLOCK_ACCESS_FS_EXECUTE,
> > +						 &layer_masks));
> > +	KUNIT_EXPECT_EQ(test, 0,
> > +			layer_masks[BIT_INDEX(LANDLOCK_ACCESS_FS_EXECUTE)]);
> > +	KUNIT_EXPECT_EQ(test, 0,
> > +			layer_masks[BIT_INDEX(LANDLOCK_ACCESS_FS_WRITE_FILE)]);
> > +}
> > +
> > +static void test_scope_to_request_with_exec_some(struct kunit *const test)
> > +{
> > +	/* Denies execute and write. */
> > +	layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {
> > +		[BIT_INDEX(LANDLOCK_ACCESS_FS_EXECUTE)] = BIT_ULL(0),
> > +		[BIT_INDEX(LANDLOCK_ACCESS_FS_WRITE_FILE)] = BIT_ULL(1),
> > +	};
> > +
> > +	/* Checks and scopes with execute. */
> > +	KUNIT_EXPECT_FALSE(test, scope_to_request(LANDLOCK_ACCESS_FS_EXECUTE,
> > +						  &layer_masks));
> > +	KUNIT_EXPECT_EQ(test, BIT_ULL(0),
> > +			layer_masks[BIT_INDEX(LANDLOCK_ACCESS_FS_EXECUTE)]);
> > +	KUNIT_EXPECT_EQ(test, 0,
> > +			layer_masks[BIT_INDEX(LANDLOCK_ACCESS_FS_WRITE_FILE)]);
> > +}
> > +
> > +static void test_scope_to_request_without_access(struct kunit *const test)
> > +{
> > +	/* Denies execute and write. */
> > +	layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {
> > +		[BIT_INDEX(LANDLOCK_ACCESS_FS_EXECUTE)] = BIT_ULL(0),
> > +		[BIT_INDEX(LANDLOCK_ACCESS_FS_WRITE_FILE)] = BIT_ULL(1),
> > +	};
> > +
> > +	/* Checks and scopes without access request. */
> > +	KUNIT_EXPECT_TRUE(test, scope_to_request(0, &layer_masks));
> > +	KUNIT_EXPECT_EQ(test, 0,
> > +			layer_masks[BIT_INDEX(LANDLOCK_ACCESS_FS_EXECUTE)]);
> > +	KUNIT_EXPECT_EQ(test, 0,
> > +			layer_masks[BIT_INDEX(LANDLOCK_ACCESS_FS_WRITE_FILE)]);
> > +}
> > +
> > +#endif /* CONFIG_SECURITY_LANDLOCK_KUNIT_TEST */
> > +
> >  /*
> >   * Returns true if there is at least one access right different than
> >   * LANDLOCK_ACCESS_FS_REFER.
> > @@ -354,6 +519,51 @@ is_eacces(const layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS],
> >  	return false;
> >  }
> >  
> > +#define IE_TRUE(...) KUNIT_EXPECT_TRUE(test, is_eacces(__VA_ARGS__))
> > +#define IE_FALSE(...) KUNIT_EXPECT_FALSE(test, is_eacces(__VA_ARGS__))
> 
> is_eacces() only has one argument anyway, so __VA_ARGS__ is not as useful as it
> was in the other case, IMHO.  But works either way.

Correct, but I think it makes it more obvious that this macro doesn't
handle the arguments at all but just pass them.  It's also the same
pattern as for NMA_*().