Message-Id: <20230814210508.never.871-kees@kernel.org>
Date:   Mon, 14 Aug 2023 14:05:12 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Brendan Higgins <brendan.higgins@...ux.dev>
Cc:     Kees Cook <keescook@...omium.org>, David Gow <davidgow@...gle.com>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Petr Skocik <pskocik@...il.com>,
        Oleg Nesterov <oleg@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Jens Axboe <axboe@...nel.dk>, linux-kselftest@...r.kernel.org,
        kunit-dev@...glegroups.com,
        Frederic Weisbecker <frederic@...nel.org>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        Mike Christie <michael.christie@...cle.com>,
        Marco Elver <elver@...gle.com>,
        Luis Chamberlain <mcgrof@...nel.org>,
        Al Viro <viro@...iv.linux.org.uk>,
        "haifeng.xu" <haifeng.xu@...pee.com>,
        Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: [PATCH] [RFC] signal: Add KUnit tests

This is a continuation of the proposal[1] for mocking init_task for
KUnit testing. Changing the behavior of kill_something_info() is moving
forward[2] and I'd _really_ like to have some unit tests in place to
actually test the behavioral changes.

I tried to incorporate feedback from Daniel and David, and I think the
result is fairly workable -- the only tricky part is building valid
task_struct instances. :)

Notably, I haven't actually gotten as far as testing the actual proposed
behavioral change since I wanted to make sure this approach wasn't going
to totally crash and burn.

Thoughts?

[1] https://lore.kernel.org/all/202212012008.D6F6109@keescook/
[2] https://lore.kernel.org/all/87jzu12pjh.fsf_-_@email.froward.int.ebiederm.org

Cc: Brendan Higgins <brendan.higgins@...ux.dev>
Cc: David Gow <davidgow@...gle.com>
Cc: "Eric W. Biederman" <ebiederm@...ssion.com>
Cc: Petr Skocik <pskocik@...il.com>
Cc: Oleg Nesterov <oleg@...hat.com>
Cc: Peter Zijlstra <peterz@...radead.org>
Cc: Thomas Gleixner <tglx@...utronix.de>
Cc: Jens Axboe <axboe@...nel.dk>
Cc: linux-kselftest@...r.kernel.org
Cc: kunit-dev@...glegroups.com
Signed-off-by: Kees Cook <keescook@...omium.org>
---
 include/kunit/resource.h     |  15 ++++
 include/linux/sched/signal.h |  11 ++-
 kernel/signal.c              | 135 +++++++++++++++++++++++++++++++++++
 3 files changed, 158 insertions(+), 3 deletions(-)

diff --git a/include/kunit/resource.h b/include/kunit/resource.h
index c7383e90f5c9..dbf84a58f7a6 100644
--- a/include/kunit/resource.h
+++ b/include/kunit/resource.h
@@ -479,4 +479,19 @@ void kunit_remove_action(struct kunit *test,
 void kunit_release_action(struct kunit *test,
 			  kunit_action_t *action,
 			  void *ctx);
+
+#define kunit_get_mock_pointer(name, actual) ({			\
+	typeof(*(actual)) *ptr = actual;			\
+	struct kunit_resource *resource;			\
+								\
+	if (kunit_get_current_test()) {				\
+		resource = kunit_find_named_resource(current->kunit_test, name); \
+		if (resource) {					\
+			ptr = resource->data;			\
+			kunit_put_resource(resource);		\
+		}						\
+	}							\
+	ptr;							\
+})
+
 #endif /* _KUNIT_RESOURCE_H */
diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
index 669e8cff40c7..700271f43491 100644
--- a/include/linux/sched/signal.h
+++ b/include/linux/sched/signal.h
@@ -637,14 +637,19 @@ static inline unsigned long sigsp(unsigned long sp, struct ksignal *ksig)
 extern void __cleanup_sighand(struct sighand_struct *);
 extern void flush_itimer_signals(void);
 
+/* This is used for KUnit mocking. */
+#ifndef __init_task_ptr
+#define __init_task_ptr	(&init_task)
+#endif
+
 #define tasklist_empty() \
-	list_empty(&init_task.tasks)
+	list_empty(&__init_task_ptr->tasks)
 
 #define next_task(p) \
 	list_entry_rcu((p)->tasks.next, struct task_struct, tasks)
 
 #define for_each_process(p) \
-	for (p = &init_task ; (p = next_task(p)) != &init_task ; )
+	for (p = __init_task_ptr ; (p = next_task(p)) != __init_task_ptr ; )
 
 extern bool current_is_single_threaded(void);
 
@@ -653,7 +658,7 @@ extern bool current_is_single_threaded(void);
  *          'break' will not work as expected - use goto instead.
  */
 #define do_each_thread(g, t) \
-	for (g = t = &init_task ; (g = t = next_task(g)) != &init_task ; ) do
+	for (g = t = __init_task_ptr ; (g = t = next_task(g)) != __init_task_ptr ; ) do
 
 #define while_each_thread(g, t) \
 	while ((t = next_thread(t)) != g)
diff --git a/kernel/signal.c b/kernel/signal.c
index b5370fe5c198..7607d302ebb9 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -11,6 +11,14 @@
  *		to allow signals to be sent reliably.
  */
 
+#if IS_ENABLED(CONFIG_KUNIT)
+/* This must be defined before we include include/linux/sched/signal.h */
+#define __init_task_ptr kunit_get_mock_pointer("mock_init_task", &init_task)
+
+#include <kunit/resource.h>
+#include <kunit/test-bug.h>
+#endif
+
 #include <linux/slab.h>
 #include <linux/export.h>
 #include <linux/init.h>
@@ -4842,3 +4850,130 @@ void kdb_send_sig(struct task_struct *t, int sig)
 		kdb_printf("Signal %d is sent to process %d.\n", sig, t->pid);
 }
 #endif	/* CONFIG_KGDB_KDB */
+
+#if IS_ENABLED(CONFIG_KUNIT)
+static void test_empty_task_list(struct kunit *test)
+{
+	struct kunit_resource resource;
+	static struct task_struct empty_task_list = {
+			.tasks	= LIST_HEAD_INIT(empty_task_list.tasks),
+		};
+	struct task_struct *p;
+	int count = 0;
+
+	kunit_add_named_resource(test, NULL, NULL, &resource,
+				 "mock_init_task", &empty_task_list);
+
+	KUNIT_EXPECT_TRUE(test, tasklist_empty());
+
+	for_each_process(p)
+		count++;
+
+	/* System hangs without this... */
+	kunit_remove_resource(test, &resource);
+
+	KUNIT_EXPECT_EQ(test, count, 0);
+}
+
+static void test_for_each_process(struct kunit *test)
+{
+	struct kunit_resource resource;
+	static struct task_struct task1 = {
+			.pid = 1,
+			.tasks	= LIST_HEAD_INIT(task1.tasks),
+		};
+	static struct task_struct task2 = {
+			.pid = 2,
+		}, task3 = {
+			.pid = 3,
+		};
+	struct task_struct *p;
+	int count = 0;
+
+	list_add(&task2.tasks, &task1.tasks);
+	list_add(&task3.tasks, &task1.tasks);
+
+	kunit_add_named_resource(test, NULL, NULL, &resource,
+				 "mock_init_task", &task1);
+
+	/* Walk the process list backwards. */
+	for_each_process(p) {
+		KUNIT_EXPECT_EQ(test, 3 - count, p->pid);
+		count++;
+	}
+
+	/* System hangs without this... */
+	kunit_remove_resource(test, &resource);
+
+	/* init_task isn't counted... */
+	KUNIT_EXPECT_EQ(test, count, 2);
+}
+
+static void test_kill_something_info(struct kunit *test)
+{
+	struct kunit_resource resource;
+	static struct task_struct task1 = {
+			.pid = 1,
+			.tasks	= LIST_HEAD_INIT(task1.tasks),
+		};
+	static struct task_struct task2 = {
+			.pid = 2,
+		}, task3 = {
+			.pid = 3,
+		};
+	struct kernel_siginfo siginfo = {
+			.si_code = SI_KERNEL,
+		};
+	struct task_struct *p;
+	int count = 0;
+
+	list_add(&task2.tasks, &task1.tasks);
+	list_add(&task3.tasks, &task1.tasks);
+
+	kunit_add_named_resource(test, NULL, NULL, &resource,
+				 "mock_init_task", &task1);
+
+	/* Make sure we have a process list. */
+	for_each_process(p)
+		count++;
+	KUNIT_EXPECT_EQ(test, count, 2);
+
+	/* INT_MIN pid must return ESRCH */
+	KUNIT_EXPECT_EQ(test, -ESRCH,
+		kill_something_info(SIGHUP, SEND_SIG_NOINFO, INT_MIN));
+
+	/* Invalid signal: EINVAL */
+	KUNIT_EXPECT_EQ(test, -EINVAL,
+		kill_something_info(_NSIG + 1, SEND_SIG_NOINFO, 2));
+
+	/* Missing pid: ESRCH */
+	KUNIT_EXPECT_EQ(test, -ESRCH,
+		kill_something_info(SIGHUP, SEND_SIG_NOINFO, 42));
+
+	/* Bypass permission checks with SEND_SIG_NOINFO. */
+	KUNIT_EXPECT_EQ(test, 0,
+		kill_something_info(SIGHUP, SEND_SIG_NOINFO, 2));
+
+	/* XXX: Hm, I was expecting this to explode in cred deref... */
+	KUNIT_EXPECT_EQ(test, 0,
+		kill_something_info(SIGHUP, &siginfo, 3));
+
+	/* XXX more tests here, perhaps after mocking out group_send_sig_info() ... */
+
+	/* System hangs without this... */
+	kunit_remove_resource(test, &resource);
+}
+
+static struct kunit_case test_cases[] = {
+	KUNIT_CASE(test_empty_task_list),
+	KUNIT_CASE(test_for_each_process),
+	KUNIT_CASE(test_kill_something_info),
+	{}
+};
+
+static struct kunit_suite test_suite = {
+	.name = "signal",
+	.test_cases = test_cases,
+};
+kunit_test_suite(test_suite);
+#endif	/* CONFIG_KUNIT */
-- 
2.34.1

Powered by blists - more mailing lists