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@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@google.com Cc: Konstantin Meskhidze konstantin.meskhidze@huawei.com Signed-off-by: Micka�l Sala�n mic@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/kunitRun all KUnit tests for Landlock with:./tools/testing/kunit/kunit.py run --kunitconfig security/landlockIf 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_*().