On Tue, Mar 16, 2021 at 09:42:50PM +0100, Mickaël Salaün wrote:
From: Mickaël Salaün mic@linux.microsoft.com
Test all Landlock system calls, ptrace hooks semantic and filesystem access-control with multiple layouts.
Test coverage for security/landlock/ is 93.6% of lines. The code not covered only deals with internal kernel errors (e.g. memory allocation) and race conditions.
Cc: James Morris jmorris@namei.org Cc: Jann Horn jannh@google.com Cc: Kees Cook keescook@chromium.org Cc: Serge E. Hallyn serge@hallyn.com Cc: Shuah Khan shuah@kernel.org Signed-off-by: Mickaël Salaün mic@linux.microsoft.com Reviewed-by: Vincent Dagonneau vincent.dagonneau@ssi.gouv.fr Link: https://lore.kernel.org/r/20210316204252.427806-11-mic@digikod.net
This is terrific. I love the coverage. How did you measure this, BTW? To increase it into memory allocation failures, have you tried allocation fault injection: https://www.kernel.org/doc/html/latest/fault-injection/fault-injection.html
[...] +TEST(inconsistent_attr) {
- const long page_size = sysconf(_SC_PAGESIZE);
- char *const buf = malloc(page_size + 1);
- struct landlock_ruleset_attr *const ruleset_attr = (void *)buf;
- ASSERT_NE(NULL, buf);
- /* Checks copy_from_user(). */
- ASSERT_EQ(-1, landlock_create_ruleset(ruleset_attr, 0, 0));
- /* The size if less than sizeof(struct landlock_attr_enforce). */
- ASSERT_EQ(EINVAL, errno);
- ASSERT_EQ(-1, landlock_create_ruleset(ruleset_attr, 1, 0));
- ASSERT_EQ(EINVAL, errno);
Almost everywhere you're using ASSERT instead of EXPECT. Is this correct (in the sense than as soon as an ASSERT fails the rest of the test is skipped)? I do see you using EXPECT is some places, but I figured I'd ask about the intention here.
+/*
- TEST_F_FORK() is useful when a test drop privileges but the corresponding
- FIXTURE_TEARDOWN() requires them (e.g. to remove files from a directory
- where write actions are denied). For convenience, FIXTURE_TEARDOWN() is
- also called when the test failed, but not when FIXTURE_SETUP() failed. For
- this to be possible, we must not call abort() but instead exit smoothly
- (hence the step print).
- */
Hm, interesting. I think this should be extracted into a separate patch and added to the test harness proper.
Could this be solved with TEARDOWN being called on SETUP failure?
+#define TEST_F_FORK(fixture_name, test_name) \
- static void fixture_name##_##test_name##_child( \
struct __test_metadata *_metadata, \FIXTURE_DATA(fixture_name) *self, \const FIXTURE_VARIANT(fixture_name) *variant); \- TEST_F(fixture_name, test_name) \
- { \
int status; \const pid_t child = fork(); \if (child < 0) \abort(); \if (child == 0) { \_metadata->no_print = 1; \fixture_name##_##test_name##_child(_metadata, self, variant); \if (_metadata->skip) \_exit(255); \if (_metadata->passed) \_exit(0); \_exit(_metadata->step); \} \if (child != waitpid(child, &status, 0)) \abort(); \if (WIFSIGNALED(status) || !WIFEXITED(status)) { \_metadata->passed = 0; \_metadata->step = 1; \return; \} \switch (WEXITSTATUS(status)) { \case 0: \_metadata->passed = 1; \break; \case 255: \_metadata->passed = 1; \_metadata->skip = 1; \break; \default: \_metadata->passed = 0; \_metadata->step = WEXITSTATUS(status); \break; \} \- } \
This looks like a subset of __wait_for_test()? Could __TEST_F_IMPL() be updated instead to do this? (Though the fork overhead might not be great for everyone.)
On 19/03/2021 18:56, Kees Cook wrote:
On Tue, Mar 16, 2021 at 09:42:50PM +0100, Mickaël Salaün wrote:
From: Mickaël Salaün mic@linux.microsoft.com
Test all Landlock system calls, ptrace hooks semantic and filesystem access-control with multiple layouts.
Test coverage for security/landlock/ is 93.6% of lines. The code not covered only deals with internal kernel errors (e.g. memory allocation) and race conditions.
Cc: James Morris jmorris@namei.org Cc: Jann Horn jannh@google.com Cc: Kees Cook keescook@chromium.org Cc: Serge E. Hallyn serge@hallyn.com Cc: Shuah Khan shuah@kernel.org Signed-off-by: Mickaël Salaün mic@linux.microsoft.com Reviewed-by: Vincent Dagonneau vincent.dagonneau@ssi.gouv.fr Link: https://lore.kernel.org/r/20210316204252.427806-11-mic@digikod.net
This is terrific. I love the coverage. How did you measure this, BTW?
I used gcov: https://www.kernel.org/doc/html/latest/dev-tools/gcov.html
To increase it into memory allocation failures, have you tried allocation fault injection: https://www.kernel.org/doc/html/latest/fault-injection/fault-injection.html
Yes, it is used by syzkaller, but I don't know how to extract this specific coverage.
[...] +TEST(inconsistent_attr) {
- const long page_size = sysconf(_SC_PAGESIZE);
- char *const buf = malloc(page_size + 1);
- struct landlock_ruleset_attr *const ruleset_attr = (void *)buf;
- ASSERT_NE(NULL, buf);
- /* Checks copy_from_user(). */
- ASSERT_EQ(-1, landlock_create_ruleset(ruleset_attr, 0, 0));
- /* The size if less than sizeof(struct landlock_attr_enforce). */
- ASSERT_EQ(EINVAL, errno);
- ASSERT_EQ(-1, landlock_create_ruleset(ruleset_attr, 1, 0));
- ASSERT_EQ(EINVAL, errno);
Almost everywhere you're using ASSERT instead of EXPECT. Is this correct (in the sense than as soon as an ASSERT fails the rest of the test is skipped)? I do see you using EXPECT is some places, but I figured I'd ask about the intention here.
I intentionally use ASSERT as much as possible, but I use EXPECT when an error could block a test or when it could stop a cleanup (i.e. teardown).
+/*
- TEST_F_FORK() is useful when a test drop privileges but the corresponding
- FIXTURE_TEARDOWN() requires them (e.g. to remove files from a directory
- where write actions are denied). For convenience, FIXTURE_TEARDOWN() is
- also called when the test failed, but not when FIXTURE_SETUP() failed. For
- this to be possible, we must not call abort() but instead exit smoothly
- (hence the step print).
- */
Hm, interesting. I think this should be extracted into a separate patch and added to the test harness proper.
I agree, but it may require some modifications to fit nicely in kselftest_harness.h . For now, it works well for my use case. I'll send patches once Landlock is merged. In fact, I already made kselftest_harness.h available for other users than seccomp. ;)
Could this be solved with TEARDOWN being called on SETUP failure?
The goal of this helper is to still be able to call TEARDOWN when TEST failed, not SETUP.
+#define TEST_F_FORK(fixture_name, test_name) \
- static void fixture_name##_##test_name##_child( \
struct __test_metadata *_metadata, \FIXTURE_DATA(fixture_name) *self, \const FIXTURE_VARIANT(fixture_name) *variant); \- TEST_F(fixture_name, test_name) \
- { \
int status; \const pid_t child = fork(); \if (child < 0) \abort(); \if (child == 0) { \_metadata->no_print = 1; \fixture_name##_##test_name##_child(_metadata, self, variant); \if (_metadata->skip) \_exit(255); \if (_metadata->passed) \_exit(0); \_exit(_metadata->step); \} \if (child != waitpid(child, &status, 0)) \abort(); \if (WIFSIGNALED(status) || !WIFEXITED(status)) { \_metadata->passed = 0; \_metadata->step = 1; \return; \} \switch (WEXITSTATUS(status)) { \case 0: \_metadata->passed = 1; \break; \case 255: \_metadata->passed = 1; \_metadata->skip = 1; \break; \default: \_metadata->passed = 0; \_metadata->step = WEXITSTATUS(status); \break; \} \- } \
This looks like a subset of __wait_for_test()? Could __TEST_F_IMPL() be updated instead to do this? (Though the fork overhead might not be great for everyone.)
Yes, it will probably be my approach to update kselftest_harness.h .
On Fri, Mar 19, 2021 at 07:41:00PM +0100, Mickaël Salaün wrote:
On 19/03/2021 18:56, Kees Cook wrote:
On Tue, Mar 16, 2021 at 09:42:50PM +0100, Mickaël Salaün wrote:
From: Mickaël Salaün mic@linux.microsoft.com
Test all Landlock system calls, ptrace hooks semantic and filesystem access-control with multiple layouts.
Test coverage for security/landlock/ is 93.6% of lines. The code not covered only deals with internal kernel errors (e.g. memory allocation) and race conditions.
Cc: James Morris jmorris@namei.org Cc: Jann Horn jannh@google.com Cc: Kees Cook keescook@chromium.org Cc: Serge E. Hallyn serge@hallyn.com Cc: Shuah Khan shuah@kernel.org Signed-off-by: Mickaël Salaün mic@linux.microsoft.com Reviewed-by: Vincent Dagonneau vincent.dagonneau@ssi.gouv.fr Link: https://lore.kernel.org/r/20210316204252.427806-11-mic@digikod.net
This is terrific. I love the coverage. How did you measure this, BTW?
I used gcov: https://www.kernel.org/doc/html/latest/dev-tools/gcov.html
To increase it into memory allocation failures, have you tried allocation fault injection: https://www.kernel.org/doc/html/latest/fault-injection/fault-injection.html
Yes, it is used by syzkaller, but I don't know how to extract this specific coverage.
[...] +TEST(inconsistent_attr) {
- const long page_size = sysconf(_SC_PAGESIZE);
- char *const buf = malloc(page_size + 1);
- struct landlock_ruleset_attr *const ruleset_attr = (void *)buf;
- ASSERT_NE(NULL, buf);
- /* Checks copy_from_user(). */
- ASSERT_EQ(-1, landlock_create_ruleset(ruleset_attr, 0, 0));
- /* The size if less than sizeof(struct landlock_attr_enforce). */
- ASSERT_EQ(EINVAL, errno);
- ASSERT_EQ(-1, landlock_create_ruleset(ruleset_attr, 1, 0));
- ASSERT_EQ(EINVAL, errno);
Almost everywhere you're using ASSERT instead of EXPECT. Is this correct (in the sense than as soon as an ASSERT fails the rest of the test is skipped)? I do see you using EXPECT is some places, but I figured I'd ask about the intention here.
I intentionally use ASSERT as much as possible, but I use EXPECT when an error could block a test or when it could stop a cleanup (i.e. teardown).
Okay. Does the test suite run sanely when landlock is missing from the kernel?
+/*
- TEST_F_FORK() is useful when a test drop privileges but the corresponding
- FIXTURE_TEARDOWN() requires them (e.g. to remove files from a directory
- where write actions are denied). For convenience, FIXTURE_TEARDOWN() is
- also called when the test failed, but not when FIXTURE_SETUP() failed. For
- this to be possible, we must not call abort() but instead exit smoothly
- (hence the step print).
- */
Hm, interesting. I think this should be extracted into a separate patch and added to the test harness proper.
I agree, but it may require some modifications to fit nicely in kselftest_harness.h . For now, it works well for my use case. I'll send patches once Landlock is merged. In fact, I already made kselftest_harness.h available for other users than seccomp. ;)
Fair points.
Could this be solved with TEARDOWN being called on SETUP failure?
The goal of this helper is to still be able to call TEARDOWN when TEST failed, not SETUP.
+#define TEST_F_FORK(fixture_name, test_name) \
- static void fixture_name##_##test_name##_child( \
struct __test_metadata *_metadata, \FIXTURE_DATA(fixture_name) *self, \const FIXTURE_VARIANT(fixture_name) *variant); \- TEST_F(fixture_name, test_name) \
- { \
int status; \const pid_t child = fork(); \if (child < 0) \abort(); \if (child == 0) { \_metadata->no_print = 1; \fixture_name##_##test_name##_child(_metadata, self, variant); \if (_metadata->skip) \_exit(255); \if (_metadata->passed) \_exit(0); \_exit(_metadata->step); \} \if (child != waitpid(child, &status, 0)) \abort(); \if (WIFSIGNALED(status) || !WIFEXITED(status)) { \_metadata->passed = 0; \_metadata->step = 1; \return; \} \switch (WEXITSTATUS(status)) { \case 0: \_metadata->passed = 1; \break; \case 255: \_metadata->passed = 1; \_metadata->skip = 1; \break; \default: \_metadata->passed = 0; \_metadata->step = WEXITSTATUS(status); \break; \} \- } \
This looks like a subset of __wait_for_test()? Could __TEST_F_IMPL() be updated instead to do this? (Though the fork overhead might not be great for everyone.)
Yes, it will probably be my approach to update kselftest_harness.h .
It seems like this would be named better as TEST_DROPS_PRIVS or something, which describes the reason for the fork.
On 19/03/2021 20:11, Kees Cook wrote:
On Fri, Mar 19, 2021 at 07:41:00PM +0100, Mickaël Salaün wrote:
On 19/03/2021 18:56, Kees Cook wrote:
On Tue, Mar 16, 2021 at 09:42:50PM +0100, Mickaël Salaün wrote:
From: Mickaël Salaün mic@linux.microsoft.com
Test all Landlock system calls, ptrace hooks semantic and filesystem access-control with multiple layouts.
Test coverage for security/landlock/ is 93.6% of lines. The code not covered only deals with internal kernel errors (e.g. memory allocation) and race conditions.
Cc: James Morris jmorris@namei.org Cc: Jann Horn jannh@google.com Cc: Kees Cook keescook@chromium.org Cc: Serge E. Hallyn serge@hallyn.com Cc: Shuah Khan shuah@kernel.org Signed-off-by: Mickaël Salaün mic@linux.microsoft.com Reviewed-by: Vincent Dagonneau vincent.dagonneau@ssi.gouv.fr Link: https://lore.kernel.org/r/20210316204252.427806-11-mic@digikod.net
This is terrific. I love the coverage. How did you measure this, BTW?
I used gcov: https://www.kernel.org/doc/html/latest/dev-tools/gcov.html
To increase it into memory allocation failures, have you tried allocation fault injection: https://www.kernel.org/doc/html/latest/fault-injection/fault-injection.html
Yes, it is used by syzkaller, but I don't know how to extract this specific coverage.
[...] +TEST(inconsistent_attr) {
- const long page_size = sysconf(_SC_PAGESIZE);
- char *const buf = malloc(page_size + 1);
- struct landlock_ruleset_attr *const ruleset_attr = (void *)buf;
- ASSERT_NE(NULL, buf);
- /* Checks copy_from_user(). */
- ASSERT_EQ(-1, landlock_create_ruleset(ruleset_attr, 0, 0));
- /* The size if less than sizeof(struct landlock_attr_enforce). */
- ASSERT_EQ(EINVAL, errno);
- ASSERT_EQ(-1, landlock_create_ruleset(ruleset_attr, 1, 0));
- ASSERT_EQ(EINVAL, errno);
Almost everywhere you're using ASSERT instead of EXPECT. Is this correct (in the sense than as soon as an ASSERT fails the rest of the test is skipped)? I do see you using EXPECT is some places, but I figured I'd ask about the intention here.
I intentionally use ASSERT as much as possible, but I use EXPECT when an error could block a test or when it could stop a cleanup (i.e. teardown).
Okay. Does the test suite run sanely when landlock is missing from the kernel?
When Landlock is disabled, the tests fail but do not hang.
+/*
- TEST_F_FORK() is useful when a test drop privileges but the corresponding
- FIXTURE_TEARDOWN() requires them (e.g. to remove files from a directory
- where write actions are denied). For convenience, FIXTURE_TEARDOWN() is
- also called when the test failed, but not when FIXTURE_SETUP() failed. For
- this to be possible, we must not call abort() but instead exit smoothly
- (hence the step print).
- */
Hm, interesting. I think this should be extracted into a separate patch and added to the test harness proper.
I agree, but it may require some modifications to fit nicely in kselftest_harness.h . For now, it works well for my use case. I'll send patches once Landlock is merged. In fact, I already made kselftest_harness.h available for other users than seccomp. ;)
Fair points.
Could this be solved with TEARDOWN being called on SETUP failure?
The goal of this helper is to still be able to call TEARDOWN when TEST failed, not SETUP.
+#define TEST_F_FORK(fixture_name, test_name) \
- static void fixture_name##_##test_name##_child( \
struct __test_metadata *_metadata, \FIXTURE_DATA(fixture_name) *self, \const FIXTURE_VARIANT(fixture_name) *variant); \- TEST_F(fixture_name, test_name) \
- { \
int status; \const pid_t child = fork(); \if (child < 0) \abort(); \if (child == 0) { \_metadata->no_print = 1; \fixture_name##_##test_name##_child(_metadata, self, variant); \if (_metadata->skip) \_exit(255); \if (_metadata->passed) \_exit(0); \_exit(_metadata->step); \} \if (child != waitpid(child, &status, 0)) \abort(); \if (WIFSIGNALED(status) || !WIFEXITED(status)) { \_metadata->passed = 0; \_metadata->step = 1; \return; \} \switch (WEXITSTATUS(status)) { \case 0: \_metadata->passed = 1; \break; \case 255: \_metadata->passed = 1; \_metadata->skip = 1; \break; \default: \_metadata->passed = 0; \_metadata->step = WEXITSTATUS(status); \break; \} \- } \
This looks like a subset of __wait_for_test()? Could __TEST_F_IMPL() be updated instead to do this? (Though the fork overhead might not be great for everyone.)
Yes, it will probably be my approach to update kselftest_harness.h .
It seems like this would be named better as TEST_DROPS_PRIVS or something, which describes the reason for the fork.
Yeah, maybe, we could discuss about that in a dedicated patch series. :)
linux-kselftest-mirror@lists.linaro.org
-
Kees Cook -
Mickaël Salaün