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

[Petr Mladek <pmladek@suse.com>]

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