lkml.org 
[lkml]   [2014]   [Nov]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: module notifier: was Re: [PATCH 2/2] kernel: add support for live patching
    On Fri, Nov 07, 2014 at 06:13:07PM +0100, Petr Mladek wrote:
    > On Thu 2014-11-06 08:39:08, Seth Jennings wrote:
    > > This commit introduces code for the live patching core. It implements
    > > an ftrace-based mechanism and kernel interface for doing live patching
    > > of kernel and kernel module functions.
    > >
    > > It represents the greatest common functionality set between kpatch and
    > > kgraft and can accept patches built using either method.
    > >
    > > This first version does not implement any consistency mechanism that
    > > ensures that old and new code do not run together. In practice, ~90% of
    > > CVEs are safe to apply in this way, since they simply add a conditional
    > > check. However, any function change that can not execute safely with
    > > the old version of the function can _not_ be safely applied in this
    > > version.
    >
    > [...]
    >
    > > +/******************************
    > > + * module notifier
    > > + *****************************/
    > > +
    > > +static int lp_module_notify(struct notifier_block *nb, unsigned long action,
    > > + void *data)
    > > +{
    > > + struct module *mod = data;
    > > + struct lpc_patch *patch;
    > > + struct lpc_object *obj;
    > > + int ret = 0;
    > > +
    > > + if (action != MODULE_STATE_COMING)
    > > + return 0;
    >
    > IMHO, we should handle also MODULE_STATE_GOING. We should unregister
    > the ftrace handlers and update the state of the affected objects
    > (ENABLED -> DISABLED)

    The mechanism we use to avoid this right now is taking a reference on
    patched module. We only release that reference after the patch is
    disabled, which unregisters all the patched functions from ftrace.

    However, your comment reminded me of an idea I had to use
    MODULE_STATE_GOING and let the lpc_mutex protect against races. I think
    it could be cleaner, but I haven't fleshed the idea out fully.

    >
    > > + down(&lpc_mutex);
    > > +
    > > + list_for_each_entry(patch, &lpc_patches, list) {
    > > + if (patch->state == DISABLED)
    > > + continue;
    > > + list_for_each_entry(obj, &patch->objs, list) {
    > > + if (strcmp(obj->name, mod->name))
    > > + continue;
    > > + pr_notice("load of module '%s' detected, applying patch '%s'\n",
    > > + mod->name, patch->mod->name);
    > > + obj->mod = mod;
    > > + ret = lpc_enable_object(patch->mod, obj);
    > > + if (ret)
    > > + goto out;
    > > + break;
    > > + }
    > > + }
    > > +
    > > + up(&lpc_mutex);
    > > + return 0;
    > > +out:
    >
    > I would name this err_our or so to make it clear that it is used when
    > something fails.

    Just "err" good?

    >
    > > + up(&lpc_mutex);
    > > + WARN("failed to apply patch '%s' to module '%s'\n",
    > > + patch->mod->name, mod->name);
    > > + return 0;
    > > +}
    > > +
    > > +static struct notifier_block lp_module_nb = {
    > > + .notifier_call = lp_module_notify,
    > > + .priority = INT_MIN, /* called last */
    >
    > The handler for MODULE_STATE_COMMING would need have higger priority,
    > if we want to cleanly unregister the ftrace handlers.

    Yes, we might need two handlers at different priorities if we decide to
    go that direction: one for MODULE_STATE_GOING at high/max and one for
    MODULE_STATE_COMING at low/min.

    Thanks,
    Seth

    >
    > Best Regards,
    > Petr


    \
     
     \ /
      Last update: 2014-11-07 19:41    [W:4.022 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site