[PATCH 2/2] livepatch: fix patched module loading race

[Josh Poimboeuf]

It's possible for klp_register_patch() to see a module before the COMING
notifier is called, or after the GOING notifier is called.

That can cause all kinds of ugly races.  As Pter Mladek reported:

  "The problem is that we do not keep the klp_mutex lock all the time when
  the module is being added or removed.

  First, the module is visible even before ftrace is ready. If we enable a patch
  in this time frame, adding ftrace ops will fail and the patch will get rejected
  just because bad timing.

  Second, if we are "lucky" and enable the patch for the coming module when the
  ftrace is ready but before the module notifier has been called. The notifier
  will try to enable the patch as well. It will detect that it is already patched,
  return error, and the module will get rejected just because bad timing.
  The more serious problem is that it will not call the notifier for
  going module, so that the mess will stay there and we wont be able to load
  the module later.

  Third, similar problems are there for going module. If a patch is enabled after
  the notifier finishes but before the module is removed from the list of modules,
  the new patch will be applied to the module. The module might disappear at
  anytime when the patch enabling is in progress, so there might be an access out
  of memory. Or the whole patch might be applied and some mess will be left,
  so it will not be possible to load/patch the module again."

Fix these races by letting the first one who sees the module do the
needed work.

Reported-by: Petr Mladek <pmladek@xxxxxxx>
Signed-off-by: Josh Poimboeuf <jpoimboe@xxxxxxxxxx>
---
 kernel/livepatch/core.c | 53 +++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 49 insertions(+), 4 deletions(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index a74e4e8..19a758c 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -89,16 +89,42 @@ static bool klp_is_object_loaded(struct klp_object *obj)
 /* sets obj->mod if object is not vmlinux and module is found */
 static void klp_find_object_module(struct klp_object *obj)
 {
+	struct module *mod;
+
 	if (!klp_is_module(obj))
 		return;
 
 	mutex_lock(&module_mutex);
+
 	/*
 	 * We don't need to take a reference on the module here because we have
 	 * the klp_mutex, which is also taken by the module notifier.  This
 	 * prevents any module from unloading until we release the klp_mutex.
 	 */
-	obj->mod = find_module(obj->name);
+	mod = find_module(obj->name);
+
+	/*
+	 * Be careful here to prevent races with the notifier:
+	 *
+	 * - MODULE_STATE_COMING: This may be before or after the notifier gets
+	 *   called.  If after, the notifier didn't see the object anyway
+	 *   because it hadn't been added to the patch list yet.  Either way,
+	 *   ftrace is already initialized, so it's safe to just go ahead and
+	 *   initialize the object here.
+	 *
+	 * - MODULE_STATE_GOING: Similar to MODULE_STATE_COMING, this may be
+	 *   before or after the notifier gets called.  If after, the notifer
+	 *   didn't see the object anyway.  Either way it's safe to just
+	 *   pretend that it's already gone and leave obj->mod at NULL.
+	 *
+	 *   MODULE_STATE_LIVE: The common case.  The module already finished
+	 *   loading.  Just like with MODULE_STATE_COMING, we know the notifier
+	 *   didn't see the object, so we init it here.
+	 */
+	if (mod && (mod->state == MODULE_STATE_COMING ||
+		    mod->state == MODULE_STATE_LIVE))
+		obj->mod = mod;
+
 	mutex_unlock(&module_mutex);
 }
 
@@ -695,8 +721,6 @@ static void klp_free_object_loaded(struct klp_object *obj)
 {
 	struct klp_func *func;
 
-	obj->mod = NULL;
-
 	for (func = obj->funcs; func->old_name; func++)
 		func->old_addr = 0;
 }
@@ -765,6 +789,7 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
 		return -EINVAL;
 
 	obj->state = KLP_DISABLED;
+	obj->mod = NULL;
 
 	klp_find_object_module(obj);
 
@@ -965,10 +990,30 @@ static int klp_module_notify(struct notifier_block *nb, unsigned long action,
 				continue;
 
 			if (action == MODULE_STATE_COMING) {
+
+				/*
+				 * Check for a small window where the register
+				 * path already initialized the object.
+				 */
+				if (obj->mod)
+					continue;
+
 				obj->mod = mod;
 				klp_module_notify_coming(patch, obj);
-			} else /* MODULE_STATE_GOING */
+			} else {
+				/* MODULE_STATE_GOING */
+
+				/*
+				 * Check for a small window where the register
+				 * path already saw the GOING state and thus
+				 * didn't set obj->mod.
+				 */
+				if (!obj->mod)
+					continue;
+
 				klp_module_notify_going(patch, obj);
+				obj->mod = NULL;
+			}
 
 			break;
 		}
-- 
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/