[RFC][PATCH] Load/Unload Hooks for Livepatch

[RFC][PATCH] Load/Unload Hooks for Livepatch

[Chris J Arges]

Load/unload hooks will be useful to have when applying livepatches. Today
one can create hooks by writing code in the livepatch init/exit functions
and properly setting up module notifier callbacks to handle various cases.
This can be error prone and unwieldy as we have the proper mechanisms
already in the livepatch proper code.

I've tested this code with some sample livepatch code that exercised the
following:
  - works with single/multiple hooks for both load/unload
  - works if load_hooks/unload_hooks are NULL
  - works if individual hooks are NULL
  - handles bad return values from hooks
  - handles properly when inserting/removing dependent modules
  - can handle load_hook for vmlinux as well

The idea of these hooks are based off of the kpatch project.

Chris J Arges (1):
  livepatch: add load/unload hooks to objects

 include/linux/livepatch.h | 33 +++++++++++++++++++++++++++------
 kernel/livepatch/core.c   | 29 +++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+), 6 deletions(-)

-- 
2.7.4

[PATCH] livepatch: add load/unload hooks to objects

[Chris J Arges]

It can be useful to execute hook functions whenever a livepatch is applied
or unapplied to a particular object. Currently this is possible by writing
logic in the __init function of the livepatch kernel module. However to
handle executing functions when a module loads requires an additional
module notifier to be set up with the correct priority.

By using load/unload hooks we can execute these functions using the
existing livepatch notifier infrastructure and ensure consistent ordering
of notifications.

The load hook executes right before enabling functions, and the unload hook
executes right after disabling functions.

Signed-off-by: Chris J Arges <chris.j.arges@canonical.com>
---
 include/linux/livepatch.h | 33 +++++++++++++++++++++++++++------
 kernel/livepatch/core.c   | 29 +++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+), 6 deletions(-)

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 9072f04..bb32a66 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -65,18 +65,32 @@ struct klp_func {
 };
 
 /**
+ * struct klp_hook - hook structure for live patching
+ * @hook:	function to be executed on hook
+ *
+ */
+struct klp_hook {
+	int (*hook)(void);
+};
+
+/**
  * struct klp_object - kernel object structure for live patching
- * @name:	module name (or NULL for vmlinux)
- * @funcs:	function entries for functions to be patched in the object
- * @kobj:	kobject for sysfs resources
- * @mod:	kernel module associated with the patched object
- * 		(NULL for vmlinux)
- * @state:	tracks object-level patch application state
+ * @name:		module name (or NULL for vmlinux)
+ * @funcs:		function entries for functions to be patched in the
+ *                      object
+ * @load_hooks:		functions to be executed before load time (optional)
+ * @unload_hooks:	functions to be executed before load time (optional)
+ * @kobj:		kobject for sysfs resources
+ * @mod:		kernel module associated with the patched object
+ *                      (NULL for vmlinux)
+ * @state:		tracks object-level patch application state
  */
 struct klp_object {
 	/* external */
 	const char *name;
 	struct klp_func *funcs;
+	struct klp_hook *load_hooks;
+	struct klp_hook *unload_hooks;
 
 	/* internal */
 	struct kobject kobj;
@@ -111,6 +125,13 @@ struct klp_patch {
 	     func->old_name || func->new_func || func->old_sympos; \
 	     func++)
 
+#define klp_for_each_load_hook(obj, hook) \
+	for (hook = obj->load_hooks; hook && hook->hook; hook++)
+
+#define klp_for_each_unload_hook(obj, hook) \
+	for (hook = obj->unload_hooks; hook && hook->hook; hook++)
+
+
 int klp_register_patch(struct klp_patch *);
 int klp_unregister_patch(struct klp_patch *);
 int klp_enable_patch(struct klp_patch *);
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 5fbabe0..00e7d9c 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -378,6 +378,14 @@ static void klp_disable_func(struct klp_func *func)
 	func->state = KLP_DISABLED;
 }
 
+static int klp_run_hook(struct klp_hook *hook)
+{
+	if (hook && hook->hook)
+		return (*hook->hook)();
+
+	return 0;
+}
+
 static int klp_enable_func(struct klp_func *func)
 {
 	struct klp_ops *ops;
@@ -448,17 +456,28 @@ err:
 static void klp_disable_object(struct klp_object *obj)
 {
 	struct klp_func *func;
+	struct klp_hook *hook;
+	int ret;
 
 	klp_for_each_func(obj, func)
 		if (func->state == KLP_ENABLED)
 			klp_disable_func(func);
 
+	klp_for_each_unload_hook(obj, hook) {
+		ret = klp_run_hook(hook);
+		if (ret) {
+			pr_warn("unload hook '%p' failed for object '%s'\n",
+				hook, klp_is_module(obj) ? obj->name : "vmlinux");
+		}
+	}
+
 	obj->state = KLP_DISABLED;
 }
 
 static int klp_enable_object(struct klp_object *obj)
 {
 	struct klp_func *func;
+	struct klp_hook *hook;
 	int ret;
 
 	if (WARN_ON(obj->state != KLP_DISABLED))
@@ -467,6 +486,16 @@ static int klp_enable_object(struct klp_object *obj)
 	if (WARN_ON(!klp_is_object_loaded(obj)))
 		return -EINVAL;
 
+	klp_for_each_load_hook(obj, hook) {
+		ret = klp_run_hook(hook);
+		if (ret) {
+			pr_warn("load hook '%p' failed for object '%s'\n",
+				hook, klp_is_module(obj) ? obj->name : "vmlinux");
+			klp_disable_object(obj);
+			return ret;
+		}
+	}
+
 	klp_for_each_func(obj, func) {
 		ret = klp_enable_func(func);
 		if (ret) {
-- 
2.7.4

Re: [PATCH] livepatch: add load/unload hooks to objects

[Petr Mladek]

On Fri 2016-08-26 13:50:27, Chris J Arges wrote:
> It can be useful to execute hook functions whenever a livepatch is applied
> or unapplied to a particular object. Currently this is possible by writing
> logic in the __init function of the livepatch kernel module. However to
> handle executing functions when a module loads requires an additional
> module notifier to be set up with the correct priority.
> 
> By using load/unload hooks we can execute these functions using the
> existing livepatch notifier infrastructure and ensure consistent ordering
> of notifications.
> 
> The load hook executes right before enabling functions, and the unload hook
> executes right after disabling functions.

Could you please provide an example(s), what these hooks will be
useful for?

The callbacks will still need to be implemented in the patch module.
If they are generally useful, it would make sense to implement them
in the livepatch code directly, so they get more review and are
shared.

Best Regards,
Petr

Re: [PATCH] livepatch: add load/unload hooks to objects

[Christopher Arges]

On Mon, Aug 29, 2016 at 05:23:30PM +0200, Petr Mladek wrote:
> On Fri 2016-08-26 13:50:27, Chris J Arges wrote:
> > It can be useful to execute hook functions whenever a livepatch is applied
> > or unapplied to a particular object. Currently this is possible by writing
> > logic in the __init function of the livepatch kernel module. However to
> > handle executing functions when a module loads requires an additional
> > module notifier to be set up with the correct priority.
> > 
> > By using load/unload hooks we can execute these functions using the
> > existing livepatch notifier infrastructure and ensure consistent ordering
> > of notifications.
> > 
> > The load hook executes right before enabling functions, and the unload hook
> > executes right after disabling functions.
> 
> Could you please provide an example(s), what these hooks will be
> useful for?
> 
> The callbacks will still need to be implemented in the patch module.
> If they are generally useful, it would make sense to implement them
> in the livepatch code directly, so they get more review and are
> shared.
> 
> Best Regards,
> Petr

These hooks could be used as a yet another tool to implement a specific patch.
And yes, the callbacks to these hooks will be part of the patch module.

If there are 'hooks' that are applicable generically to livepatch they should
absolutely go into the core code.

As an example, CVE-2015-5307 requires that a bit be set in the exception bitmap
in order to handle #AC exceptions. One could write code in the init function of
the patch that checks if the module is loaded and then applies this fix. Or if
hooks are available, write a load hook that sets this structure whenever the
patch is loaded and the kvm module is loaded. In the future when patch
unloading is possible, one could also write an unload hook to return the
exception bitmap back to normal as the patched function(s) may not be available
any longer.

Another example is CVE-2016-2117. Here we need to unset NETIF_F_SG on a
particular device. If the device is already loaded we need a way to fixup
hw_features on an already allocated network device. Again this could be done
in the init code of the patch, but a nicer solution would be to do this on a
load/unload hook appropriately.

In general these hooks would help to better handle what needs to be setup before
we patch and cleanup after we unpatch. While one could write their own code and
notifiers in the patch init (and later exit) to do this, having a hook makes
this implementation cleaner and can tie it to a specific object. In addition,
tools like kpatch-build for livepatch (or whatever comes in the future) could
also utilize these hooks in much of the same way that kpatch does already.

--chris

Re: [PATCH] livepatch: add load/unload hooks to objects

[Jiri Kosina]

On Mon, 29 Aug 2016, Christopher Arges wrote:

> Another example is CVE-2016-2117. Here we need to unset NETIF_F_SG on a 
> particular device. If the device is already loaded we need a way to 
> fixup hw_features on an already allocated network device. Again this 
> could be done in the init code of the patch, but a nicer solution would 
> be to do this on a load/unload hook appropriately.

I am afraid this is more complicated than what you describe. You can't 
just unset NETIF_F_SG and be done with it; look for example what might 
happen if you clear the flag while skb_segment() is running and gcc is 
refetching netdev_features_t (there is no READ_ONCE() for that). The same 
holds for __ip6_append_data().
I am not saying this can't be worked around, but it's way much more 
complicated than just clearing a bit in a callback.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] livepatch: add load/unload hooks to objects

[Christopher Arges]

On Tue, Aug 30, 2016 at 11:41:28AM +0200, Jiri Kosina wrote:
> On Mon, 29 Aug 2016, Christopher Arges wrote:
> 
> > Another example is CVE-2016-2117. Here we need to unset NETIF_F_SG on a 
> > particular device. If the device is already loaded we need a way to 
> > fixup hw_features on an already allocated network device. Again this 
> > could be done in the init code of the patch, but a nicer solution would 
> > be to do this on a load/unload hook appropriately.
> 
> I am afraid this is more complicated than what you describe. You can't 
> just unset NETIF_F_SG and be done with it; look for example what might 
> happen if you clear the flag while skb_segment() is running and gcc is 
> refetching netdev_features_t (there is no READ_ONCE() for that). The same 
> holds for __ip6_append_data().
> I am not saying this can't be worked around, but it's way much more 
> complicated than just clearing a bit in a callback.
> 
> -- 
> Jiri Kosina
> SUSE Labs
>

Jiri,

Yes this example was meant more for showing how something like a load/unload
hook could make patching certain situations easier for a patch author.
Essentially it would be nice to have a place to run code right before patching,
without having to write an additional notifier for module load events.

In this specific example, for safety of setting hw_features perhaps one could
check if a set of functions are on the stacks of any tasks before executing
these hooks. Or ignore any skbs that are already in flight.

--chris

Re: [PATCH] livepatch: add load/unload hooks to objects

[Petr Mladek]

On Mon 2016-08-29 11:16:28, Christopher Arges wrote:
> On Mon, Aug 29, 2016 at 05:23:30PM +0200, Petr Mladek wrote:
> > On Fri 2016-08-26 13:50:27, Chris J Arges wrote:
> > > It can be useful to execute hook functions whenever a livepatch is applied
> > > or unapplied to a particular object. Currently this is possible by writing
> > > logic in the __init function of the livepatch kernel module. However to
> > > handle executing functions when a module loads requires an additional
> > > module notifier to be set up with the correct priority.
> > > 
> > > By using load/unload hooks we can execute these functions using the
> > > existing livepatch notifier infrastructure and ensure consistent ordering
> > > of notifications.
> > > 
> > > The load hook executes right before enabling functions, and the unload hook
> > > executes right after disabling functions.
> > 
> > Could you please provide an example(s), what these hooks will be
> > useful for?
> > 
> > The callbacks will still need to be implemented in the patch module.
> > If they are generally useful, it would make sense to implement them
> > in the livepatch code directly, so they get more review and are
> > shared.
> > 
> > Best Regards,
> > Petr
> 
> These hooks could be used as a yet another tool to implement a specific patch.
> And yes, the callbacks to these hooks will be part of the patch module.
> 
> If there are 'hooks' that are applicable generically to livepatch they should
> absolutely go into the core code.
> 
> As an example, CVE-2015-5307 requires that a bit be set in the exception bitmap
> in order to handle #AC exceptions. One could write code in the init function of
> the patch that checks if the module is loaded and then applies this fix. Or if
> hooks are available, write a load hook that sets this structure whenever the
> patch is loaded and the kvm module is loaded. In the future when patch
> unloading is possible, one could also write an unload hook to return the
> exception bitmap back to normal as the patched function(s) may not be available
> any longer.

Also this change looks racy when done by the hooks. I did not study it
in detail. But I wonder if it is correct to set the bit in the mask
before update_exception_bitmap() and ac_interception() are avalable.

My feeling is that you try to find a solution for something that
need to be supported by a more strict consistency model. You
try to change values of structures that might already be in use
and we need to be very careful here.

Your hooks are called for both already loaded objects and for objects
that are being loaded. Something that is safe for a module in COMMING
state might be dangerous for an already loaded one.

Best Regards,
Petr

Re: [PATCH] livepatch: add load/unload hooks to objects

[Christopher Arges]

On Tue, Aug 30, 2016 at 04:43:30PM +0200, Petr Mladek wrote:
> On Mon 2016-08-29 11:16:28, Christopher Arges wrote:
> > On Mon, Aug 29, 2016 at 05:23:30PM +0200, Petr Mladek wrote:
> > > On Fri 2016-08-26 13:50:27, Chris J Arges wrote:
> > > > It can be useful to execute hook functions whenever a livepatch is applied
> > > > or unapplied to a particular object. Currently this is possible by writing
> > > > logic in the __init function of the livepatch kernel module. However to
> > > > handle executing functions when a module loads requires an additional
> > > > module notifier to be set up with the correct priority.
> > > > 
> > > > By using load/unload hooks we can execute these functions using the
> > > > existing livepatch notifier infrastructure and ensure consistent ordering
> > > > of notifications.
> > > > 
> > > > The load hook executes right before enabling functions, and the unload hook
> > > > executes right after disabling functions.
> > > 
> > > Could you please provide an example(s), what these hooks will be
> > > useful for?
> > > 
> > > The callbacks will still need to be implemented in the patch module.
> > > If they are generally useful, it would make sense to implement them
> > > in the livepatch code directly, so they get more review and are
> > > shared.
> > > 
> > > Best Regards,
> > > Petr
> > 
> > These hooks could be used as a yet another tool to implement a specific patch.
> > And yes, the callbacks to these hooks will be part of the patch module.
> > 
> > If there are 'hooks' that are applicable generically to livepatch they should
> > absolutely go into the core code.
> > 
> > As an example, CVE-2015-5307 requires that a bit be set in the exception bitmap
> > in order to handle #AC exceptions. One could write code in the init function of
> > the patch that checks if the module is loaded and then applies this fix. Or if
> > hooks are available, write a load hook that sets this structure whenever the
> > patch is loaded and the kvm module is loaded. In the future when patch
> > unloading is possible, one could also write an unload hook to return the
> > exception bitmap back to normal as the patched function(s) may not be available
> > any longer.
> 
> Also this change looks racy when done by the hooks. I did not study it
> in detail. But I wonder if it is correct to set the bit in the mask
> before update_exception_bitmap() and ac_interception() are avalable.
> 
> My feeling is that you try to find a solution for something that
> need to be supported by a more strict consistency model. You
> try to change values of structures that might already be in use
> and we need to be very careful here.
> 

This is a good point. Perhaps the strict consistency will obviate the need for
hooks of this sort.

> Your hooks are called for both already loaded objects and for objects
> that are being loaded. Something that is safe for a module in COMMING
> state might be dangerous for an already loaded one.
> 
> Best Regards,
> Petr

Yea maybe this should have been [DRAFT RFC], I think more thought will need to
be done here about how to handle modifying existing data structures (and I see
you already have a proposal for this during plumbers).

In both cases; however I see the need for allowing patch authors to be able to
write some custom logic to safely handle changing existing data structures.
This could also be dependent on any user-space tooling requirements too.

--chris