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@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.
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_*().
