Messages in this thread | | | Date | Tue, 14 Dec 2021 16:40:11 +0100 | From | Petr Mladek <> | Subject | Re: [PATCH v2 1/2] livepatch: Allow user to specify functions to search for on a stack |
| |
On Tue 2021-12-14 13:27:33, Petr Mladek wrote: > On Tue 2021-12-14 09:47:59, Miroslav Benes wrote: > > On Mon, 13 Dec 2021, Josh Poimboeuf wrote: > > > On Fri, Dec 10, 2021 at 01:44:48PM +0100, Miroslav Benes wrote: > > > > --- a/kernel/livepatch/transition.c > > > > +++ b/kernel/livepatch/transition.c > > > > @@ -200,7 +200,10 @@ static int klp_check_stack_func(struct klp_func *func, unsigned long *entries, > > > > for (i = 0; i < nr_entries; i++) { > > > > address = entries[i]; > > > > > > > > - if (klp_target_state == KLP_UNPATCHED) { > > > > + if (func->stack_only) { > > > > + func_addr = (unsigned long)func->old_func; > > > > + func_size = func->old_size; > > > > + } else if (klp_target_state == KLP_UNPATCHED) { > > > > > > Hm, what does this mean for the unpatching case? What if the new > > > function's .cold child is on the stack when we're trying to unpatch? > > > > Good question. I did not realize it worked both ways. Of course it does. > > > > > Would it make sense to allow the user specify a 'new_func' for > > > stack_only, which is a func to check on the stack when unpatching? Then > > > new_func could point to the new .cold child. And then > > > klp_check_stack_func() wouldn't need a special case. > > I am confused. My understanding is that .cold child is explicitly > livepatched to the new .cold child like it is done in the selftest: > > static struct klp_func funcs_stack_only[] = { > { > .old_name = "child_function", > .new_func = livepatch_child_function, > }, { > > We should not need anything special to check it on stack. > We only need to make sure that we check all .stack_only functions of > the to-be-disabled livepatch.
We have discussed this with Miroslav and it seems to be even more complicated. My current understanding is that we actually have three functions involved:
parent_func() call child_func() jmp child_func.cold
We livepatch child_func() that uses jmp and need not be on stack. This is why we want to check parent_func() on stack. For this, we define something like:
static struct klp_func funcs[] = { { .old_name = "child_func", .new_func = livepatch_child_func, // livepatched func }, { .old_name = "parent_func", .stack_only = true, // stack only },
Now, there might be the same problem with livepatch_child_func. The call chain would be:
parent_func() call child_func() ---> livepatch_child_func() jmp livepatch_child_func.cold
=> We need to check the very same parent_func() also when unpatching.
Note that already do the same for nops:
static struct klp_func *klp_alloc_func_nop(struct klp_func *old_func, struct klp_object *obj) { [...] klp_init_func_early(obj, func); /* * func->new_func is same as func->old_func. These addresses are * set when the object is loaded, see klp_init_object_loaded(). */ func->old_sympos = old_func->old_sympos; func->nop = true; [...] }
where
static int klp_init_object_loaded(struct klp_patch *patch, struct klp_object *obj) { [...] if (func->nop) func->new_func = func->old_func; [...]
This is another argument that we should somehow reuse the nops code also for stack_only checks.
Does it make sense, please? ;-)
Best Regards, Petr
| |