* [RFC][PATCH] Load/Unload Hooks for Livepatch @ 2016-08-26 18:50 Chris J Arges 2016-08-26 18:50 ` [PATCH] livepatch: add load/unload hooks to objects Chris J Arges 0 siblings, 1 reply; 8+ messages in thread From: Chris J Arges @ 2016-08-26 18:50 UTC (permalink / raw) To: live-patching Cc: Chris J Arges, Josh Poimboeuf, Jessica Yu, Jiri Kosina, Miroslav Benes, Petr Mladek, linux-kernel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] livepatch: add load/unload hooks to objects 2016-08-26 18:50 [RFC][PATCH] Load/Unload Hooks for Livepatch Chris J Arges @ 2016-08-26 18:50 ` Chris J Arges 2016-08-29 15:23 ` Petr Mladek 0 siblings, 1 reply; 8+ messages in thread From: Chris J Arges @ 2016-08-26 18:50 UTC (permalink / raw) To: live-patching Cc: Chris J Arges, Josh Poimboeuf, Jessica Yu, Jiri Kosina, Miroslav Benes, Petr Mladek, linux-kernel 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] livepatch: add load/unload hooks to objects 2016-08-26 18:50 ` [PATCH] livepatch: add load/unload hooks to objects Chris J Arges @ 2016-08-29 15:23 ` Petr Mladek 2016-08-29 16:16 ` Christopher Arges 0 siblings, 1 reply; 8+ messages in thread From: Petr Mladek @ 2016-08-29 15:23 UTC (permalink / raw) To: Chris J Arges Cc: live-patching, Josh Poimboeuf, Jessica Yu, Jiri Kosina, Miroslav Benes, linux-kernel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] livepatch: add load/unload hooks to objects 2016-08-29 15:23 ` Petr Mladek @ 2016-08-29 16:16 ` Christopher Arges 2016-08-30 9:41 ` Jiri Kosina 2016-08-30 14:43 ` Petr Mladek 0 siblings, 2 replies; 8+ messages in thread From: Christopher Arges @ 2016-08-29 16:16 UTC (permalink / raw) To: Petr Mladek Cc: live-patching, Josh Poimboeuf, Jessica Yu, Jiri Kosina, Miroslav Benes, linux-kernel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] livepatch: add load/unload hooks to objects 2016-08-29 16:16 ` Christopher Arges @ 2016-08-30 9:41 ` Jiri Kosina 2016-08-30 12:52 ` Christopher Arges 2016-08-30 14:43 ` Petr Mladek 1 sibling, 1 reply; 8+ messages in thread From: Jiri Kosina @ 2016-08-30 9:41 UTC (permalink / raw) To: Christopher Arges Cc: Petr Mladek, live-patching, Josh Poimboeuf, Jessica Yu, Miroslav Benes, linux-kernel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] livepatch: add load/unload hooks to objects 2016-08-30 9:41 ` Jiri Kosina @ 2016-08-30 12:52 ` Christopher Arges 0 siblings, 0 replies; 8+ messages in thread From: Christopher Arges @ 2016-08-30 12:52 UTC (permalink / raw) To: Jiri Kosina Cc: Petr Mladek, live-patching, Josh Poimboeuf, Jessica Yu, Miroslav Benes, linux-kernel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] livepatch: add load/unload hooks to objects 2016-08-29 16:16 ` Christopher Arges 2016-08-30 9:41 ` Jiri Kosina @ 2016-08-30 14:43 ` Petr Mladek 2016-08-30 15:15 ` Christopher Arges 1 sibling, 1 reply; 8+ messages in thread From: Petr Mladek @ 2016-08-30 14:43 UTC (permalink / raw) To: Christopher Arges Cc: live-patching, Josh Poimboeuf, Jessica Yu, Jiri Kosina, Miroslav Benes, linux-kernel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] livepatch: add load/unload hooks to objects 2016-08-30 14:43 ` Petr Mladek @ 2016-08-30 15:15 ` Christopher Arges 0 siblings, 0 replies; 8+ messages in thread From: Christopher Arges @ 2016-08-30 15:15 UTC (permalink / raw) To: Petr Mladek Cc: live-patching, Josh Poimboeuf, Jessica Yu, Jiri Kosina, Miroslav Benes, linux-kernel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-08-30 15:16 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-08-26 18:50 [RFC][PATCH] Load/Unload Hooks for Livepatch Chris J Arges 2016-08-26 18:50 ` [PATCH] livepatch: add load/unload hooks to objects Chris J Arges 2016-08-29 15:23 ` Petr Mladek 2016-08-29 16:16 ` Christopher Arges 2016-08-30 9:41 ` Jiri Kosina 2016-08-30 12:52 ` Christopher Arges 2016-08-30 14:43 ` Petr Mladek 2016-08-30 15:15 ` Christopher Arges
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).