On Fri 2017-08-25 11:26:23, Petr Mladek wrote:
> On Wed 2017-07-19 13:18:06, Jason Baron wrote:
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index e63f478..bf353da 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -602,6 +605,118 @@ static void klp_free_patch(struct klp_patch *patch)
> > +
> > +static int klp_init_patch_no_ops(struct klp_patch *patch)
> > +{
> > + struct klp_object *obj, *prev_obj, *new_obj;
> > + struct klp_func *prev_func, *func;
> > + struct klp_func_no_op *new;
> > + struct klp_patch *prev_patch;
> > + struct obj_iter o_iter, prev_o_iter;
> > + struct func_iter prev_f_iter, f_iter;
> > + bool found, mod;
> > +
> > + if (patch->list.prev == &klp_patches)
> > + return 0;
> > +
> > + prev_patch = list_prev_entry(patch, list);
> > + klp_for_each_object(prev_patch, prev_obj, &prev_o_iter) {
> > + if (!klp_is_object_loaded(prev_obj))
> > + continue;
> > +
> > + klp_for_each_func(prev_obj, prev_func, &prev_f_iter) {
> > + found = false;
> > + klp_for_each_object(patch, obj, &o_iter) {
> > + klp_for_each_func(obj, func, &f_iter) {
> > + if ((strcmp(prev_func->old_name,
> > + func->old_name) == 0) &&
> > + (prev_func->old_sympos ==
> > + func->old_sympos)) {
> > + found = true;
> > + break;
> > + }
> > + }
> > + if (found)
> > + break;
> > + }
> > + if (found)
> > + continue;
> > +
> > + new = kmalloc(sizeof(*new), GFP_KERNEL);
> > + if (!new)
> > + return -ENOMEM;
> > + new->orig_func = *prev_func;
> > + new->orig_func.old_name = prev_func->old_name;
> > + new->orig_func.new_func = NULL;
>
> This is OK if the operation replaces all older patches. Otherwise,
> you would need to store here the address from the patch down the stack.
>
>
> > + new->orig_func.old_sympos = prev_func->old_sympos;
> > + new->orig_func.immediate = prev_func->immediate;
> > + new->orig_func.old_addr = prev_func->old_addr;
>
> Hmm, this should be
>
> new->orig_func.old_addr = prev_func->new_func;
>
> klp_check_stack_func() should check the address of the old function
> that is currently running. It is the variant of the function that
> is on top of stack.
>
> I think that we actually have bug in the current livepatch code
> because old_addr always points to the original function!!!
> I am going to look at it.
I take this back. old_addr and old_size points to the original
(unpatched) code. The values are the same in all patches
for the same function. klp_check_stack_func() use this information
only when there is only one patch on the func_stack. Otherwise,
it checks new_func, new_size from the previous patch on the stack.
By other words, you assign old_addr and old_size correctly.
Also the livepatch handle this correctly. Ufff :-)
Best Regards,
Petr
--
To unsubscribe from this list: send the line "unsubscribe live-patching" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
