Message-Id: <aafde1ef5f1177cc362913abf93d99062ba52e09.1515786085.git.jbaron@akamai.com>
Date:   Fri, 12 Jan 2018 14:55:16 -0500
From:   Jason Baron <jbaron@...mai.com>
To:     linux-kernel@...r.kernel.org, live-patching@...r.kernel.org
Cc:     jpoimboe@...hat.com, jeyu@...nel.org, jikos@...nel.org,
        mbenes@...e.cz, pmladek@...e.com
Subject: [PATCH v5 3/3] livepatch: add atomic replace

Sometimes we would like to revert a particular fix. Currently, this
is not easy because we want to keep all other fixes active and we
could revert only the last applied patch.

One solution would be to apply new patch that implemented all
the reverted functions like in the original code. It would work
as expected but there will be unnecessary redirections. In addition,
it would also require knowing which functions need to be reverted at
build time.

A better solution would be to say that a new patch replaces
an older one. This might be complicated if we want to replace
a particular patch. But it is rather easy when a new cummulative
patch replaces all others.

Add a new "replace" flag to struct klp_patch. When it is enabled, a set of
'nop' klp_func will be dynamically created for all functions that are
already being patched but that will not longer be modified by the new
patch. They are temporarily used as a new target during the patch
transition.

Since 'atomic replace' has completely replaced all previous livepatch
modules, it explicitly disables all previous livepatch modules. However,
previous livepatch modules that have been replaced, can be re-enabled
by removing them (rmmod) and then re-inserting them (insmod).

Signed-off-by: Jason Baron <jbaron@...mai.com>
Cc: Josh Poimboeuf <jpoimboe@...hat.com>
Cc: Jessica Yu <jeyu@...nel.org>
Cc: Jiri Kosina <jikos@...nel.org>
Cc: Miroslav Benes <mbenes@...e.cz>
Cc: Petr Mladek <pmladek@...e.com>
---
 include/linux/livepatch.h     |   6 +-
 kernel/livepatch/core.c       | 280 ++++++++++++++++++++++++++++++++++++++++--
 kernel/livepatch/core.h       |   6 +
 kernel/livepatch/patch.c      |  22 +++-
 kernel/livepatch/patch.h      |   4 +-
 kernel/livepatch/transition.c |  49 +++++++-
 6 files changed, 345 insertions(+), 22 deletions(-)

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index e5db2ba..2a4a0db 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -49,6 +49,7 @@
  * @new_size:	size of the new function
  * @patched:	the func has been added to the klp_ops list
  * @transition:	the func is currently being applied or reverted
+ * @nop:	used to revert a previously patched function
  *
  * The patched and transition variables define the func's patching state.  When
  * patching, a func is always in one of the following states:
@@ -86,6 +87,7 @@ struct klp_func {
 	unsigned long old_size, new_size;
 	bool patched;
 	bool transition;
+	bool nop;
 };
 
 struct klp_object;
@@ -142,6 +144,7 @@ struct klp_object {
  * struct klp_patch - patch structure for live patching
  * @mod:	reference to the live patch module
  * @objs:	object entries for kernel objects to be patched
+ * @replace:	revert previously patched functions not included in the patch
  * @list:	list node for global list of registered patches
  * @kobj:	kobject for sysfs resources
  * @obj_list:	head of list for struct klp_object
@@ -152,6 +155,7 @@ struct klp_patch {
 	/* external */
 	struct module *mod;
 	struct klp_object *objs;
+	bool replace;
 
 	/* internal */
 	struct list_head list;
@@ -169,7 +173,7 @@ struct klp_patch {
 
 #define klp_for_each_func_static(obj, func) \
 	for (func = obj->funcs; \
-	     func->old_name || func->new_func || func->old_sympos; \
+	     func && (func->old_name || func->new_func || func->old_sympos); \
 	     func++)
 
 #define klp_for_each_func(obj, func)	\
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 5f1de35..776b825 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -45,7 +45,14 @@
  */
 DEFINE_MUTEX(klp_mutex);
 
-static LIST_HEAD(klp_patches);
+LIST_HEAD(klp_patches);
+
+/*
+ * List of 'replaced' patches that have been replaced by a patch that has the
+ * 'replace' bit set. When they are added to this list, they are disabled and
+ * can not be re-enabled, but they can be unregistered().
+ */
+LIST_HEAD(klp_replaced_patches);
 
 static struct kobject *klp_root_kobj;
 
@@ -82,17 +89,28 @@ static void klp_find_object_module(struct klp_object *obj)
 	mutex_unlock(&module_mutex);
 }
 
-static bool klp_is_patch_registered(struct klp_patch *patch)
+static bool klp_is_patch_in_list(struct klp_patch *patch,
+				 struct list_head *head)
 {
 	struct klp_patch *mypatch;
 
-	list_for_each_entry(mypatch, &klp_patches, list)
+	list_for_each_entry(mypatch, head, list)
 		if (mypatch == patch)
 			return true;
 
 	return false;
 }
 
+static bool klp_is_patch_registered(struct klp_patch *patch)
+{
+	return klp_is_patch_in_list(patch, &klp_patches);
+}
+
+static bool klp_is_patch_replaced(struct klp_patch *patch)
+{
+	return klp_is_patch_in_list(patch, &klp_replaced_patches);
+}
+
 static bool klp_initialized(void)
 {
 	return !!klp_root_kobj;
@@ -278,8 +296,21 @@ static int klp_write_object_relocations(struct module *pmod,
 	return ret;
 }
 
+atomic_t klp_nop_release_count;
+static DECLARE_WAIT_QUEUE_HEAD(klp_nop_release_wait);
+
 static void klp_kobj_release_object(struct kobject *kobj)
 {
+	struct klp_object *obj;
+
+	obj = container_of(kobj, struct klp_object, kobj);
+	/* Free dynamically allocated object */
+	if (!obj->funcs) {
+		kfree(obj->name);
+		kfree(obj);
+		atomic_dec(&klp_nop_release_count);
+		wake_up(&klp_nop_release_wait);
+	}
 }
 
 static struct kobj_type klp_ktype_object = {
@@ -289,6 +320,16 @@ static struct kobj_type klp_ktype_object = {
 
 static void klp_kobj_release_func(struct kobject *kobj)
 {
+	struct klp_func *func;
+
+	func = container_of(kobj, struct klp_func, kobj);
+	/* Free dynamically allocated functions */
+	if (!func->new_func) {
+		kfree(func->old_name);
+		kfree(func);
+		atomic_dec(&klp_nop_release_count);
+		wake_up(&klp_nop_release_wait);
+	}
 }
 
 static struct kobj_type klp_ktype_func = {
@@ -344,7 +385,7 @@ static void klp_free_patch(struct klp_patch *patch)
 
 static int klp_init_func(struct klp_object *obj, struct klp_func *func)
 {
-	if (!func->old_name || !func->new_func)
+	if (!func->old_name || (!func->nop && !func->new_func))
 		return -EINVAL;
 
 	INIT_LIST_HEAD(&func->stack_node);
@@ -400,6 +441,10 @@ static int klp_init_object_loaded(struct klp_patch *patch,
 			return -ENOENT;
 		}
 
+		/* nop funcs don't have a new_func */
+		if (func->nop)
+			continue;
+
 		ret = kallsyms_lookup_size_offset((unsigned long)func->new_func,
 						  &func->new_size, NULL);
 		if (!ret) {
@@ -412,13 +457,14 @@ static int klp_init_object_loaded(struct klp_patch *patch,
 	return 0;
 }
 
-static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
+static int klp_init_object(struct klp_patch *patch, struct klp_object *obj,
+			   bool nop)
 {
 	struct klp_func *func;
 	int ret;
 	const char *name;
 
-	if (!obj->funcs)
+	if (!obj->funcs && !nop)
 		return -EINVAL;
 
 	obj->patched = false;
@@ -454,6 +500,216 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
 	return ret;
 }
 
+static struct klp_func *klp_find_func(struct klp_object *obj,
+				      struct klp_func *old_func)
+{
+	struct klp_func *func;
+
+	klp_for_each_func(obj, func) {
+		if ((strcmp(old_func->old_name, func->old_name) == 0) &&
+		    (old_func->old_sympos == func->old_sympos)) {
+			return func;
+		}
+	}
+
+	return NULL;
+}
+
+static struct klp_object *klp_find_object(struct klp_patch *patch,
+					  struct klp_object *old_obj)
+{
+	struct klp_object *obj;
+	bool mod = klp_is_module(old_obj);
+
+	klp_for_each_object(patch, obj) {
+		if (mod) {
+			if (klp_is_module(obj) &&
+			    strcmp(old_obj->name, obj->name) == 0) {
+				return obj;
+			}
+		} else if (!klp_is_module(obj)) {
+			return obj;
+		}
+	}
+
+	return NULL;
+}
+
+static struct klp_func *klp_alloc_nop_func(struct klp_func *old_func,
+					   struct klp_object *obj)
+{
+	struct klp_func *func;
+	int err;
+
+	func = kzalloc(sizeof(*func), GFP_KERNEL);
+	if (!func)
+		return ERR_PTR(-ENOMEM);
+
+	INIT_LIST_HEAD(&func->func_entry);
+	if (old_func->old_name) {
+		func->old_name = kstrdup(old_func->old_name, GFP_KERNEL);
+		if (!func->old_name) {
+			kfree(func);
+			return ERR_PTR(-ENOMEM);
+		}
+	}
+	func->old_sympos = old_func->old_sympos;
+	func->nop = true;
+	err = klp_init_func(obj, func);
+	if (err) {
+		if (!list_empty(&func->func_entry))
+			list_del(&func->func_entry);
+		kfree(func->old_name);
+		kfree(func);
+		return ERR_PTR(err);
+	}
+	func->old_addr = old_func->old_addr;
+	func->old_size = old_func->old_size;
+
+	return func;
+}
+
+static struct klp_object *klp_alloc_nop_object(const char *name,
+					       struct klp_patch *patch)
+{
+	struct klp_object *obj;
+	int err;
+
+	obj = kzalloc(sizeof(*obj), GFP_KERNEL);
+	if (!obj)
+		return ERR_PTR(-ENOMEM);
+
+	if (name) {
+		obj->name = kstrdup(name, GFP_KERNEL);
+		if (!obj->name) {
+			kfree(obj);
+			return ERR_PTR(-ENOMEM);
+		}
+	}
+	err = klp_init_object(patch, obj, true);
+	if (err) {
+		/* need to free here b/c may not have been added
+		 * to patch list
+		 */
+		if (!list_empty(&obj->obj_entry))
+			list_del(&obj->obj_entry);
+		kfree(obj->name);
+		kfree(obj);
+		return ERR_PTR(err);
+	}
+
+	return obj;
+}
+
+static int klp_add_nop_func(struct klp_object *obj,
+			    struct klp_func *old_func)
+{
+	struct klp_func *func;
+
+	func = klp_find_func(obj, old_func);
+	if (func)
+		return 0;
+	func = klp_alloc_nop_func(old_func, obj);
+	if (IS_ERR(func))
+		return PTR_ERR(func);
+
+	return 0;
+}
+
+static int klp_add_nop_object(struct klp_patch *patch,
+			      struct klp_object *old_obj)
+{
+	struct klp_object *obj;
+	struct klp_func *old_func;
+	int err = 0;
+
+	obj = klp_find_object(patch, old_obj);
+	if (!obj) {
+		obj = klp_alloc_nop_object(old_obj->name, patch);
+
+		if (IS_ERR(obj))
+			return PTR_ERR(obj);
+	}
+	klp_for_each_func(old_obj, old_func) {
+		err = klp_add_nop_func(obj, old_func);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
+static bool klp_is_obj_dyn(struct klp_object *obj)
+{
+	struct klp_func *func;
+	bool ret = true;
+
+	klp_for_each_func(obj, func) {
+		if (!func->nop) {
+			ret = false;
+			break;
+		}
+	}
+	return ret;
+}
+
+void klp_remove_nops(struct klp_patch *patch)
+{
+	struct klp_object *obj, *tmp_obj;
+	struct klp_func *func, *tmp_func;
+
+	klp_for_each_object(patch, obj) {
+		list_for_each_entry_safe(func, tmp_func, &obj->func_list,
+					 func_entry) {
+			if (func->nop) {
+				list_del(&func->func_entry);
+				atomic_inc(&klp_nop_release_count);
+				kobject_put(&func->kobj);
+			}
+		}
+	}
+	list_for_each_entry_safe(obj, tmp_obj, &patch->obj_list,
+				 obj_entry) {
+		if (klp_is_obj_dyn(obj)) {
+			list_del(&obj->obj_entry);
+			atomic_inc(&klp_nop_release_count);
+			kobject_put(&obj->kobj);
+		}
+	}
+
+	wait_event(klp_nop_release_wait,
+		   atomic_read(&klp_nop_release_count) == 0);
+}
+
+/* Add 'nop' functions which simply return to the caller to run
+ * the original function. The 'nop' functions are added to a
+ * patch to facilitate a 'replace' mode
+ */
+static int klp_add_nops(struct klp_patch *patch)
+{
+	struct klp_patch *old_patch;
+	struct klp_object *old_obj;
+	int err = 0;
+
+	if (!patch->replace)
+		return 0;
+
+	list_for_each_entry(old_patch, &klp_patches, list) {
+		if (old_patch == patch)
+			break;
+
+		klp_for_each_object(old_patch, old_obj) {
+			err = klp_add_nop_object(patch, old_obj);
+			if (err) {
+				klp_remove_nops(patch);
+				return err;
+			}
+		}
+	}
+
+	return 0;
+}
+
 static int __klp_disable_patch(struct klp_patch *patch)
 {
 	struct klp_object *obj;
@@ -539,6 +795,11 @@ static int __klp_enable_patch(struct klp_patch *patch)
 	    !list_prev_entry(patch, list)->enabled)
 		return -EBUSY;
 
+	/* setup nops */
+	ret = klp_add_nops(patch);
+	if (ret)
+		return ret;
+
 	/*
 	 * A reference is taken on the patch module to prevent it from being
 	 * unloaded.
@@ -803,7 +1064,7 @@ static int klp_init_patch(struct klp_patch *patch)
 
 	INIT_LIST_HEAD(&patch->obj_list);
 	klp_for_each_object_static(patch, obj) {
-		ret = klp_init_object(patch, obj);
+		ret = klp_init_object(patch, obj, false);
 		if (ret)
 			goto free;
 	}
@@ -839,7 +1100,7 @@ int klp_unregister_patch(struct klp_patch *patch)
 
 	mutex_lock(&klp_mutex);
 
-	if (!klp_is_patch_registered(patch)) {
+	if (!klp_is_patch_registered(patch) && !klp_is_patch_replaced(patch)) {
 		ret = -EINVAL;
 		goto err;
 	}
@@ -849,6 +1110,7 @@ int klp_unregister_patch(struct klp_patch *patch)
 		goto err;
 	}
 
+	klp_remove_nops(patch);
 	klp_free_patch(patch);
 
 	mutex_unlock(&klp_mutex);
@@ -928,7 +1190,7 @@ static void klp_cleanup_module_patches_limited(struct module *mod,
 
 				pr_notice("reverting patch '%s' on unloading module '%s'\n",
 					  patch->mod->name, obj->mod->name);
-				klp_unpatch_object(obj);
+				klp_unpatch_object(obj, false);
 
 				klp_post_unpatch_callback(obj);
 			}
diff --git a/kernel/livepatch/core.h b/kernel/livepatch/core.h
index 48a83d4..49ffceb 100644
--- a/kernel/livepatch/core.h
+++ b/kernel/livepatch/core.h
@@ -4,7 +4,13 @@
 
 #include <linux/livepatch.h>
 
+#include <linux/livepatch.h>
+
 extern struct mutex klp_mutex;
+extern struct list_head klp_patches;
+extern struct list_head klp_replaced_patches;
+
+void klp_remove_nops(struct klp_patch *patch);
 
 static inline bool klp_is_object_loaded(struct klp_object *obj)
 {
diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
index 82d5842..9330c3f 100644
--- a/kernel/livepatch/patch.c
+++ b/kernel/livepatch/patch.c
@@ -118,6 +118,9 @@ static void notrace klp_ftrace_handler(unsigned long ip,
 		}
 	}
 
+	/* if its a 'nop', then run the original function */
+	if (func->nop)
+		goto unlock;
 	klp_arch_set_pc(regs, (unsigned long)func->new_func);
 unlock:
 	preempt_enable_notrace();
@@ -236,15 +239,21 @@ static int klp_patch_func(struct klp_func *func)
 	return ret;
 }
 
-void klp_unpatch_object(struct klp_object *obj)
+/* if nop is set, only unpatch the nop functions */
+void klp_unpatch_object(struct klp_object *obj, bool nop)
 {
 	struct klp_func *func;
 
-	klp_for_each_func(obj, func)
+	klp_for_each_func(obj, func) {
+		if (nop && !func->nop)
+			continue;
+
 		if (func->patched)
 			klp_unpatch_func(func);
+	}
 
-	obj->patched = false;
+	if (!nop)
+		obj->patched = false;
 }
 
 int klp_patch_object(struct klp_object *obj)
@@ -258,7 +267,7 @@ int klp_patch_object(struct klp_object *obj)
 	klp_for_each_func(obj, func) {
 		ret = klp_patch_func(func);
 		if (ret) {
-			klp_unpatch_object(obj);
+			klp_unpatch_object(obj, false);
 			return ret;
 		}
 	}
@@ -267,11 +276,12 @@ int klp_patch_object(struct klp_object *obj)
 	return 0;
 }
 
-void klp_unpatch_objects(struct klp_patch *patch)
+/* if nop is set, only unpatch the nop functions */
+void klp_unpatch_objects(struct klp_patch *patch, bool nop)
 {
 	struct klp_object *obj;
 
 	klp_for_each_object(patch, obj)
 		if (obj->patched)
-			klp_unpatch_object(obj);
+			klp_unpatch_object(obj, nop);
 }
diff --git a/kernel/livepatch/patch.h b/kernel/livepatch/patch.h
index e72d825..5608757 100644
--- a/kernel/livepatch/patch.h
+++ b/kernel/livepatch/patch.h
@@ -28,7 +28,7 @@ struct klp_ops {
 struct klp_ops *klp_find_ops(unsigned long old_addr);
 
 int klp_patch_object(struct klp_object *obj);
-void klp_unpatch_object(struct klp_object *obj);
-void klp_unpatch_objects(struct klp_patch *patch);
+void klp_unpatch_object(struct klp_object *obj, bool nop);
+void klp_unpatch_objects(struct klp_patch *patch, bool nop);
 
 #endif /* _LIVEPATCH_PATCH_H */
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index 7c6631e..b3991e4 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -81,18 +81,39 @@ static void klp_complete_transition(void)
 	struct klp_object *obj;
 	struct klp_func *func;
 	struct task_struct *g, *task;
+	struct klp_patch *old_patch, *tmp_patch;
 	unsigned int cpu;
 
 	pr_debug("'%s': completing %s transition\n",
 		 klp_transition_patch->mod->name,
 		 klp_target_state == KLP_PATCHED ? "patching" : "unpatching");
 
+	/*
+	 * for replace patches, we disable all previous patches, and replace
+	 * the dynamic no-op functions by removing the ftrace hook.
+	 */
+	if (klp_target_state == KLP_PATCHED && klp_transition_patch->replace) {
+		list_for_each_entry_safe(old_patch, tmp_patch, &klp_patches,
+					 list) {
+
+			if (old_patch == klp_transition_patch)
+				break;
+
+			klp_unpatch_objects(old_patch, false);
+			old_patch->enabled = false;
+			list_move(&old_patch->list, &klp_replaced_patches);
+			if (!klp_forced)
+				module_put(old_patch->mod);
+		}
+		klp_unpatch_objects(klp_transition_patch, true);
+	}
+
 	if (klp_target_state == KLP_UNPATCHED) {
 		/*
 		 * All tasks have transitioned to KLP_UNPATCHED so we can now
 		 * remove the new functions from the func_stack.
 		 */
-		klp_unpatch_objects(klp_transition_patch);
+		klp_unpatch_objects(klp_transition_patch, false);
 
 		/*
 		 * Make sure klp_ftrace_handler() can no longer see functions
@@ -143,6 +164,19 @@ static void klp_complete_transition(void)
 	if (!klp_forced && klp_target_state == KLP_UNPATCHED)
 		module_put(klp_transition_patch->mod);
 
+	/*
+	 * Free the added nops to free the memory and make ensure they are
+	 * re-calculated by a subsequent enable. There are 3 cases:
+	 * 1) enable succeeded -> we've called ftrace_shutdown(), which
+	 *                        means ftrace hooks are no longer visible.
+	 * 2) disable after enable -> nothing to free, since freed by previous
+	 *                            enable
+	 * 3) disable after failed enable -> klp_synchronize_transition() has
+	 *                                   been called above, so we should be
+	 *                                   ok to free as per usual rcu.
+	 */
+	klp_remove_nops(klp_transition_patch);
+
 	klp_target_state = KLP_UNDEFINED;
 	klp_transition_patch = NULL;
 }
@@ -215,10 +249,17 @@ static int klp_check_stack_func(struct klp_func *func,
 		if (klp_target_state == KLP_UNPATCHED) {
 			 /*
 			  * Check for the to-be-unpatched function
-			  * (the func itself).
+			  * (the func itself). If we're unpatching
+			  * a nop, then we're running the original
+			  * function.
 			  */
-			func_addr = (unsigned long)func->new_func;
-			func_size = func->new_size;
+			if (func->nop) {
+				func_addr = (unsigned long)func->old_addr;
+				func_size = func->old_size;
+			} else {
+				func_addr = (unsigned long)func->new_func;
+				func_size = func->new_size;
+			}
 		} else {
 			/*
 			 * Check for the to-be-patched function
-- 
2.6.1

Powered by blists - more mailing lists