Thank you, this is really nice!
Only tiny style nitpicks here.
Reviewed-by: G�nther Noack <gnoack@xxxxxxxxxx>
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@xxxxxxxxxx>
> Cc: Konstantin Meskhidze <konstantin.meskhidze@xxxxxxxxxx>
> Signed-off-by: Micka�l Sala�n <mic@xxxxxxxxxxx>
> ---
> 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.
> +
> +#ifdef CONFIG_SECURITY_LANDLOCK_KUNIT_TEST
> +
> +static void test_is_eacces_with_none(struct kunit *const test)
> +{
> + const layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {};
> +
> + IE_FALSE(&layer_masks, 0);
> + IE_FALSE(&layer_masks, LANDLOCK_ACCESS_FS_REFER);
> + IE_FALSE(&layer_masks, LANDLOCK_ACCESS_FS_EXECUTE);
> + IE_FALSE(&layer_masks, LANDLOCK_ACCESS_FS_WRITE_FILE);
> +}
> +
> +static void test_is_eacces_with_refer(struct kunit *const test)
> +{
> + const layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {
> + [BIT_INDEX(LANDLOCK_ACCESS_FS_REFER)] = BIT_ULL(0),
> + };
> +
> + IE_FALSE(&layer_masks, 0);
> + IE_FALSE(&layer_masks, LANDLOCK_ACCESS_FS_REFER);
> + IE_FALSE(&layer_masks, LANDLOCK_ACCESS_FS_EXECUTE);
> + IE_FALSE(&layer_masks, LANDLOCK_ACCESS_FS_WRITE_FILE);
> +}
> +
> +static void test_is_eacces_with_write(struct kunit *const test)
> +{
> + const layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {
> + [BIT_INDEX(LANDLOCK_ACCESS_FS_WRITE_FILE)] = BIT_ULL(0),
> + };
> +
> + IE_FALSE(&layer_masks, 0);
> + IE_FALSE(&layer_masks, LANDLOCK_ACCESS_FS_REFER);
> + IE_FALSE(&layer_masks, LANDLOCK_ACCESS_FS_EXECUTE);
> +
> + IE_TRUE(&layer_masks, LANDLOCK_ACCESS_FS_WRITE_FILE);
> +}
> +
> +#endif /* CONFIG_SECURITY_LANDLOCK_KUNIT_TEST */
> +
> +#undef IE_TRUE
> +#undef IE_FALSE
> +
> /**
> * is_access_to_paths_allowed - Check accesses for requests with a common path
> *
> @@ -1225,3 +1435,27 @@ __init void landlock_add_fs_hooks(void)
> security_add_hooks(landlock_hooks, ARRAY_SIZE(landlock_hooks),
> LANDLOCK_NAME);
> }
> +
> +#ifdef CONFIG_SECURITY_LANDLOCK_KUNIT_TEST
> +
> +/* clang-format off */
> +static struct kunit_case test_cases[] = {
> + KUNIT_CASE(test_no_more_access),
> + KUNIT_CASE(test_scope_to_request_with_exec_none),
> + KUNIT_CASE(test_scope_to_request_with_exec_some),
> + KUNIT_CASE(test_scope_to_request_without_access),
> + KUNIT_CASE(test_is_eacces_with_none),
> + KUNIT_CASE(test_is_eacces_with_refer),
> + KUNIT_CASE(test_is_eacces_with_write),
> + {}
> +};
> +/* clang-format on */
> +
> +static struct kunit_suite test_suite = {
> + .name = "landlock_fs",
> + .test_cases = test_cases,
> +};
> +
> +kunit_test_suite(test_suite);
> +
> +#endif /* CONFIG_SECURITY_LANDLOCK_KUNIT_TEST */
> diff --git a/tools/testing/kunit/configs/all_tests.config b/tools/testing/kunit/configs/all_tests.config
> index 3bf506d4a63c..1b8f1abfedf0 100644
> --- a/tools/testing/kunit/configs/all_tests.config
> +++ b/tools/testing/kunit/configs/all_tests.config
> @@ -37,6 +37,7 @@ CONFIG_REGMAP_BUILD=y
>
> CONFIG_SECURITY=y
> CONFIG_SECURITY_APPARMOR=y
> +CONFIG_SECURITY_LANDLOCK=y
>
> CONFIG_SOUND=y
> CONFIG_SND=y
>
> base-commit: 0daaa610c8e033cdfb420db728c2b40eb3a75134
> --
> 2.43.0
>
