On 19/03/2021 20:11, Kees Cook wrote:

On Fri, Mar 19, 2021 at 07:41:00PM +0100, Mickaël Salaün wrote:

quoted

On 19/03/2021 18:56, Kees Cook wrote:

quoted

On Tue, Mar 16, 2021 at 09:42:50PM +0100, Mickaël Salaün wrote:

quoted

From: Mickaël Salaün <redacted>

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 <redacted>
Cc: Serge E. Hallyn <serge@hallyn.com>
Cc: Shuah Khan <shuah@kernel.org>
Signed-off-by: Mickaël Salaün <redacted>
Reviewed-by: Vincent Dagonneau <redacted>
Link: https://lore.kernel.org/r/20210316204252.427806-11-mic@digikod.net (local)

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

quoted

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.

quoted

quoted

[...]
+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.

quoted

quoted

quoted

+/*
+ * 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.

quoted

quoted

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.

quoted

quoted

+#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. :)