[PATCH] livepatch: Fix races between stack check and module load/unload

[Andrea Parri <parri.andrea@gmail.com>]

Unloading a patched module while a livepatch transition is in progress
can crash the kernel in klp_find_ops(), called from the scheduler.
KCSAN reports

  BUG: KCSAN: data-race in klp_find_ops / klp_unpatch_func

  write (marked) to 0xffff9094c09ece10 of 8 bytes by task 381 on cpu 2:
   klp_unpatch_func+0x1bd/0x280
   klp_unpatch_object+0x6a/0x90
   klp_cleanup_module_patches_limited.cold+0x4a/0xd4
   klp_module_going+0x54/0x80
   __do_sys_delete_module+0x247/0x380
   __x64_sys_delete_module+0x39/0x50
   ...

  read to 0xffff9094c09ece10 of 8 bytes by task 146 on cpu 1:
   klp_find_ops+0x55/0x90
   klp_check_and_switch_task+0x228/0x320
   klp_try_switch_task+0xb4/0xc0
   __klp_sched_try_switch+0x9a/0xb0
   __schedule+0x60a/0x1450
   schedule+0x3b/0x100
   schedule_timeout+0xc0/0x160
   ra_target_a_loop+0x97/0xa0 [klp_race_target_a]
   ...

followed by

  Oops: general protection fault, probably for non-canonical address
  0xdead000000000110: 0000 [#2] SMP NOPTI
  RIP: 0010:klp_find_ops+0x55/0x90
   ...
   klp_check_and_switch_task+0x228/0x320
   klp_try_switch_task+0xb4/0xc0
   __klp_sched_try_switch+0x9a/0xb0
   __schedule+0x60a/0x1450

0xdead000000000110 is LIST_POISON1 (0xdead000000000100) plus the offset
of func_stack in struct klp_ops.

klp_check_stack_func() walks the global klp_ops list in klp_find_ops().
It is reached from the scheduler through __klp_sched_try_switch() ->
klp_try_switch_task() -> klp_check_stack() without klp_mutex, while
klp_patch_func() and klp_unpatch_func() add, remove and free klp_ops
entries under klp_mutex when a to-be-patched module is loaded or
unloaded:

  CPU 1                                 CPU 2
  __schedule()                          delete_module()
    klp_check_stack_func()                klp_module_going()
      klp_find_ops()                        klp_unpatch_func()
        pos = ops
                                              list_del(&ops->node)
                                              kfree(ops)
        pos = ops->node.next  /* LIST_POISON1 */
        pos->func_stack.next  /* oops */

Fix it by RCU protecting the klp_ops list.  klp_find_ops() uses
list_for_each_entry_rcu() and list_first_or_null_rcu(), klp_patch_func()
publishes a new ops with list_add_rcu() only after its func_stack has
been initialized, and klp_unpatch_func() and the klp_patch_func() error
path free it with kfree_rcu() after list_del_rcu().

klp_check_stack() takes the RCU read lock around the walk to make the
dependency explicit.  It already runs with preemption disabled and with
RCU watching, so the grace period used by kfree_rcu() covers the reader.

If klp_find_ops() no longer finds the ops, the function is not
redirected anymore and klp_check_stack_func() has nothing to check.

Unload also unpatches the transition patch, so a found func_stack can
shrink, even unlinking func itself, and list_is_singular() and
list_next_entry() then pick a bogus klp_func.  Use
list_next_or_null_rcu() and treat NULL as the original function.

klp_free_object_loaded() then clears func->old_func, and new_func of NOP
functions, without waiting for readers.  Mark these accesses with
READ_ONCE() and WRITE_ONCE().  A NULL new_func matches no stack entry,
and a NULL old_func is skipped, as the lookup could otherwise match
another removed entry.

A reload during the transition needs ordering too.
func->stack_node.next is read through func, not through the ops found
under RCU, so a task that sees obj->patched set can read a stale value:
func's own node from INIT_LIST_HEAD() on the first load, or what
list_del_rcu() left at the last unload, such as a freed ops head.

The first makes klp_check_stack_func() check func->new_func instead of
the original function, so a task sleeping in the original function
could be switched.  The second can read new_func and new_size from
freed memory.  Publish obj->patched with smp_store_release() in
klp_patch_object() and read it with smp_load_acquire() in
klp_check_stack().

Clearing obj->patched needs no ordering, since a task that still sees
it set is handled by the RCU lookup, and the synchronize_rcu() in the
unload path waits for the walk before the module can be loaded again.
patched_show() reads obj->patched for sysfs output only, so mark that
read with data_race().

The load-side ordering was found by code inspection and has not been
reproduced: the reproducer runs on x86, where neither the CPU nor, given
the barriers in between, the compiler reorders these loads.

Fixes: e3ff7c609f39 ("livepatch,sched: Add livepatch task switching to cond_resched()")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Andrea Parri <parri.andrea@gmail.com>
---
Tested on x86_64 in a 4-CPU vng guest with CONFIG_KCSAN_STRICT=y, with
and without CONFIG_PROVE_LOCKING=y and CONFIG_PROVE_RCU_LIST=y.  A
livepatch transition is kept in progress while a patched module is
unloaded and reloaded for 30 minutes (about 370 reloads).  No KCSAN
report involves livepatch code, and lockdep and RCU stay silent.  On a
kernel without this patch, the same reproducer faults in klp_find_ops()
as shown above.

 kernel/livepatch/core.c       |  8 +++++---
 kernel/livepatch/patch.c      | 33 ++++++++++++++++++++------------
 kernel/livepatch/patch.h      |  2 ++
 kernel/livepatch/transition.c | 36 +++++++++++++++++++++++++++--------
 4 files changed, 56 insertions(+), 23 deletions(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index a240d1144e89c..ccae5cc5169b6 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -499,7 +499,8 @@ static ssize_t patched_show(struct kobject *kobj,
 	struct klp_object *obj;
 
 	obj = container_of(kobj, struct klp_object, kobj);
-	return sysfs_emit(buf, "%d\n", obj->patched);
+	/* Racy without klp_mutex, but only reported to user space. */
+	return sysfs_emit(buf, "%d\n", data_race(obj->patched));
 }
 
 static struct kobj_attribute patched_kobj_attr = __ATTR_RO(patched);
@@ -696,10 +697,11 @@ static void klp_free_object_loaded(struct klp_object *obj)
 	obj->mod = NULL;
 
 	klp_for_each_func(obj, func) {
-		func->old_func = NULL;
+		/* Read without klp_mutex, see klp_check_stack_func(). */
+		WRITE_ONCE(func->old_func, NULL);
 
 		if (func->nop)
-			func->new_func = NULL;
+			WRITE_ONCE(func->new_func, NULL);
 	}
 }
 
diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
index 3f54a017bbf69..d01ec7a6256ab 100644
--- a/kernel/livepatch/patch.c
+++ b/kernel/livepatch/patch.c
@@ -22,15 +22,19 @@
 
 static LIST_HEAD(klp_ops);
 
+/*
+ * Caller must hold klp_mutex or rcu_read_lock(): klp_check_stack_func() walks
+ * klp_ops from the scheduler without klp_mutex.
+ */
 struct klp_ops *klp_find_ops(void *old_func)
 {
 	struct klp_ops *ops;
 	struct klp_func *func;
 
-	list_for_each_entry(ops, &klp_ops, node) {
-		func = list_first_entry(&ops->func_stack, struct klp_func,
-					stack_node);
-		if (func->old_func == old_func)
+	list_for_each_entry_rcu(ops, &klp_ops, node, lockdep_is_held(&klp_mutex)) {
+		func = list_first_or_null_rcu(&ops->func_stack, struct klp_func,
+					      stack_node);
+		if (func && READ_ONCE(func->old_func) == old_func)
 			return ops;
 	}
 
@@ -148,8 +152,8 @@ static void klp_unpatch_func(struct klp_func *func)
 		WARN_ON(ftrace_set_filter_ip(&ops->fops, ftrace_loc, 1, 0));
 
 		list_del_rcu(&func->stack_node);
-		list_del(&ops->node);
-		kfree(ops);
+		list_del_rcu(&ops->node);
+		kfree_rcu(ops, rcu_head);
 	} else {
 		list_del_rcu(&func->stack_node);
 	}
@@ -191,10 +195,9 @@ static int klp_patch_func(struct klp_func *func)
 				  FTRACE_OPS_FL_IPMODIFY |
 				  FTRACE_OPS_FL_PERMANENT;
 
-		list_add(&ops->node, &klp_ops);
-
 		INIT_LIST_HEAD(&ops->func_stack);
 		list_add_rcu(&func->stack_node, &ops->func_stack);
+		list_add_rcu(&ops->node, &klp_ops);
 
 		ret = ftrace_set_filter_ip(&ops->fops, ftrace_loc, 0, 0);
 		if (ret) {
@@ -222,8 +225,8 @@ static int klp_patch_func(struct klp_func *func)
 
 err:
 	list_del_rcu(&func->stack_node);
-	list_del(&ops->node);
-	kfree(ops);
+	list_del_rcu(&ops->node);
+	kfree_rcu(ops, rcu_head);
 	return ret;
 }
 
@@ -240,7 +243,7 @@ static void __klp_unpatch_object(struct klp_object *obj, bool nops_only)
 	}
 
 	if (obj->dynamic || !nops_only)
-		obj->patched = false;
+		WRITE_ONCE(obj->patched, false);
 }
 
 
@@ -264,7 +267,13 @@ int klp_patch_object(struct klp_object *obj)
 			return ret;
 		}
 	}
-	obj->patched = true;
+
+	/*
+	 * Pairs with smp_load_acquire() in klp_check_stack(): a task that sees
+	 * the object patched also sees the func_stack linkage and the
+	 * old_func/new_func addresses and sizes of all its funcs.
+	 */
+	smp_store_release(&obj->patched, true);
 
 	return 0;
 }
diff --git a/kernel/livepatch/patch.h b/kernel/livepatch/patch.h
index d5f2fbe373e05..20e8704daca53 100644
--- a/kernel/livepatch/patch.h
+++ b/kernel/livepatch/patch.h
@@ -17,11 +17,13 @@
  *
  * @node:	node for the global klp_ops list
  * @func_stack:	list head for the stack of klp_func's (active func is on top)
+ * @rcu_head:	used to free this structure with kfree_rcu()
  * @fops:	registered ftrace ops struct
  */
 struct klp_ops {
 	struct list_head node;
 	struct list_head func_stack;
+	struct rcu_head rcu_head;
 	struct ftrace_ops fops;
 };
 
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index 2351a19ac2a9e..e9aa16baf1743 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -8,6 +8,7 @@
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/cpu.h>
+#include <linux/rculist.h>
 #include <linux/stacktrace.h>
 #include <linux/static_call.h>
 #include "core.h"
@@ -201,12 +202,16 @@ void klp_update_patch_state(struct task_struct *task)
 /*
  * Determine whether the given stack trace includes any references to a
  * to-be-patched or to-be-unpatched function.
+ *
+ * Must be called under rcu_read_lock(), see klp_check_stack().
  */
 static int klp_check_stack_func(struct klp_func *func, unsigned long *entries,
 				unsigned int nr_entries)
 {
 	unsigned long func_addr, func_size, address;
+	struct klp_func *prev;
 	struct klp_ops *ops;
+	void *old_func;
 	int i;
 
 	if (klp_target_state == KLP_TRANSITION_UNPATCHED) {
@@ -214,24 +219,35 @@ static int klp_check_stack_func(struct klp_func *func, unsigned long *entries,
 		  * Check for the to-be-unpatched function
 		  * (the func itself).
 		  */
-		func_addr = (unsigned long)func->new_func;
+		func_addr = (unsigned long)READ_ONCE(func->new_func);
 		func_size = func->new_size;
 	} else {
 		/*
 		 * Check for the to-be-patched function
 		 * (the previous func).
 		 */
-		ops = klp_find_ops(func->old_func);
+		old_func = READ_ONCE(func->old_func);
+		if (!old_func)
+			return 0;
+
+		ops = klp_find_ops(old_func);
 
-		if (list_is_singular(&ops->func_stack)) {
+		/*
+		 * The ops might have just been removed by a concurrent module
+		 * unload.  The function is not redirected anymore in that case.
+		 */
+		if (!ops)
+			return 0;
+
+		/* func_stack may shrink under us, even unlinking func itself. */
+		prev = list_next_or_null_rcu(&ops->func_stack, &func->stack_node,
+					     struct klp_func, stack_node);
+		if (!prev) {
 			/* original function */
-			func_addr = (unsigned long)func->old_func;
+			func_addr = (unsigned long)old_func;
 			func_size = func->old_size;
 		} else {
 			/* previously patched function */
-			struct klp_func *prev;
-
-			prev = list_next_entry(func, stack_node);
 			func_addr = (unsigned long)prev->new_func;
 			func_size = prev->new_size;
 		}
@@ -266,8 +282,12 @@ static int klp_check_stack(struct task_struct *task, const char **oldname)
 		return -EINVAL;
 	nr_entries = ret;
 
+	/* A concurrent module unload may free klp_ops, see klp_find_ops(). */
+	guard(rcu)();
+
 	klp_for_each_object(klp_transition_patch, obj) {
-		if (!obj->patched)
+		/* Pairs with smp_store_release() in klp_patch_object(). */
+		if (!smp_load_acquire(&obj->patched))
 			continue;
 		klp_for_each_func(obj, func) {
 			ret = klp_check_stack_func(func, entries, nr_entries);

base-commit: 24a19b953e2a3d0ea5a8c4951a0ab8f32720c907
-- 
2.53.0