| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 1 Dec 2014 13:40:55 +0100
From: Petr Mladek <pmladek@...e.cz>
To: Miroslav Benes <mbenes@...e.cz>
Cc: Seth Jennings <sjenning@...hat.com>,
Josh Poimboeuf <jpoimboe@...hat.com>,
Jiri Kosina <jkosina@...e.cz>,
Vojtech Pavlik <vojtech@...e.cz>,
Steven Rostedt <rostedt@...dmis.org>,
Christoph Hellwig <hch@...radead.org>,
Greg KH <gregkh@...uxfoundation.org>,
Andy Lutomirski <luto@...capital.net>,
Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
live-patching@...r.kernel.org, x86@...nel.org, kpatch@...hat.com,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] livepatch: clean up klp_find_object_module() usage: was:
Re: [PATCHv4 2/3] kernel: add support for live patching
On Mon 2014-12-01 13:08:50, Miroslav Benes wrote:
> On Fri, 28 Nov 2014, Petr Mladek wrote:
>
> > On Fri 2014-11-28 18:07:37, Petr Mladek wrote:
> > > On Tue 2014-11-25 11:15: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.
> >
> > [...]
> >
> > > > +/* sets obj->mod if object is not vmlinux and module is found */
> > > > +static bool klp_find_object_module(struct klp_object *obj)
> > > > +{
> > > > + if (!obj->name)
> > > > + return 1;
> > > > +
> > > > + 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);
> > > > + mutex_unlock(&module_mutex);
> > > > +
> > > > + return !!obj->mod;
> > >
> > > I know that this is effective to return boolean here because
> > > it handles also patch against the kernel core (vmlinux). But
> > > the logic looks tricky. I would prefer a cleaner design and
> > > use this function only to set obj->mod.
> > >
> > > I wanted to see how it would look like, so I will send a patch for
> > > this in a separate mail.
> >
> > The patch is below. Of course, merge it into the original
> > patch if you agree with the idea, please.
> >
> >
> > >From 93eb9f9a25ad8aa0301e246f7685d3e787037566 Mon Sep 17 00:00:00 2001
> > From: Petr Mladek <pmladek@...e.cz>
> > Date: Fri, 28 Nov 2014 15:32:27 +0100
> > Subject: [PATCH 1/2] livepatch: clean up klp_find_object_module() usage
> >
> > The function klp_find_object_module() looks quite tricky. It has two effects:
> > sets obj->mod and decides whether the module is available or not. The second
> > effect is the tricky part because it handles also the code kernel code "vmlinux"
> > and is not module related. It causes returning bool, and doing the crazy double
> > negation.
> >
> > This patch tries to make a bit cleaner design:
> >
> > 1. klp_find_object_module() handles only obj->mod. It returns
> > the pointer or NULL.
> >
> > 2. It modifies klp_enable_object() to do nothing when the related
> > module has not been loaded yet.
> >
> > 3. The result is that the return value klp_find_object_module() is
> > not used in the end.
> >
> > We lose a check for potential klp_enable_object() misuse but it makes the code
> > easier. In fact, the check for unloaded module is rather long. We might want
> > to make it easier using some extra flag or another state of the object.
> > Such flag might be used for the check of misuse.
> >
> > Signed-off-by: Petr Mladek <pmladek@...e.cz>
>
> Hi,
>
> I agree with the idea but actually don't like the implementation. I'll try
> to propose few changes which would hopefully preserve the effect but make
> the end result slightly better.
>
> > ---
> > kernel/livepatch/core.c | 14 +++++++-------
> > 1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index 8e2e8cd242f5..9b1601729014 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -40,10 +40,10 @@ static LIST_HEAD(klp_patches);
> > static struct kobject *klp_root_kobj;
> >
> > /* sets obj->mod if object is not vmlinux and module is found */
> > -static bool klp_find_object_module(struct klp_object *obj)
> > +static struct module *klp_find_object_module(struct klp_object *obj)
> > {
> > if (!obj->name)
> > - return 1;
> > + return NULL;
> >
> > mutex_lock(&module_mutex);
> > /*
> > @@ -54,7 +54,7 @@ static bool klp_find_object_module(struct klp_object *obj)
> > obj->mod = find_module(obj->name);
> > mutex_unlock(&module_mutex);
> >
> > - return !!obj->mod;
> > + return obj->mod;
> > }
>
> As we do not need the return value in the end, we could maybe drop it
> completely, leading to change in if condition
>
> if (!obj->name)
> return;
Sounds good.
> > struct klp_find_arg {
> > @@ -318,8 +318,9 @@ static int klp_enable_object(struct module *pmod, struct klp_object *obj)
> > struct klp_func *func;
> > int ret;
> >
> > - if (WARN_ON(!obj->mod && obj->name))
> > - return -EINVAL;
> > + /* nope when the patched module has not been loaded yet */
> > + if (obj->name && !obj->mod)
> > + return 0;
>
> I have a problem with this one. In case the condition is true we do
> nothing, return back and pretend that everything is alright (return 0).
> I see that we would call klp_enable_object everytime in your proposal and
> decide here whether we want to do something or not. But I think that we
> should return some error and deal with it in the caller function. Thus
> original WARN_ON should stay here.
I agree.
> > if (obj->relocs) {
> > ret = klp_write_object_relocations(pmod, obj);
> > @@ -401,8 +402,7 @@ static int __klp_enable_patch(struct klp_patch *patch)
> > pr_notice("enabling patch '%s'\n", patch->mod->name);
> >
> > for (obj = patch->objs; obj->funcs; obj++) {
> > - if (!klp_find_object_module(obj))
> > - continue;
> > + klp_find_object_module(obj);
> > ret = klp_enable_object(patch->mod, obj);
> > if (ret)
> > goto unregister;
>
> I propose this piece of code
>
> for (obj = patch->objs; obj->funcs; obj++) {
> klp_find_object_module(obj);
> if (obj->name && !obj->mod)
> continue;
> ret = klp_enable_object(patch->mod, obj);
> ...
> }
I like this change, especially if we replace the condition with a
macro. It keeps klp_find_object_module() without the side effect
and it keeps the warning in klp_enable_object().
> What do you think? Also it could pay off to define inline function for the
> check. Somethink like klp_is_module_and_loaded...
klp_is_object_loaded() is easier and still clear. Also we might want
to add a macro klp_is_module(obj).
Best Regards,
Petr
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists