The previous patches can be applied, once the corresponding module is
loaded. In general, the patch will do relocation (if necessary) and
obtain/verify function address before we start to enable patch.
In some case, the uninitialized patch can be applied to the kernel.
Following is the case to describe the scenario step by step.
1) Patch a patch to the kernel before the corresponding module being
loaded.
2) Call klp_module_notify_coming to enable the patch, once the module
is loaded.
3) Do the instruction "obj->mod = mod", whatever the result of
klp_module_notify_coming is
4) Fail to call the klp_init_object_loaded or klp_enable_object
5) klp_module_notify_coming returns, now the module is working
6) Enable the patch from the userspace (disable patch firstly, then
enable the patch via sysfs)
7) Call __klp_enable_patch to enable patch
8) Pass the limitation (klp_init_object_loaded), because the value
of obj->mod is not NULL (obtain the value from step 3)
9) Patch is applied, though it is uninitialized (do not relocate
and obtain old_addr)
It is fatal to kernel, once the uninitialized patch is appled. To
fix it, obj->mod will nerver obtain the value, if livepatch fails
to call the klp_module_notify_coming.
Signed-off-by: Minfei Huang <[email protected]>
---
v1:
- modify the commit log, describe the issue more details
---
kernel/livepatch/core.c | 23 +++++++++++++----------
1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 284e269..4bbcdda 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -883,30 +883,30 @@ int klp_register_patch(struct klp_patch *patch)
}
EXPORT_SYMBOL_GPL(klp_register_patch);
-static void klp_module_notify_coming(struct klp_patch *patch,
+static int klp_module_notify_coming(struct klp_patch *patch,
struct klp_object *obj)
{
struct module *pmod = patch->mod;
struct module *mod = obj->mod;
- int ret;
+ int ret = 0;
ret = klp_init_object_loaded(patch, obj);
if (ret)
- goto err;
+ goto out;
if (patch->state == KLP_DISABLED)
- return;
+ goto out;
pr_notice("applying patch '%s' to loading module '%s'\n",
pmod->name, mod->name);
ret = klp_enable_object(obj);
- if (!ret)
- return;
-err:
- pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
- pmod->name, mod->name, ret);
+out:
+ if (ret)
+ pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
+ pmod->name, mod->name, ret);
+ return ret;
}
static void klp_module_notify_going(struct klp_patch *patch,
@@ -930,6 +930,7 @@ disabled:
static int klp_module_notify(struct notifier_block *nb, unsigned long action,
void *data)
{
+ int ret = 0;
struct module *mod = data;
struct klp_patch *patch;
struct klp_object *obj;
@@ -955,7 +956,9 @@ static int klp_module_notify(struct notifier_block *nb, unsigned long action,
if (action == MODULE_STATE_COMING) {
obj->mod = mod;
- klp_module_notify_coming(patch, obj);
+ ret = klp_module_notify_coming(patch, obj);
+ if (ret)
+ obj->mod = NULL;
} else /* MODULE_STATE_GOING */
klp_module_notify_going(patch, obj);
--
2.2.2
On Tue, 12 May 2015, Minfei Huang wrote:
> The previous patches can be applied, once the corresponding module is
> loaded. In general, the patch will do relocation (if necessary) and
> obtain/verify function address before we start to enable patch.
>
> In some case, the uninitialized patch can be applied to the kernel.
> Following is the case to describe the scenario step by step.
>
> 1) Patch a patch to the kernel before the corresponding module being
> loaded.
> 2) Call klp_module_notify_coming to enable the patch, once the module
> is loaded.
> 3) Do the instruction "obj->mod = mod", whatever the result of
> klp_module_notify_coming is
> 4) Fail to call the klp_init_object_loaded or klp_enable_object
> 5) klp_module_notify_coming returns, now the module is working
>
> 6) Enable the patch from the userspace (disable patch firstly, then
> enable the patch via sysfs)
> 7) Call __klp_enable_patch to enable patch
> 8) Pass the limitation (klp_init_object_loaded), because the value
> of obj->mod is not NULL (obtain the value from step 3)
> 9) Patch is applied, though it is uninitialized (do not relocate
> and obtain old_addr)
>
> It is fatal to kernel, once the uninitialized patch is appled. To
> fix it, obj->mod will nerver obtain the value, if livepatch fails
> to call the klp_module_notify_coming.
Hi,
I still think the changelog needs to be improved a bit.
There are three different situations in which the coming module notifier
can fail:
1. relocations are not applied for some reason. In this case kallsyms for
module symbol is not called at all. The patch is not applied to the
module. If the user disable and enable patch again, there is possible bug
in klp_enable_func. If the user specified func->old_addr for some function
in the module (and he shouldn't do that, but nevertheless) our warning
would not catch it, ftrace will fail in the process and the patch is
not enabled.
2. relocations are applied successfully, but kallsyms lookup fails. In
this case func->old_addr can be correct for all previous lookups, 0 for
current failed one, and "unspecified" for the rest. If we undergo the same
scenario as in 1. the behaviour differs for three cases, but the patch is
not enabled anyway.
3. the object is initialized but klp_enable_object fails in the notifier
due to possible ftrace error. Since it is improbable that ftrace would
heal itself in the future, we would get those errors everytime the patch
is enabled.
Your patch fixes all three cases, but consider to change the changelog to
describe it all.
See few more remarks below.
>
> Signed-off-by: Minfei Huang <[email protected]>
> ---
> v1:
> - modify the commit log, describe the issue more details
> ---
> kernel/livepatch/core.c | 23 +++++++++++++----------
> 1 file changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 284e269..4bbcdda 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -883,30 +883,30 @@ int klp_register_patch(struct klp_patch *patch)
> }
> EXPORT_SYMBOL_GPL(klp_register_patch);
>
> -static void klp_module_notify_coming(struct klp_patch *patch,
> +static int klp_module_notify_coming(struct klp_patch *patch,
> struct klp_object *obj)
> {
> struct module *pmod = patch->mod;
> struct module *mod = obj->mod;
> - int ret;
> + int ret = 0;
>
> ret = klp_init_object_loaded(patch, obj);
> if (ret)
> - goto err;
> + goto out;
>
> if (patch->state == KLP_DISABLED)
> - return;
> + goto out;
>
> pr_notice("applying patch '%s' to loading module '%s'\n",
> pmod->name, mod->name);
>
> ret = klp_enable_object(obj);
> - if (!ret)
> - return;
>
> -err:
> - pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> - pmod->name, mod->name, ret);
> +out:
> + if (ret)
> + pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> + pmod->name, mod->name, ret);
I think this message should be extended. We failed to apply the patch to
this object and we won't ever try that again. The user should know that.
Also note, that this warning is printed in two different situations.
Either the initialization failed, or klp_enable_object failed. Maybe it
would be nice to inform the user about that.
> + return ret;
> }
>
> static void klp_module_notify_going(struct klp_patch *patch,
> @@ -930,6 +930,7 @@ disabled:
> static int klp_module_notify(struct notifier_block *nb, unsigned long action,
> void *data)
> {
> + int ret = 0;
Do we really need the initialization? klp_module_notify_coming returns the
error or 0 by itself.
Thanks a lot,
Miroslav
> struct module *mod = data;
> struct klp_patch *patch;
> struct klp_object *obj;
> @@ -955,7 +956,9 @@ static int klp_module_notify(struct notifier_block *nb, unsigned long action,
>
> if (action == MODULE_STATE_COMING) {
> obj->mod = mod;
> - klp_module_notify_coming(patch, obj);
> + ret = klp_module_notify_coming(patch, obj);
> + if (ret)
> + obj->mod = NULL;
> } else /* MODULE_STATE_GOING */
> klp_module_notify_going(patch, obj);
>
> --
> 2.2.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
On 05/12/15 at 01:41P, Miroslav Benes wrote:
> On Tue, 12 May 2015, Minfei Huang wrote:
>
> > The previous patches can be applied, once the corresponding module is
> > loaded. In general, the patch will do relocation (if necessary) and
> > obtain/verify function address before we start to enable patch.
> >
> > In some case, the uninitialized patch can be applied to the kernel.
> > Following is the case to describe the scenario step by step.
> >
> > 1) Patch a patch to the kernel before the corresponding module being
> > loaded.
> > 2) Call klp_module_notify_coming to enable the patch, once the module
> > is loaded.
> > 3) Do the instruction "obj->mod = mod", whatever the result of
> > klp_module_notify_coming is
> > 4) Fail to call the klp_init_object_loaded or klp_enable_object
> > 5) klp_module_notify_coming returns, now the module is working
> >
> > 6) Enable the patch from the userspace (disable patch firstly, then
> > enable the patch via sysfs)
> > 7) Call __klp_enable_patch to enable patch
> > 8) Pass the limitation (klp_init_object_loaded), because the value
> > of obj->mod is not NULL (obtain the value from step 3)
> > 9) Patch is applied, though it is uninitialized (do not relocate
> > and obtain old_addr)
> >
> > It is fatal to kernel, once the uninitialized patch is appled. To
> > fix it, obj->mod will nerver obtain the value, if livepatch fails
> > to call the klp_module_notify_coming.
>
> Hi,
>
> I still think the changelog needs to be improved a bit.
>
> There are three different situations in which the coming module notifier
> can fail:
>
> 1. relocations are not applied for some reason. In this case kallsyms for
> module symbol is not called at all. The patch is not applied to the
> module. If the user disable and enable patch again, there is possible bug
> in klp_enable_func. If the user specified func->old_addr for some function
> in the module (and he shouldn't do that, but nevertheless) our warning
> would not catch it, ftrace will fail in the process and the patch is
> not enabled.
The function may be registered successfully by ftrace, although the
function address is incorrect.
>
> 2. relocations are applied successfully, but kallsyms lookup fails. In
> this case func->old_addr can be correct for all previous lookups, 0 for
> current failed one, and "unspecified" for the rest. If we undergo the same
> scenario as in 1. the behaviour differs for three cases, but the patch is
> not enabled anyway.
>
> 3. the object is initialized but klp_enable_object fails in the notifier
> due to possible ftrace error. Since it is improbable that ftrace would
> heal itself in the future, we would get those errors everytime the patch
> is enabled.
>
> Your patch fixes all three cases, but consider to change the changelog to
> describe it all.
Thanks for your effort about the patch log. It is more clearly to the
patch log as you suggested.
I will modify the patch log, then post the next version.
>
> See few more remarks below.
>
> >
> > Signed-off-by: Minfei Huang <[email protected]>
> > ---
> > v1:
> > - modify the commit log, describe the issue more details
> > ---
> > kernel/livepatch/core.c | 23 +++++++++++++----------
> > 1 file changed, 13 insertions(+), 10 deletions(-)
> >
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index 284e269..4bbcdda 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -883,30 +883,30 @@ int klp_register_patch(struct klp_patch *patch)
> > }
> > EXPORT_SYMBOL_GPL(klp_register_patch);
> >
> > -static void klp_module_notify_coming(struct klp_patch *patch,
> > +static int klp_module_notify_coming(struct klp_patch *patch,
> > struct klp_object *obj)
> > {
> > struct module *pmod = patch->mod;
> > struct module *mod = obj->mod;
> > - int ret;
> > + int ret = 0;
> >
> > ret = klp_init_object_loaded(patch, obj);
> > if (ret)
> > - goto err;
> > + goto out;
> >
> > if (patch->state == KLP_DISABLED)
> > - return;
> > + goto out;
> >
> > pr_notice("applying patch '%s' to loading module '%s'\n",
> > pmod->name, mod->name);
> >
> > ret = klp_enable_object(obj);
> > - if (!ret)
> > - return;
> >
> > -err:
> > - pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> > - pmod->name, mod->name, ret);
> > +out:
> > + if (ret)
> > + pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
> > + pmod->name, mod->name, ret);
>
> I think this message should be extended. We failed to apply the patch to
> this object and we won't ever try that again. The user should know that.
I will modify the error message.
>
> Also note, that this warning is printed in two different situations.
> Either the initialization failed, or klp_enable_object failed. Maybe it
> would be nice to inform the user about that.
>
> > + return ret;
> > }
> >
> > static void klp_module_notify_going(struct klp_patch *patch,
> > @@ -930,6 +930,7 @@ disabled:
> > static int klp_module_notify(struct notifier_block *nb, unsigned long action,
> > void *data)
> > {
> > + int ret = 0;
>
> Do we really need the initialization? klp_module_notify_coming returns the
> error or 0 by itself.
>
Will modify.
Thanks
Minfei